Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: pragalla=codeaurora.org@mg.codeaurora.org
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Thu, 04 Mar 2021 20:46:35 +0530
From:   pragalla@codeaurora.org
To:     Ulf Hansson <ulf.hansson@linaro.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 Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V2] mmc: sdhci: Check for reset prior to DMA address unmap
In-Reply-To: <CAPDyKFqFNr7AiOdVP07XS=CKpMbDKC7n0gMPu0516fgH3=S18Q@mail.gmail.com>
References: <1614760331-43499-1-git-send-email-pragalla@qti.qualcomm.com>
 <CAPDyKFqFNr7AiOdVP07XS=CKpMbDKC7n0gMPu0516fgH3=S18Q@mail.gmail.com>
Message-ID: <3962320b58beaa4626ed69b3120d4246@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Precedence: bulk

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