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.
