Messages in this thread | | | Date | Wed, 10 Mar 2021 09:15:08 +0100 | From | Christoph Hellwig <> | Subject | Re: [PATCH 9/9] vfio/pci: export igd support into vendor vfio_pci driver |
| |
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.
| |