Re: [PATCH] accel/qaic: Add support for AI200 virtual functions

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Missing pci_disable_sriov() in the physical function driver removal path causes SR-IOV VFs to leak.
- [High] Device state check in qaic_pci_sriov_configure incorrectly blocks SR-IOV VF teardown when the device is offline or booting.
- [High] The aic200vf_config structure is never added to the qaic_ids PCI device ID table, preventing the driver from matching and binding to VF devices.

Pre-existing issues:
- [High] Missing pci_free_irq_vectors() in MSI initialization error paths after a successful pci_alloc_irq_vectors() call.
--

--- Patch [1]: [PATCH] accel/qaic: Add support for AI200 virtual functions ---
commit c554793b8689d7226dadc15db51370aa624d506a
Author: Youssef Samir <[email protected]>

accel/qaic: Add support for AI200 virtual functions

Register the qaic_pci_sriov_configure callback to allow VFs to be
created through AIC200's sysfs numvfs entry. Additionally, define
a new MHI family for the 5 channels supported by AIC200 VFs: NNC,
Telemetry, IPCR, Loopback, and SSR.

> diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
> index 0acf6cdd44f81..cde8eb8c7a71b 100644
> --- a/drivers/accel/qaic/qaic_drv.c
> +++ b/drivers/accel/qaic/qaic_drv.c
> @@ -77,6 +77,13 @@ static const struct qaic_device_config aic200_config = {
>  	.dbc_bar_idx = 2,
>  };
>  
> +static const struct qaic_device_config aic200vf_config = {
> +	.family = FAMILY_AIC200_VF,
> +	.bar_mask = BIT(0) | BIT(1) | BIT(2) | BIT(4),
> +	.mhi_bar_idx = 1,
> +	.dbc_bar_idx = 2,
> +};
> +

[Severity: High]
This structure is defined, but is it missing an entry in the qaic_ids
PCI device ID table?

If the VF's PCI ID is not registered in the table in
drivers/accel/qaic/qaic_drv.c, won't the driver fail to match and bind
to the newly created VFs, making this configuration unreachable?

[ ... ]

> @@ -761,6 +768,34 @@ static const struct dev_pm_ops qaic_pm_ops = {
>  	SYSTEM_SLEEP_PM_OPS(qaic_pm_suspend, qaic_pm_resume)
>  };
>  
> +static int qaic_pci_sriov_configure(struct pci_dev *pdev, int num_vfs)
> +{
> +	struct qaic_device *qdev = pci_get_drvdata(pdev);
> +	int ret;
> +
> +	/* Qaic device must be online to process VF bringup */
> +	if (qdev->dev_state == QAIC_OFFLINE)
> +		return -ENODEV;
> +
> +	if (qdev->dev_state == QAIC_BOOT)
> +		return -EBUSY;
> +
> +	if (num_vfs == 0) {
> +		pci_disable_sriov(pdev);
> +		return 0;
> +	}

[Severity: High]
Does the dev_state check in qaic_pci_sriov_configure() correctly handle the
num_vfs == 0 teardown path?

If the device encounters a hardware error and enters QAIC_OFFLINE, an admin
might write 0 to sriov_numvfs to tear down the VFs. Since the state checks
happen before checking for num_vfs == 0, won't this incorrectly return
-ENODEV and permanently trap the VFs in an enabled state?

Could the num_vfs == 0 check be moved before the dev_state validations?

> +
> +	ret = pci_enable_sriov(pdev, num_vfs);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to enable SR-IOV: %d (requested %d VFs, max %d)\n",
> +			ret, num_vfs, pci_sriov_get_totalvfs(pdev));
> +		return ret;
> +	}
> +
> +	dev_dbg(&pdev->dev, "Successfully enabled %d VFs\n", num_vfs);
> +	return num_vfs;
> +}
> +
>  static struct pci_driver qaic_pci_driver = {
>  	.name = QAIC_NAME,
>  	.id_table = qaic_ids,
> @@ -768,6 +803,7 @@ static struct pci_driver qaic_pci_driver = {
>  	.remove = qaic_pci_remove,
>  	.shutdown = qaic_pci_shutdown,
>  	.err_handler = &qaic_pci_err_handler,
> +	.sriov_configure = qaic_pci_sriov_configure,

[Severity: High]
Since this patch enables SR-IOV, does qaic_pci_remove() in 
drivers/accel/qaic/qaic_drv.c need to call pci_disable_sriov() before 
tearing down the device resources?

If an administrator unbinds the PF driver while VFs are still active,
won't leaving SR-IOV enabled while destroying the PF's MHI and DRM backing
state lead to a resource leak and potential use-after-free when the active
VFs attempt to access the hardware?

[Severity: High]
This is a pre-existing issue, but looking at init_msi() in 
drivers/accel/qaic/qaic_drv.c:

drivers/accel/qaic/qaic_drv.c:init_msi() {
    ...
	mhi_irq = pci_irq_vector(pdev, 0);
	if (mhi_irq < 0)
		return mhi_irq;

	for (i = 0; i < qdev->num_dbc; ++i) {
		ret = devm_request_threaded_irq(&pdev->dev,
						pci_irq_vector(pdev, qdev->single_msi ? 0 : i + 1),
						dbc_irq_handler, dbc_irq_threaded_fn, IRQF_SHARED,
						"qaic_dbc", &qdev->dbc[i]);
		if (ret)
			return ret;
    ...
}

Are we leaking IRQ vectors here?

According to the PCI subsystem guidelines, failing to call
pci_free_irq_vectors() in error paths after a successful
pci_alloc_irq_vectors() leaks IRQ resources. If devm_request_threaded_irq()
fails, this path returns the error code directly without freeing the vectors.

>  	.driver = {
>  		.pm = pm_sleep_ptr(&qaic_pm_ops),
>  	},

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1