| Message ID | 20180503180945.3502-1-robh@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | bc519d9574618e47a0c788000fb78da95e18d953 |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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>
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.
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) &
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(+)