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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux