Re: [PATCH] iommu/riscv: prefer WSI on IGS=BOTH when wired IRQs are described

[Robin Murphy <robin.murphy@arm.com>]

On 2026-05-20 10:54 am, fangyu.yu@linux.alibaba.com wrote:
>>>
>>> From: Fangyu Yu <fangyu.yu@linux.alibaba.com>
>>>
>>> The RISC-V IOMMU spec defines IGS=BOTH as supporting both MSI and
>>> WSI, with software selecting the path. The DT path already behaves
>>> as expected by selecting WSI when wired IRQ resources are described.
>>> The ACPI path, however, currently falls back to MSI even when
>>> firmware describes wired IRQ resources.
>>>
>>> Use firmware-described wired IRQ resources as the trigger to select
>>> WSI for IGS=BOTH:
>>>    - DT: "interrupts" present, no "msi-parent"
>>>    - ACPI: DSDT _CRS Interrupt() descriptors
>>>      (mainline does not yet parse the RIMT Interrupt Wire Array)
>>>
>>> When triggered, rewrite igs to IGS_WSI and reuse the existing WSI
>>> handling. Keep the existing behaviour otherwise.
>>>
>>> Fixes: d5f88acdd6ff ("iommu/riscv: Add support for platform msi")
>>> Signed-off-by: Fangyu Yu <fangyu.yu@linux.alibaba.com>
>>> ---
>>>   drivers/iommu/riscv/iommu-platform.c | 15 +++++++++++++++
>>>   1 file changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/iommu/riscv/iommu-platform.c b/drivers/iommu/riscv/iommu-platform.c
>>> index 399ba8fe1b3e..bd7712231140 100644
>>> --- a/drivers/iommu/riscv/iommu-platform.c
>>> +++ b/drivers/iommu/riscv/iommu-platform.c
>>> @@ -71,6 +71,21 @@ static int riscv_iommu_platform_probe(struct platform_device *pdev)
>>>          iommu->irqs_count = RISCV_IOMMU_INTR_COUNT;
>>>
>>>          igs = FIELD_GET(RISCV_IOMMU_CAPABILITIES_IGS, iommu->caps);
>>> +
>>> +       /*
>>> +        * IGS=BOTH means the IOMMU supports either MSI or WSI;
>>> +        * the spec leaves the choice to software. Use the firmware-described
>>> +        * wired interrupt resources as the trigger:
>>> +        *   - DT  : "interrupts" property present, no "msi-parent"  -> WSI
>>> +        *   - ACPI: DSDT _CRS Interrupt() present  -> WSI
>>> +        * Otherwise default to the MSI path.
>>> +        */
>>> +       if (igs == RISCV_IOMMU_CAPABILITIES_IGS_BOTH &&
>>> +           platform_irq_count(pdev) > 0) {
>>> +               dev_info(dev, "firmware describes wired IRQs; preferring WSI on IGS=BOTH\n");
>>> +               igs = RISCV_IOMMU_CAPABILITIES_IGS_WSI;
>>> +       }
>>> +
>> Won't it change the DT behavior as it doesn't check msi-parent
>> anymore?  IOW, should this be made specific to ACPI
>> by checking whether the device node is acpi node?
>>
> 
> Thanks for the review, you're right that the current condition would
> affect DT as well.
> 
> My assumption was that DT would describe either "interrupts" for WSI
> or "msi-parent" for MSI, but not both, so platform_irq_count() > 0
> would effectively imply "no msi-parent" in practice.
> 
> That said, I agree that this should be limited to the ACPI path. That
> matches the actual bug scope — only ACPI was falling back to MSI when
> wired IRQs were described — and leaves DT unchanged.
> 
> I'll respin a v2 to scope the fix to ACPI only, and tighten the commit
> message accordingly.

Why would this be specific to ACPI? AFAICS if DT specifies an 
"msi-parent" property such that an MSI domain exists, then MSIs will be 
preferred as well, since the entire function is structured to prefer 
MSIs if available, and fall back to wired if not. If the system does 
support both options then DT should describe that, same as ACPI; what 
Linux chooses to use is entirely Linux's own policy.

But if you do want to change the Linux driver's policy for some reason 
that the commit message isn't really explaining, then rearrange the 
whole switch statement; don't just add a weird hack to try to defeat the 
existing logic _in the same function_...

In general though, I would have thought preferring MSIs makes the most 
sense, since MSI vectors are generally cheaper than wires, so there's a 
better chance of being able to have unique IRQs per interrupt source, 
whereas with wires you may be stuck with a combined IRQ.

(As a side note, is there a reason this is calling of_msi_configure() on 
a platform device when of_platform_device_create_pdata() will have done 
that already?)

Thanks,
Robin.