Date:   Thu, 5 Jul 2018 14:56:17 +0100
From:   Marc Zyngier <marc.zyngier@....com>
To:     Ulf Hansson <ulf.hansson@...aro.org>
Cc:     Stefan Mavrodiev <stefan@...mex.com>,
        Maxime Ripard <maxime.ripard@...tlin.com>,
        Chen-Yu Tsai <wens@...e.org>,
        "open list:MULTIMEDIA CARD (MMC), SECURE DIGITAL (SD) AND..." 
        <linux-mmc@...r.kernel.org>,
        "moderated list:ARM/Allwinner sunXi SoC support" 
        <linux-arm-kernel@...ts.infradead.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/1] mmc: sunxi: Disable irq during pm_suspend

On 05/07/18 13:07, Ulf Hansson wrote:
> On 5 July 2018 at 13:40, Marc Zyngier <marc.zyngier@....com> wrote:
>> On 05/07/18 12:12, Ulf Hansson wrote:
>>> On 4 July 2018 at 22:29, Marc Zyngier <marc.zyngier@....com> wrote:
>>>> On Wed, 4 Jul 2018 15:34:36 +0200
>>>> Ulf Hansson <ulf.hansson@...aro.org> wrote:
>>>>
>>>>> On 4 July 2018 at 13:34, Marc Zyngier <marc.zyngier@....com> wrote:
>>>>>> On 04/07/18 11:50, Ulf Hansson wrote:
>>>>>>> + Marc
>>>>>>>
>>>>>>> On 4 July 2018 at 08:28, Stefan Mavrodiev <stefan@...mex.com> wrote:
>>>>>>>> When mmc host controller enters suspend state, the clocks are
>>>>>>>> disabled, but irqs are not. For some reason the irqchip emits
>>>>>>>> false interrupts, which causes system lock loop.
>>>>>>>>
>>>>>>>> Debug log is:
>>>>>>>>   ...
>>>>>>>>   sunxi-mmc 1c11000.mmc: setting clk to 52000000, rounded 51200000
>>>>>>>>   sunxi-mmc 1c11000.mmc: enabling the clock
>>>>>>>>   sunxi-mmc 1c11000.mmc: cmd 13(8000014d) arg 10000 ie 0x0000bbc6 len 0
>>>>>>>>   sunxi-mmc 1c11000.mmc: irq: rq (ptrval) mi 00000004 idi 00000000
>>>>>>>>   sunxi-mmc 1c11000.mmc: cmd 6(80000146) arg 3210101 ie 0x0000bbc6 len 0
>>>>>>>>   sunxi-mmc 1c11000.mmc: irq: rq (ptrval) mi 00000004 idi 00000000
>>>>>>>>   sunxi-mmc 1c11000.mmc: cmd 13(8000014d) arg 10000 ie 0x0000bbc6 len 0
>>>>>>>>   sunxi-mmc 1c11000.mmc: irq: rq (ptrval) mi 00000004 idi 00000000
>>>>>>>>   mmc1: new DDR MMC card at address 0001
>>>>>>>>   mmcblk1: mmc1:0001 AGND3R 14.6 GiB
>>>>>>>>   mmcblk1boot0: mmc1:0001 AGND3R partition 1 4.00 MiB
>>>>>>>>   mmcblk1boot1: mmc1:0001 AGND3R partition 2 4.00 MiB
>>>>>>>>   sunxi-mmc 1c11000.mmc: cmd 18(80003352) arg 0 ie 0x0000fbc2 len 409
>>>>>>>>   sunxi-mmc 1c11000.mmc: irq: rq (ptrval) mi 00004000 idi 00000002
>>>>>>>>    mmcblk1: p1
>>>>>>>>   sunxi-mmc 1c11000.mmc: irq: rq   (null) mi 00000000 idi 00000000
>>>>>>>>   sunxi-mmc 1c11000.mmc: irq: rq   (null) mi 00000000 idi 00000000
>>>>>>>>   sunxi-mmc 1c11000.mmc: irq: rq   (null) mi 00000000 idi 00000000
>>>>>>>>   sunxi-mmc 1c11000.mmc: irq: rq   (null) mi 00000000 idi 00000000
>>>>>>>> and so on...
>>>>>>>>
>>>>>>>> This issue apears on eMMC cards, routed on MMC2 slot. The patch is
>>>>>>>> tested with A20-OLinuXino-MICRO/LIME/LIME2 boards.
>>>>>>>>
>>>>>>>> Fixes: 9a8e1e8cc2c0 ("mmc: sunxi: Add runtime_pm support")
>>>>>>>> Signed-off-by: Stefan Mavrodiev <stefan@...mex.com>
>>>>>>>> ---
>>>>>>>>  Changes in v2:
>>>>>>>>   - Add comment why disable_irq() is necessary
>>>>>>>>
>>>>>>>> ---
>>>>>>>>  drivers/mmc/host/sunxi-mmc.c | 7 +++++++
>>>>>>>>  1 file changed, 7 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
>>>>>>>> index e747259..8e7f3e3 100644
>>>>>>>> --- a/drivers/mmc/host/sunxi-mmc.c
>>>>>>>> +++ b/drivers/mmc/host/sunxi-mmc.c
>>>>>>>> @@ -1446,6 +1446,7 @@ static int sunxi_mmc_runtime_resume(struct device *dev)
>>>>>>>>         sunxi_mmc_init_host(host);
>>>>>>>>         sunxi_mmc_set_bus_width(host, mmc->ios.bus_width);
>>>>>>>>         sunxi_mmc_set_clk(host, &mmc->ios);
>>>>>>>> +       enable_irq(host->irq);
>>>>>>>>
>>>>>>>>         return 0;
>>>>>>>>  }
>>>>>>>> @@ -1455,6 +1456,12 @@ static int sunxi_mmc_runtime_suspend(struct device *dev)
>>>>>>>>         struct mmc_host *mmc = dev_get_drvdata(dev);
>>>>>>>>         struct sunxi_mmc_host *host = mmc_priv(mmc);
>>>>>>>>
>>>>>>>> +       /*
>>>>>>>> +        * When clocks are off, it's possible receiving
>>>>>>>> +        * fake interrupts, which will stall the system.
>>>>>>>> +        * Disabling the irq  will prevent this.
>>>>>>>> +        */
>>>>>>>> +       disable_irq(host->irq);
>>>>>>>
>>>>>>> No, this doesn't work for shared IRQs.
>>>>>>
>>>>>> Well, in this case, it does work, because that interrupt line cannot be
>>>>>> shared with anything else, if I understand how the SoC is wired: each
>>>>>> MMC controller has a dedicated interrupt line to the GIC, and it isn't
>>>>>> shared with anything (that's on the A20 though, and I don't know about
>>>>>> other SoCs integrating the same IP).
>>>>>
>>>>> That's the problem. This may work on some SoCs but not on others.
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>         sunxi_mmc_reset_host(host);
>>>>>>>>         sunxi_mmc_disable(host);
>>>>>>>>
>>>>>>>> --
>>>>>>>> 2.7.4
>>>>>>>>
>>>>>>>
>>>>>>> The only option today is to use free_irq() in runtime suspend and then
>>>>>>> re-request the irq to re-install the handler at runtime resume.
>>>>>>>
>>>>>>> That's not an optimal solution, which is pointed out in the below
>>>>>>> discussion as well. Moreover, it has also turned out using free_irq()
>>>>>>> is also problematic in cases threaded handlers are used.
>>>>>>>
>>>>>>> Here's the link to the discussion, it's not the only one I know of, so
>>>>>>> this is common problem.
>>>>>>> https://lkml.org/lkml/2016/1/28/213
>>>>>>>
>>>>>>> Care to have a hack on the "common" solution, which in principle means
>>>>>>> adding APIs to genirq that can disable/enable handlers from being
>>>>>>> called, rather than the entire IRQ line.
>>>>>>
>>>>>> That doesn't work. You still end-up with a screaming interrupt, and you
>>>>>> will still spend 100% of your time in interrupt context for nothing.
>>>>>>
>>>>>> Eventually, the kernel will have enough (the /other/ shared handlers
>>>>>> returning IRQ_NONE all the time), and will forcefully kill that
>>>>>> particular interrupt interrupt line, meaning you end-up in the same
>>>>>> situation of having the line disabled for all the users of that
>>>>>> interrupt line. Except that now, it is disabled forever.
>>>>>
>>>>> Ahh, correct!
>>>>>
>>>>> Sounds like free_irq() is what we need. Only that it's bit heavy
>>>>> weight as we need to re-install handlers.
>>>>
>>>> BTW, free_irq() doesn't help you either in the case of a shared
>>>> handler. You'll end-up in the exact same scenario as above.
>>>
>>> In regards to the spurious interrupt storm issue, yes, I fully agree.
>>>
>>> On the other hand, in case of a shared IRQ, don't we want the genirq
>>> core to deal with disabling the IRQ, rather than the driver?
>>
>> How do you propose we do that? You have an OR gate between two device,
>> and the result of that gate is directly plugged in the interrupt controller.
>>
>> The only thing the genirq subsystem can do is take the interrupt. If
>> nobody cares, the whole interrupt *line* will eventually get disabled.
> 
> Yep, something like that. That would work, right?
> 
>>
>>> Also, don't forget the other related issue, which is when the IRQ
>>> handler gets invoked (not as a storm, but once is enough), either
>>> because of a spurious IRQ or because of a shared IRQ - while the
>>> device is in a low power state (runtime suspended with clock gated for
>>> example). If that happens and the handler accesses a register the
>>> handler may hang.
>>
>> Doing a free_irq() in that case is fine, as long as the rate of spurious
>> interrupts is low.
> 
> Yep.
> 
>>
>>>> The real solution to this is to prevent the device itself from
>>>> generating interrupts (or to forbid interrupt sharing if it isn't
>>>> possible).
>>>
>>> I fully agree that the device should be configured to not deliver
>>> interrupt, this is the first and most important step a driver should
>>> take. For example it should mask its device's IRQ register bits.
>>>
>>> However, this isn't sufficient, because of shared IRQs and buggy HWs
>>> delivering spurious IRQs.
>>
>> It *is* sufficient for shared IRQs. Actually, it is the only way to
>> sanely implement shared IRQs (you must gate the interrupt upstream of
>> the summing interrupt controller). Buggy HW is another story (and that's
>> probably the case here).
>>
>> Now: can we please get this patch merged? ;-)
> 
> Right, I have applied it for fixes!

Thanks a lot for that.

> Thanks for the discussion! However it would be nice to reach a
> conclusion for the problem generically.

The only thing I can come up with is to have a requester-specific
callback that would get called when doing a requester-specific
disable_irq(). This callback would have to disable the interrupt at the
source level, instead of doing it at the irqchip level (and would only
make sense for shared interrupts).

You'd need a per-action refcount so that enable/disable can nest, and
some new APIs to request, enable and disable specific actions.

I could look into it if there would be more than one user...

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Powered by blists - more mailing lists