Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
References: <1571637541-119016-1-git-send-email-jianxin.pan@amlogic.com> <fc1f61e1-b156-11e6-3f21-c498d2f0a8c6@baylibre.com>
User-agent: mu4e 1.3.3; emacs 26.2
From:   Jerome Brunet <jbrunet@baylibre.com>
To:     Neil Armstrong <narmstrong@baylibre.com>,
        Jianxin Pan <jianxin.pan@amlogic.com>,
        Ulf Hansson <ulf.hansson@linaro.org>,
        Kevin Hilman <khilman@baylibre.com>
Cc:     Nan Li <nan.li@amlogic.com>, linux-amlogic@lists.infradead.org,
        linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org,
        Victor Wan <victor.wan@amlogic.com>
Subject: Re: [PATCH] mmc: fix mmc dma operation
In-reply-to: <fc1f61e1-b156-11e6-3f21-c498d2f0a8c6@baylibre.com>
Date:   Mon, 21 Oct 2019 11:17:15 +0200
Message-ID: <1jwocybgpw.fsf@starbuckisacylon.baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On Mon 21 Oct 2019 at 09:57, Neil Armstrong <narmstrong@baylibre.com> 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@amlogic.com>
>> 
>> 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 ?

>
> You should add a "Fixes:" tag so it can be backported on stable kernels.
>
>> 
>> Signed-off-by: Nan Li <nan.li@amlogic.com>
>> Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com>
>> ---
>>  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