On Fri, Nov 25, 2011 at 1:03 PM, Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
> On 24/11/11 20:58, Per Forlin wrote:
>> On Sun, Nov 20, 2011 at 9:50 PM, Per Forlin <per.lkml@xxxxxxxxx> wrote:
>>> Hi Adrian,
>>>
>>> On Fri, Nov 18, 2011 at 10:56 AM, Per Förlin <per.forlin@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>>>>>> ---
>>>>>> 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.
>
I agree. The intention of this patch is only top let mmc_rescan claim the host.
Flow: card detect IRQ -> mmc_detect_change -> mmc_rescan -> mmc_claim_host
If doing this check in mmc block instead this patch is not needed.
Let's wait and see what comes out of the patch "mmc: Kill block
requests if card is removed".
Thanks,
Per
--
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
