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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Marc,

Marc Zyngier <marc.zyngier@xxxxxxx> 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@xxxxxxx> 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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux