Re: [PATCH 4/5] virtio: introduce a vDPA based transport

[Jason Gunthorpe <jgg@mellanox.com>]

On Fri, Jan 17, 2020 at 05:32:35PM +0800, Jason Wang wrote:
> > 
> > > +	const struct vdpa_config_ops *ops = vdpa->config;
> > > +	struct virtio_vdpa_device *vd_dev;
> > > +	int rc;
> > > +
> > > +	vd_dev = devm_kzalloc(dev, sizeof(*vd_dev), GFP_KERNEL);
> > > +	if (!vd_dev)
> > > +		return -ENOMEM;
> > This is not right, the struct device lifetime is controled by a kref,
> > not via devm. If you want to use a devm unwind then the unwind is
> > put_device, not devm_kfree.
> 
> I'm not sure I get the point here. The lifetime is bound to underlying vDPA
> device and devres allow to be freed before the vpda device is released. But
> I agree using devres of underlying vdpa device looks wired.

Once device_initialize is called the only way to free a struct device
is via put_device, while here you have a devm trigger that will
unconditionally do kfree on a struct device without respecting the
reference count.

reference counted memory must never be allocated with devm.

> > > +	vd_dev->vdev.dev.release = virtio_vdpa_release_dev;
> > > +	vd_dev->vdev.config = &virtio_vdpa_config_ops;
> > > +	vd_dev->vdpa = vdpa;
> > > +	INIT_LIST_HEAD(&vd_dev->virtqueues);
> > > +	spin_lock_init(&vd_dev->lock);
> > > +
> > > +	vd_dev->vdev.id.device = ops->get_device_id(vdpa);
> > > +	if (vd_dev->vdev.id.device == 0)
> > > +		return -ENODEV;
> > > +
> > > +	vd_dev->vdev.id.vendor = ops->get_vendor_id(vdpa);
> > > +	rc = register_virtio_device(&vd_dev->vdev);
> > > +	if (rc)
> > > +		put_device(dev);
> > And a ugly unwind like this is why you want to have device_initialize()
> > exposed to the driver,
> 
> In this context, which "driver" did you mean here? (Note, virtio-vdpa is the
> driver for vDPA bus here).

'driver' is the thing using the 'core' library calls to implement a
device, so here the 'vd_dev' is the driver and
'register_virtio_device' is the core

> > 
> > Where is the various THIS_MODULE's I expect to see in a scheme like
> > this?
> > 
> > All function pointers must be protected by a held module reference
> > count, ie the above probe/remove and all the pointers in ops.
> 
> Will double check, since I don't see this in other virtio transport drivers
> (PCI or MMIO).

pci_register_driver is a macro that provides a THIS_MODULE, and the
pci core code sets driver.owner, then the rest of the stuff related to
driver ops is supposed to work against that to protect the driver ops.

For the device module refcounting you either need to ensure that
'unregister' is a strong fence and guanentees that no device ops are
called past unregister (noting that this is impossible for release),
or you need to hold the module lock until release.

It is common to see non-core subsystems get this stuff wrong.

Jason