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
