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

Jason Gunthorpe
2021-05-04T16:55:45Z

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