[PATCH] acpi: fix generic acpi_register_gsi() handling of shared interrupts

[PATCH] acpi: fix generic acpi_register_gsi() handling of shared interrupts

[Mark Salter]

If there are devices sharing an interrupt, acpi_register_gsi() could
be called multiple times for the same gsi. Currently, it just maps
the gsi without checking for a previous mapping. This patch adds a
check for an existing mapping and uses that if found.

Cc: Hanjun Guo <hanjun.guo@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Mark Salter <msalter@redhat.com>
---
 drivers/acpi/gsi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c
index 38208f2..5698c4f 100644
--- a/drivers/acpi/gsi.c
+++ b/drivers/acpi/gsi.c
@@ -76,6 +76,13 @@ int acpi_register_gsi(struct device *dev, u32 gsi, int trigger,
 	unsigned int irq_type = acpi_gsi_get_irq_type(trigger, polarity);
 
 	/*
+	 * The gsi may be shared with other devices, so check for a
+	 * previous mapping and use that if found.
+	 */
+	if (acpi_gsi_to_irq(gsi, &irq) > 0)
+		return irq;
+
+	/*
 	 * There is no way at present to look-up the IRQ domain on ACPI,
 	 * hence always create mapping referring to the default domain
 	 * by passing NULL as irq_domain parameter
-- 
1.8.3.1

Re: [PATCH] acpi: fix generic acpi_register_gsi() handling of shared interrupts

[Marc Zyngier]

On 28/04/15 17:51, Mark Salter wrote:
> If there are devices sharing an interrupt, acpi_register_gsi() could
> be called multiple times for the same gsi. Currently, it just maps
> the gsi without checking for a previous mapping. This patch adds a
> check for an existing mapping and uses that if found.
> 
> Cc: Hanjun Guo <hanjun.guo@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Mark Salter <msalter@redhat.com>
> ---
>  drivers/acpi/gsi.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c
> index 38208f2..5698c4f 100644
> --- a/drivers/acpi/gsi.c
> +++ b/drivers/acpi/gsi.c
> @@ -76,6 +76,13 @@ int acpi_register_gsi(struct device *dev, u32 gsi, int trigger,
>  	unsigned int irq_type = acpi_gsi_get_irq_type(trigger, polarity);
>  
>  	/*
> +	 * The gsi may be shared with other devices, so check for a
> +	 * previous mapping and use that if found.
> +	 */
> +	if (acpi_gsi_to_irq(gsi, &irq) > 0)
> +		return irq;

I'm afraid that's completely racy, and doesn't prevent much (two CPUs
can execute this at the same time and still end up executing the rest of
the code).

Furthermore, irq_create_mapping is *designed* to handle that situation,
and returns the same virtual IRQ for a given hwirq (and it has the
proper locking...).

So maybe it would be worth explaining the actual problem you're seeing,
because the code as it stands should handle the problem you seem to
describe.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] acpi: fix generic acpi_register_gsi() handling of shared interrupts

[Mark Salter]

On Tue, 2015-04-28 at 18:08 +0100, Marc Zyngier wrote:
> On 28/04/15 17:51, Mark Salter wrote:
> > If there are devices sharing an interrupt, acpi_register_gsi() could
> > be called multiple times for the same gsi. Currently, it just maps
> > the gsi without checking for a previous mapping. This patch adds a
> > check for an existing mapping and uses that if found.
> > 
> > Cc: Hanjun Guo <hanjun.guo@linaro.org>
> > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Signed-off-by: Mark Salter <msalter@redhat.com>
> > ---
> >  drivers/acpi/gsi.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c
> > index 38208f2..5698c4f 100644
> > --- a/drivers/acpi/gsi.c
> > +++ b/drivers/acpi/gsi.c
> > @@ -76,6 +76,13 @@ int acpi_register_gsi(struct device *dev, u32 gsi, int trigger,
> >  	unsigned int irq_type = acpi_gsi_get_irq_type(trigger, polarity);
> >  
> >  	/*
> > +	 * The gsi may be shared with other devices, so check for a
> > +	 * previous mapping and use that if found.
> > +	 */
> > +	if (acpi_gsi_to_irq(gsi, &irq) > 0)
> > +		return irq;
> 
> I'm afraid that's completely racy, and doesn't prevent much (two CPUs
> can execute this at the same time and still end up executing the rest of
> the code).
> 
> Furthermore, irq_create_mapping is *designed* to handle that situation,

You're right. irq_create_mapping does that check already. But if it
is truly racy, irq_create_mapping has the same problem.

> and returns the same virtual IRQ for a given hwirq (and it has the
> proper locking...).
> 
> So maybe it would be worth explaining the actual problem you're seeing,
> because the code as it stands should handle the problem you seem to
> describe.

Sorry for the noise. I was experimenting with some other code
in acpi_register_gsi which called __irq_domain_alloc_irqs
directly which bypassed the check in irq_create_mapping. And
that turns out to be a silly thing to do anyway, so nevermind.