On 10/03/15 15:55, Ulf Hansson wrote:
> [...]
>
>>>> @@ -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()
No, the timer handler does not invoke anything. It just sets a flag.
> 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].
It is a bit different. Re-tuning is about doing something before a
request, rather than before a suspend.
> 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.
For the re-tune case I did not want to assume what the host driver
needed to do, so I added ->hold_tuning() and ->release_tuning()
host operations.
Please see V3 of the patches I sent earlier today.
Thanks very much for looking at the patches by the way! :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
