On 24/11/11 20:58, Per Forlin wrote:
> On Sun, Nov 20, 2011 at 9:50 PM, Per Forlin <per.l...@gmail.com> wrote:
>> Hi Adrian,
>>
>> On Fri, Nov 18, 2011 at 10:56 AM, Per Förlin <per.for...@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.for...@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
