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
