Re: [PATCH 9/9] vfio/pci: export igd support into vendor vfio_pci driver

Christoph Hellwig
2021-03-10T08:16:05Z

The terminology is all weird here. You don't export functionality
you move it. And this is not a "vendor" driver, but just a device
specific one.

> +struct igd_vfio_pci_device {
> + struct vfio_pci_core_device vdev;
> +};

Why do you need this separate structure? You could just use
vfio_pci_core_device directly.

> +static void igd_vfio_pci_release(void *device_data)
> +{
> + struct vfio_pci_core_device *vdev = device_data;
> +
> + mutex_lock(&vdev->reflck->lock);
> + if (!(--vdev->refcnt)) {

No need for the braces here.

> + vfio_pci_vf_token_user_add(vdev, -1);
> + vfio_pci_core_spapr_eeh_release(vdev);
> + vfio_pci_core_disable(vdev);
> + }
> + mutex_unlock(&vdev->reflck->lock);

But more importantly all this code should be in a helper exported
from the core code.

> +
> + module_put(THIS_MODULE);

This looks bogus - the module reference is now gone while
igd_vfio_pci_release is still running. Module refcounting always
need to be done by the caller, not the individual driver.

> +static int igd_vfio_pci_open(void *device_data)
> +{
> + struct vfio_pci_core_device *vdev = device_data;
> + int ret = 0;
> +
> + if (!try_module_get(THIS_MODULE))
> + return -ENODEV;

Same here - thi is something the caller needs to do.

> + mutex_lock(&vdev->reflck->lock);
> +
> + if (!vdev->refcnt) {
> + ret = vfio_pci_core_enable(vdev);
> + if (ret)
> + goto error;
> +
> + ret = vfio_pci_igd_init(vdev);
> + if (ret && ret != -ENODEV) {
> + pci_warn(vdev->pdev, "Failed to setup Intel IGD regions\n");
> + vfio_pci_core_disable(vdev);
> + goto error;
> + }
> + ret = 0;
> + vfio_pci_probe_mmaps(vdev);
> + vfio_pci_core_spapr_eeh_open(vdev);
> + vfio_pci_vf_token_user_add(vdev, 1);

Way to much boilerplate. Why doesn't the core only call a function
that does the vfio_pci_igd_init?

> +static const struct pci_device_id igd_vfio_pci_table[] = {
> + { PCI_VENDOR_ID_INTEL, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA << 8, 0xff0000, 0 },

Please avoid the overly long line. And a match as big as any Intel
graphics at very least needs a comment documenting why this is safe
and will perpetually remain safe.

> +#ifdef CONFIG_VFIO_PCI_DRIVER_COMPAT
> +struct pci_driver *get_igd_vfio_pci_driver(struct pci_dev *pdev)
> +{
> + if (pci_match_id(igd_vfio_pci_driver.id_table, pdev))
> + return &igd_vfio_pci_driver;
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(get_igd_vfio_pci_driver);
> +#endif

> + case PCI_VENDOR_ID_INTEL:
> + if (pdev->class == PCI_CLASS_DISPLAY_VGA << 8)
> + return get_igd_vfio_pci_driver(pdev);

And this now means that the core code has a dependency on the igd
one, making the whole split rather pointless.