Re: [PATCH v2 7/7] irqchip/gic-v5: Add ACPI IWB probing

Jonathan Cameron
2026-01-05T15:35:26Z

On Thu, 18 Dec 2025 11:14:33 +0100
Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:

> To probe an IWB in an ACPI based system it is required:
>
> - to implement the IORT functions handling the IWB IORT node and create
> functions to retrieve IWB firmware information
> - to augment the driver to match the DSDT ACPI "ARMH0003" device and
> retrieve the IWB wire and trigger mask from the GSI interrupt descriptor
> in the IWB msi_domain_ops.msi_translate() function
>
> Make the required driver changes to enable IWB probing in ACPI systems.
>
> The GICv5 GSI format requires special handling for IWB routed IRQs.
>
> Add IWB GSI detection to the top level driver gic_v5_get_gsi_domain_id()
> function so that the correct IRQ domain for a GSI can be detected by
> parsing the GSI and check whether it is an IWB-backed IRQ or not.
>
> Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Hanjun Guo <guohanjun@huawei.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
A couple of trivial comments inline. Overall this series looks in a good
state to me.
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> ---
> drivers/acpi/arm64/iort.c | 95 ++++++++++++++++++++++++++++++++------
> drivers/irqchip/irq-gic-v5-iwb.c | 42 +++++++++++++----
> drivers/irqchip/irq-gic-v5.c | 4 ++
> include/linux/acpi_iort.h | 1 +
> include/linux/irqchip/arm-gic-v5.h | 6 +++
> 5 files changed, 123 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 17dbe66da804..4b0b753db738 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c

> @@ -317,12 +325,28 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
> return status;
> }
>
> +static acpi_status iort_match_iwb_callback(struct acpi_iort_node *node, void *context)
> +{
> + acpi_status status = AE_NOT_FOUND;
> + u32 *id = context;
> +
> + if (node->type == ACPI_IORT_NODE_IWB) {
> + struct acpi_iort_iwb *iwb;
> +
> + iwb = (struct acpi_iort_iwb *)node->node_data;
> + status = iwb->iwb_index == *id ? AE_OK : AE_NOT_FOUND;
> + }
> +
> + return status;
Simpler flow with a quick exclusion of wrong nodes.
if (node->type != ACPI_IORT_NODE_IWB)
return AE_NOT_FOUND;
....
iwb = ...

Also not sure I'd use a ternary here given it's only slightly more code
as more readable.
if (iwb->iwb_index != *id)
return AE_NOT_FOUND;

return AE_OK;

> +}

> diff --git a/drivers/irqchip/irq-gic-v5-iwb.c b/drivers/irqchip/irq-gic-v5-iwb.c
> index ad9fdc14d1c6..c7d5fd34d053 100644
> --- a/drivers/irqchip/irq-gic-v5-iwb.c
> +++ b/drivers/irqchip/irq-gic-v5-iwb.c
> @@ -4,6 +4,7 @@
> */
> #define pr_fmt(fmt) "GICv5 IWB: " fmt
>
> +#include <linux/acpi.h>
> #include <linux/init.h>
> #include <linux/kernel.h>
> #include <linux/msi.h>
> @@ -136,18 +137,31 @@ static int gicv5_iwb_irq_domain_translate(struct irq_domain *d, struct irq_fwspe
> irq_hw_number_t *hwirq,
> unsigned int *type)
> {
> - if (!is_of_node(fwspec->fwnode))
> - return -EINVAL;
> + if (is_of_node(fwspec->fwnode)) {
>
> - if (fwspec->param_count < 2)
> - return -EINVAL;
> + if (fwspec->param_count < 2)
> + return -EINVAL;
>
> - /*
> - * param[0] is be the wire
> - * param[1] is the interrupt type
> - */
> - *hwirq = fwspec->param[0];
> - *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
> + /*
> + * param[0] is be the wire
> + * param[1] is the interrupt type
> + */
> + *hwirq = fwspec->param[0];
> + *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;

As below, FIELD_GET() would improve reviewability a little.

> + }
> +
> + if (is_acpi_device_node(fwspec->fwnode)) {
> +
> + if (fwspec->param_count < 2)
> + return -EINVAL;
> +
> + /*
> + * Extract the wire from param[0]
> + * param[1] is the interrupt type
> + */
> + *hwirq = FIELD_GET(GICV5_GSI_IWB_WIRE, fwspec->param[0]);
> + *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;

I'd prefer this FIELD_GET() for this as well so there is no need to
go sanity check that it is the lowest bits.

> + }
>
> return 0;