Re: [PATCH v2 02/13] 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: Dan Williams <dan.j.williams@intel.com>,
	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>,
	Daniel Vetter <daniel@ffwll.ch>,
	Leon Romanovsky <leonro@nvidia.com>,
	Max Gurtovoy <mgurtovoy@nvidia.com>,
	Tarun Gupta <targupta@nvidia.com>
Subject: Re: [PATCH v2 02/13] vfio/mdev: Allow the mdev_parent_ops to specify the device driver to bind
Date: Tue, 4 May 2021 08:30:29 -0300	[thread overview]
Message-ID: <20210504113029.GQ1370958@nvidia.com> (raw)
In-Reply-To: <20210504093636.GA24834@lst.de>

On Tue, May 04, 2021 at 11:36:36AM +0200, Christoph Hellwig wrote:
> On Wed, Apr 28, 2021 at 11:00:05AM -0300, Jason Gunthorpe wrote:
> > I thought about doing it like that, it is generally a good idea,
> > however, if I add new API surface to the driver core I really want to
> > get rid of device_bind_driver(), or at least most of its users.
> > 
> > I'm pretty sure Greg will ask for it too.
> > 
> > So, I need a way to sequence that which doesn't mean I have to shelf
> > the mdev stuff for ages while I try to get acks from lots of places.
> > 
> > Leave this alone and fix it after? Export device_driver_attach() and
> > say to try and fix the rest after?
> 
> Maybe.  Or convert one or two samples.

The conversions are easy I just can't test them or completely tell if
they are correct..

> > I think this will still need the ugly errno capture though..
> 
> Why?

Several of the mdev drivers are checking some predicate during their
new probe function, like total # of devices. So if userspace exceeds
that then the old behavior was to fail the sysfs create operation. eg:

static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
{
	if (private->state == VFIO_CCW_STATE_NOT_OPER)
		return -ENODEV;

	if (atomic_dec_if_positive(&private->avail) < 0)
		return -EPERM;

Without the errno capture this doesn't work anymore and things end
succeeding to create a device and but failing to attach a driver.

It could be changed to loose the errno and just return with some
generic -EINVAL if no driver bound, but that seems pretty ugly too.

Returning the probe error from some device_driver_attach() also make
some sense, but revising the code to do that is a big touch and this
is so strange I don't know if it is worth it.

Jason