| Message ID | 20240125-mmc-proper-kmap-v1-1-ba953c1ac3f9@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: Try to do proper kmap_local() for scatterlists | expand |
On Thu, Jan 25, 2024, at 15:37, Linus Walleij wrote: > Use kmap_local_page() instead of sg_virt() to obtain a page > from the scatterlist: sg_virt() will not perform bounce > buffering if the page happens to be located in high memory, > which the driver may or may not be using. > > Suggested-by: Christoph Hellwig <hch@lst.de> > Link: https://lore.kernel.org/linux-mmc/20240122073423.GA25859@lst.de/ > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/mmc/host/davinci_mmc.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/davinci_mmc.c > b/drivers/mmc/host/davinci_mmc.c > index ee3b1a4e0848..4e9f96b1caf3 100644 > --- a/drivers/mmc/host/davinci_mmc.c > +++ b/drivers/mmc/host/davinci_mmc.c > @@ -216,7 +216,7 @@ static irqreturn_t mmc_davinci_irq(int irq, void > *dev_id); > static void mmc_davinci_sg_to_buf(struct mmc_davinci_host *host) > { > host->buffer_bytes_left = sg_dma_len(host->sg); > - host->buffer = sg_virt(host->sg); > + host->buffer = kmap_local_page(sg_page(host->sg)); > if (host->buffer_bytes_left > host->bytes_left) > host->buffer_bytes_left = host->bytes_left; > } I see multiple problems here: - you are missing the offset within the page, which you get by adding sg->offset - kmap_local_page() only maps one page at a time, so this will fail if the scatterlist entry spans one or more pages. - the first call to mmc_davinci_sg_to_buf() may happen in mmc_davinci_prepare_data(), while the rest is done in the interrupt handler, and you can't hold the kmap reference across multiple contexts - It looks like you are missing the unmap inside of loop when moving to the next sg element. I think to do this properly, the driver would have to use struct sg_mapping_iter like the cb710 driver does, but the conversion is not as simple as your patch here. Arnd
On Thu, Jan 25, 2024 at 5:37 PM Arnd Bergmann <arnd@arndb.de> wrote: > I think to do this properly, the driver would have to > use struct sg_mapping_iter like the cb710 driver does, > but the conversion is not as simple as your patch here. Ack, how typical, so that is what I write in the cover letter that I wanted to avoid but it seems there is no avoiding it then. It's a bit trickier but I guess I can pull it off, it better get some testing. Thanks Arnd! Yours, Linus Walleij
diff --git a/drivers/mmc/host/davinci_mmc.c b/drivers/mmc/host/davinci_mmc.c index ee3b1a4e0848..4e9f96b1caf3 100644 --- a/drivers/mmc/host/davinci_mmc.c +++ b/drivers/mmc/host/davinci_mmc.c @@ -216,7 +216,7 @@ static irqreturn_t mmc_davinci_irq(int irq, void *dev_id); static void mmc_davinci_sg_to_buf(struct mmc_davinci_host *host) { host->buffer_bytes_left = sg_dma_len(host->sg); - host->buffer = sg_virt(host->sg); + host->buffer = kmap_local_page(sg_page(host->sg)); if (host->buffer_bytes_left > host->bytes_left) host->buffer_bytes_left = host->bytes_left; } @@ -261,7 +261,13 @@ static void davinci_fifo_data_trans(struct mmc_davinci_host *host, p = p + (n & 3); } } - host->buffer = p; + + if (host->buffer_bytes_left == 0) { + kunmap_local(host->buffer); + host->buffer = NULL; + } else { + host->buffer = p; + } } static void mmc_davinci_start_command(struct mmc_davinci_host *host,
Use kmap_local_page() instead of sg_virt() to obtain a page from the scatterlist: sg_virt() will not perform bounce buffering if the page happens to be located in high memory, which the driver may or may not be using. Suggested-by: Christoph Hellwig <hch@lst.de> Link: https://lore.kernel.org/linux-mmc/20240122073423.GA25859@lst.de/ Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/mmc/host/davinci_mmc.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)