Linux Virtualization - Feb 2020 - [PATCH V2 3/5] vDPA: introduce vDPA bus

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

The ida_simple_remove should probably be part of the class release
function to make everything work right
> +/**
> + * 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.
> +/**
> + * 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

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
>

On Wed, Feb 12, 2020 at 03:55:31PM +0800, Jason Wang
wrote:
> > 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.
device_release() doesn't call the bus release? You have dev, type or
class to choose from. Type is rarely used and doesn't seem to be used
by vdpa, so class seems the right choice

Jason