Re: [PATCH] mmc: block: release host in case of error

[Adrian Hunter <adrian.hunter@intel.com>]

On 24/11/11 20:58, Per Forlin wrote:
> On Sun, Nov 20, 2011 at 9:50 PM, Per Forlin <per.lkml@gmail.com> wrote:
>> Hi Adrian,
>>
>> On Fri, Nov 18, 2011 at 10:56 AM, Per Förlin <per.forlin@stericsson.com> wrote:
>>> On 11/17/2011 10:18 AM, Adrian Hunter wrote:
>>>> On 14/11/11 13:12, Per Forlin wrote:
>>>>> Host is claimed as long as there are requests in the block queue
>>>>> and all request are completed successfully. If an error occur release
>>>>> the host in case someone else needs to claim it, for instance if the card
>>>>> is removed during a transfer.
>>>>>
>>>>> Signed-off-by: Per Forlin <per.forlin@stericsson.com>
>>>>> ---
>>>>>  drivers/mmc/card/block.c |   37 +++++++++++++++++++++++++++++--------
>>>>>  1 files changed, 29 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>>>> index c80bb6d..c21fd2c 100644
>>>>> --- a/drivers/mmc/card/block.c
>>>>> +++ b/drivers/mmc/card/block.c
>>>>> @@ -1158,6 +1158,28 @@ static int mmc_blk_cmd_err(struct mmc_blk_data *md, struct mmc_card *card,
>>>>>      return ret;
>>>>>  }
>>>>>
>>>>> +/*
>>>>> + * This function should be called to resend a request after failure.
>>>>> + * Prepares and starts the request.
>>>>> + */
>>>>> +static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
>>>>> +                                               struct mmc_queue *mq,
>>>>> +                                               struct mmc_queue_req *mqrq,
>>>>> +                                               int disable_multi,
>>>>> +                                               struct mmc_async_req *areq)
>>>>> +{
>>>>> +    /*
>>>>> +     * Release host after failure in case the host is needed
>>>>> +     * by someone else. For instance, if the card is removed the
>>>>> +     * worker thread needs to claim the host in order to do mmc_rescan.
>>>>> +     */
>>>>> +    mmc_release_host(card->host);
>>>>> +    mmc_claim_host(card->host);
>>>>
>>>> Does this work?  Won't the current thread win the race
>>>> to claim the host again?
>>>>
>>> Good question. I've tested it and I haven't seen any cases where current has claimed the host again. Sujit has tested the patch as well.
>>> But I can't say that your scenario can't happen. I will study the wake_up and wait_queue code to see if I can find the answer.
>>>
>>
>> mmc_release_host() -> wake_up() -> schedule(). If the waking process
>> has higher prio than current it will preempt current on NOSMP. If SMP,
>> current and waking process may be on separate CPUs and in that case
>> it's difficult to guarantee that the waking process will win the race.
>> I'm proposing to add yield() in order to give the waking process
>> better chances to win the race.
>> Here's a patch:
>> --------------------------------
>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>> index c21fd2c..add1c38 100644
>> --- a/drivers/mmc/card/block.c
>> +++ b/drivers/mmc/card/block.c
>> @@ -1173,8 +1173,11 @@ static inline struct mmc_async_req
>> *mmc_blk_resend(struct mmc_card *card,
>>         * by someone else. For instance, if the card is removed the
>>         * worker thread needs to claim the host in order to do mmc_rescan.
>>         */
>> -       mmc_release_host(card->host);
>> -       mmc_claim_host(card->host);
>> +       if (mmc_card_rescan(card)) {
>> +               mmc_release_host(card->host);
>> +               yield();
>> +               mmc_claim_host(card->host);
>> +       }
>>
>>        mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
>>        return mmc_start_req(card->host, areq, NULL);
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 271efea..83f03a3 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -2059,6 +2059,8 @@ void mmc_rescan(struct work_struct *work)
>>        if (host->rescan_disable)
>>                return;
>>
>> +       mmc_card_set_rescan(host->card);
>> +
>>
>>
>>        /*
>> @@ -2101,6 +2103,7 @@
>>
>>
>>  out:
>> +       mmc_card_clr_rescan(host->card);
>>
>>
>>  }
>> -----------------------
> I'm not sure if this patch-extension is really needed, it may only
> make the patch more complex. If the race condition Adrian refers to is
> unlikely, there may be a few extra retries before mmc_rescan get the
> chance to claim the host.
> I'm in favor of skipping my proposed extension and staying with the
> original v1 patch.
> Adrian, what do you say?

As far as I can see, if mmc block is checking / setting whether the
card has been removed, then mmc_blk_resend would not be needed.