* [PATCH 0/7] mmc: Try to do proper kmap_local() for scatterlists
@ 2024-01-25 14:37 Linus Walleij
2024-01-25 14:37 ` [PATCH 1/7] mmc: davinci_mmc: Map the virtual page for PIO Linus Walleij
` (7 more replies)
0 siblings, 8 replies; 11+ messages in thread
From: Linus Walleij @ 2024-01-25 14:37 UTC (permalink / raw
To: Christoph Hellwig, Jens Axboe, Ming Lei, Arnd Bergmann,
Ulf Hansson, Nicolas Pitre, Aaro Koskinen, Adrian Hunter,
Angelo Dureghello
Cc: linux-mmc, linux-block, linux-omap, 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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/7] mmc: davinci_mmc: Map the virtual page for PIO
2024-01-25 14:37 [PATCH 0/7] mmc: Try to do proper kmap_local() for scatterlists Linus Walleij
@ 2024-01-25 14:37 ` Linus Walleij
2024-01-25 16:37 ` Arnd Bergmann
2024-01-25 14:37 ` [PATCH 2/7] mmc: moxart-mmc: " Linus Walleij
` (6 subsequent siblings)
7 siblings, 1 reply; 11+ messages in thread
From: Linus Walleij @ 2024-01-25 14:37 UTC (permalink / raw
To: Christoph Hellwig, Jens Axboe, Ming Lei, Arnd Bergmann,
Ulf Hansson, Nicolas Pitre, Aaro Koskinen, Adrian Hunter,
Angelo Dureghello
Cc: linux-mmc, linux-block, linux-omap, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/7] mmc: moxart-mmc: Map the virtual page for PIO
2024-01-25 14:37 [PATCH 0/7] mmc: Try to do proper kmap_local() for scatterlists Linus Walleij
2024-01-25 14:37 ` [PATCH 1/7] mmc: davinci_mmc: Map the virtual page for PIO Linus Walleij
@ 2024-01-25 14:37 ` Linus Walleij
2024-01-25 14:37 ` [PATCH 3/7] mmc: mvsdio: " Linus Walleij
` (5 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Linus Walleij @ 2024-01-25 14:37 UTC (permalink / raw
To: Christoph Hellwig, Jens Axboe, Ming Lei, Arnd Bergmann,
Ulf Hansson, Nicolas Pitre, Aaro Koskinen, Adrian Hunter,
Angelo Dureghello
Cc: linux-mmc, linux-block, linux-omap, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/7] mmc: mvsdio: Map the virtual page for PIO
2024-01-25 14:37 [PATCH 0/7] mmc: Try to do proper kmap_local() for scatterlists Linus Walleij
2024-01-25 14:37 ` [PATCH 1/7] mmc: davinci_mmc: Map the virtual page for PIO Linus Walleij
2024-01-25 14:37 ` [PATCH 2/7] mmc: moxart-mmc: " Linus Walleij
@ 2024-01-25 14:37 ` Linus Walleij
2024-01-25 14:37 ` [PATCH 4/7] mmc: mxcmmc: " Linus Walleij
` (4 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Linus Walleij @ 2024-01-25 14:37 UTC (permalink / raw
To: Christoph Hellwig, Jens Axboe, Ming Lei, Arnd Bergmann,
Ulf Hansson, Nicolas Pitre, Aaro Koskinen, Adrian Hunter,
Angelo Dureghello
Cc: linux-mmc, linux-block, linux-omap, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 4/7] mmc: mxcmmc: Map the virtual page for PIO
2024-01-25 14:37 [PATCH 0/7] mmc: Try to do proper kmap_local() for scatterlists Linus Walleij
` (2 preceding siblings ...)
2024-01-25 14:37 ` [PATCH 3/7] mmc: mvsdio: " Linus Walleij
@ 2024-01-25 14:37 ` Linus Walleij
2024-01-25 14:37 ` [PATCH 5/7] mmc: omap: " Linus Walleij
` (3 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Linus Walleij @ 2024-01-25 14:37 UTC (permalink / raw
To: Christoph Hellwig, Jens Axboe, Ming Lei, Arnd Bergmann,
Ulf Hansson, Nicolas Pitre, Aaro Koskinen, Adrian Hunter,
Angelo Dureghello
Cc: linux-mmc, linux-block, linux-omap, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 5/7] mmc: omap: Map the virtual page for PIO
2024-01-25 14:37 [PATCH 0/7] mmc: Try to do proper kmap_local() for scatterlists Linus Walleij
` (3 preceding siblings ...)
2024-01-25 14:37 ` [PATCH 4/7] mmc: mxcmmc: " Linus Walleij
@ 2024-01-25 14:37 ` Linus Walleij
2024-01-25 14:37 ` [PATCH 6/7] mmc: sdhci-esdhc-mcf: Map the virtual page for swapping Linus Walleij
` (2 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Linus Walleij @ 2024-01-25 14:37 UTC (permalink / raw
To: Christoph Hellwig, Jens Axboe, Ming Lei, Arnd Bergmann,
Ulf Hansson, Nicolas Pitre, Aaro Koskinen, Adrian Hunter,
Angelo Dureghello
Cc: linux-mmc, linux-block, linux-omap, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 6/7] mmc: sdhci-esdhc-mcf: Map the virtual page for swapping
2024-01-25 14:37 [PATCH 0/7] mmc: Try to do proper kmap_local() for scatterlists Linus Walleij
` (4 preceding siblings ...)
2024-01-25 14:37 ` [PATCH 5/7] mmc: omap: " Linus Walleij
@ 2024-01-25 14:37 ` Linus Walleij
2024-01-25 14:37 ` [PATCH 7/7] mmc: sh_mmcif: Map the virtual page for PIO Linus Walleij
2024-02-13 16:45 ` [PATCH 0/7] mmc: Try to do proper kmap_local() for scatterlists Ulf Hansson
7 siblings, 0 replies; 11+ messages in thread
From: Linus Walleij @ 2024-01-25 14:37 UTC (permalink / raw
To: Christoph Hellwig, Jens Axboe, Ming Lei, Arnd Bergmann,
Ulf Hansson, Nicolas Pitre, Aaro Koskinen, Adrian Hunter,
Angelo Dureghello
Cc: linux-mmc, linux-block, linux-omap, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 7/7] mmc: sh_mmcif: Map the virtual page for PIO
2024-01-25 14:37 [PATCH 0/7] mmc: Try to do proper kmap_local() for scatterlists Linus Walleij
` (5 preceding siblings ...)
2024-01-25 14:37 ` [PATCH 6/7] mmc: sdhci-esdhc-mcf: Map the virtual page for swapping Linus Walleij
@ 2024-01-25 14:37 ` Linus Walleij
2024-02-13 16:45 ` [PATCH 0/7] mmc: Try to do proper kmap_local() for scatterlists Ulf Hansson
7 siblings, 0 replies; 11+ messages in thread
From: Linus Walleij @ 2024-01-25 14:37 UTC (permalink / raw
To: Christoph Hellwig, Jens Axboe, Ming Lei, Arnd Bergmann,
Ulf Hansson, Nicolas Pitre, Aaro Koskinen, Adrian Hunter,
Angelo Dureghello
Cc: linux-mmc, linux-block, linux-omap, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/7] mmc: davinci_mmc: Map the virtual page for PIO
2024-01-25 14:37 ` [PATCH 1/7] mmc: davinci_mmc: Map the virtual page for PIO Linus Walleij
@ 2024-01-25 16:37 ` Arnd Bergmann
2024-01-25 23:18 ` Linus Walleij
0 siblings, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2024-01-25 16:37 UTC (permalink / raw
To: Linus Walleij, Christoph Hellwig, Jens Axboe, Ming Lei,
Ulf Hansson, Nicolas Pitre, Aaro Koskinen, Adrian Hunter,
Angelo Dureghello
Cc: linux-mmc @ vger . kernel . org, linux-block, Linux-OMAP
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/7] mmc: davinci_mmc: Map the virtual page for PIO
2024-01-25 16:37 ` Arnd Bergmann
@ 2024-01-25 23:18 ` Linus Walleij
0 siblings, 0 replies; 11+ messages in thread
From: Linus Walleij @ 2024-01-25 23:18 UTC (permalink / raw
To: Arnd Bergmann
Cc: Christoph Hellwig, Jens Axboe, Ming Lei, Ulf Hansson,
Nicolas Pitre, Aaro Koskinen, Adrian Hunter, Angelo Dureghello,
linux-mmc @ vger . kernel . org, linux-block, Linux-OMAP
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/7] mmc: Try to do proper kmap_local() for scatterlists
2024-01-25 14:37 [PATCH 0/7] mmc: Try to do proper kmap_local() for scatterlists Linus Walleij
` (6 preceding siblings ...)
2024-01-25 14:37 ` [PATCH 7/7] mmc: sh_mmcif: Map the virtual page for PIO Linus Walleij
@ 2024-02-13 16:45 ` Ulf Hansson
7 siblings, 0 replies; 11+ messages in thread
From: Ulf Hansson @ 2024-02-13 16:45 UTC (permalink / raw
To: Linus Walleij
Cc: Christoph Hellwig, Jens Axboe, Ming Lei, Arnd Bergmann,
Nicolas Pitre, Aaro Koskinen, Adrian Hunter, Angelo Dureghello,
linux-mmc, linux-block, linux-omap
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>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-02-13 16:46 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-25 14:37 [PATCH 0/7] mmc: Try to do proper kmap_local() for scatterlists Linus Walleij
2024-01-25 14:37 ` [PATCH 1/7] mmc: davinci_mmc: Map the virtual page for PIO Linus Walleij
2024-01-25 16:37 ` Arnd Bergmann
2024-01-25 23:18 ` Linus Walleij
2024-01-25 14:37 ` [PATCH 2/7] mmc: moxart-mmc: " Linus Walleij
2024-01-25 14:37 ` [PATCH 3/7] mmc: mvsdio: " Linus Walleij
2024-01-25 14:37 ` [PATCH 4/7] mmc: mxcmmc: " Linus Walleij
2024-01-25 14:37 ` [PATCH 5/7] mmc: omap: " Linus Walleij
2024-01-25 14:37 ` [PATCH 6/7] mmc: sdhci-esdhc-mcf: Map the virtual page for swapping Linus Walleij
2024-01-25 14:37 ` [PATCH 7/7] mmc: sh_mmcif: Map the virtual page for PIO Linus Walleij
2024-02-13 16:45 ` [PATCH 0/7] mmc: Try to do proper kmap_local() for scatterlists Ulf Hansson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).