diff mbox series

[v4,01/10] ata: libahci: Ensure the host interrupt status bits are cleared

Message ID 20190521143023.31810-2-miquel.raynal@bootlin.com
State New, archived
Headers show
Series Enable per-port SATA interrupts and drop a hack in the IRQ subsystem | expand

Commit Message

Miquel Raynal May 21, 2019, 2:30 p.m. UTC
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(+)

Comments

raymond pang May 23, 2019, 3:11 a.m. UTC | #1
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
>
Marc Zyngier May 23, 2019, 9:26 a.m. UTC | #2
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
>>
Miquel Raynal May 29, 2019, 10:08 a.m. UTC | #3
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
Marc Zyngier May 29, 2019, 10:37 a.m. UTC | #4
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.
Miquel Raynal May 29, 2019, 12:13 p.m. UTC | #5
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
Marc Zyngier May 29, 2019, 1:33 p.m. UTC | #6
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 mbox series

Patch

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;