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

[Christoph Hellwig <hch@xxxxxx>]

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:

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;

	mdev->dev.driver = &drv->driver;
	ret = device_bind_driver(&mdev->dev);
	if (ret)
		mdev->dev.driver = NULL;
out_unlock:
	device_unlock(&mdev->dev);
	return ret;
}