| Message ID | 1614163985-38914-1-git-send-email-pragalla@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | [V1] mmc: sdhci: Check for reset prior to DMA address unmap | expand |
On 24/02/21 12:53 pm, Pradeep P V K wrote: > 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(). > > Suggested-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org> > Signed-off-by: Pradeep P V K <pragalla@codeaurora.org> Seems like a good change to make. A couple of cosmetic tweaks below, but: Acked-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/host/sdhci.c | 58 ++++++++++++++++++++++++------------------------ > 1 file changed, 29 insertions(+), 29 deletions(-) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index 646823d..e78d84c 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -2996,6 +2996,35 @@ static bool sdhci_request_done(struct sdhci_host *host) > spin_unlock_irqrestore(&host->lock, flags); > return true; > } Blank line here please. > + /* > + * 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. > + */ Please change comment style: /* * 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 > @@ -3060,35 +3089,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); >
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 646823d..e78d84c 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -2996,6 +2996,35 @@ static bool sdhci_request_done(struct sdhci_host *host) spin_unlock_irqrestore(&host->lock, flags); return true; } + /* + * 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 @@ -3060,35 +3089,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);
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(). Suggested-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org> Signed-off-by: Pradeep P V K <pragalla@codeaurora.org> --- drivers/mmc/host/sdhci.c | 58 ++++++++++++++++++++++++------------------------ 1 file changed, 29 insertions(+), 29 deletions(-)