Re: [PATCH 02/12] vfio/mdev: Allow the mdev_parent_ops to specify the device driver to bind

From: Jason Gunthorpe <jgg@nvidia.com>
To: Christoph Hellwig <hch@lst.de>
Cc: Alex Williamson <alex.williamson@redhat.com>,
	Cornelia Huck <cohuck@redhat.com>,
	kvm@vger.kernel.org, Kirti Wankhede <kwankhede@nvidia.com>,
	"Raj, Ashok" <ashok.raj@intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Daniel Vetter <daniel@ffwll.ch>,
	Leon Romanovsky <leonro@nvidia.com>,
	Max Gurtovoy <mgurtovoy@nvidia.com>,
	Tarun Gupta <targupta@nvidia.com>
Subject: Re: [PATCH 02/12] vfio/mdev: Allow the mdev_parent_ops to specify the device driver to bind
Date: Mon, 26 Apr 2021 11:11:40 -0300	[thread overview]
Message-ID: <20210426141140.GU1370958@nvidia.com> (raw)
In-Reply-To: <20210426140257.GA15209@lst.de>

On Mon, Apr 26, 2021 at 04:02:57PM +0200, Christoph Hellwig wrote:
> On Fri, Apr 23, 2021 at 08:02:59PM -0300, Jason Gunthorpe wrote:
> > +/*
> > + * mdev drivers can refuse to bind during probe(), in this case we want to fail
> > + * the creation of the mdev all the way back to sysfs. This is a weird model
> > + * that doesn't fit in the driver core well, nor does it seem to appear any
> > + * place else in the kernel, so use a simple hack.
> > + */
> > +static int mdev_bind_driver(struct mdev_device *mdev)
> > +{
> > +	struct mdev_driver *drv = mdev->type->parent->ops->device_driver;
> > +	int ret;
> > +
> > +	if (!drv)
> > +		drv = &vfio_mdev_driver;
> > +
> > +	while (1) {
> > +		device_lock(&mdev->dev);
> > +		if (mdev->dev.driver == &drv->driver) {
> > +			ret = 0;
> > +			goto out_unlock;
> > +		}
> > +		if (mdev->probe_err) {
> > +			ret = mdev->probe_err;
> > +			goto out_unlock;
> > +		}
> > +		device_unlock(&mdev->dev);
> > +		ret = device_attach(&mdev->dev);
> > +		if (ret)
> > +			return ret;
> > +		mdev->probe_err = -EINVAL;
> > +	}
> > +	return 0;
> > +
> > +out_unlock:
> > +	device_unlock(&mdev->dev);
> > +	return ret;
> > +}
> 
> This looks strange to me, and I think by open coding
> device_attach we could do much better here, something like:

I look at this for a long time, it is strange.

> static int mdev_bind_driver(struct mdev_device *mdev)
> {
> 	struct mdev_driver *drv = mdev->type->parent->ops->device_driver;
> 	int ret = -EINVAL;
> 
> 	if (!drv)
> 		drv = &vfio_mdev_driver;
> 
> 	device_lock(&mdev->dev);
> 	if (WARN_ON_ONCE(device_is_bound(dev)))
> 		goto out_unlock;
> 	if (mdev->dev.p->dead)
> 	 	goto out_unlock;

'p' is private to the driver core so we can't touch it here

> 	mdev->dev.driver = &drv->driver;
> 	ret = device_bind_driver(&mdev->dev);

It is really counter intuitive but device_bind_driver() doesn't
actually call probe, or do a lot of other essential stuff.

As far as I can see the driver core has three different ways to bind
drivers:
 - The normal 'really_probe()' path with all the bells and whistles.
 - You can set dev.driver before calling device_add() and related
 - You can call device_bind_driver() 'somehow'.

The later two completely skip all the really_probe() stuff, so things
like devm and more become broken. They also don't call probe(), that
is up to the caller. They seem only usable in very niche special
cases, unfortunately.

Some callers open code the probe() but then they have ordering
problems with the sysfs and other little issues.

In this case 99% of the time the driver will already be bound here and
this routine does nothing - the only case I worried about about is
some kind of defered probe by default which calling device_attach()
will defeat.

Thanks,
Jason