Re: [PATCH V2] mmc: sdhci: Check for reset prior to DMA address unmap

[pragalla@xxxxxxxxxxxxxx]

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

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