Re: [PATCH v2 06/13] vfio/ap_ops: Convert to use vfio_register_group_dev()

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, May 04, 2021 at 10:42:04AM -0400, Tony Krowiak wrote:
> 
> 
> On 4/26/21 4:00 PM, Jason Gunthorpe wrote:
> > This is straightforward conversion, the ap_matrix_mdev is actually serving
> > as the vfio_device and we can replace all the mdev_get_drvdata()'s with a
> > simple container_of().
> 
> This is a nit, but most of the mdev_get_drvdata()'s are not being
> replaced by container_of(); in most places, the replacement is
> dev_get_drvdata().

Hum, only sysfs uses dev_get_drvdata(), but OK but numbers there are
more of them.


> >   	vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix);
> > -	mdev_set_drvdata(mdev, matrix_mdev);
> >   	matrix_mdev->pqap_hook.hook = handle_pqap;
> >   	matrix_mdev->pqap_hook.owner = THIS_MODULE;
> >   	mutex_lock(&matrix_dev->lock);
> >   	list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
> >   	mutex_unlock(&matrix_dev->lock);
> > +	ret = vfio_register_group_dev(&matrix_mdev->vdev);
> > +	if (ret)
> > +		goto err_list;
> > +	dev_set_drvdata(&mdev->dev, matrix_mdev);
> 
> I'm wondering whether the code above should be done under
> the matrix_dev->lock. Does the mdev exist in the sysfs at the
> point the probe is called? If so, then is it possible for functions
> that acquire the matrix_mdev from the drvdata to get called before
> the drvdata is set?

The sysfs is inserted by 

  vfio_ap_matrix_driver.driver.dev_groups = vfio_ap_mdev_attr_groups

And if you look in really_probe() you can see the order is:

                ret = drv->probe(dev);
        if (device_add_groups(dev, drv->dev_groups)) {

Any access to drvdata has to be protected by the device_lock or inside
one of these special ordered regions like a driver core created sysfs
callback.

Thanks,
Jason