* [PATCH] spi: bcm2835aux: ensure interrupts are enabled for shared handler
@ 2018-05-03 18:09 ` Rob Herring
0 siblings, 0 replies; 14+ messages in thread
From: Rob Herring @ 2018-05-03 18:09 UTC (permalink / raw)
To: linux-arm-kernel
The BCM2835 AUX SPI has a shared interrupt line (with AUX UART).
Downstream fixes this with an AUX irqchip to demux the IRQ sources and a
DT change which breaks compatibility with older kernels. The AUX irqchip
was already rejected for upstream[1] and the DT change would break
working systems if the DTB is updated to a newer one. The latter issue
was brought to my attention by Alex Graf.
The root cause however is a bug in the shared handler. Shared handlers
must check that interrupts are actually enabled before servicing the
interrupt. Add a check that the TXEMPTY or IDLE interrupts are enabled.
[1] https://patchwork.kernel.org/patch/9781221/
Cc: Alexander Graf <agraf@suse.de>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Eric Anholt <eric@anholt.net>
Cc: Stefan Wahren <stefan.wahren@i2se.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Ray Jui <rjui@broadcom.com>
Cc: Scott Branden <sbranden@broadcom.com>
Cc: bcm-kernel-feedback-list at broadcom.com
Cc: linux-spi at vger.kernel.org
Cc: linux-rpi-kernel at lists.infradead.org
Cc: linux-arm-kernel at lists.infradead.org
Signed-off-by: Rob Herring <robh@kernel.org>
---
Compile tested only. I'll add something to the related github issue on
this and hopefully someone can test and confirm.
I'm assuming the 8250 driver can handle shared irqs correctly.
Rob
drivers/spi/spi-bcm2835aux.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index 1431cb98fe40..3094d818cf06 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -184,6 +184,11 @@ static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
irqreturn_t ret = IRQ_NONE;
+ /* IRQ may be shared, so return if our interrupts are disabled */
+ if (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_CNTL1) &
+ (BCM2835_AUX_SPI_CNTL1_TXEMPTY | BCM2835_AUX_SPI_CNTL1_IDLE)))
+ return ret;
+
/* check if we have data to read */
while (bs->rx_len &&
(!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
--
2.17.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] spi: bcm2835aux: ensure interrupts are enabled for shared handler
2018-05-03 18:09 ` Rob Herring
@ 2018-05-03 18:31 ` Stefan Wahren
-1 siblings, 0 replies; 14+ messages in thread
From: Stefan Wahren @ 2018-05-03 18:31 UTC (permalink / raw)
To: Martin Sperl, Noralf Trønnes, Phil Elwell
Cc: Rob Herring, Florian Fainelli, Scott Branden, Marc Zyngier,
Ray Jui, Alexander Graf, linux-spi, Eric Anholt, Mark Brown,
bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
Hi,
[added Martin, Noralf and Phil]
> Rob Herring <robh@kernel.org> hat am 3. Mai 2018 um 20:09 geschrieben:
>
>
> The BCM2835 AUX SPI has a shared interrupt line (with AUX UART).
> Downstream fixes this with an AUX irqchip to demux the IRQ sources and a
> DT change which breaks compatibility with older kernels. The AUX irqchip
> was already rejected for upstream[1] and the DT change would break
> working systems if the DTB is updated to a newer one. The latter issue
> was brought to my attention by Alex Graf.
>
> The root cause however is a bug in the shared handler. Shared handlers
> must check that interrupts are actually enabled before servicing the
> interrupt. Add a check that the TXEMPTY or IDLE interrupts are enabled.
>
> [1] https://patchwork.kernel.org/patch/9781221/
>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Stefan Wahren <stefan.wahren@i2se.com>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Ray Jui <rjui@broadcom.com>
> Cc: Scott Branden <sbranden@broadcom.com>
> Cc: bcm-kernel-feedback-list@broadcom.com
> Cc: linux-spi@vger.kernel.org
> Cc: linux-rpi-kernel@lists.infradead.org
> Cc: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> Compile tested only. I'll add something to the related github issue on
> this and hopefully someone can test and confirm.
>
> I'm assuming the 8250 driver can handle shared irqs correctly.
>
> Rob
>
> drivers/spi/spi-bcm2835aux.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> index 1431cb98fe40..3094d818cf06 100644
> --- a/drivers/spi/spi-bcm2835aux.c
> +++ b/drivers/spi/spi-bcm2835aux.c
> @@ -184,6 +184,11 @@ static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
> struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
> irqreturn_t ret = IRQ_NONE;
>
> + /* IRQ may be shared, so return if our interrupts are disabled */
> + if (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_CNTL1) &
> + (BCM2835_AUX_SPI_CNTL1_TXEMPTY | BCM2835_AUX_SPI_CNTL1_IDLE)))
> + return ret;
> +
> /* check if we have data to read */
> while (bs->rx_len &&
> (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
> --
> 2.17.0
does anyone have a setup to test this patch (i assume it requires a compute module)?
@Rob: Thank you very much for this patch
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] spi: bcm2835aux: ensure interrupts are enabled for shared handler
@ 2018-05-03 18:31 ` Stefan Wahren
0 siblings, 0 replies; 14+ messages in thread
From: Stefan Wahren @ 2018-05-03 18:31 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
[added Martin, Noralf and Phil]
> Rob Herring <robh@kernel.org> hat am 3. Mai 2018 um 20:09 geschrieben:
>
>
> The BCM2835 AUX SPI has a shared interrupt line (with AUX UART).
> Downstream fixes this with an AUX irqchip to demux the IRQ sources and a
> DT change which breaks compatibility with older kernels. The AUX irqchip
> was already rejected for upstream[1] and the DT change would break
> working systems if the DTB is updated to a newer one. The latter issue
> was brought to my attention by Alex Graf.
>
> The root cause however is a bug in the shared handler. Shared handlers
> must check that interrupts are actually enabled before servicing the
> interrupt. Add a check that the TXEMPTY or IDLE interrupts are enabled.
>
> [1] https://patchwork.kernel.org/patch/9781221/
>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Stefan Wahren <stefan.wahren@i2se.com>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Ray Jui <rjui@broadcom.com>
> Cc: Scott Branden <sbranden@broadcom.com>
> Cc: bcm-kernel-feedback-list at broadcom.com
> Cc: linux-spi at vger.kernel.org
> Cc: linux-rpi-kernel at lists.infradead.org
> Cc: linux-arm-kernel at lists.infradead.org
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> Compile tested only. I'll add something to the related github issue on
> this and hopefully someone can test and confirm.
>
> I'm assuming the 8250 driver can handle shared irqs correctly.
>
> Rob
>
> drivers/spi/spi-bcm2835aux.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> index 1431cb98fe40..3094d818cf06 100644
> --- a/drivers/spi/spi-bcm2835aux.c
> +++ b/drivers/spi/spi-bcm2835aux.c
> @@ -184,6 +184,11 @@ static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
> struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
> irqreturn_t ret = IRQ_NONE;
>
> + /* IRQ may be shared, so return if our interrupts are disabled */
> + if (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_CNTL1) &
> + (BCM2835_AUX_SPI_CNTL1_TXEMPTY | BCM2835_AUX_SPI_CNTL1_IDLE)))
> + return ret;
> +
> /* check if we have data to read */
> while (bs->rx_len &&
> (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
> --
> 2.17.0
does anyone have a setup to test this patch (i assume it requires a compute module)?
@Rob: Thank you very much for this patch
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] spi: bcm2835aux: ensure interrupts are enabled for shared handler
2018-05-03 18:09 ` Rob Herring
@ 2018-05-03 21:15 ` Eric Anholt
-1 siblings, 0 replies; 14+ messages in thread
From: Eric Anholt @ 2018-05-03 21:15 UTC (permalink / raw)
To: Rob Herring, Mark Brown
Cc: Stefan Wahren, Florian Fainelli, Scott Branden, Marc Zyngier,
Ray Jui, Alexander Graf, linux-spi, bcm-kernel-feedback-list,
linux-rpi-kernel, linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 918 bytes --]
Rob Herring <robh@kernel.org> writes:
> The BCM2835 AUX SPI has a shared interrupt line (with AUX UART).
> Downstream fixes this with an AUX irqchip to demux the IRQ sources and a
> DT change which breaks compatibility with older kernels. The AUX irqchip
> was already rejected for upstream[1] and the DT change would break
> working systems if the DTB is updated to a newer one. The latter issue
> was brought to my attention by Alex Graf.
>
> The root cause however is a bug in the shared handler. Shared handlers
> must check that interrupts are actually enabled before servicing the
> interrupt. Add a check that the TXEMPTY or IDLE interrupts are enabled.
It looks to me like we'd only return IRQ_HANDLED if we did work that
needed doing. Is this check effectively doing some interlock to make
sure that we've already started bcm2835aux_spi_transfer_one_irq() and
aren't just racing against transaction setup?
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] spi: bcm2835aux: ensure interrupts are enabled for shared handler
@ 2018-05-03 21:15 ` Eric Anholt
0 siblings, 0 replies; 14+ messages in thread
From: Eric Anholt @ 2018-05-03 21:15 UTC (permalink / raw)
To: linux-arm-kernel
Rob Herring <robh@kernel.org> writes:
> The BCM2835 AUX SPI has a shared interrupt line (with AUX UART).
> Downstream fixes this with an AUX irqchip to demux the IRQ sources and a
> DT change which breaks compatibility with older kernels. The AUX irqchip
> was already rejected for upstream[1] and the DT change would break
> working systems if the DTB is updated to a newer one. The latter issue
> was brought to my attention by Alex Graf.
>
> The root cause however is a bug in the shared handler. Shared handlers
> must check that interrupts are actually enabled before servicing the
> interrupt. Add a check that the TXEMPTY or IDLE interrupts are enabled.
It looks to me like we'd only return IRQ_HANDLED if we did work that
needed doing. Is this check effectively doing some interlock to make
sure that we've already started bcm2835aux_spi_transfer_one_irq() and
aren't just racing against transaction setup?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180503/566dc920/attachment.sig>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] spi: bcm2835aux: ensure interrupts are enabled for shared handler
2018-05-03 21:15 ` Eric Anholt
@ 2018-05-03 22:06 ` Rob Herring
-1 siblings, 0 replies; 14+ messages in thread
From: Rob Herring @ 2018-05-03 22:06 UTC (permalink / raw)
To: Eric Anholt
Cc: Stefan Wahren, Florian Fainelli, Scott Branden, Marc Zyngier,
Ray Jui, Alexander Graf, linux-spi, Mark Brown,
bcm-kernel-feedback-list,
moderated list:BROADCOM BCM2835 ARM ARCHITECTURE,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
On Thu, May 3, 2018 at 4:15 PM, Eric Anholt <eric@anholt.net> wrote:
> Rob Herring <robh@kernel.org> writes:
>
>> The BCM2835 AUX SPI has a shared interrupt line (with AUX UART).
>> Downstream fixes this with an AUX irqchip to demux the IRQ sources and a
>> DT change which breaks compatibility with older kernels. The AUX irqchip
>> was already rejected for upstream[1] and the DT change would break
>> working systems if the DTB is updated to a newer one. The latter issue
>> was brought to my attention by Alex Graf.
>>
>> The root cause however is a bug in the shared handler. Shared handlers
>> must check that interrupts are actually enabled before servicing the
>> interrupt. Add a check that the TXEMPTY or IDLE interrupts are enabled.
>
> It looks to me like we'd only return IRQ_HANDLED if we did work that
> needed doing. Is this check effectively doing some interlock to make
> sure that we've already started bcm2835aux_spi_transfer_one_irq() and
> aren't just racing against transaction setup?
What if you are in polled mode for the SPI and the 8250 irq (or other
SPI instance) causes the SPI irq handler to run?
The way I look at it is, this change in the irq handler was also said
to fix the problem[1]:
u32 irq_flags = readl(bcm2835_aux_irq_reg);
if(!(irq_flags & (1<<master->bus_num)))
{
return IRQ_NONE;
}
Is checking whether the interrupt is pending in the aux reg any
different than checking for interrupt being enabled in the device? I
could have checked the status bits too, but as you say that is handled
farther down.
Rob
[1] https://github.com/raspberrypi/linux/issues/1573
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH] spi: bcm2835aux: ensure interrupts are enabled for shared handler
@ 2018-05-03 22:06 ` Rob Herring
0 siblings, 0 replies; 14+ messages in thread
From: Rob Herring @ 2018-05-03 22:06 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, May 3, 2018 at 4:15 PM, Eric Anholt <eric@anholt.net> wrote:
> Rob Herring <robh@kernel.org> writes:
>
>> The BCM2835 AUX SPI has a shared interrupt line (with AUX UART).
>> Downstream fixes this with an AUX irqchip to demux the IRQ sources and a
>> DT change which breaks compatibility with older kernels. The AUX irqchip
>> was already rejected for upstream[1] and the DT change would break
>> working systems if the DTB is updated to a newer one. The latter issue
>> was brought to my attention by Alex Graf.
>>
>> The root cause however is a bug in the shared handler. Shared handlers
>> must check that interrupts are actually enabled before servicing the
>> interrupt. Add a check that the TXEMPTY or IDLE interrupts are enabled.
>
> It looks to me like we'd only return IRQ_HANDLED if we did work that
> needed doing. Is this check effectively doing some interlock to make
> sure that we've already started bcm2835aux_spi_transfer_one_irq() and
> aren't just racing against transaction setup?
What if you are in polled mode for the SPI and the 8250 irq (or other
SPI instance) causes the SPI irq handler to run?
The way I look at it is, this change in the irq handler was also said
to fix the problem[1]:
u32 irq_flags = readl(bcm2835_aux_irq_reg);
if(!(irq_flags & (1<<master->bus_num)))
{
return IRQ_NONE;
}
Is checking whether the interrupt is pending in the aux reg any
different than checking for interrupt being enabled in the device? I
could have checked the status bits too, but as you say that is handled
farther down.
Rob
[1] https://github.com/raspberrypi/linux/issues/1573
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] spi: bcm2835aux: ensure interrupts are enabled for shared handler
2018-05-03 22:06 ` Rob Herring
@ 2018-05-03 22:36 ` Eric Anholt
-1 siblings, 0 replies; 14+ messages in thread
From: Eric Anholt @ 2018-05-03 22:36 UTC (permalink / raw)
To: Rob Herring
Cc: Stefan Wahren, Florian Fainelli, Scott Branden, Marc Zyngier,
Ray Jui, Alexander Graf, linux-spi, Mark Brown,
bcm-kernel-feedback-list,
moderated list:BROADCOM BCM2835 ARM ARCHITECTURE,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
[-- Attachment #1.1: Type: text/plain, Size: 1710 bytes --]
Rob Herring <robh@kernel.org> writes:
> On Thu, May 3, 2018 at 4:15 PM, Eric Anholt <eric@anholt.net> wrote:
>> Rob Herring <robh@kernel.org> writes:
>>
>>> The BCM2835 AUX SPI has a shared interrupt line (with AUX UART).
>>> Downstream fixes this with an AUX irqchip to demux the IRQ sources and a
>>> DT change which breaks compatibility with older kernels. The AUX irqchip
>>> was already rejected for upstream[1] and the DT change would break
>>> working systems if the DTB is updated to a newer one. The latter issue
>>> was brought to my attention by Alex Graf.
>>>
>>> The root cause however is a bug in the shared handler. Shared handlers
>>> must check that interrupts are actually enabled before servicing the
>>> interrupt. Add a check that the TXEMPTY or IDLE interrupts are enabled.
>>
>> It looks to me like we'd only return IRQ_HANDLED if we did work that
>> needed doing. Is this check effectively doing some interlock to make
>> sure that we've already started bcm2835aux_spi_transfer_one_irq() and
>> aren't just racing against transaction setup?
>
> What if you are in polled mode for the SPI and the 8250 irq (or other
> SPI instance) causes the SPI irq handler to run?
>
> Is checking whether the interrupt is pending in the aux reg any
> different than checking for interrupt being enabled in the device? I
> could have checked the status bits too, but as you say that is handled
> farther down.
It seems clearly different to me, in that one is about allowing the
interrupt line to go high and the other is about whether the interrupt
line is actually high right now.
However, the polled mode note explains to me what was going wrong, so:
Reviewed-by: Eric Anholt <eric@anholt.net>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] spi: bcm2835aux: ensure interrupts are enabled for shared handler
@ 2018-05-03 22:36 ` Eric Anholt
0 siblings, 0 replies; 14+ messages in thread
From: Eric Anholt @ 2018-05-03 22:36 UTC (permalink / raw)
To: linux-arm-kernel
Rob Herring <robh@kernel.org> writes:
> On Thu, May 3, 2018 at 4:15 PM, Eric Anholt <eric@anholt.net> wrote:
>> Rob Herring <robh@kernel.org> writes:
>>
>>> The BCM2835 AUX SPI has a shared interrupt line (with AUX UART).
>>> Downstream fixes this with an AUX irqchip to demux the IRQ sources and a
>>> DT change which breaks compatibility with older kernels. The AUX irqchip
>>> was already rejected for upstream[1] and the DT change would break
>>> working systems if the DTB is updated to a newer one. The latter issue
>>> was brought to my attention by Alex Graf.
>>>
>>> The root cause however is a bug in the shared handler. Shared handlers
>>> must check that interrupts are actually enabled before servicing the
>>> interrupt. Add a check that the TXEMPTY or IDLE interrupts are enabled.
>>
>> It looks to me like we'd only return IRQ_HANDLED if we did work that
>> needed doing. Is this check effectively doing some interlock to make
>> sure that we've already started bcm2835aux_spi_transfer_one_irq() and
>> aren't just racing against transaction setup?
>
> What if you are in polled mode for the SPI and the 8250 irq (or other
> SPI instance) causes the SPI irq handler to run?
>
> Is checking whether the interrupt is pending in the aux reg any
> different than checking for interrupt being enabled in the device? I
> could have checked the status bits too, but as you say that is handled
> farther down.
It seems clearly different to me, in that one is about allowing the
interrupt line to go high and the other is about whether the interrupt
line is actually high right now.
However, the polled mode note explains to me what was going wrong, so:
Reviewed-by: Eric Anholt <eric@anholt.net>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180503/f9180838/attachment.sig>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] spi: bcm2835aux: ensure interrupts are enabled for shared handler
2018-05-03 18:09 ` Rob Herring
@ 2018-05-03 23:06 ` Mark Brown
-1 siblings, 0 replies; 14+ messages in thread
From: Mark Brown @ 2018-05-03 23:06 UTC (permalink / raw)
To: Rob Herring
Cc: Stefan Wahren, Florian Fainelli, Scott Branden, Marc Zyngier,
Ray Jui, Alexander Graf, linux-spi, Eric Anholt,
bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 657 bytes --]
On Thu, May 03, 2018 at 01:09:44PM -0500, Rob Herring wrote:
> The root cause however is a bug in the shared handler. Shared handlers
> must check that interrupts are actually enabled before servicing the
> interrupt. Add a check that the TXEMPTY or IDLE interrupts are enabled.
The requirement is more that the handler should correctly identify if it
actually handled an interrupt - especially if the driver doesn't enable
and disable the interrupt at runtime it's not going to upset anything to
always run the interrupt handling (and of course some hardware can't
disable things anyway) but if nohing happened then the handler needs to
return IRQ_NONE.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] spi: bcm2835aux: ensure interrupts are enabled for shared handler
@ 2018-05-03 23:06 ` Mark Brown
0 siblings, 0 replies; 14+ messages in thread
From: Mark Brown @ 2018-05-03 23:06 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, May 03, 2018 at 01:09:44PM -0500, Rob Herring wrote:
> The root cause however is a bug in the shared handler. Shared handlers
> must check that interrupts are actually enabled before servicing the
> interrupt. Add a check that the TXEMPTY or IDLE interrupts are enabled.
The requirement is more that the handler should correctly identify if it
actually handled an interrupt - especially if the driver doesn't enable
and disable the interrupt at runtime it's not going to upset anything to
always run the interrupt handling (and of course some hardware can't
disable things anyway) but if nohing happened then the handler needs to
return IRQ_NONE.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180504/fb07c019/attachment.sig>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Applied "spi: bcm2835aux: ensure interrupts are enabled for shared handler" to the spi tree
2018-05-03 18:09 ` Rob Herring
@ 2018-05-03 23:19 ` Mark Brown
-1 siblings, 0 replies; 14+ messages in thread
From: Mark Brown @ 2018-05-03 23:19 UTC (permalink / raw)
To: Rob Herring
Cc: Stefan Wahren, Florian Fainelli, Scott Branden, Marc Zyngier,
Ray Jui, Alexander Graf, linux-spi, Eric Anholt, Mark Brown,
bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
The patch
spi: bcm2835aux: ensure interrupts are enabled for shared handler
has been applied to the spi tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
>From bc519d9574618e47a0c788000fb78da95e18d953 Mon Sep 17 00:00:00 2001
From: Rob Herring <robh@kernel.org>
Date: Thu, 3 May 2018 13:09:44 -0500
Subject: [PATCH] spi: bcm2835aux: ensure interrupts are enabled for shared
handler
The BCM2835 AUX SPI has a shared interrupt line (with AUX UART).
Downstream fixes this with an AUX irqchip to demux the IRQ sources and a
DT change which breaks compatibility with older kernels. The AUX irqchip
was already rejected for upstream[1] and the DT change would break
working systems if the DTB is updated to a newer one. The latter issue
was brought to my attention by Alex Graf.
The root cause however is a bug in the shared handler. Shared handlers
must check that interrupts are actually enabled before servicing the
interrupt. Add a check that the TXEMPTY or IDLE interrupts are enabled.
[1] https://patchwork.kernel.org/patch/9781221/
Cc: Alexander Graf <agraf@suse.de>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Eric Anholt <eric@anholt.net>
Cc: Stefan Wahren <stefan.wahren@i2se.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Ray Jui <rjui@broadcom.com>
Cc: Scott Branden <sbranden@broadcom.com>
Cc: bcm-kernel-feedback-list@broadcom.com
Cc: linux-spi@vger.kernel.org
Cc: linux-rpi-kernel@lists.infradead.org
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Rob Herring <robh@kernel.org>
Reviewed-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
drivers/spi/spi-bcm2835aux.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index 1431cb98fe40..3094d818cf06 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -184,6 +184,11 @@ static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
irqreturn_t ret = IRQ_NONE;
+ /* IRQ may be shared, so return if our interrupts are disabled */
+ if (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_CNTL1) &
+ (BCM2835_AUX_SPI_CNTL1_TXEMPTY | BCM2835_AUX_SPI_CNTL1_IDLE)))
+ return ret;
+
/* check if we have data to read */
while (bs->rx_len &&
(!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
--
2.17.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Applied "spi: bcm2835aux: ensure interrupts are enabled for shared handler" to the spi tree
@ 2018-05-03 23:19 ` Mark Brown
0 siblings, 0 replies; 14+ messages in thread
From: Mark Brown @ 2018-05-03 23:19 UTC (permalink / raw)
To: linux-arm-kernel
The patch
spi: bcm2835aux: ensure interrupts are enabled for shared handler
has been applied to the spi tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
>From bc519d9574618e47a0c788000fb78da95e18d953 Mon Sep 17 00:00:00 2001
From: Rob Herring <robh@kernel.org>
Date: Thu, 3 May 2018 13:09:44 -0500
Subject: [PATCH] spi: bcm2835aux: ensure interrupts are enabled for shared
handler
The BCM2835 AUX SPI has a shared interrupt line (with AUX UART).
Downstream fixes this with an AUX irqchip to demux the IRQ sources and a
DT change which breaks compatibility with older kernels. The AUX irqchip
was already rejected for upstream[1] and the DT change would break
working systems if the DTB is updated to a newer one. The latter issue
was brought to my attention by Alex Graf.
The root cause however is a bug in the shared handler. Shared handlers
must check that interrupts are actually enabled before servicing the
interrupt. Add a check that the TXEMPTY or IDLE interrupts are enabled.
[1] https://patchwork.kernel.org/patch/9781221/
Cc: Alexander Graf <agraf@suse.de>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Eric Anholt <eric@anholt.net>
Cc: Stefan Wahren <stefan.wahren@i2se.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Ray Jui <rjui@broadcom.com>
Cc: Scott Branden <sbranden@broadcom.com>
Cc: bcm-kernel-feedback-list at broadcom.com
Cc: linux-spi at vger.kernel.org
Cc: linux-rpi-kernel at lists.infradead.org
Cc: linux-arm-kernel at lists.infradead.org
Signed-off-by: Rob Herring <robh@kernel.org>
Reviewed-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
drivers/spi/spi-bcm2835aux.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index 1431cb98fe40..3094d818cf06 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -184,6 +184,11 @@ static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
irqreturn_t ret = IRQ_NONE;
+ /* IRQ may be shared, so return if our interrupts are disabled */
+ if (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_CNTL1) &
+ (BCM2835_AUX_SPI_CNTL1_TXEMPTY | BCM2835_AUX_SPI_CNTL1_IDLE)))
+ return ret;
+
/* check if we have data to read */
while (bs->rx_len &&
(!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
--
2.17.0
^ permalink raw reply related [flat|nested] 14+ messages in thread