| Message ID | 20190521143023.31810-2-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable per-port SATA interrupts and drop a hack in the IRQ subsystem | expand |
Hi Miquel, This patch adds clearing GHC.IS into hot path, could you explain how irq storm is generated? thanks According to AHCI Spec, HBA should not refer to GHC.IS to generate MSI when applying multiple MSIs. Best Regards, Raymond On Tue, May 21, 2019 at 2:31 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > ahci_multi_irqs_intr_hard() is going to be used as interrupt handler > to support SATA per-port interrupts. The current logic is to check and > clear the SATA port interrupt status register only. To avoid spurious > IRQs and interrupt storms, it will be needed to clear the port > interrupt bit in the host interrupt status register as well. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/ata/libahci.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c > index 692782dddc0f..9db6f488db59 100644 > --- a/drivers/ata/libahci.c > +++ b/drivers/ata/libahci.c > @@ -1912,7 +1912,10 @@ static void ahci_port_intr(struct ata_port *ap) > static irqreturn_t ahci_multi_irqs_intr_hard(int irq, void *dev_instance) > { > struct ata_port *ap = dev_instance; > + struct ata_host *host = ap->host; > + struct ahci_host_priv *hpriv = host->private_data; > void __iomem *port_mmio = ahci_port_base(ap); > + void __iomem *mmio = hpriv->mmio; > u32 status; > > VPRINTK("ENTER\n"); > @@ -1924,6 +1927,8 @@ static irqreturn_t ahci_multi_irqs_intr_hard(int irq, void *dev_instance) > ahci_handle_port_interrupt(ap, port_mmio, status); > spin_unlock(ap->lock); > > + writel(BIT(ap->port_no), mmio + HOST_IRQ_STAT); > + > VPRINTK("EXIT\n"); > > return IRQ_HANDLED; > -- > 2.19.1 >
On 23/05/2019 04:11, raymond pang wrote: > Hi Miquel, > > This patch adds clearing GHC.IS into hot path, could you explain how > irq storm is generated? thanks > According to AHCI Spec, HBA should not refer to GHC.IS to generate > MSI when applying multiple MSIs. Well spotted. I have the ugly feeling that this is because the Marvell AHCI implementation is not using MSIs at all, but instead a pair of wired interrupts (which are level triggered instead of edge, hence the screaming interrupts). The changes in the following patches abuse the rest of the driver by pretending this is a a multi-MSI setup, while it clearly doesn't match the expectation of the AHCI spec for MSIs. It looks like this shouldn't be imposed on other unsuspecting implementations which correctly use edge-triggered MSIs and do not require such an MMIO access. Thanks, M. > > Best Regards, > Raymond > > On Tue, May 21, 2019 at 2:31 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: >> >> ahci_multi_irqs_intr_hard() is going to be used as interrupt handler >> to support SATA per-port interrupts. The current logic is to check and >> clear the SATA port interrupt status register only. To avoid spurious >> IRQs and interrupt storms, it will be needed to clear the port >> interrupt bit in the host interrupt status register as well. >> >> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> >> --- >> drivers/ata/libahci.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c >> index 692782dddc0f..9db6f488db59 100644 >> --- a/drivers/ata/libahci.c >> +++ b/drivers/ata/libahci.c >> @@ -1912,7 +1912,10 @@ static void ahci_port_intr(struct ata_port *ap) >> static irqreturn_t ahci_multi_irqs_intr_hard(int irq, void *dev_instance) >> { >> struct ata_port *ap = dev_instance; >> + struct ata_host *host = ap->host; >> + struct ahci_host_priv *hpriv = host->private_data; >> void __iomem *port_mmio = ahci_port_base(ap); >> + void __iomem *mmio = hpriv->mmio; >> u32 status; >> >> VPRINTK("ENTER\n"); >> @@ -1924,6 +1927,8 @@ static irqreturn_t ahci_multi_irqs_intr_hard(int irq, void *dev_instance) >> ahci_handle_port_interrupt(ap, port_mmio, status); >> spin_unlock(ap->lock); >> >> + writel(BIT(ap->port_no), mmio + HOST_IRQ_STAT); >> + >> VPRINTK("EXIT\n"); >> >> return IRQ_HANDLED; >> -- >> 2.19.1 >>
Hi Marc & Raymond, Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 23 May 2019 10:26:01 +0100: > On 23/05/2019 04:11, raymond pang wrote: > > Hi Miquel, > > > > This patch adds clearing GHC.IS into hot path, could you explain how > > irq storm is generated? thanks > > According to AHCI Spec, HBA should not refer to GHC.IS to generate > > MSI when applying multiple MSIs. > > Well spotted. > > I have the ugly feeling that this is because the Marvell AHCI > implementation is not using MSIs at all, but instead a pair of wired > interrupts (which are level triggered instead of edge, hence the > screaming interrupts). > > The changes in the following patches abuse the rest of the driver by > pretending this is a a multi-MSI setup, while it clearly doesn't match > the expectation of the AHCI spec for MSIs. > > It looks like this shouldn't be imposed on other unsuspecting > implementations which correctly use edge-triggered MSIs and do not > require such an MMIO access. I understand your concern, let me add a AHCI_HFLAG_LEVEL_MSI in hpriv->flags which will be used by the mvebu_ahci.c driver to request for this MMIO access. This way, the hot path remains the same. Thanks, Miquèl
On 29/05/2019 11:08, Miquel Raynal wrote: > Hi Marc & Raymond, > > Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 23 May 2019 10:26:01 > +0100: > >> On 23/05/2019 04:11, raymond pang wrote: >>> Hi Miquel, >>> >>> This patch adds clearing GHC.IS into hot path, could you explain how >>> irq storm is generated? thanks >>> According to AHCI Spec, HBA should not refer to GHC.IS to generate >>> MSI when applying multiple MSIs. >> >> Well spotted. >> >> I have the ugly feeling that this is because the Marvell AHCI >> implementation is not using MSIs at all, but instead a pair of wired >> interrupts (which are level triggered instead of edge, hence the >> screaming interrupts). >> >> The changes in the following patches abuse the rest of the driver by >> pretending this is a a multi-MSI setup, while it clearly doesn't match >> the expectation of the AHCI spec for MSIs. >> >> It looks like this shouldn't be imposed on other unsuspecting >> implementations which correctly use edge-triggered MSIs and do not >> require such an MMIO access. > > I understand your concern, let me add a AHCI_HFLAG_LEVEL_MSI in > hpriv->flags which will be used by the mvebu_ahci.c driver to request > for this MMIO access. This way, the hot path remains the same. I'm not convinced that's a good idea, if only because from the PoV of the AHCI device itself, these are not MSIs at all, but wired interrupts. The fact that there is some glue logic in the middle that turns it into a message (and then back into a wire) is a regrettable implementation detail. I'd rather you stick to the normal interrupt handler, or provide your own, which would solve most problems. Thanks, M.
Hi Marc, Marc Zyngier <marc.zyngier@arm.com> wrote on Wed, 29 May 2019 11:37:58 +0100: > On 29/05/2019 11:08, Miquel Raynal wrote: > > Hi Marc & Raymond, > > > > Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 23 May 2019 10:26:01 > > +0100: > > > >> On 23/05/2019 04:11, raymond pang wrote: > >>> Hi Miquel, > >>> > >>> This patch adds clearing GHC.IS into hot path, could you explain how > >>> irq storm is generated? thanks > >>> According to AHCI Spec, HBA should not refer to GHC.IS to generate > >>> MSI when applying multiple MSIs. > >> > >> Well spotted. > >> > >> I have the ugly feeling that this is because the Marvell AHCI > >> implementation is not using MSIs at all, but instead a pair of wired > >> interrupts (which are level triggered instead of edge, hence the > >> screaming interrupts). > >> > >> The changes in the following patches abuse the rest of the driver by > >> pretending this is a a multi-MSI setup, while it clearly doesn't match > >> the expectation of the AHCI spec for MSIs. > >> > >> It looks like this shouldn't be imposed on other unsuspecting > >> implementations which correctly use edge-triggered MSIs and do not > >> require such an MMIO access. > > > > I understand your concern, let me add a AHCI_HFLAG_LEVEL_MSI in > > hpriv->flags which will be used by the mvebu_ahci.c driver to request > > for this MMIO access. This way, the hot path remains the same. > > I'm not convinced that's a good idea, if only because from the PoV of > the AHCI device itself, these are not MSIs at all, but wired interrupts. > The fact that there is some glue logic in the middle that turns it into > a message (and then back into a wire) is a regrettable implementation > detail. > > I'd rather you stick to the normal interrupt handler, or provide your > own, which would solve most problems. Unless I don't understand your proposal, "stick to the normal interrupt handler" is not possible as I need this register write to happen at this time, not at any other moment. However, on the possibility of having a separate interrupt handler, I may use the new AHCI_HFALG_LEVEL_MSI flag to change the devm_request_irq call here [1] and use my own at this moment. The handler will be in libahci.c though. Would this be a better approach? Thanks, Miquèl [1] https://elixir.bootlin.com/linux/v5.2-rc2/source/drivers/ata/libahci.c#L2557
On 29/05/2019 13:13, Miquel Raynal wrote: > Hi Marc, > > Marc Zyngier <marc.zyngier@arm.com> wrote on Wed, 29 May 2019 11:37:58 > +0100: > >> On 29/05/2019 11:08, Miquel Raynal wrote: >>> Hi Marc & Raymond, >>> >>> Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 23 May 2019 10:26:01 >>> +0100: >>> >>>> On 23/05/2019 04:11, raymond pang wrote: >>>>> Hi Miquel, >>>>> >>>>> This patch adds clearing GHC.IS into hot path, could you explain how >>>>> irq storm is generated? thanks >>>>> According to AHCI Spec, HBA should not refer to GHC.IS to generate >>>>> MSI when applying multiple MSIs. >>>> >>>> Well spotted. >>>> >>>> I have the ugly feeling that this is because the Marvell AHCI >>>> implementation is not using MSIs at all, but instead a pair of wired >>>> interrupts (which are level triggered instead of edge, hence the >>>> screaming interrupts). >>>> >>>> The changes in the following patches abuse the rest of the driver by >>>> pretending this is a a multi-MSI setup, while it clearly doesn't match >>>> the expectation of the AHCI spec for MSIs. >>>> >>>> It looks like this shouldn't be imposed on other unsuspecting >>>> implementations which correctly use edge-triggered MSIs and do not >>>> require such an MMIO access. >>> >>> I understand your concern, let me add a AHCI_HFLAG_LEVEL_MSI in >>> hpriv->flags which will be used by the mvebu_ahci.c driver to request >>> for this MMIO access. This way, the hot path remains the same. >> >> I'm not convinced that's a good idea, if only because from the PoV of >> the AHCI device itself, these are not MSIs at all, but wired interrupts. >> The fact that there is some glue logic in the middle that turns it into >> a message (and then back into a wire) is a regrettable implementation >> detail. >> >> I'd rather you stick to the normal interrupt handler, or provide your >> own, which would solve most problems. > > Unless I don't understand your proposal, "stick to the normal interrupt > handler" is not possible as I need this register write to happen at > this time, not at any other moment. > > However, on the possibility of having a separate interrupt handler, I > may use the new AHCI_HFALG_LEVEL_MSI flag to change the > devm_request_irq call here [1] and use my own at this moment. The > handler will be in libahci.c though. > > Would this be a better approach? Again, level-triggered MSIs are not a property of the AHCI block. If anything, that's a property of the ICU block. Please do not conflate the two. What you have here is a set of wired interrupts, one per port. I'd suggest you implement it a separate interrupt handler keyed on a particular flag if you cannot detect this case by other means. Thanks, M.
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c index 692782dddc0f..9db6f488db59 100644 --- a/drivers/ata/libahci.c +++ b/drivers/ata/libahci.c @@ -1912,7 +1912,10 @@ static void ahci_port_intr(struct ata_port *ap) static irqreturn_t ahci_multi_irqs_intr_hard(int irq, void *dev_instance) { struct ata_port *ap = dev_instance; + struct ata_host *host = ap->host; + struct ahci_host_priv *hpriv = host->private_data; void __iomem *port_mmio = ahci_port_base(ap); + void __iomem *mmio = hpriv->mmio; u32 status; VPRINTK("ENTER\n"); @@ -1924,6 +1927,8 @@ static irqreturn_t ahci_multi_irqs_intr_hard(int irq, void *dev_instance) ahci_handle_port_interrupt(ap, port_mmio, status); spin_unlock(ap->lock); + writel(BIT(ap->port_no), mmio + HOST_IRQ_STAT); + VPRINTK("EXIT\n"); return IRQ_HANDLED;
ahci_multi_irqs_intr_hard() is going to be used as interrupt handler to support SATA per-port interrupts. The current logic is to check and clear the SATA port interrupt status register only. To avoid spurious IRQs and interrupt storms, it will be needed to clear the port interrupt bit in the host interrupt status register as well. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/ata/libahci.c | 5 +++++ 1 file changed, 5 insertions(+)