Messages in this thread | | | Date | Thu, 04 Mar 2021 20:46:35 +0530 | From | pragalla@codeauro ... | Subject | Re: [PATCH V2] mmc: sdhci: Check for reset prior to DMA address unmap |
| |
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
> >> --- >> 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 >>
| |