| Message ID | 1614760331-43499-1-git-send-email-pragalla@qti.qualcomm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V2] mmc: sdhci: Check for reset prior to DMA address unmap | expand |
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> 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 <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 >>
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 > >>
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);