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