Re: [PATCH] mmc: fix mmc dma operation
在 2019/10/21 17:17, Jerome Brunet 写道: > On Mon 21 Oct 2019 at 09:57, Neil Armstrong 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 >>> >>> 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 >>> Signed-off-by: Jianxin Pan >>> --- >>> 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
Re: [PATCH] mmc: fix mmc dma operation
On Mon 21 Oct 2019 at 16:48, Ulf Hansson wrote: > On Mon, 21 Oct 2019 at 11:17, Jerome Brunet wrote: >> >> >> On Mon 21 Oct 2019 at 09:57, Neil Armstrong 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 >> >> >> >> 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 >> >> Signed-off-by: Jianxin Pan >> >> --- >> >> 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 ? > > This is tricky, unfortunately. > > The main problem boils done to that, there is no guarantee that the > ->pre|post_request() host callbacks is called at all, as that depends > on if the mmc block layer has more than one requests in the pipe to > send. Additionally, that of course varies dynamically on a running > system. > >> >> 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 ? > > Whether we theoretically could simplify the path, by for example > always calling the ->pre|post_request() callbacks if those exists, is > probably too late to change. Well, unless we can change all host > drivers implementing them as well... so it's probably just easier to > accept this as is. Don't worry, I was not suggesting to change the framework. I was questionning our driver implementation. If I understand, the framework will call pre/post_req only if it has more than one request ? Our driver only enable "chained mode" (and the related dma mapping) in these callback. I don't think it worth enabling "chained mode" if there is only one request (nothing to chain) According to you: * Is it a good idea to enable chained mode only when framework calls pre/post req ? (AFAICT, this is what the dw_mmc.c driver is doing) There is a pretty interresting comment in jz4740_mmc.c about that: /* * The MMC core allows to prepare a mmc_request while another mmc_request * is in-flight. This is used via th
Re: [PATCH] mmc: fix mmc dma operation
On Mon, 21 Oct 2019 at 11:17, Jerome Brunet wrote: > > > On Mon 21 Oct 2019 at 09:57, Neil Armstrong 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 > >> > >> 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 > >> Signed-off-by: Jianxin Pan > >> --- > >> 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 ? This is tricky, unfortunately. The main problem boils done to that, there is no guarantee that the ->pre|post_request() host callbacks is called at all, as that depends on if the mmc block layer has more than one requests in the pipe to send. Additionally, that of course varies dynamically on a running system. > > 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 ? Whether we theoretically could simplify the path, by for example always calling the ->pre|post_request() callbacks if those exists, is probably too late to change. Well, unless we can change all host drivers implementing them as well... so it's probably just easier to accept this as is. One thing though, make sure you have a nice self descriptive naming of variables and functions, to deal with this. That helps a lot. Kind regards Uffe
Re: [PATCH] mmc: fix mmc dma operation
On Mon 21 Oct 2019 at 09:57, Neil Armstrong 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 >> >> 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 >> Signed-off-by: Jianxin Pan >> --- >> 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
Re: [PATCH] mmc: fix mmc dma operation
Hi Neil, Thanks for the review, I will update the subject and commit message in the next version. On 2019/10/21 15:57, Neil Armstrong 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 >> >> 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. > > > You should add a "Fixes:" tag so it can be backported on stable kernels. > >> >> Signed-off-by: Nan Li >> Signed-off-by: Jianxin Pan >> --- >> drivers/mmc/host/meson-gx-mmc.c | 15 --- >> 1 file changed, 8 insertions(+), 7 deletions(-) >> [...] >> } >> >> static void meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command >> *cmd) >> > Neil > > . >
Re: [PATCH] mmc: fix mmc dma operation
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 > > 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. You should add a "Fixes:" tag so it can be backported on stable kernels. > > Signed-off-by: Nan Li > Signed-off-by: Jianxin Pan > --- > 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); > } > > static void meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command > *cmd) > Neil
[PATCH] mmc: fix mmc dma operation
From: Nan Li 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. Signed-off-by: Nan Li Signed-off-by: Jianxin Pan --- 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); } static void meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd) -- 2.7.4