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

From: Jason Gunthorpe <jgg@nvidia.com>
To: Tony Krowiak <akrowiak@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>,
	Harald Freudenberger <freude@linux.ibm.com>,
	Vasily Gorbik <gor@linux.ibm.com>,
	Heiko Carstens <hca@linux.ibm.com>,
	linux-s390@vger.kernel.org, Halil Pasic <pasic@linux.ibm.com>,
	Pierre Morel <pmorel@linux.ibm.com>,
	"Raj, Ashok" <ashok.raj@intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Daniel Vetter <daniel@ffwll.ch>, Christoph Hellwig <hch@lst.de>,
	Leon Romanovsky <leonro@nvidia.com>,
	Max Gurtovoy <mgurtovoy@nvidia.com>,
	Tarun Gupta <targupta@nvidia.com>
Subject: Re: [PATCH v2 06/13] vfio/ap_ops: Convert to use vfio_register_group_dev()
Date: Tue, 4 May 2021 13:55:40 -0300	[thread overview]
Message-ID: <20210504165540.GY1370958@nvidia.com> (raw)
In-Reply-To: <80caaa31-3918-c96d-d7ff-727ddd1a4d1e@linux.ibm.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