Re: [PATCH v2 1/1] mmc: sunxi: Disable irq during pm_suspend

[Ulf Hansson]

On 4 July 2018 at 13:34, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> On 04/07/18 11:50, Ulf Hansson wrote:
>> + Marc
>>
>> On 4 July 2018 at 08:28, Stefan Mavrodiev <stefan@xxxxxxxxxx> 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@xxxxxxxxxx>
>>> ---
>>>  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.

>
> A better fix would be to kill the interrupt generation at the source
> (the MMC controller in this particular case) when suspending.

Right. But using disable_irq() doesn't work a general solution, so
something else is needed.

An option is to allow us to use disable|enable_irq() for this
particular case as a simple fix, then work on a long term solution.

Kind regards
Uffe