Re: [PATCH] mmc: fix mmc dma operation

[Nan Li]

å 2019/10/21 17:17, Jerome Brunet åé:
> On Mon 21 Oct 2019 at 09:57, Neil Armstrong <narmstrong@xxxxxxxxxxxx> wrote:
>
>> Hi,
>>
>> Thanks for the fix.
>>
>> First, you should add "mmc: meson-gx:" in the subject.
>>
>> On 21/10/2019 07:59, Jianxin Pan wrote:
>>> From: Nan Li <nan.li@xxxxxxxxxxx>
>>>
>>> In MMC dma transfer, the region requested by dma_map_sg() may be released
>>> by dma_unmap_sg() before the transfer is completed.
>>>
>>> Put the unmap operation in front of mmc_request_done() to avoid this.
> Since we have seen this problem (yet), could you briefly how you've
> triggered it ?

The problem we found in the stress test was that the sdio device was 
constantly operated on and off electricity to make it repeatedly 
initialized.

During the test, we found that there was a chance that the information 
read by the controller from the sdio device side was wrong, which made 
the sdio initialization fail.

>> You should add a "Fixes:" tag so it can be backported on stable kernels.
>>
>>> Signed-off-by: Nan Li <nan.li@xxxxxxxxxxx>
>>> Signed-off-by: Jianxin Pan <jianxin.pan@xxxxxxxxxxx>
>>> ---
>>>   drivers/mmc/host/meson-gx-mmc.c | 15 ++++++++-------
>>>   1 file changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
>>> index e712315..7667e8a 100644
>>> --- a/drivers/mmc/host/meson-gx-mmc.c
>>> +++ b/drivers/mmc/host/meson-gx-mmc.c
>>> @@ -173,6 +173,7 @@ struct meson_host {
>>>   	int irq;
>>>   
>>>   	bool vqmmc_enabled;
>>> +	bool needs_pre_post_req;
>>>   };
>>>   
>>>   #define CMD_CFG_LENGTH_MASK GENMASK(8, 0)
>>> @@ -654,6 +655,8 @@ static void meson_mmc_request_done(struct mmc_host *mmc,
>>>   	struct meson_host *host = mmc_priv(mmc);
>>>   
>>>   	host->cmd = NULL;
>>> +	if (host->needs_pre_post_req)
>>> +		meson_mmc_post_req(mmc, mrq, 0);
>>>   	mmc_request_done(host->mmc, mrq);
>>>   }
>>>   
>>> @@ -803,25 +806,23 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd)
>>>   static void meson_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>>   {
>>>   	struct meson_host *host = mmc_priv(mmc);
>>> -	bool needs_pre_post_req = mrq->data &&
>>> +
>>> +	host->needs_pre_post_req = mrq->data &&
>>>   			!(mrq->data->host_cookie & SD_EMMC_PRE_REQ_DONE);
>>>   
>>> -	if (needs_pre_post_req) {
>>> +	if (host->needs_pre_post_req) {
>>>   		meson_mmc_get_transfer_mode(mmc, mrq);
>>>   		if (!meson_mmc_desc_chain_mode(mrq->data))
>>> -			needs_pre_post_req = false;
>>> +			host->needs_pre_post_req = false;
>>>   	}
>>>   
>>> -	if (needs_pre_post_req)
>>> +	if (host->needs_pre_post_req)
>>>   		meson_mmc_pre_req(mmc, mrq);
>>>   
>>>   	/* Stop execution */
>>>   	writel(0, host->regs + SD_EMMC_START);
>>>   
>>>   	meson_mmc_start_cmd(mmc, mrq->sbc ?: mrq->cmd);
>>> -
>>> -	if (needs_pre_post_req)
>>> -		meson_mmc_post_req(mmc, mrq, 0);
>>>   }
> The code around all this is getting quite difficult to follow eventhough
> it does not actually do much
>
> The root of the problem seems be that meson_mmc_pre_req() and
> meson_mmc_post_req() are passed to framework but also called manually
> from meson_mmc_request().
>
> Because of this, some code is added to make sure we don't do things twice.
> Maybe I'm missing something but it look weird ? Ulf, could you give us
> your view ?
>
> As far as I can tell:
>   * pre_req : determine if we use CHAIN_MODE or not AND
>               dma_map_sg() if we do
>   * post_req : dma_unmap_sg() if previously allocated
>
> Do we really need to do all this meson_mmc_request() ? Shouldn't we let the
> framework do the calls to pre/post_req for us ?
>
>>>   
>>>   static void meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd)
>>>
>> Neil