On Mon 21 Oct 2019 at 16:48, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> On Mon, 21 Oct 2019 at 11:17, Jerome Brunet <jbrunet@xxxxxxxxxxxx> wrote:
>>
>>
>> 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 ?
>
> 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 the pre_req/post_req hooks.
* This driver uses the pre_req/post_req hooks to map/unmap the mmc_request.
* Following what other drivers do (sdhci, dw_mmc) we use the following cookie
* flags to keep track of the mmc_request mapping state.
*
* COOKIE_UNMAPPED: the request is not mapped.
* COOKIE_PREMAPPED: the request was mapped in pre_req,
* and should be unmapped in post_req.
* COOKIE_MAPPED: the request was mapped in the irq handler,
* and should be unmapped before mmc_request_done is called..
*/
Should we try to follow that ?
* OR, we should keep enabling it whenever we can ? In this case, it is
probably better to not provide pre/post_req to the framework and
manage things directly in the .request() callback ?
At the moment, we are doing both so it is difficult to figure out what
is doing what ...
>
> 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
