| Message ID | 20181120182121.19778-1-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: jz4740: rework pre_req/post_req implementation | expand |
On 20 November 2018 at 19:21, Ezequiel Garcia <ezequiel@collabora.com> 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@crapouillou.net> > Cc: Mathieu Malaterre <malat@debian.org> > Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> 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? 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 >
Hi, On Tue, Nov 20, 2018 at 03:21:21PM -0300, Ezequiel Garcia 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.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@crapouillou.net> > Cc: Mathieu Malaterre <malat@debian.org> > Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> Thanks for this! I tested this on top of v4.19 on CI20 with PREEMPT_NONE=y and MMC works now fine without spam. I also did a longer stress test compiling Python natively on a file system on MMC. Also briefly checked with the PREEMPT=y kernel config and at least the rootfs got mounted fine. So: Tested-by: Aaro Koskinen <aaro.koskinen@iki.fi> A.
On Tuesday, November 20, 2018 20:21 -03, Aaro Koskinen <aaro.koskinen@iki.fi> wrote: > Hi, > > On Tue, Nov 20, 2018 at 03:21:21PM -0300, Ezequiel Garcia 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.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@crapouillou.net> > > Cc: Mathieu Malaterre <malat@debian.org> > > Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi> > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > Thanks for this! I tested this on top of v4.19 on CI20 with PREEMPT_NONE=y > and MMC works now fine without spam. I also did a longer stress test > compiling Python natively on a file system on MMC. Also briefly checked > with the PREEMPT=y kernel config and at least the rootfs got mounted > fine. So: > > Tested-by: Aaro Koskinen <aaro.koskinen@iki.fi> > Thanks a lot for the good testing!
On Tuesday, November 20, 2018 19:19 -03, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 20 November 2018 at 19:21, Ezequiel Garcia <ezequiel@collabora.com> 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@crapouillou.net> > > Cc: Mathieu Malaterre <malat@debian.org> > > Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi> > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > 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 > >
On Wed, 21 Nov 2018 at 14:20, Ezequiel Garcia <ezequiel@collabora.com> wrote: > > On Tuesday, November 20, 2018 19:19 -03, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > On 20 November 2018 at 19:21, Ezequiel Garcia <ezequiel@collabora.com> 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@crapouillou.net> > > > Cc: Mathieu Malaterre <malat@debian.org> > > > Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi> > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > > 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. Great! [...] Applied for next then, thanks! Kind regards Uffe
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); }
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@crapouillou.net> Cc: Mathieu Malaterre <malat@debian.org> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- drivers/mmc/host/jz4740_mmc.c | 118 ++++++++++++++++------------------ 1 file changed, 54 insertions(+), 64 deletions(-)