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?
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
