Re: [PATCH 3/6] iommu/amd: Split probe error paths to preserve IRQ remapping

[sashiko-bot@kernel.org]

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

New issues:
- [High] `iommu_ignore_device()` suffers from dead code and incorrect reordering, resulting in broken alias clearance, log spam, and unsafe non-atomic DTE clearing (torn writes).
- [Low] The commit silently fixes a build failure by adding a missing `dev_data` variable declaration that was used in the previous commit, breaking git bisect.

Pre-existing issues:
- [High] Skipping configuration on `PD_MODE_NONE` leaves `dev_data->max_irqs` uninitialized (0), completely breaking IRQ remapping.
--

commit b695f2fee16ac1c72b6f21b22dedf39b5ec55656
Author: Pranjal Shrivastava <praan@google.com>

iommu/amd: Split probe error paths to preserve IRQ remapping

This commit splits the error paths in amd_iommu_probe_device() to ensure
proper initialization failures clear the device, while configuration failures
like PD_MODE_NONE preserve the rlookup_table entry for IRQ remapping.

Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/all/20260529153216.2AD1E1F00899@smtp.kernel.org/
Suggested-by: Ankit Soni <ankit.soni@amd.com>

> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 9b5861e241d71..c3b3750d4a22d 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -737,6 +737,7 @@ static int iommu_init_device(struct amd_iommu *iommu, struct device *dev)
>  static void iommu_ignore_device(struct amd_iommu *iommu, struct device *dev)
>  {
>  	struct amd_iommu_pci_seg *pci_seg = iommu->pci_seg;
> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);

[Severity: Low]
Does this silently fix a build failure from a previous commit?

It appears the preceding commit in this series introduced the use of dev_data
in iommu_ignore_device() but failed to declare it. Adding the declaration here
fixes the build, but breaks bisectability between the two commits.

Could this fix be squashed into the commit that introduced the bug to ensure
the series remains fully bisectable?

[Severity: High]
Is dev_data always NULL when iommu_ignore_device() is called here?

Since iommu_ignore_device() is exclusively invoked from the error path of
amd_iommu_probe_device() when iommu_init_device() fails, dev_data will be
NULL because iommu_init_device() fails before dev_iommu_priv_set() is called.

If dev_data is NULL, the atomic DTE clearing block in this function becomes
dead code, and it falls back to the unsafe memset(), which risks torn writes
on live hardware:

drivers/iommu/amd/iommu.c:iommu_ignore_device() {
    ...
    if (dev_data) {
        struct dev_table_entry new = {};
        amd_iommu_make_clear_dte(dev_data, &new);
        update_dte256(iommu, dev_data, &new);
    } else {
        memset(&dev_table[devid], 0, sizeof(struct dev_table_entry));
    }
    ...
}

Additionally, because setup_aliases() is unconditionally called before this,
its internal clone_alias() will abort on the NULL dev_data and generate pr_err
log spam without actually clearing the alias DTEs.

[ ... ]

> @@ -2512,6 +2512,10 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)
>  	if (dev_is_pci(dev))
>  		pci_prepare_ats(to_pci_dev(dev), PAGE_SHIFT);
>  
> +	return iommu_dev;
> +
> +err_deinit:
> +	iommu_ignore_device(iommu, dev);
>  out_err:

[Severity: High]
This is a pre-existing issue, but does skipping the device configuration
leave max_irqs uninitialized?

The commit message notes that configuration failures like PD_MODE_NONE
skip iommu_ignore_device() to preserve the rlookup_table entry for IRQ
remapping.

However, because amd_iommu_probe_device() jumps directly to out_err on
PD_MODE_NONE, it bypasses the initialization of dev_data->max_irqs, leaving
it at 0.

Later, when alloc_irq_index() tries to allocate an IRQ, it relies on max_irqs:

drivers/iommu/amd/iommu.c:alloc_irq_index() {
    ...
    for (index = ALIGN(table->min_index, alignment), c = 0;
         index < max_irqs;) {
    ...
}

Since max_irqs is 0, the loop is skipped and it returns -ENOSPC, which
completely breaks IRQ allocation for the device, defeating the purpose
of preserving the rlookup entry.

>  	return iommu_dev;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601134204.2150602-1-praan@google.com?part=3