Received-SPF: pass (google.com: domain of linux-crypto+bounces-2461-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 1 Mar 2024 16:58:48 -0700
From: Alex Williamson <alex.williamson@redhat.com>
To: Xin Zeng <xin.zeng@intel.com>
Cc: herbert@gondor.apana.org.au, jgg@nvidia.com, yishaih@nvidia.com,
 shameerali.kolothum.thodi@huawei.com, kevin.tian@intel.com,
 linux-crypto@vger.kernel.org, kvm@vger.kernel.org, qat-linux@intel.com,
 Yahui Cao <yahui.cao@intel.com>
Subject: Re: [PATCH v4 10/10] vfio/qat: Add vfio_pci driver for Intel QAT VF
 devices
Message-ID: <20240301165848.37cfffaf.alex.williamson@redhat.com>
In-Reply-To: <20240228143402.89219-11-xin.zeng@intel.com>
References: <20240228143402.89219-1-xin.zeng@intel.com>
	<20240228143402.89219-11-xin.zeng@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

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;
> +}