Re: [PATCH] mmc: block: release host in case of error
On Fri, Nov 25, 2011 at 1:03 PM, Adrian Hunter wrote:
> On 24/11/11 20:58, Per Forlin wrote:
>> On Sun, Nov 20, 2011 at 9:50 PM, Per Forlin wrote:
>>> Hi Adrian,
>>>
>>> On Fri, Nov 18, 2011 at 10:56 AM, Per Förlin
>>> 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
>> ---
>> 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
Re: [PATCH] mmc: block: release host in case of error
On 24/11/11 20:58, Per Forlin wrote:
> On Sun, Nov 20, 2011 at 9:50 PM, Per Forlin wrote:
>> Hi Adrian,
>>
>> On Fri, Nov 18, 2011 at 10:56 AM, Per Förlin
>> 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
> ---
> 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
Re: [PATCH] mmc: block: release host in case of error
On Sun, Nov 20, 2011 at 9:50 PM, Per Forlin wrote:
> Hi Adrian,
>
> On Fri, Nov 18, 2011 at 10:56 AM, Per Förlin
> 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
---
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: block: release host in case of error
Hi Adrian,
On Fri, Nov 18, 2011 at 10:56 AM, Per Förlin 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
>>> ---
>>> 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);
}
---
Thanks,
Per
--
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
Re: [PATCH] mmc: block: release host in case of error
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
>> ---
>> 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.
Thanks,
Per
--
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
Re: [PATCH] mmc: block: release host in case of error
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
> ---
> 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?
> +
> + mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
> + return mmc_start_req(card->host, areq, NULL);
> +}
> +
> static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
> {
> struct mmc_blk_data *md = mq->data;
> @@ -1257,14 +1279,14 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq,
> struct request *rqc)
> break;
> }
>
> - if (ret) {
> + if (ret)
> /*
>* In case of a incomplete request
>* prepare it again and resend.
>*/
> - mmc_blk_rw_rq_prep(mq_rq, card, disable_multi, mq);
> - mmc_start_req(card->host, &mq_rq->mmc_active, NULL);
> - }
> + mmc_blk_resend(card, mq, mq_rq, disable_multi,
> +&mq_rq->mmc_active);
> +
> } while (ret);
>
> return 1;
> @@ -1276,10 +1298,9 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq,
> struct request *rqc)
> spin_unlock_irq(&md->lock);
>
> start_new_req:
> - if (rqc) {
> - mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
> - mmc_start_req(card->host, &mq->mqrq_cur->mmc_active, NULL);
> - }
> + if (rqc)
> + mmc_blk_resend(card, mq, mq->mqrq_cur, 0,
> +&mq->mqrq_cur->mmc_active);
>
> return 0;
> }
--
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
Re: [PATCH] mmc: block: release host in case of error
On Mon, Nov 14, 2011 at 12:12 PM, 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 Acked-by: Linus Walleij Thanks, Linus Walleij -- 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
Re: [PATCH] mmc: block: release host in case of error
On 11/14/2011 4:42 PM, 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 --- drivers/mmc/card/block.c | 37 + 1 files changed, 29 insertions(+), 8 deletions(-) Tested-by: Sujit Reddy Thumma Thanks Sujit -- 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
