Re: [PATCH 08/12] vfio/gvt: Convert to use vfio_register_group_dev()

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Apr 26, 2021 at 04:13:55PM +0200, Christoph Hellwig wrote:
> > diff --git a/drivers/vfio/mdev/Makefile b/drivers/vfio/mdev/Makefile
> > index ff9ecd80212503..7c236ba1b90eb1 100644
> > +++ b/drivers/vfio/mdev/Makefile
> > @@ -1,5 +1,5 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> >  
> > -mdev-y := mdev_core.o mdev_sysfs.o mdev_driver.o vfio_mdev.o
> > +mdev-y := mdev_core.o mdev_sysfs.o mdev_driver.o
> >  
> >  obj-$(CONFIG_VFIO_MDEV) += mdev.o
> > diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> > index 51b8a9fcf866ad..f95d01b57fb168 100644
> > +++ b/drivers/vfio/mdev/mdev_core.c
> 
> I think all these mdev core changes belong into a separate commit with a
> separate commit log.

Gah, they were split, I must have flubbed up a rebase on Friday :\

commit daeb9dd3a152e21d11960805b55e34967987e8cf

    vfio/mdev: Remove vfio_mdev.c
    
    Now that all mdev drivers directly create their own mdev_device driver and
    directly register with the vfio core's vfio_device_ops this is all dead
    code.
    
    Delete vfio_mdev.c and the mdev_parent_ops members that are connected to
    it.
    
    Preserve VFIO's design of allowing mdev drivers to be !GPL by allowing the
    three functions that replace this module for !GPL usage. This goes along
    with the other 19 symbols that are already marked !GPL in VFIO.
    
    Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

I'll fix it

> >  static int __init mdev_init(void)
> >  {
> > -	int rc;
> > -
> > -	rc = mdev_bus_register();
> > -	if (rc)
> > -		return rc;
> > -	rc = mdev_register_driver(&vfio_mdev_driver);
> > -	if (rc)
> > -		goto err_bus;
> > -	return 0;
> > -err_bus:
> > -	mdev_bus_unregister();
> > -	return rc;
> > +	return  mdev_bus_register();
> 
> Weird indentation.  But I think it would be best to just kill off the
> mdev_init wrapper anyway.

Oh, right good point

> > diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c
> > index 6e96c023d7823d..0012a9ee7cb0a4 100644
> > +++ b/drivers/vfio/mdev/mdev_driver.c
> > @@ -74,15 +74,8 @@ static int mdev_remove(struct device *dev)
> >  static int mdev_match(struct device *dev, struct device_driver *drv)
> >  {
> >  	struct mdev_device *mdev = to_mdev_device(dev);
> > +
> > +	return drv == &mdev->type->parent->ops->device_driver->driver;
> >  }
> 
> Btw, I think we don't even need ->match with the switch to use
> device_bind_driver that I suggested.

See my other email for why it is like this..
 
> > -EXPORT_SYMBOL_GPL(vfio_init_group_dev);
> > +EXPORT_SYMBOL(vfio_init_group_dev);
> 
> > -EXPORT_SYMBOL_GPL(vfio_register_group_dev);
> > +EXPORT_SYMBOL(vfio_register_group_dev);
> 
> > -EXPORT_SYMBOL_GPL(vfio_unregister_group_dev);
> > +EXPORT_SYMBOL(vfio_unregister_group_dev); 
> 
> Err, no.  vfio should remain EXPORT_SYMBOL_GPL, just because the weird
> mdev "GPL condom" that should never have been merged in that form went away.

VFIO is already !GPL - there are 19 symbols supporting this
today. What happened here is that this patch make all of those symbols
unusable !GPL by changing how registration works so you can't get the
vfio_device argument to use with the API family.

So, either the two registration functions need to be !GPL to make the
other 19 symbols make sense, or the entire !GPL needs to be ripped
out. The lost commit message above was explaining this.

Since it is predominately !GPL today, I'd prefer a discussion on
changing VFIO to be GPL only to be in its own patch proposing removing
all 22 !GPL symbols. Those are always fun threads..

Jason
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel