Re: [PATCH v6 5/8] PCI: vmd: Replace hardcoded values with enum and defines

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

New issues:
- [Medium] The patch failed to completely replace the hardcoded resource indices 1 and 2 with VMD_RES_MBAR_1 and VMD_RES_MBAR_2 when calling vmd_configure_membar().

Pre-existing issues:
- [High] Missing pci_free_irq_vectors() in error paths and module removal, leaking IRQ vectors.
--

--- Patch [5]: [PATCH v6 5/8] PCI: vmd: Replace hardcoded values with enum and defines ---
commit d7c8cf241184af8f2b0d45fc458cfb88afbce3a5
Author: Szymon Durawa <[email protected]>

PCI: vmd: Replace hardcoded values with enum and defines

Add enum vmd_resource type to replace hardcoded values. Add defines for
vmd bus start number based on VMD restriction value. No functional
changes.

> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index f88a72b6309e7..78e8bdf7c3d79 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c

[ ... ]

> @@ -838,8 +850,8 @@ static int vmd_configure_membar1_membar2(struct vmd_dev *vmd,
>  
>  	ret = vmd_configure_membar(vmd, 2, VMD_MEMBAR2, mbar2_ofs, 0);

[Severity: Medium]
Since the cleanup paths now use the enum value VMD_RES_MBAR_1, should this call
and the preceding call to vmd_configure_membar() for MEMBAR1 use the new enum
instead of the hardcoded 1 and 2?

>  	if (ret) {
> -		devm_kfree(&vmd->dev->dev, (void *)vmd->resources[1].name);
> -		memset(&vmd->resources[1], 0, sizeof(vmd->resources[1]));
> +		devm_kfree(&vmd->dev->dev, (void *)vmd->resources[VMD_RES_MBAR_1].name);
> +		memset(&vmd->resources[VMD_RES_MBAR_1], 0, sizeof(vmd->resources[VMD_RES_MBAR_1]));
>  		return ret;
>  	}

[Severity: High]
This is a pre-existing issue, but I noticed a potential IRQ leak during review.

In vmd_alloc_irqs(), if devm_kcalloc() fails, does this leak the IRQ vectors
allocated by pci_alloc_irq_vectors() since pci_free_irq_vectors() is not called?

drivers/pci/controller/vmd.c:vmd_alloc_irqs() {
    ...
	vmd->msix_count = pci_alloc_irq_vectors(dev, vmd->first_vec + 1,
						vmd->msix_count, PCI_IRQ_MSIX);
	if (vmd->msix_count < 0)
		return vmd->msix_count;

	vmd->irqs = devm_kcalloc(&dev->dev, vmd->msix_count, sizeof(*vmd->irqs),
				 GFP_KERNEL);
	if (!vmd->irqs)
		return -ENOMEM;
    ...
}

Similarly, does vmd_remove() permanently leak IRQs on unbind since it tears
down other resources but fails to invoke pci_free_irq_vectors()?

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