On Tue, Apr 27, 2021 at 11:04 PM Christoph Hellwig <hch@xxxxxx> 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?
