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

From: Ulf Hansson <ulf.hansson@linaro.org>
To: Pradeep P V K <pragalla@codeaurora.org>
Cc: Pradeep P V K <pragalla@qti.qualcomm.com>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Asutosh Das <asutoshd@codeaurora.org>,
	Sahitya Tummala <stummala@codeaurora.org>,
	Ram Prakash Gupta <rampraka@codeaurora.org>,
	Veerabhadrarao Badiganti <vbadigan@codeaurora.org>,
	"linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V2] mmc: sdhci: Check for reset prior to DMA address unmap
Date: Thu, 4 Mar 2021 19:08:39 +0100	[thread overview]
Message-ID: <CAPDyKFrTHiZyVAsP5TR5evOdbSi4dS_c+k5u6rdA4UwhAc6YuA@mail.gmail.com> (raw)
In-Reply-To: <3962320b58beaa4626ed69b3120d4246@codeaurora.org>

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