On Mon 21 Oct 2019 at 09:57, Neil Armstrong <[email protected]> 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 <[email protected]> >> >> 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 <[email protected]> >> Signed-off-by: Jianxin Pan <[email protected]> >> --- >> 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
