[PATCH 0/7] mmc: Try to do proper kmap_local() for scatterlists

[PATCH 0/7] mmc: Try to do proper kmap_local() for scatterlists

[Linus Walleij]

It was brought to our attention that some MMC host drivers
are referencing sg_virt(sg) directly on scatterlist entries,
which will not perform buffer bouncing for CONFIG_HIGHMEM
pages that reside in highmem.

See the following mail from Christoph and the discussion:
https://lore.kernel.org/linux-mmc/20240122073423.GA25859@lst.de/

This means that bugs with highmem pages can go unnoticed
until an actual highmem page is finally used and not bounced,
resulting in things like unpredictable file corruption.

Attempt to fix this by amending all host controllers
calling sg_virt() for PIO to instead do proper mapping
and unmapping of the scatterlist entry, possibly bouncing
it from highmem if need be.

More complicated patches are possible, the most obvious
to rewrite the PIO loops to use sg_miter_[start|next|stop]()
see for example mmci.c, but I leave this refactoring as
a suggestion to each device driver maintainer because I
can't really test the patches.

All patches are compile-tested except the m68k one,
sdhci-esdhc-mcf.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Linus Walleij (7):
      mmc: davinci_mmc: Map the virtual page for PIO
      mmc: moxart-mmc: Map the virtual page for PIO
      mmc: mvsdio: Map the virtual page for PIO
      mmc: mxcmmc: Map the virtual page for PIO
      mmc: omap: Map the virtual page for PIO
      mmc: sdhci-esdhc-mcf: Map the virtual page for swapping
      mmc: sh_mmcif: Map the virtual page for PIO

 drivers/mmc/host/davinci_mmc.c     | 10 ++++++++--
 drivers/mmc/host/moxart-mmc.c      |  3 ++-
 drivers/mmc/host/mvsdio.c          |  3 ++-
 drivers/mmc/host/mxcmmc.c          | 23 +++++++++++++++--------
 drivers/mmc/host/omap.c            |  7 ++++++-
 drivers/mmc/host/sdhci-esdhc-mcf.c |  3 ++-
 drivers/mmc/host/sh_mmcif.c        | 22 ++++++++++++++++++----
 7 files changed, 53 insertions(+), 18 deletions(-)
---
base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
change-id: 20240125-mmc-proper-kmap-f2d4cf5d1756

Best regards,
-- 
Linus Walleij <linus.walleij@linaro.org>

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

[Linus Walleij]

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;
 }
@@ -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,

-- 
2.34.1

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

[Arnd Bergmann]

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

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

[Linus Walleij]

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

[PATCH 2/7] mmc: moxart-mmc: Map the virtual page for PIO

[Linus Walleij]

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/moxart-mmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/moxart-mmc.c b/drivers/mmc/host/moxart-mmc.c
index 5cfdd3a86e54..7ccfe872415a 100644
--- a/drivers/mmc/host/moxart-mmc.c
+++ b/drivers/mmc/host/moxart-mmc.c
@@ -310,7 +310,7 @@ static void moxart_transfer_pio(struct moxart_host *host)
 	if (host->data_len == data->bytes_xfered)
 		return;
 
-	sgp = sg_virt(host->cur_sg);
+	sgp = kmap_local_page(sg_page(host->cur_sg));
 	remain = host->data_remain;
 
 	if (data->flags & MMC_DATA_WRITE) {
@@ -346,6 +346,7 @@ static void moxart_transfer_pio(struct moxart_host *host)
 		}
 	}
 
+	kunmap_local(sgp);
 	data->bytes_xfered += host->data_remain - remain;
 	host->data_remain = remain;
 

-- 
2.34.1

[PATCH 3/7] mmc: mvsdio: Map the virtual page for PIO

[Linus Walleij]

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/mvsdio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/mvsdio.c b/drivers/mmc/host/mvsdio.c
index ca01b7d204ba..a004a523bd2a 100644
--- a/drivers/mmc/host/mvsdio.c
+++ b/drivers/mmc/host/mvsdio.c
@@ -115,7 +115,7 @@ static int mvsd_setup_data(struct mvsd_host *host, struct mmc_data *data)
 		 * boundary.
 		 */
 		host->pio_size = data->blocks * data->blksz;
-		host->pio_ptr = sg_virt(data->sg);
+		host->pio_ptr = kmap_local_page(sg_page(data->sg));
 		if (!nodma)
 			dev_dbg(host->dev, "fallback to PIO for data at 0x%p size %d\n",
 				host->pio_ptr, host->pio_size);
@@ -289,6 +289,7 @@ static u32 mvsd_finish_data(struct mvsd_host *host, struct mmc_data *data,
 	void __iomem *iobase = host->base;
 
 	if (host->pio_ptr) {
+		kunmap_local(host->pio_ptr);
 		host->pio_ptr = NULL;
 		host->pio_size = 0;
 	} else {

-- 
2.34.1

[PATCH 4/7] mmc: mxcmmc: Map the virtual page for PIO

[Linus Walleij]

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/mxcmmc.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index 5b3ab0e20505..04c0e4ea02ff 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -267,10 +267,14 @@ static inline void buffer_swap32(u32 *buf, int len)
 static void mxcmci_swap_buffers(struct mmc_data *data)
 {
 	struct scatterlist *sg;
+	u32 *buf;
 	int i;
 
-	for_each_sg(data->sg, sg, data->sg_len, i)
-		buffer_swap32(sg_virt(sg), sg->length);
+	for_each_sg(data->sg, sg, data->sg_len, i) {
+		buf = kmap_local_page(sg_page(sg));
+		buffer_swap32(buf, sg->length);
+		kunmap_local(buf);
+	}
 }
 #else
 static inline void mxcmci_swap_buffers(struct mmc_data *data) {}
@@ -526,10 +530,9 @@ static int mxcmci_poll_status(struct mxcmci_host *host, u32 mask)
 	} while (1);
 }
 
-static int mxcmci_pull(struct mxcmci_host *host, void *_buf, int bytes)
+static int mxcmci_pull(struct mxcmci_host *host, u32 *buf, int bytes)
 {
 	unsigned int stat;
-	u32 *buf = _buf;
 
 	while (bytes > 3) {
 		stat = mxcmci_poll_status(host,
@@ -555,10 +558,9 @@ static int mxcmci_pull(struct mxcmci_host *host, void *_buf, int bytes)
 	return 0;
 }
 
-static int mxcmci_push(struct mxcmci_host *host, void *_buf, int bytes)
+static int mxcmci_push(struct mxcmci_host *host, u32 *buf, int bytes)
 {
 	unsigned int stat;
-	u32 *buf = _buf;
 
 	while (bytes > 3) {
 		stat = mxcmci_poll_status(host, STATUS_BUF_WRITE_RDY);
@@ -588,20 +590,25 @@ static int mxcmci_transfer_data(struct mxcmci_host *host)
 	struct mmc_data *data = host->req->data;
 	struct scatterlist *sg;
 	int stat, i;
+	u32 *buf;
 
 	host->data = data;
 	host->datasize = 0;
 
 	if (data->flags & MMC_DATA_READ) {
 		for_each_sg(data->sg, sg, data->sg_len, i) {
-			stat = mxcmci_pull(host, sg_virt(sg), sg->length);
+			buf = kmap_local_page(sg_page(sg));
+			stat = mxcmci_pull(host, buf, sg->length);
+			kunmap_local(buf);
 			if (stat)
 				return stat;
 			host->datasize += sg->length;
 		}
 	} else {
 		for_each_sg(data->sg, sg, data->sg_len, i) {
-			stat = mxcmci_push(host, sg_virt(sg), sg->length);
+			buf = kmap_local_page(sg_page(sg));
+			stat = mxcmci_push(host, buf, sg->length);
+			kunmap_local(buf);
 			if (stat)
 				return stat;
 			host->datasize += sg->length;

-- 
2.34.1

[PATCH 5/7] mmc: omap: Map the virtual page for PIO

[Linus Walleij]

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/omap.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/omap.c b/drivers/mmc/host/omap.c
index 9fb8995b43a1..3e36480b22ad 100644
--- a/drivers/mmc/host/omap.c
+++ b/drivers/mmc/host/omap.c
@@ -659,7 +659,7 @@ mmc_omap_sg_to_buf(struct mmc_omap_host *host)
 
 	sg = host->data->sg + host->sg_idx;
 	host->buffer_bytes_left = sg->length;
-	host->buffer = sg_virt(sg);
+	host->buffer = kmap_local_page(sg_page(sg));
 	if (host->buffer_bytes_left > host->total_bytes_left)
 		host->buffer_bytes_left = host->total_bytes_left;
 }
@@ -691,6 +691,11 @@ mmc_omap_xfer_data(struct mmc_omap_host *host, int write)
 	nwords = DIV_ROUND_UP(n, 2);
 
 	host->buffer_bytes_left -= n;
+	if (host->buffer_bytes_left == 0) {
+		kunmap_local(host->buffer);
+		host->buffer = NULL;
+	}
+
 	host->total_bytes_left -= n;
 	host->data->bytes_xfered += n;
 

-- 
2.34.1

[PATCH 6/7] mmc: sdhci-esdhc-mcf: Map the virtual page for swapping

[Linus Walleij]

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/sdhci-esdhc-mcf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-mcf.c b/drivers/mmc/host/sdhci-esdhc-mcf.c
index a07f8333cd6b..ed851afbf9bc 100644
--- a/drivers/mmc/host/sdhci-esdhc-mcf.c
+++ b/drivers/mmc/host/sdhci-esdhc-mcf.c
@@ -314,8 +314,9 @@ static void esdhc_mcf_request_done(struct sdhci_host *host,
 	 * transfer endiannes. A swap after the transfer is needed.
 	 */
 	for_each_sg(mrq->data->sg, sg, mrq->data->sg_len, i) {
-		buffer = (u32 *)sg_virt(sg);
+		buffer = kmap_local_page(sg_page(sg));
 		esdhc_mcf_buffer_swap32(buffer, sg->length);
+		kunmap_local(buffer);
 	}
 
 exit_done:

-- 
2.34.1

[PATCH 7/7] mmc: sh_mmcif: Map the virtual page for PIO

[Linus Walleij]

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.

This code is a bit asynchronous due to being called
repeatedly from an interrupt handler, so it would be great
if someone can test this.

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/sh_mmcif.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 077d711e964e..f9f56d653ff4 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -611,13 +611,27 @@ static bool sh_mmcif_next_block(struct sh_mmcif_host *host, u32 *p)
 
 	if (host->sg_blkidx == data->sg->length) {
 		host->sg_blkidx = 0;
+		/* Unmap previous sg and map the next one */
+		if (host->pio_ptr) {
+			kunmap_local(host->pio_ptr);
+			host->pio_ptr = NULL;
+		}
 		if (++host->sg_idx < data->sg_len)
-			host->pio_ptr = sg_virt(++data->sg);
+			host->pio_ptr = kmap_local_page(sg_page(++data->sg));
 	} else {
 		host->pio_ptr = p;
 	}
 
-	return host->sg_idx != data->sg_len;
+	/*
+	 * We return true of there are more blocks, and false if there is no
+	 * next block.
+	 */
+	if (host->sg_idx != data->sg_len)
+		return true;
+	/* Unmap the last buffer if any */
+	if (host->pio_ptr)
+		kunmap_local(host->pio_ptr);
+	return false;
 }
 
 static void sh_mmcif_single_read(struct sh_mmcif_host *host,
@@ -669,7 +683,7 @@ static void sh_mmcif_multi_read(struct sh_mmcif_host *host,
 	host->wait_for = MMCIF_WAIT_FOR_MREAD;
 	host->sg_idx = 0;
 	host->sg_blkidx = 0;
-	host->pio_ptr = sg_virt(data->sg);
+	host->pio_ptr = kmap_local_page(sg_page(data->sg));
 
 	sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MBUFREN);
 }
@@ -749,7 +763,7 @@ static void sh_mmcif_multi_write(struct sh_mmcif_host *host,
 	host->wait_for = MMCIF_WAIT_FOR_MWRITE;
 	host->sg_idx = 0;
 	host->sg_blkidx = 0;
-	host->pio_ptr = sg_virt(data->sg);
+	host->pio_ptr = kmap_local_page(sg_page(data->sg));
 
 	sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MBUFWEN);
 }

-- 
2.34.1

Re: [PATCH 0/7] mmc: Try to do proper kmap_local() for scatterlists

[Ulf Hansson]

On Thu, 25 Jan 2024 at 15:37, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> It was brought to our attention that some MMC host drivers
> are referencing sg_virt(sg) directly on scatterlist entries,
> which will not perform buffer bouncing for CONFIG_HIGHMEM
> pages that reside in highmem.
>
> See the following mail from Christoph and the discussion:
> https://lore.kernel.org/linux-mmc/20240122073423.GA25859@lst.de/
>
> This means that bugs with highmem pages can go unnoticed
> until an actual highmem page is finally used and not bounced,
> resulting in things like unpredictable file corruption.
>
> Attempt to fix this by amending all host controllers
> calling sg_virt() for PIO to instead do proper mapping
> and unmapping of the scatterlist entry, possibly bouncing
> it from highmem if need be.
>
> More complicated patches are possible, the most obvious
> to rewrite the PIO loops to use sg_miter_[start|next|stop]()
> see for example mmci.c, but I leave this refactoring as
> a suggestion to each device driver maintainer because I
> can't really test the patches.
>
> All patches are compile-tested except the m68k one,
> sdhci-esdhc-mcf.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Thanks for helping out with this! Applied for next!

Kind regards
Uffe


> ---
> Linus Walleij (7):
>       mmc: davinci_mmc: Map the virtual page for PIO
>       mmc: moxart-mmc: Map the virtual page for PIO
>       mmc: mvsdio: Map the virtual page for PIO
>       mmc: mxcmmc: Map the virtual page for PIO
>       mmc: omap: Map the virtual page for PIO
>       mmc: sdhci-esdhc-mcf: Map the virtual page for swapping
>       mmc: sh_mmcif: Map the virtual page for PIO
>
>  drivers/mmc/host/davinci_mmc.c     | 10 ++++++++--
>  drivers/mmc/host/moxart-mmc.c      |  3 ++-
>  drivers/mmc/host/mvsdio.c          |  3 ++-
>  drivers/mmc/host/mxcmmc.c          | 23 +++++++++++++++--------
>  drivers/mmc/host/omap.c            |  7 ++++++-
>  drivers/mmc/host/sdhci-esdhc-mcf.c |  3 ++-
>  drivers/mmc/host/sh_mmcif.c        | 22 ++++++++++++++++++----
>  7 files changed, 53 insertions(+), 18 deletions(-)
> ---
> base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
> change-id: 20240125-mmc-proper-kmap-f2d4cf5d1756
>
> Best regards,
> --
> Linus Walleij <linus.walleij@linaro.org>
>