From: Pradeep P V K <[email protected]>
For data read commands, SDHC may initiate data transfers even before it
completely process the command response. In case command itself fails,
driver un-maps the memory associated with data transfer but this memory
can still be accessed by SDHC for the already initiated data transfer.
This scenario can lead to un-mapped memory access error.
To avoid this scenario, reset SDHC (when command fails) prior to
un-mapping memory. Resetting SDHC ensures that all in-flight data
transfers are either aborted or completed. So we don't run into this
scenario.
Swap the reset, un-map steps sequence in sdhci_request_done().
Changes since V1:
- Added an empty line and fixed the comment style.
- Retained the Acked-by signoff.
Suggested-by: Veerabhadrarao Badiganti <[email protected]>
Signed-off-by: Pradeep P V K <[email protected]>
Acked-by: Adrian Hunter <[email protected]>
---
drivers/mmc/host/sdhci.c | 60 +++++++++++++++++++++++++-----------------------
1 file changed, 31 insertions(+), 29 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 646823d..130fd2d 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2998,6 +2998,37 @@ static bool sdhci_request_done(struct sdhci_host *host)
}
/*
+ * The controller needs a reset of internal state machines
+ * upon error conditions.
+ */
+ if (sdhci_needs_reset(host, mrq)) {
+ /*
+ * Do not finish until command and data lines are available for
+ * reset. Note there can only be one other mrq, so it cannot
+ * also be in mrqs_done, otherwise host->cmd and host->data_cmd
+ * would both be null.
+ */
+ if (host->cmd || host->data_cmd) {
+ spin_unlock_irqrestore(&host->lock, flags);
+ return true;
+ }
+
+ /* Some controllers need this kick or reset won't work here */
+ if (host->quirks & SDHCI_QUIRK_CLOCK_BEFORE_RESET)
+ /* This is to force an update */
+ host->ops->set_clock(host, host->clock);
+
+ /*
+ * Spec says we should do both at the same time, but Ricoh
+ * controllers do not like that.
+ */
+ sdhci_do_reset(host, SDHCI_RESET_CMD);
+ sdhci_do_reset(host, SDHCI_RESET_DATA);
+
+ host->pending_reset = false;
+ }
+
+ /*
* Always unmap the data buffers if they were mapped by
* sdhci_prepare_data() whenever we finish with a request.
* This avoids leaking DMA mappings on error.
@@ -3060,35 +3091,6 @@ static bool sdhci_request_done(struct sdhci_host *host)
}
}
- /*
- * The controller needs a reset of internal state machines
- * upon error conditions.
- */
- if (sdhci_needs_reset(host, mrq)) {
- /*
- * Do not finish until command and data lines are available for
- * reset. Note there can only be one other mrq, so it cannot
- * also be in mrqs_done, otherwise host->cmd and host->data_cmd
- * would both be null.
- */
- if (host->cmd || host->data_cmd) {
- spin_unlock_irqrestore(&host->lock, flags);
- return true;
- }
-
- /* Some controllers need this kick or reset won't work here */
- if (host->quirks & SDHCI_QUIRK_CLOCK_BEFORE_RESET)
- /* This is to force an update */
- host->ops->set_clock(host, host->clock);
-
- /* Spec says we should do both at the same time, but Ricoh
- controllers do not like that. */
- sdhci_do_reset(host, SDHCI_RESET_CMD);
- sdhci_do_reset(host, SDHCI_RESET_DATA);
-
- host->pending_reset = false;
- }
-
host->mrqs_done[i] = NULL;
spin_unlock_irqrestore(&host->lock, flags);
--
2.7.4
On Wed, 3 Mar 2021 at 09:32, Pradeep P V K <[email protected]> wrote:
>
> From: Pradeep P V K <[email protected]>
>
> For data read commands, SDHC may initiate data transfers even before it
> completely process the command response. In case command itself fails,
> driver un-maps the memory associated with data transfer but this memory
> can still be accessed by SDHC for the already initiated data transfer.
> This scenario can lead to un-mapped memory access error.
>
> To avoid this scenario, reset SDHC (when command fails) prior to
> un-mapping memory. Resetting SDHC ensures that all in-flight data
> transfers are either aborted or completed. So we don't run into this
> scenario.
>
> Swap the reset, un-map steps sequence in sdhci_request_done().
>
> Changes since V1:
> - Added an empty line and fixed the comment style.
> - Retained the Acked-by signoff.
>
> Suggested-by: Veerabhadrarao Badiganti <[email protected]>
> Signed-off-by: Pradeep P V K <[email protected]>
> Acked-by: Adrian Hunter <[email protected]>
Seems like it might be a good idea to tag this for stable? I did that,
but awaiting for your confirmation.
So, applied for next, thanks!
Kind regards
Uffe
> ---
> drivers/mmc/host/sdhci.c | 60 +++++++++++++++++++++++++-----------------------
> 1 file changed, 31 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 646823d..130fd2d 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2998,6 +2998,37 @@ static bool sdhci_request_done(struct sdhci_host *host)
> }
>
> /*
> + * The controller needs a reset of internal state machines
> + * upon error conditions.
> + */
> + if (sdhci_needs_reset(host, mrq)) {
> + /*
> + * Do not finish until command and data lines are available for
> + * reset. Note there can only be one other mrq, so it cannot
> + * also be in mrqs_done, otherwise host->cmd and host->data_cmd
> + * would both be null.
> + */
> + if (host->cmd || host->data_cmd) {
> + spin_unlock_irqrestore(&host->lock, flags);
> + return true;
> + }
> +
> + /* Some controllers need this kick or reset won't work here */
> + if (host->quirks & SDHCI_QUIRK_CLOCK_BEFORE_RESET)
> + /* This is to force an update */
> + host->ops->set_clock(host, host->clock);
> +
> + /*
> + * Spec says we should do both at the same time, but Ricoh
> + * controllers do not like that.
> + */
> + sdhci_do_reset(host, SDHCI_RESET_CMD);
> + sdhci_do_reset(host, SDHCI_RESET_DATA);
> +
> + host->pending_reset = false;
> + }
> +
> + /*
> * Always unmap the data buffers if they were mapped by
> * sdhci_prepare_data() whenever we finish with a request.
> * This avoids leaking DMA mappings on error.
> @@ -3060,35 +3091,6 @@ static bool sdhci_request_done(struct sdhci_host *host)
> }
> }
>
> - /*
> - * The controller needs a reset of internal state machines
> - * upon error conditions.
> - */
> - if (sdhci_needs_reset(host, mrq)) {
> - /*
> - * Do not finish until command and data lines are available for
> - * reset. Note there can only be one other mrq, so it cannot
> - * also be in mrqs_done, otherwise host->cmd and host->data_cmd
> - * would both be null.
> - */
> - if (host->cmd || host->data_cmd) {
> - spin_unlock_irqrestore(&host->lock, flags);
> - return true;
> - }
> -
> - /* Some controllers need this kick or reset won't work here */
> - if (host->quirks & SDHCI_QUIRK_CLOCK_BEFORE_RESET)
> - /* This is to force an update */
> - host->ops->set_clock(host, host->clock);
> -
> - /* Spec says we should do both at the same time, but Ricoh
> - controllers do not like that. */
> - sdhci_do_reset(host, SDHCI_RESET_CMD);
> - sdhci_do_reset(host, SDHCI_RESET_DATA);
> -
> - host->pending_reset = false;
> - }
> -
> host->mrqs_done[i] = NULL;
>
> spin_unlock_irqrestore(&host->lock, flags);
> --
> 2.7.4
>
On 2021-03-04 19:19, Ulf Hansson wrote:
> On Wed, 3 Mar 2021 at 09:32, Pradeep P V K <[email protected]>
> wrote:
>>
>> From: Pradeep P V K <[email protected]>
>>
>> For data read commands, SDHC may initiate data transfers even before
>> it
>> completely process the command response. In case command itself fails,
>> driver un-maps the memory associated with data transfer but this
>> memory
>> can still be accessed by SDHC for the already initiated data transfer.
>> This scenario can lead to un-mapped memory access error.
>>
>> To avoid this scenario, reset SDHC (when command fails) prior to
>> un-mapping memory. Resetting SDHC ensures that all in-flight data
>> transfers are either aborted or completed. So we don't run into this
>> scenario.
>>
>> Swap the reset, un-map steps sequence in sdhci_request_done().
>>
>> Changes since V1:
>> - Added an empty line and fixed the comment style.
>> - Retained the Acked-by signoff.
>>
>> Suggested-by: Veerabhadrarao Badiganti <[email protected]>
>> Signed-off-by: Pradeep P V K <[email protected]>
>> Acked-by: Adrian Hunter <[email protected]>
Hi Uffe,
>
> Seems like it might be a good idea to tag this for stable? I did that,
> but awaiting for your confirmation.
>
Yes, this fix is applicable for all stable starting from 4.9 (n/a for
4.4).
Kindly go ahead.
> So, applied for next, thanks!
>
> Kind regards
> Uffe
>
Thanks and Regards,
Pradeep
>
>> ---
>> drivers/mmc/host/sdhci.c | 60
>> +++++++++++++++++++++++++-----------------------
>> 1 file changed, 31 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 646823d..130fd2d 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -2998,6 +2998,37 @@ static bool sdhci_request_done(struct
>> sdhci_host *host)
>> }
>>
>> /*
>> + * The controller needs a reset of internal state machines
>> + * upon error conditions.
>> + */
>> + if (sdhci_needs_reset(host, mrq)) {
>> + /*
>> + * Do not finish until command and data lines are
>> available for
>> + * reset. Note there can only be one other mrq, so it
>> cannot
>> + * also be in mrqs_done, otherwise host->cmd and
>> host->data_cmd
>> + * would both be null.
>> + */
>> + if (host->cmd || host->data_cmd) {
>> + spin_unlock_irqrestore(&host->lock, flags);
>> + return true;
>> + }
>> +
>> + /* Some controllers need this kick or reset won't work
>> here */
>> + if (host->quirks & SDHCI_QUIRK_CLOCK_BEFORE_RESET)
>> + /* This is to force an update */
>> + host->ops->set_clock(host, host->clock);
>> +
>> + /*
>> + * Spec says we should do both at the same time, but
>> Ricoh
>> + * controllers do not like that.
>> + */
>> + sdhci_do_reset(host, SDHCI_RESET_CMD);
>> + sdhci_do_reset(host, SDHCI_RESET_DATA);
>> +
>> + host->pending_reset = false;
>> + }
>> +
>> + /*
>> * Always unmap the data buffers if they were mapped by
>> * sdhci_prepare_data() whenever we finish with a request.
>> * This avoids leaking DMA mappings on error.
>> @@ -3060,35 +3091,6 @@ static bool sdhci_request_done(struct
>> sdhci_host *host)
>> }
>> }
>>
>> - /*
>> - * The controller needs a reset of internal state machines
>> - * upon error conditions.
>> - */
>> - if (sdhci_needs_reset(host, mrq)) {
>> - /*
>> - * Do not finish until command and data lines are
>> available for
>> - * reset. Note there can only be one other mrq, so it
>> cannot
>> - * also be in mrqs_done, otherwise host->cmd and
>> host->data_cmd
>> - * would both be null.
>> - */
>> - if (host->cmd || host->data_cmd) {
>> - spin_unlock_irqrestore(&host->lock, flags);
>> - return true;
>> - }
>> -
>> - /* Some controllers need this kick or reset won't work
>> here */
>> - if (host->quirks & SDHCI_QUIRK_CLOCK_BEFORE_RESET)
>> - /* This is to force an update */
>> - host->ops->set_clock(host, host->clock);
>> -
>> - /* Spec says we should do both at the same time, but
>> Ricoh
>> - controllers do not like that. */
>> - sdhci_do_reset(host, SDHCI_RESET_CMD);
>> - sdhci_do_reset(host, SDHCI_RESET_DATA);
>> -
>> - host->pending_reset = false;
>> - }
>> -
>> host->mrqs_done[i] = NULL;
>>
>> spin_unlock_irqrestore(&host->lock, flags);
>> --
>> 2.7.4
>>
On Thu, 4 Mar 2021 at 16:16, <[email protected]> wrote:
>
> On 2021-03-04 19:19, Ulf Hansson wrote:
> > On Wed, 3 Mar 2021 at 09:32, Pradeep P V K <[email protected]>
> > wrote:
> >>
> >> From: Pradeep P V K <[email protected]>
> >>
> >> For data read commands, SDHC may initiate data transfers even before
> >> it
> >> completely process the command response. In case command itself fails,
> >> driver un-maps the memory associated with data transfer but this
> >> memory
> >> can still be accessed by SDHC for the already initiated data transfer.
> >> This scenario can lead to un-mapped memory access error.
> >>
> >> To avoid this scenario, reset SDHC (when command fails) prior to
> >> un-mapping memory. Resetting SDHC ensures that all in-flight data
> >> transfers are either aborted or completed. So we don't run into this
> >> scenario.
> >>
> >> Swap the reset, un-map steps sequence in sdhci_request_done().
> >>
> >> Changes since V1:
> >> - Added an empty line and fixed the comment style.
> >> - Retained the Acked-by signoff.
> >>
> >> Suggested-by: Veerabhadrarao Badiganti <[email protected]>
> >> Signed-off-by: Pradeep P V K <[email protected]>
> >> Acked-by: Adrian Hunter <[email protected]>
>
> Hi Uffe,
> >
> > Seems like it might be a good idea to tag this for stable? I did that,
> > but awaiting for your confirmation.
> >
> Yes, this fix is applicable for all stable starting from 4.9 (n/a for
> 4.4).
> Kindly go ahead.
>
> > So, applied for next, thanks!
> >
> > Kind regards
> > Uffe
> >
> Thanks and Regards,
> Pradeep
Thanks for confirming, I have updated the stable tag.
Kind regards
Uffe
>
> >
> >> ---
> >> drivers/mmc/host/sdhci.c | 60
> >> +++++++++++++++++++++++++-----------------------
> >> 1 file changed, 31 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> >> index 646823d..130fd2d 100644
> >> --- a/drivers/mmc/host/sdhci.c
> >> +++ b/drivers/mmc/host/sdhci.c
> >> @@ -2998,6 +2998,37 @@ static bool sdhci_request_done(struct
> >> sdhci_host *host)
> >> }
> >>
> >> /*
> >> + * The controller needs a reset of internal state machines
> >> + * upon error conditions.
> >> + */
> >> + if (sdhci_needs_reset(host, mrq)) {
> >> + /*
> >> + * Do not finish until command and data lines are
> >> available for
> >> + * reset. Note there can only be one other mrq, so it
> >> cannot
> >> + * also be in mrqs_done, otherwise host->cmd and
> >> host->data_cmd
> >> + * would both be null.
> >> + */
> >> + if (host->cmd || host->data_cmd) {
> >> + spin_unlock_irqrestore(&host->lock, flags);
> >> + return true;
> >> + }
> >> +
> >> + /* Some controllers need this kick or reset won't work
> >> here */
> >> + if (host->quirks & SDHCI_QUIRK_CLOCK_BEFORE_RESET)
> >> + /* This is to force an update */
> >> + host->ops->set_clock(host, host->clock);
> >> +
> >> + /*
> >> + * Spec says we should do both at the same time, but
> >> Ricoh
> >> + * controllers do not like that.
> >> + */
> >> + sdhci_do_reset(host, SDHCI_RESET_CMD);
> >> + sdhci_do_reset(host, SDHCI_RESET_DATA);
> >> +
> >> + host->pending_reset = false;
> >> + }
> >> +
> >> + /*
> >> * Always unmap the data buffers if they were mapped by
> >> * sdhci_prepare_data() whenever we finish with a request.
> >> * This avoids leaking DMA mappings on error.
> >> @@ -3060,35 +3091,6 @@ static bool sdhci_request_done(struct
> >> sdhci_host *host)
> >> }
> >> }
> >>
> >> - /*
> >> - * The controller needs a reset of internal state machines
> >> - * upon error conditions.
> >> - */
> >> - if (sdhci_needs_reset(host, mrq)) {
> >> - /*
> >> - * Do not finish until command and data lines are
> >> available for
> >> - * reset. Note there can only be one other mrq, so it
> >> cannot
> >> - * also be in mrqs_done, otherwise host->cmd and
> >> host->data_cmd
> >> - * would both be null.
> >> - */
> >> - if (host->cmd || host->data_cmd) {
> >> - spin_unlock_irqrestore(&host->lock, flags);
> >> - return true;
> >> - }
> >> -
> >> - /* Some controllers need this kick or reset won't work
> >> here */
> >> - if (host->quirks & SDHCI_QUIRK_CLOCK_BEFORE_RESET)
> >> - /* This is to force an update */
> >> - host->ops->set_clock(host, host->clock);
> >> -
> >> - /* Spec says we should do both at the same time, but
> >> Ricoh
> >> - controllers do not like that. */
> >> - sdhci_do_reset(host, SDHCI_RESET_CMD);
> >> - sdhci_do_reset(host, SDHCI_RESET_DATA);
> >> -
> >> - host->pending_reset = false;
> >> - }
> >> -
> >> host->mrqs_done[i] = NULL;
> >>
> >> spin_unlock_irqrestore(&host->lock, flags);
> >> --
> >> 2.7.4
> >>