Re: [PATCH v4 10/10] vfio/qat: Add vfio_pci driver for Intel QAT VF devices

[Alex Williamson <alex.williamson@redhat.com>]

On Wed, 28 Feb 2024 22:34:02 +0800
Xin Zeng <xin.zeng@intel.com> wrote:
> +static int
> +qat_vf_vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct qat_vf_core_device *qat_vdev;
> +	int ret;
> +
> +	if (!pci_match_id(id, pdev)) {
> +		pci_err(pdev, "Incompatible device, disallowing driver_override\n");
> +		return -ENODEV;
> +	}

I think the question of whether this is the right thing to do is still
up for debate, but as noted in the thread where I raised the question,
this mechanism doesn't actually work.

The probe callback is passed the matching ID from the set of dynamic
IDs added via new_id, the driver id_table, or pci_device_id_any for a
strictly driver_override match.  Any of those would satisfy
pci_match_id().

If we wanted the probe function to exclusively match devices in the
id_table, we should call this as:

	if (!pci_match_id(qat_vf_vfio_pci_table, pdev))...

If we wanted to still allow dynamic IDs, the test might be more like:

	if (id == &pci_device_id_any)...

Allowing dynamic IDs but failing driver_override requires a slightly
more sophisticated user, but is inconsistent.  Do we have any
consensus on this?  Thanks,

Alex


> +
> +	qat_vdev = vfio_alloc_device(qat_vf_core_device, core_device.vdev, dev, &qat_vf_pci_ops);
> +	if (IS_ERR(qat_vdev))
> +		return PTR_ERR(qat_vdev);
> +
> +	pci_set_drvdata(pdev, &qat_vdev->core_device);
> +	ret = vfio_pci_core_register_device(&qat_vdev->core_device);
> +	if (ret)
> +		goto out_put_device;
> +
> +	return 0;
> +
> +out_put_device:
> +	vfio_put_device(&qat_vdev->core_device.vdev);
> +	return ret;
> +}