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

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

On Mon, Jan 05, 2026 at 03:35:21PM +0000, Jonathan Cameron wrote:
> 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;

Updated.

> > +}
> 
> 
> > 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.

It is a common pattern in the kernel, that's why I am not convinced that
changing just this instance would improve much.

Thanks,
Lorenzo

> 
> > +	}
> >  
> >  	return 0;