Re: [PATCH V2 11/15] mmc: sdhci: Change to new way of doing re-tuning

[Ulf Hansson <ulf.hansson@linaro.org>]

[...]

>>> @@ -2834,11 +2763,8 @@ int sdhci_runtime_suspend_host(struct sdhci_host *host)
>>>  {
>>>         unsigned long flags;
>>>
>>> -       /* Disable tuning since we are suspending */
>>> -       if (host->flags & SDHCI_USING_RETUNING_TIMER) {
>>> -               del_timer_sync(&host->tuning_timer);
>>> -               host->flags &= ~SDHCI_NEEDS_RETUNING;
>>> -       }
>>> +       mmc_retune_timer_stop(host->mmc);
>>
>> I think this could give a deadlock.
>>
>> What if the retuning is just about to start and thus sdhci's
>> ->execute_tuning() callback has been invoked, which is waiting for the
>> pm_runtime_get_sync() to return.
>
> The re-tune timer is mmc_retune_timer() and it does not take any locks
> so it can't deadlock.
>

You missed my point. The problem is related to runtime PM.

Here the sequence I think will cause the deadlock.
mmc_retune_timer_stop()
  ->del_timer_sync()
...
Wait for timer-handler to finish.

If the timer-handler is running, it has invoked the ->execute_tuning()
callback and is thus waiting for a pm_runtime_get_sync() to return.

Now, waiting for a pm_runtime_get_sync() to return from a runtime PM
suspend callback will deadlock!

>>
>>> +       mmc_retune_needed(host->mmc);
>>
>> This seems racy.
>>
>> What if a new request has already been started from the mmc core
>> (waiting for sdhci's ->request() callback to return). That would mean
>> the mmc core won't detect that a retune was needed.
>
> That is a good point. The host controller must not runtime suspend after
> re-tuning until retuning is released. I can think of a couple of options:
>         - move the retuning call into the ->request function
>         - add extra host ops for the host to runtime resume/suspend
>

I am not sure which approach I prefer yet. Need some more time to think.

For your information, Neil Brown is having a similar issue which he is
trying to address [1].

I think we need an generic approach to deal with the runtime PM
synchronization issue described above. More precisely in those
scenarios when mmc hosts needs to notify the mmc core to take some
specific actions, from a mmc host's runtime PM callback.

Kind regards
Uffe

[1]
https://lkml.org/lkml/2015/2/23/721