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
