On Tuesday, November 20, 2018 19:19 -03, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> On 20 November 2018 at 19:21, Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx> wrote:
> > As reported by Aaro, the JZ4740 MMC driver throws a warning
> > when the kernel is built without preemption (CONFIG_PREEMPT_NONE=y).
> >
> > [ 16.461094] jz4740-mmc 13450000.mmc: [jz4740_mmc_prepare_dma_data] invalid cookie: data->host_cookie 567 host->next_data.cookie 568
> > [ 16.473120] jz4740-mmc 13450000.mmc: [jz4740_mmc_prepare_dma_data] invalid cookie: data->host_cookie 568 host->next_data.cookie 569
> > [ 16.485144] jz4740-mmc 13450000.mmc: [jz4740_mmc_prepare_dma_data] invalid cookie: data->host_cookie 569 host->next_data.cookie 570
> > [ 16.497170] jz4740-mmc 13450000.mmc: [jz4740_mmc_prepare_dma_data] invalid cookie: data->host_cookie 570 host->next_data.cookie 571
> > [ 16.509194] jz4740-mmc 13450000.mmc: [jz4740_mmc_prepare_dma_data] invalid cookie: data->host_cookie 571 host->next_data.cookie 572
> > [ 16.532421] jz4740-mmc 13450000.mmc: [jz4740_mmc_prepare_dma_data] invalid cookie: data->host_cookie 572 host->next_data.cookie 573
> > [ 16.544594] jz4740-mmc 13450000.mmc: [jz4740_mmc_prepare_dma_data] invalid cookie: data->host_cookie 573 host->next_data.cookie 574
> > [ 16.556621] jz4740-mmc 13450000.mmc: [jz4740_mmc_prepare_dma_data] invalid cookie: data->host_cookie 574 host->next_data.cookie 575
> > [ 16.568638] jz4740-mmc 13450000.mmc: [jz4740_mmc_prepare_dma_data] invalid cookie: data->host_cookie 575 host->next_data.cookie 576
> > [ 16.601092] jz4740-mmc 13450000.mmc: [jz4740_mmc_prepare_dma_data] invalid cookie: data->host_cookie 582 host->next_data.cookie 583
> >
> > The problem seems to be related to how pre_req/post_req is implemented.
> > Currently, it seems the driver expects jz4740_mmc_prepare_dma_data()
> > to be called with monotonically increasing host_cookie values,
> > which is wrong.
> >
> > Moreover, the implementation is overly complicated, keeping track
> > of unneeded "next cookie" state.
> >
> > So, instead of attempting to fix the current pre_req/post_req
> > implementation, this commit refactors the driver, dropping
> > the state, following other drivers such as dw_mmc and sdhci.
> >
> > Cc: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> > Cc: Mathieu Malaterre <malat@xxxxxxxxxx>
> > Reported-by: Aaro Koskinen <aaro.koskinen@xxxxxx>
> > Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
>
> This looks good to me, however because of the rather big changes I
> doubt this can be tagged for stable - or did you have a try to apply
> it for an old kernel?
>
> In either case, I can queue this up for next, which anyway seems like
> it should be sufficient. Right?
>
Yes, that is enough for me.
If anyone is interested in a fix for stable, it might go with
disabling pre_req/post_req altogether.
> Kind regards
> Uffe
>
> > ---
> > drivers/mmc/host/jz4740_mmc.c | 118 ++++++++++++++++------------------
> > 1 file changed, 54 insertions(+), 64 deletions(-)
> >
> > diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
> > index 0c1efd5100b7..ca20e2f81be8 100644
> > --- a/drivers/mmc/host/jz4740_mmc.c
> > +++ b/drivers/mmc/host/jz4740_mmc.c
> > @@ -126,9 +126,23 @@ enum jz4740_mmc_state {
> > JZ4740_MMC_STATE_DONE,
> > };
> >
> > -struct jz4740_mmc_host_next {
> > - int sg_len;
> > - s32 cookie;
> > +/*
> > + * 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..
> > + */
> > +enum jz4780_cookie {
> > + COOKIE_UNMAPPED = 0,
> > + COOKIE_PREMAPPED,
> > + COOKIE_MAPPED,
> > };
> >
> > struct jz4740_mmc_host {
> > @@ -162,9 +176,7 @@ struct jz4740_mmc_host {
> > /* DMA support */
> > struct dma_chan *dma_rx;
> > struct dma_chan *dma_tx;
> > - struct jz4740_mmc_host_next next_data;
> > bool use_dma;
> > - int sg_len;
> >
> > /* The DMA trigger level is 8 words, that is to say, the DMA read
> > * trigger is when data words in MSC_RXFIFO is >= 8 and the DMA write
> > @@ -226,9 +238,6 @@ static int jz4740_mmc_acquire_dma_channels(struct jz4740_mmc_host *host)
> > return PTR_ERR(host->dma_rx);
> > }
> >
> > - /* Initialize DMA pre request cookie */
> > - host->next_data.cookie = 1;
> > -
> > return 0;
> > }
> >
> > @@ -245,60 +254,44 @@ static void jz4740_mmc_dma_unmap(struct jz4740_mmc_host *host,
> > enum dma_data_direction dir = mmc_get_dma_dir(data);
> >
> > dma_unmap_sg(chan->device->dev, data->sg, data->sg_len, dir);
> > + data->host_cookie = COOKIE_UNMAPPED;
> > }
> >
> > -/* Prepares DMA data for current/next transfer, returns non-zero on failure */
> > +/* Prepares DMA data for current or next transfer.
> > + * A request can be in-flight when this is called.
> > + */
> > static int jz4740_mmc_prepare_dma_data(struct jz4740_mmc_host *host,
> > struct mmc_data *data,
> > - struct jz4740_mmc_host_next *next,
> > - struct dma_chan *chan)
> > + int cookie)
> > {
> > - struct jz4740_mmc_host_next *next_data = &host->next_data;
> > + struct dma_chan *chan = jz4740_mmc_get_dma_chan(host, data);
> > enum dma_data_direction dir = mmc_get_dma_dir(data);
> > - int sg_len;
> > -
> > - if (!next && data->host_cookie &&
> > - data->host_cookie != host->next_data.cookie) {
> > - dev_warn(mmc_dev(host->mmc),
> > - "[%s] invalid cookie: data->host_cookie %d host->next_data.cookie %d\n",
> > - __func__,
> > - data->host_cookie,
> > - host->next_data.cookie);
> > - data->host_cookie = 0;
> > - }
> > + int sg_count;
> >
> > - /* Check if next job is already prepared */
> > - if (next || data->host_cookie != host->next_data.cookie) {
> > - sg_len = dma_map_sg(chan->device->dev,
> > - data->sg,
> > - data->sg_len,
> > - dir);
> > + if (data->host_cookie == COOKIE_PREMAPPED)
> > + return data->sg_count;
> >
> > - } else {
> > - sg_len = next_data->sg_len;
> > - next_data->sg_len = 0;
> > - }
> > + sg_count = dma_map_sg(chan->device->dev,
> > + data->sg,
> > + data->sg_len,
> > + dir);
> >
> > - if (sg_len <= 0) {
> > + if (sg_count <= 0) {
> > dev_err(mmc_dev(host->mmc),
> > "Failed to map scatterlist for DMA operation\n");
> > return -EINVAL;
> > }
> >
> > - if (next) {
> > - next->sg_len = sg_len;
> > - data->host_cookie = ++next->cookie < 0 ? 1 : next->cookie;
> > - } else
> > - host->sg_len = sg_len;
> > + data->sg_count = sg_count;
> > + data->host_cookie = cookie;
> >
> > - return 0;
> > + return data->sg_count;
> > }
> >
> > static int jz4740_mmc_start_dma_transfer(struct jz4740_mmc_host *host,
> > struct mmc_data *data)
> > {
> > - int ret;
> > - struct dma_chan *chan;
> > + struct dma_chan *chan = jz4740_mmc_get_dma_chan(host, data);
> > struct dma_async_tx_descriptor *desc;
> > struct dma_slave_config conf = {
> > .src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES,
> > @@ -306,29 +299,26 @@ static int jz4740_mmc_start_dma_transfer(struct jz4740_mmc_host *host,
> > .src_maxburst = JZ4740_MMC_FIFO_HALF_SIZE,
> > .dst_maxburst = JZ4740_MMC_FIFO_HALF_SIZE,
> > };
> > + int sg_count;
> >
> > if (data->flags & MMC_DATA_WRITE) {
> > conf.direction = DMA_MEM_TO_DEV;
> > conf.dst_addr = host->mem_res->start + JZ_REG_MMC_TXFIFO;
> > conf.slave_id = JZ4740_DMA_TYPE_MMC_TRANSMIT;
> > - chan = host->dma_tx;
> > } else {
> > conf.direction = DMA_DEV_TO_MEM;
> > conf.src_addr = host->mem_res->start + JZ_REG_MMC_RXFIFO;
> > conf.slave_id = JZ4740_DMA_TYPE_MMC_RECEIVE;
> > - chan = host->dma_rx;
> > }
> >
> > - ret = jz4740_mmc_prepare_dma_data(host, data, NULL, chan);
> > - if (ret)
> > - return ret;
> > + sg_count = jz4740_mmc_prepare_dma_data(host, data, COOKIE_MAPPED);
> > + if (sg_count < 0)
> > + return sg_count;
> >
> > dmaengine_slave_config(chan, &conf);
> > - desc = dmaengine_prep_slave_sg(chan,
> > - data->sg,
> > - host->sg_len,
> > - conf.direction,
> > - DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> > + desc = dmaengine_prep_slave_sg(chan, data->sg, sg_count,
> > + conf.direction,
> > + DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> > if (!desc) {
> > dev_err(mmc_dev(host->mmc),
> > "Failed to allocate DMA %s descriptor",
> > @@ -342,7 +332,8 @@ static int jz4740_mmc_start_dma_transfer(struct jz4740_mmc_host *host,
> > return 0;
> >
> > dma_unmap:
> > - jz4740_mmc_dma_unmap(host, data);
> > + if (data->host_cookie == COOKIE_MAPPED)
> > + jz4740_mmc_dma_unmap(host, data);
> > return -ENOMEM;
> > }
> >
> > @@ -351,16 +342,13 @@ static void jz4740_mmc_pre_request(struct mmc_host *mmc,
> > {
> > struct jz4740_mmc_host *host = mmc_priv(mmc);
> > struct mmc_data *data = mrq->data;
> > - struct jz4740_mmc_host_next *next_data = &host->next_data;
> >
> > - BUG_ON(data->host_cookie);
> > -
> > - if (host->use_dma) {
> > - struct dma_chan *chan = jz4740_mmc_get_dma_chan(host, data);
> > + if (!host->use_dma)
> > + return;
> >
> > - if (jz4740_mmc_prepare_dma_data(host, data, next_data, chan))
> > - data->host_cookie = 0;
> > - }
> > + data->host_cookie = COOKIE_UNMAPPED;
> > + if (jz4740_mmc_prepare_dma_data(host, data, COOKIE_PREMAPPED) < 0)
> > + data->host_cookie = COOKIE_UNMAPPED;
> > }
> >
> > static void jz4740_mmc_post_request(struct mmc_host *mmc,
> > @@ -370,10 +358,8 @@ static void jz4740_mmc_post_request(struct mmc_host *mmc,
> > struct jz4740_mmc_host *host = mmc_priv(mmc);
> > struct mmc_data *data = mrq->data;
> >
> > - if (host->use_dma && data->host_cookie) {
> > + if (data && data->host_cookie != COOKIE_UNMAPPED)
> > jz4740_mmc_dma_unmap(host, data);
> > - data->host_cookie = 0;
> > - }
> >
> > if (err) {
> > struct dma_chan *chan = jz4740_mmc_get_dma_chan(host, data);
> > @@ -436,10 +422,14 @@ static void jz4740_mmc_reset(struct jz4740_mmc_host *host)
> > static void jz4740_mmc_request_done(struct jz4740_mmc_host *host)
> > {
> > struct mmc_request *req;
> > + struct mmc_data *data;
> >
> > req = host->req;
> > + data = req->data;
> > host->req = NULL;
> >
> > + if (data && data->host_cookie == COOKIE_MAPPED)
> > + jz4740_mmc_dma_unmap(host, data);
> > mmc_request_done(host->mmc, req);
> > }
> >
> > --
> > 2.19.1
> >
