On 29/05/2019 13:13, Miquel Raynal wrote: > 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? 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. -- Jazz is not dead. It just smells funny...
