On Thu, 4 Mar 2021 at 16:16, <pragalla@xxxxxxxxxxxxxx> wrote:
>
> On 2021-03-04 19:19, Ulf Hansson wrote:
> > On Wed, 3 Mar 2021 at 09:32, Pradeep P V K <pragalla@xxxxxxxxxxxxxxxx>
> > wrote:
> >>
> >> From: Pradeep P V K <pragalla@xxxxxxxxxxxxxx>
> >>
> >> 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 <vbadigan@xxxxxxxxxxxxxx>
> >> Signed-off-by: Pradeep P V K <pragalla@xxxxxxxxxxxxxx>
> >> Acked-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
>
> 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
> >>
