Messages in this thread | | | Subject | Re: [PATCH V2 3/5] vDPA: introduce vDPA bus | From | Jason Wang <> | Date | Wed, 12 Feb 2020 15:55:31 +0800 |
| |
On 2020/2/11 下午9:47, Jason Gunthorpe wrote: > On Mon, Feb 10, 2020 at 11:56:06AM +0800, Jason Wang wrote: >> +/** >> + * vdpa_register_device - register a vDPA device >> + * Callers must have a succeed call of vdpa_init_device() before. >> + * @vdev: the vdpa device to be registered to vDPA bus >> + * >> + * Returns an error when fail to add to vDPA bus >> + */ >> +int vdpa_register_device(struct vdpa_device *vdev) >> +{ >> + int err = device_add(&vdev->dev); >> + >> + if (err) { >> + put_device(&vdev->dev); >> + ida_simple_remove(&vdpa_index_ida, vdev->index); >> + } > This is a very dangerous construction, I've seen it lead to driver > bugs. Better to require the driver to always do the put_device on > error unwind
Ok.
> > The ida_simple_remove should probably be part of the class release > function to make everything work right
It looks to me bus instead of class is the correct abstraction here since the devices share a set of programming interface but not the semantics.
Or do you actually mean type here?
> >> +/** >> + * vdpa_unregister_driver - unregister a vDPA device driver >> + * @drv: the vdpa device driver to be unregistered >> + */ >> +void vdpa_unregister_driver(struct vdpa_driver *drv) >> +{ >> + driver_unregister(&drv->driver); >> +} >> +EXPORT_SYMBOL_GPL(vdpa_unregister_driver); >> + >> +static int vdpa_init(void) >> +{ >> + if (bus_register(&vdpa_bus) != 0) >> + panic("virtio bus registration failed"); >> + return 0; >> +} > Linus will tell you not to kill the kernel - return the error code and > propagate it up to the module init function.
Yes, will fix.
> >> +/** >> + * vDPA device - representation of a vDPA device >> + * @dev: underlying device >> + * @dma_dev: the actual device that is performing DMA >> + * @config: the configuration ops for this device. >> + * @index: device index >> + */ >> +struct vdpa_device { >> + struct device dev; >> + struct device *dma_dev; >> + const struct vdpa_config_ops *config; >> + int index; > unsigned values shuld be unsigned int > > Jason
Will fix.
Thanks
>
| |