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]

 



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...



[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