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

[Dan Williams <dan.j.williams@intel.com>]

On Tue, Apr 27, 2021 at 11:04 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Mon, Apr 26, 2021 at 05:00:04PM -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;
> > +}
>
> > +++ b/drivers/vfio/mdev/mdev_driver.c
> > @@ -49,7 +49,7 @@ static int mdev_probe(struct device *dev)
> >               return ret;
> >
> >       if (drv->probe) {
> > -             ret = drv->probe(mdev);
> > +             ret = mdev->probe_err = drv->probe(mdev);
> >               if (ret)
> >                       mdev_detach_iommu(mdev);
> >       }
>
> >       return 0;
> >  }
> >
> > +static int mdev_match(struct device *dev, struct device_driver *drv)
> > +{
> > +     struct mdev_device *mdev = to_mdev_device(dev);
> > +     struct mdev_driver *target = mdev->type->parent->ops->device_driver;
> > +
> > +     /*
> > +      * The ops specify the device driver to connect, fall back to the old
> > +      * shim driver if the driver hasn't been converted.
> > +      */
> > +     if (!target)
> > +             target = &vfio_mdev_driver;
> > +     return drv == &target->driver;
> > +}
>
> I still think this going the wrong way.  Why can't we enhance the core
> driver code with a version of device_bind_driver() that does call into
> ->probe?  That probably seems like a better model for those existing
> direct users of device_bind_driver or device_attach with a pre-set
> ->drv anyway.

Wouldn't that just be "export device_driver_attach()" so that drivers
can implement their own custom bind implementation?