diff mbox series

[1/7] mmc: davinci_mmc: Map the virtual page for PIO

Message ID 20240125-mmc-proper-kmap-v1-1-ba953c1ac3f9@linaro.org (mailing list archive)
State New, archived
Headers show
Series mmc: Try to do proper kmap_local() for scatterlists | expand

Commit Message

Linus Walleij Jan. 25, 2024, 2:37 p.m. UTC
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(-)

Comments

Arnd Bergmann Jan. 25, 2024, 4:37 p.m. UTC | #1
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
Linus Walleij Jan. 25, 2024, 11:18 p.m. UTC | #2
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 mbox series

Patch

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,