[PATCH v2 0/9] mmc: Use proper sg_miter for scatterlists

[PATCH v2 0/9] mmc: Use proper sg_miter 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 traversal
of the scatterlists using sg_miter helpers that will
kmap() the pages properly, possibly bouncing
it from highmem if need be.

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

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes in v2:
- Bite the bullet and just patch all drivers to use sg_miter
- Link to v1: https://lore.kernel.org/r/20240125-mmc-proper-kmap-v1-0-ba953c1ac3f9@linaro.org

---
Linus Walleij (9):
      mmc: davinci_mmc: Use sg_miter for PIO
      mmc: moxart-mmc: Factor out moxart_use_dma() helper
      mmc: moxart-mmc: Fix accounting in DMA transfer
      mmc: moxart-mmc: Use sg_miter for PIO
      mmc: mvsdio: Use sg_miter for PIO
      mmc: mxcmmc: Use sg_miter for PIO
      mmc: omap: Use sg_miter for PIO
      mmc: sdhci-esdhc-mcf: Use sg_miter for swapping
      mmc: sh_mmcif: Use sg_miter for PIO

 drivers/mmc/host/davinci_mmc.c     |  61 ++++++++++------------
 drivers/mmc/host/moxart-mmc.c      |  90 ++++++++++++++++----------------
 drivers/mmc/host/mvsdio.c          |  71 +++++++++++++++++++-------
 drivers/mmc/host/mxcmmc.c          |  53 +++++++++++--------
 drivers/mmc/host/omap.c            |  53 +++++++++----------
 drivers/mmc/host/sdhci-esdhc-mcf.c |  12 +++--
 drivers/mmc/host/sh_mmcif.c        | 102 +++++++++++++++++++++++--------------
 7 files changed, 251 insertions(+), 191 deletions(-)
---
base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
change-id: 20240125-mmc-proper-kmap-f2d4cf5d1756

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

[PATCH v2 1/9] mmc: davinci_mmc: Use sg_miter for PIO

[Linus Walleij]

Use the scatterlist memory iterator instead of just
dereferencing virtual memory using sg_virt().
This make highmem references work properly.

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 | 61 +++++++++++++++++++-----------------------
 1 file changed, 28 insertions(+), 33 deletions(-)

diff --git a/drivers/mmc/host/davinci_mmc.c b/drivers/mmc/host/davinci_mmc.c
index ee3b1a4e0848..c46577305138 100644
--- a/drivers/mmc/host/davinci_mmc.c
+++ b/drivers/mmc/host/davinci_mmc.c
@@ -180,12 +180,6 @@ struct mmc_davinci_host {
 #define DAVINCI_MMC_DATADIR_WRITE	2
 	unsigned char data_dir;
 
-	/* buffer is used during PIO of one scatterlist segment, and
-	 * is updated along with buffer_bytes_left.  bytes_left applies
-	 * to all N blocks of the PIO transfer.
-	 */
-	u8 *buffer;
-	u32 buffer_bytes_left;
 	u32 bytes_left;
 
 	struct dma_chan *dma_tx;
@@ -196,8 +190,8 @@ struct mmc_davinci_host {
 	bool active_request;
 
 	/* For PIO we walk scatterlists one segment at a time. */
+	struct sg_mapping_iter sg_miter;
 	unsigned int		sg_len;
-	struct scatterlist *sg;
 
 	/* Version of the MMC/SD controller */
 	u8 version;
@@ -213,30 +207,24 @@ struct mmc_davinci_host {
 static irqreturn_t mmc_davinci_irq(int irq, void *dev_id);
 
 /* PIO only */
-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);
-	if (host->buffer_bytes_left > host->bytes_left)
-		host->buffer_bytes_left = host->bytes_left;
-}
-
 static void davinci_fifo_data_trans(struct mmc_davinci_host *host,
 					unsigned int n)
 {
+	struct sg_mapping_iter *sgm = &host->sg_miter;
+	size_t sglen;
 	u8 *p;
 	unsigned int i;
 
-	if (host->buffer_bytes_left == 0) {
-		host->sg = sg_next(host->data->sg);
-		mmc_davinci_sg_to_buf(host);
+	/*
+	 * By adjusting sgm->consumed this will give a pointer to the
+	 * current index into the sgm.
+	 */
+	if (!sg_miter_next(sgm)) {
+		dev_err(mmc_dev(host->mmc), "ran out of sglist prematurely\n");
+		return;
 	}
-
-	p = host->buffer;
-	if (n > host->buffer_bytes_left)
-		n = host->buffer_bytes_left;
-	host->buffer_bytes_left -= n;
-	host->bytes_left -= n;
+	p = sgm->addr;
+	sglen = sgm->length;
 
 	/* NOTE:  we never transfer more than rw_threshold bytes
 	 * to/from the fifo here; there's no I/O overlap.
@@ -261,7 +249,9 @@ static void davinci_fifo_data_trans(struct mmc_davinci_host *host,
 			p = p + (n & 3);
 		}
 	}
-	host->buffer = p;
+
+	sgm->consumed = n;
+	host->bytes_left -= n;
 }
 
 static void mmc_davinci_start_command(struct mmc_davinci_host *host,
@@ -517,6 +507,7 @@ mmc_davinci_prepare_data(struct mmc_davinci_host *host, struct mmc_request *req)
 	int fifo_lev = (rw_threshold == 32) ? MMCFIFOCTL_FIFOLEV : 0;
 	int timeout;
 	struct mmc_data *data = req->data;
+	unsigned int flags = SG_MITER_ATOMIC; /* Used from IRQ */
 
 	if (host->version == MMC_CTLR_VERSION_2)
 		fifo_lev = (rw_threshold == 64) ? MMCFIFOCTL_FIFOLEV : 0;
@@ -545,12 +536,14 @@ mmc_davinci_prepare_data(struct mmc_davinci_host *host, struct mmc_request *req)
 
 	/* Configure the FIFO */
 	if (data->flags & MMC_DATA_WRITE) {
+		flags |= SG_MITER_FROM_SG;
 		host->data_dir = DAVINCI_MMC_DATADIR_WRITE;
 		writel(fifo_lev | MMCFIFOCTL_FIFODIR_WR | MMCFIFOCTL_FIFORST,
 			host->base + DAVINCI_MMCFIFOCTL);
 		writel(fifo_lev | MMCFIFOCTL_FIFODIR_WR,
 			host->base + DAVINCI_MMCFIFOCTL);
 	} else {
+		flags |= SG_MITER_TO_SG;
 		host->data_dir = DAVINCI_MMC_DATADIR_READ;
 		writel(fifo_lev | MMCFIFOCTL_FIFODIR_RD | MMCFIFOCTL_FIFORST,
 			host->base + DAVINCI_MMCFIFOCTL);
@@ -558,7 +551,6 @@ mmc_davinci_prepare_data(struct mmc_davinci_host *host, struct mmc_request *req)
 			host->base + DAVINCI_MMCFIFOCTL);
 	}
 
-	host->buffer = NULL;
 	host->bytes_left = data->blocks * data->blksz;
 
 	/* For now we try to use DMA whenever we won't need partial FIFO
@@ -576,8 +568,7 @@ mmc_davinci_prepare_data(struct mmc_davinci_host *host, struct mmc_request *req)
 	} else {
 		/* Revert to CPU Copy */
 		host->sg_len = data->sg_len;
-		host->sg = host->data->sg;
-		mmc_davinci_sg_to_buf(host);
+		sg_miter_start(&host->sg_miter, data->sg, data->sg_len, flags);
 	}
 }
 
@@ -843,6 +834,8 @@ davinci_abort_data(struct mmc_davinci_host *host, struct mmc_data *data)
 {
 	mmc_davinci_reset_ctrl(host, 1);
 	mmc_davinci_reset_ctrl(host, 0);
+	if (!host->do_dma)
+		sg_miter_stop(&host->sg_miter);
 }
 
 static irqreturn_t mmc_davinci_sdio_irq(int irq, void *dev_id)
@@ -919,11 +912,13 @@ static irqreturn_t mmc_davinci_irq(int irq, void *dev_id)
 	if (qstatus & MMCST0_DATDNE) {
 		/* All blocks sent/received, and CRC checks passed */
 		if (data != NULL) {
-			if ((host->do_dma == 0) && (host->bytes_left > 0)) {
-				/* if datasize < rw_threshold
-				 * no RX ints are generated
-				 */
-				davinci_fifo_data_trans(host, host->bytes_left);
+			if (!host->do_dma) {
+				if (host->bytes_left > 0)
+					/* if datasize < rw_threshold
+					 * no RX ints are generated
+					 */
+					davinci_fifo_data_trans(host, host->bytes_left);
+				sg_miter_stop(&host->sg_miter);
 			}
 			end_transfer = 1;
 			data->bytes_xfered = data->blocks * data->blksz;

-- 
2.34.1

[PATCH v2 2/9] mmc: moxart-mmc: Factor out moxart_use_dma() helper

[Linus Walleij]

The same code is in two places and we will add a third place.
Break this out into its own function.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/moxart-mmc.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/moxart-mmc.c b/drivers/mmc/host/moxart-mmc.c
index 5cfdd3a86e54..d12d7d79b19c 100644
--- a/drivers/mmc/host/moxart-mmc.c
+++ b/drivers/mmc/host/moxart-mmc.c
@@ -254,6 +254,11 @@ static void moxart_dma_complete(void *param)
 	complete(&host->dma_complete);
 }
 
+static bool moxart_use_dma(struct moxart_host *host)
+{
+	return (host->data_len > host->fifo_width) && host->have_dma;
+}
+
 static void moxart_transfer_dma(struct mmc_data *data, struct moxart_host *host)
 {
 	u32 len, dir_slave;
@@ -375,7 +380,7 @@ static void moxart_prepare_data(struct moxart_host *host)
 	if (data->flags & MMC_DATA_WRITE)
 		datactrl |= DCR_DATA_WRITE;
 
-	if ((host->data_len > host->fifo_width) && host->have_dma)
+	if (moxart_use_dma(host))
 		datactrl |= DCR_DMA_EN;
 
 	writel(DCR_DATA_FIFO_RESET, host->base + REG_DATA_CONTROL);
@@ -407,7 +412,7 @@ static void moxart_request(struct mmc_host *mmc, struct mmc_request *mrq)
 	moxart_send_command(host, host->mrq->cmd);
 
 	if (mrq->cmd->data) {
-		if ((host->data_len > host->fifo_width) && host->have_dma) {
+		if (moxart_use_dma(host)) {
 
 			writel(CARD_CHANGE, host->base + REG_INTERRUPT_MASK);
 

-- 
2.34.1

[PATCH v2 3/9] mmc: moxart-mmc: Fix accounting in DMA transfer

[Linus Walleij]

The whole scatterlist chain is submitted to the DMA engine,
but the code is written to just account for the length of
the first sg entry.

When the DMA transfer is finished, all the data in the
request has been transferred, account for this instead.

This only works because the moxart_request() function isn't
checking that all data was transferred and will
unconditionally issue mmc_request_done() after returning
successfully from moxart_transfer_dma().

Keep the assignment of accounted bytes in .bytes_xfered
but move it after the completion where we know it has
actually happened.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/moxart-mmc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/moxart-mmc.c b/drivers/mmc/host/moxart-mmc.c
index d12d7d79b19c..8ede4ce93271 100644
--- a/drivers/mmc/host/moxart-mmc.c
+++ b/drivers/mmc/host/moxart-mmc.c
@@ -296,11 +296,11 @@ static void moxart_transfer_dma(struct mmc_data *data, struct moxart_host *host)
 		dma_async_issue_pending(dma_chan);
 	}
 
-	data->bytes_xfered += host->data_remain;
-
 	wait_for_completion_interruptible_timeout(&host->dma_complete,
 						  host->timeout);
 
+	data->bytes_xfered = host->data_len;
+
 	dma_unmap_sg(dma_chan->device->dev,
 		     data->sg, data->sg_len,
 		     mmc_get_dma_dir(data));

-- 
2.34.1

[PATCH v2 4/9] mmc: moxart-mmc: Use sg_miter for PIO

[Linus Walleij]

Use the scatterlist memory iterator instead of just
dereferencing virtual memory using sg_virt().
This make highmem references work properly.

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 | 77 +++++++++++++++++++------------------------
 1 file changed, 34 insertions(+), 43 deletions(-)

diff --git a/drivers/mmc/host/moxart-mmc.c b/drivers/mmc/host/moxart-mmc.c
index 8ede4ce93271..b88d6dec209f 100644
--- a/drivers/mmc/host/moxart-mmc.c
+++ b/drivers/mmc/host/moxart-mmc.c
@@ -131,12 +131,10 @@ struct moxart_host {
 	struct dma_async_tx_descriptor	*tx_desc;
 	struct mmc_host			*mmc;
 	struct mmc_request		*mrq;
-	struct scatterlist		*cur_sg;
 	struct completion		dma_complete;
 	struct completion		pio_complete;
 
-	u32				num_sg;
-	u32				data_remain;
+	struct sg_mapping_iter		sg_miter;
 	u32				data_len;
 	u32				fifo_width;
 	u32				timeout;
@@ -148,35 +146,6 @@ struct moxart_host {
 	bool				is_removed;
 };
 
-static inline void moxart_init_sg(struct moxart_host *host,
-				  struct mmc_data *data)
-{
-	host->cur_sg = data->sg;
-	host->num_sg = data->sg_len;
-	host->data_remain = host->cur_sg->length;
-
-	if (host->data_remain > host->data_len)
-		host->data_remain = host->data_len;
-}
-
-static inline int moxart_next_sg(struct moxart_host *host)
-{
-	int remain;
-	struct mmc_data *data = host->mrq->cmd->data;
-
-	host->cur_sg++;
-	host->num_sg--;
-
-	if (host->num_sg > 0) {
-		host->data_remain = host->cur_sg->length;
-		remain = host->data_len - data->bytes_xfered;
-		if (remain > 0 && remain < host->data_remain)
-			host->data_remain = remain;
-	}
-
-	return host->num_sg;
-}
-
 static int moxart_wait_for_status(struct moxart_host *host,
 				  u32 mask, u32 *status)
 {
@@ -309,14 +278,28 @@ static void moxart_transfer_dma(struct mmc_data *data, struct moxart_host *host)
 
 static void moxart_transfer_pio(struct moxart_host *host)
 {
+	struct sg_mapping_iter *sgm = &host->sg_miter;
 	struct mmc_data *data = host->mrq->cmd->data;
 	u32 *sgp, len = 0, remain, status;
 
 	if (host->data_len == data->bytes_xfered)
 		return;
 
-	sgp = sg_virt(host->cur_sg);
-	remain = host->data_remain;
+	/*
+	 * By updating sgm->consumes this will get a proper pointer into the
+	 * buffer at any time.
+	 */
+	if (!sg_miter_next(sgm)) {
+		/* This shold not happen */
+		dev_err(mmc_dev(host->mmc), "ran out of scatterlist prematurely\n");
+		data->error = -EINVAL;
+		complete(&host->pio_complete);
+		return;
+	}
+	sgp = sgm->addr;
+	remain = sgm->length;
+	if (remain > host->data_len)
+		remain = host->data_len;
 
 	if (data->flags & MMC_DATA_WRITE) {
 		while (remain > 0) {
@@ -331,6 +314,7 @@ static void moxart_transfer_pio(struct moxart_host *host)
 				sgp++;
 				len += 4;
 			}
+			sgm->consumed += len;
 			remain -= len;
 		}
 
@@ -347,22 +331,22 @@ static void moxart_transfer_pio(struct moxart_host *host)
 				sgp++;
 				len += 4;
 			}
+			sgm->consumed += len;
 			remain -= len;
 		}
 	}
 
-	data->bytes_xfered += host->data_remain - remain;
-	host->data_remain = remain;
-
-	if (host->data_len != data->bytes_xfered)
-		moxart_next_sg(host);
-	else
+	data->bytes_xfered += sgm->consumed;
+	if (host->data_len == data->bytes_xfered) {
 		complete(&host->pio_complete);
+		return;
+	}
 }
 
 static void moxart_prepare_data(struct moxart_host *host)
 {
 	struct mmc_data *data = host->mrq->cmd->data;
+	unsigned int flags = SG_MITER_ATOMIC; /* Used from IRQ */
 	u32 datactrl;
 	int blksz_bits;
 
@@ -373,15 +357,19 @@ static void moxart_prepare_data(struct moxart_host *host)
 	blksz_bits = ffs(data->blksz) - 1;
 	BUG_ON(1 << blksz_bits != data->blksz);
 
-	moxart_init_sg(host, data);
-
 	datactrl = DCR_DATA_EN | (blksz_bits & DCR_BLK_SIZE);
 
-	if (data->flags & MMC_DATA_WRITE)
+	if (data->flags & MMC_DATA_WRITE) {
+		flags |= SG_MITER_FROM_SG;
 		datactrl |= DCR_DATA_WRITE;
+	} else {
+		flags |= SG_MITER_TO_SG;
+	}
 
 	if (moxart_use_dma(host))
 		datactrl |= DCR_DMA_EN;
+	else
+		sg_miter_start(&host->sg_miter, data->sg, data->sg_len, flags);
 
 	writel(DCR_DATA_FIFO_RESET, host->base + REG_DATA_CONTROL);
 	writel(MASK_DATA | FIFO_URUN | FIFO_ORUN, host->base + REG_CLEAR);
@@ -454,6 +442,9 @@ static void moxart_request(struct mmc_host *mmc, struct mmc_request *mrq)
 	}
 
 request_done:
+	if (!moxart_use_dma(host))
+		sg_miter_stop(&host->sg_miter);
+
 	spin_unlock_irqrestore(&host->lock, flags);
 	mmc_request_done(host->mmc, mrq);
 }

-- 
2.34.1

[PATCH v2 5/9] mmc: mvsdio: Use sg_miter for PIO

[Linus Walleij]

Use the scatterlist memory iterator instead of just
dereferencing virtual memory using sg_virt().
This make highmem references work properly.

This driver also has a bug in the PIO sglist handling that
is fixed as part of the patch: it does not travers the
list of scatterbuffers: it will just process the first
item in the list. This is fixed by augmenting the logic
such that we do not process more than one sgitem
per IRQ instead of counting down potentially the whole
length of the request.

We can suspect that the PIO path is quite untested.

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 | 71 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 53 insertions(+), 18 deletions(-)

diff --git a/drivers/mmc/host/mvsdio.c b/drivers/mmc/host/mvsdio.c
index ca01b7d204ba..af7f21888e27 100644
--- a/drivers/mmc/host/mvsdio.c
+++ b/drivers/mmc/host/mvsdio.c
@@ -38,8 +38,9 @@ struct mvsd_host {
 	unsigned int xfer_mode;
 	unsigned int intr_en;
 	unsigned int ctrl;
+	bool use_pio;
+	struct sg_mapping_iter sg_miter;
 	unsigned int pio_size;
-	void *pio_ptr;
 	unsigned int sg_frags;
 	unsigned int ns_per_clk;
 	unsigned int clock;
@@ -114,11 +115,18 @@ static int mvsd_setup_data(struct mvsd_host *host, struct mmc_data *data)
 		 * data when the buffer is not aligned on a 64 byte
 		 * boundary.
 		 */
+		unsigned int miter_flags = SG_MITER_ATOMIC; /* Used from IRQ */
+
+		if (data->flags & MMC_DATA_READ)
+			miter_flags |= SG_MITER_TO_SG;
+		else
+			miter_flags |= SG_MITER_FROM_SG;
+
 		host->pio_size = data->blocks * data->blksz;
-		host->pio_ptr = sg_virt(data->sg);
+		sg_miter_start(&host->sg_miter, data->sg, data->sg_len, miter_flags);
 		if (!nodma)
-			dev_dbg(host->dev, "fallback to PIO for data at 0x%p size %d\n",
-				host->pio_ptr, host->pio_size);
+			dev_dbg(host->dev, "fallback to PIO for data\n");
+		host->use_pio = true;
 		return 1;
 	} else {
 		dma_addr_t phys_addr;
@@ -129,6 +137,7 @@ static int mvsd_setup_data(struct mvsd_host *host, struct mmc_data *data)
 		phys_addr = sg_dma_address(data->sg);
 		mvsd_write(MVSD_SYS_ADDR_LOW, (u32)phys_addr & 0xffff);
 		mvsd_write(MVSD_SYS_ADDR_HI,  (u32)phys_addr >> 16);
+		host->use_pio = false;
 		return 0;
 	}
 }
@@ -288,8 +297,8 @@ static u32 mvsd_finish_data(struct mvsd_host *host, struct mmc_data *data,
 {
 	void __iomem *iobase = host->base;
 
-	if (host->pio_ptr) {
-		host->pio_ptr = NULL;
+	if (host->use_pio) {
+		sg_miter_stop(&host->sg_miter);
 		host->pio_size = 0;
 	} else {
 		dma_unmap_sg(mmc_dev(host->mmc), data->sg, host->sg_frags,
@@ -344,9 +353,12 @@ static u32 mvsd_finish_data(struct mvsd_host *host, struct mmc_data *data,
 static irqreturn_t mvsd_irq(int irq, void *dev)
 {
 	struct mvsd_host *host = dev;
+	struct sg_mapping_iter *sgm = &host->sg_miter;
 	void __iomem *iobase = host->base;
 	u32 intr_status, intr_done_mask;
 	int irq_handled = 0;
+	u16 *p;
+	int s;
 
 	intr_status = mvsd_read(MVSD_NOR_INTR_STATUS);
 	dev_dbg(host->dev, "intr 0x%04x intr_en 0x%04x hw_state 0x%04x\n",
@@ -370,15 +382,36 @@ static irqreturn_t mvsd_irq(int irq, void *dev)
 	spin_lock(&host->lock);
 
 	/* PIO handling, if needed. Messy business... */
-	if (host->pio_size &&
+	if (host->use_pio) {
+		/*
+		 * As we set sgm->consumed this always gives a valid buffer
+		 * position.
+		 */
+		if (!sg_miter_next(sgm)) {
+			/* This should not happen */
+			dev_err(host->dev, "ran out of scatter segments\n");
+			spin_unlock(&host->lock);
+			host->intr_en &=
+				~(MVSD_NOR_RX_READY | MVSD_NOR_RX_FIFO_8W |
+				  MVSD_NOR_TX_AVAIL | MVSD_NOR_TX_FIFO_8W);
+			mvsd_write(MVSD_NOR_INTR_EN, host->intr_en);
+			return IRQ_HANDLED;
+		}
+		p = sgm->addr;
+		s = sgm->length;
+		if (s > host->pio_size)
+			s = host->pio_size;
+	}
+
+	if (host->use_pio &&
 	    (intr_status & host->intr_en &
 	     (MVSD_NOR_RX_READY | MVSD_NOR_RX_FIFO_8W))) {
-		u16 *p = host->pio_ptr;
-		int s = host->pio_size;
+
 		while (s >= 32 && (intr_status & MVSD_NOR_RX_FIFO_8W)) {
 			readsw(iobase + MVSD_FIFO, p, 16);
 			p += 16;
 			s -= 32;
+			sgm->consumed += 32;
 			intr_status = mvsd_read(MVSD_NOR_INTR_STATUS);
 		}
 		/*
@@ -391,6 +424,7 @@ static irqreturn_t mvsd_irq(int irq, void *dev)
 				put_unaligned(mvsd_read(MVSD_FIFO), p++);
 				put_unaligned(mvsd_read(MVSD_FIFO), p++);
 				s -= 4;
+				sgm->consumed += 4;
 				intr_status = mvsd_read(MVSD_NOR_INTR_STATUS);
 			}
 			if (s && s < 4 && (intr_status & MVSD_NOR_RX_READY)) {
@@ -398,10 +432,13 @@ static irqreturn_t mvsd_irq(int irq, void *dev)
 				val[0] = mvsd_read(MVSD_FIFO);
 				val[1] = mvsd_read(MVSD_FIFO);
 				memcpy(p, ((void *)&val) + 4 - s, s);
+				sgm->consumed += s;
 				s = 0;
 				intr_status = mvsd_read(MVSD_NOR_INTR_STATUS);
 			}
-			if (s == 0) {
+			/* PIO transfer done */
+			host->pio_size -= sgm->consumed;
+			if (host->pio_size == 0) {
 				host->intr_en &=
 				     ~(MVSD_NOR_RX_READY | MVSD_NOR_RX_FIFO_8W);
 				mvsd_write(MVSD_NOR_INTR_EN, host->intr_en);
@@ -413,14 +450,10 @@ static irqreturn_t mvsd_irq(int irq, void *dev)
 		}
 		dev_dbg(host->dev, "pio %d intr 0x%04x hw_state 0x%04x\n",
 			s, intr_status, mvsd_read(MVSD_HW_STATE));
-		host->pio_ptr = p;
-		host->pio_size = s;
 		irq_handled = 1;
-	} else if (host->pio_size &&
+	} else if (host->use_pio &&
 		   (intr_status & host->intr_en &
 		    (MVSD_NOR_TX_AVAIL | MVSD_NOR_TX_FIFO_8W))) {
-		u16 *p = host->pio_ptr;
-		int s = host->pio_size;
 		/*
 		 * The TX_FIFO_8W bit is unreliable. When set, bursting
 		 * 16 halfwords all at once in the FIFO drops data. Actually
@@ -431,6 +464,7 @@ static irqreturn_t mvsd_irq(int irq, void *dev)
 			mvsd_write(MVSD_FIFO, get_unaligned(p++));
 			mvsd_write(MVSD_FIFO, get_unaligned(p++));
 			s -= 4;
+			sgm->consumed += 4;
 			intr_status = mvsd_read(MVSD_NOR_INTR_STATUS);
 		}
 		if (s < 4) {
@@ -439,10 +473,13 @@ static irqreturn_t mvsd_irq(int irq, void *dev)
 				memcpy(((void *)&val) + 4 - s, p, s);
 				mvsd_write(MVSD_FIFO, val[0]);
 				mvsd_write(MVSD_FIFO, val[1]);
+				sgm->consumed += s;
 				s = 0;
 				intr_status = mvsd_read(MVSD_NOR_INTR_STATUS);
 			}
-			if (s == 0) {
+			/* PIO transfer done */
+			host->pio_size -= sgm->consumed;
+			if (host->pio_size == 0) {
 				host->intr_en &=
 				     ~(MVSD_NOR_TX_AVAIL | MVSD_NOR_TX_FIFO_8W);
 				mvsd_write(MVSD_NOR_INTR_EN, host->intr_en);
@@ -450,8 +487,6 @@ static irqreturn_t mvsd_irq(int irq, void *dev)
 		}
 		dev_dbg(host->dev, "pio %d intr 0x%04x hw_state 0x%04x\n",
 			s, intr_status, mvsd_read(MVSD_HW_STATE));
-		host->pio_ptr = p;
-		host->pio_size = s;
 		irq_handled = 1;
 	}
 

-- 
2.34.1

Re: [PATCH v2 5/9] mmc: mvsdio: Use sg_miter for PIO

[Nicolas Pitre]

On Sat, 27 Jan 2024, Linus Walleij wrote:

> Use the scatterlist memory iterator instead of just
> dereferencing virtual memory using sg_virt().
> This make highmem references work properly.
> 
> This driver also has a bug in the PIO sglist handling that
> is fixed as part of the patch: it does not travers the
> list of scatterbuffers: it will just process the first
> item in the list. This is fixed by augmenting the logic
> such that we do not process more than one sgitem
> per IRQ instead of counting down potentially the whole
> length of the request.
> 
> We can suspect that the PIO path is quite untested.

It was tested for sure ... at least by myself ... some 17 years ago !

> 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>

[...]

>  		if (!nodma)
> -			dev_dbg(host->dev, "fallback to PIO for data at 0x%p size %d\n",
> -				host->pio_ptr, host->pio_size);
> +			dev_dbg(host->dev, "fallback to PIO for data\n");

Given this message is about telling you why PIO is used despite not 
having asked for it, I think it would be nicer to preserve the 
equivalent info responsible for this infliction i.e. data->sg->offset 
and data->blksz.

The rest looks sane to me ( ... I think).


Nicolas

Re: [PATCH v2 5/9] mmc: mvsdio: Use sg_miter for PIO

[Linus Walleij]

Hi Nico!

nice to mail with you as always!

On Sat, Jan 27, 2024 at 4:51 AM Nicolas Pitre <nico@fluxnic.net> wrote:
> On Sat, 27 Jan 2024, Linus Walleij wrote:
>
> > Use the scatterlist memory iterator instead of just
> > dereferencing virtual memory using sg_virt().
> > This make highmem references work properly.
> >
> > This driver also has a bug in the PIO sglist handling that
> > is fixed as part of the patch: it does not travers the
> > list of scatterbuffers: it will just process the first
> > item in the list. This is fixed by augmenting the logic
> > such that we do not process more than one sgitem
> > per IRQ instead of counting down potentially the whole
> > length of the request.
> >
> > We can suspect that the PIO path is quite untested.
>
> It was tested for sure ... at least by myself ... some 17 years ago !

Hm, on the DMA path the code is taking struct mmc_data .sg_len
into account but not on the polled I/O path.

But I think sg_len is very often 1, as long as the memory isn't very
fragmented so pieces of a file you read/write are all over the place.

It needs to be tested under high memory pressure to provoke errors
I think. I'm not sure, the block layer people may have some secret
testing trick! (I actually have this hardware in a Kirkwood NAS.)

> >               if (!nodma)
> > -                     dev_dbg(host->dev, "fallback to PIO for data at 0x%p size %d\n",
> > -                             host->pio_ptr, host->pio_size);
> > +                     dev_dbg(host->dev, "fallback to PIO for data\n");
>
> Given this message is about telling you why PIO is used despite not
> having asked for it, I think it would be nicer to preserve the
> equivalent info responsible for this infliction i.e. data->sg->offset
> and data->blksz.

OK I fix!

Yours,
Linus Walleij

Re: [PATCH v2 5/9] mmc: mvsdio: Use sg_miter for PIO

[Nicolas Pitre]

[-- Attachment #1: Type: text/plain, Size: 2327 bytes --]

On Sat, 27 Jan 2024, Linus Walleij wrote:

> Hi Nico!
> 
> nice to mail with you as always!
> 
> On Sat, Jan 27, 2024 at 4:51 AM Nicolas Pitre <nico@fluxnic.net> wrote:
> > On Sat, 27 Jan 2024, Linus Walleij wrote:
> >
> > > Use the scatterlist memory iterator instead of just
> > > dereferencing virtual memory using sg_virt().
> > > This make highmem references work properly.
> > >
> > > This driver also has a bug in the PIO sglist handling that
> > > is fixed as part of the patch: it does not travers the
> > > list of scatterbuffers: it will just process the first
> > > item in the list. This is fixed by augmenting the logic
> > > such that we do not process more than one sgitem
> > > per IRQ instead of counting down potentially the whole
> > > length of the request.
> > >
> > > We can suspect that the PIO path is quite untested.
> >
> > It was tested for sure ... at least by myself ... some 17 years ago !
> 
> Hm, on the DMA path the code is taking struct mmc_data .sg_len
> into account but not on the polled I/O path.
> 
> But I think sg_len is very often 1, as long as the memory isn't very
> fragmented so pieces of a file you read/write are all over the place.
> 
> It needs to be tested under high memory pressure to provoke errors
> I think. I'm not sure, the block layer people may have some secret
> testing trick! (I actually have this hardware in a Kirkwood NAS.)

Oh, I don't mean to imply that the testing was thorough. Especially 
given that, under normal circumstances, you're always using DMA with 
nicely aligned and sized blocks.

But SDIO is different (smallish buffers). So the PIO support was added 
only to work around hw bugs in that case.

Good you fixed it nevertheless.

> > >               if (!nodma)
> > > -                     dev_dbg(host->dev, "fallback to PIO for data at 0x%p size %d\n",
> > > -                             host->pio_ptr, host->pio_size);
> > > +                     dev_dbg(host->dev, "fallback to PIO for data\n");
> >
> > Given this message is about telling you why PIO is used despite not
> > having asked for it, I think it would be nicer to preserve the
> > equivalent info responsible for this infliction i.e. data->sg->offset
> > and data->blksz.
> 
> OK I fix!
> 
> Yours,
> Linus Walleij
> 

[PATCH v2 6/9] mmc: mxcmmc: Use sg_miter for PIO

[Linus Walleij]

Use the scatterlist memory iterator instead of just
dereferencing virtual memory using sg_virt().
This make highmem references work properly.

Since this driver is using a worker, no atomic trickery
is needed.

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 | 53 +++++++++++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index 5b3ab0e20505..1edf65291354 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -266,11 +266,18 @@ static inline void buffer_swap32(u32 *buf, int len)
 
 static void mxcmci_swap_buffers(struct mmc_data *data)
 {
-	struct scatterlist *sg;
-	int i;
+	struct sg_mapping_iter sgm;
+	u32 *buf;
+
+	sg_miter_start(&sgm, data->sg, data->sg_len,
+		       SG_MITER_TO_SG | SG_MITER_FROM_SG);
+
+	while (sg_miter_next(&sgm)) {
+		buf = sgm.addr;
+		buffer_swap32(buf, sgm.length);
+	}
 
-	for_each_sg(data->sg, sg, data->sg_len, i)
-		buffer_swap32(sg_virt(sg), sg->length);
+	sg_miter_stop(&sgm);
 }
 #else
 static inline void mxcmci_swap_buffers(struct mmc_data *data) {}
@@ -526,10 +533,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 +561,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);
@@ -586,31 +591,39 @@ static int mxcmci_push(struct mxcmci_host *host, void *_buf, int bytes)
 static int mxcmci_transfer_data(struct mxcmci_host *host)
 {
 	struct mmc_data *data = host->req->data;
-	struct scatterlist *sg;
-	int stat, i;
+	struct sg_mapping_iter sgm;
+	int stat;
+	u32 *buf;
 
 	host->data = data;
 	host->datasize = 0;
+	sg_miter_start(&sgm, data->sg, data->sg_len,
+		       (data->flags & MMC_DATA_READ) ? SG_MITER_TO_SG : SG_MITER_FROM_SG);
 
 	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);
+		while (sg_miter_next(&sgm)) {
+			buf = sgm.addr;
+			stat = mxcmci_pull(host, buf, sgm.length);
 			if (stat)
-				return stat;
-			host->datasize += sg->length;
+				goto transfer_error;
+			host->datasize += sgm.length;
 		}
 	} else {
-		for_each_sg(data->sg, sg, data->sg_len, i) {
-			stat = mxcmci_push(host, sg_virt(sg), sg->length);
+		while (sg_miter_next(&sgm)) {
+			buf = sgm.addr;
+			stat = mxcmci_push(host, buf, sgm.length);
 			if (stat)
-				return stat;
-			host->datasize += sg->length;
+				goto transfer_error;
+			host->datasize += sgm.length;
 		}
 		stat = mxcmci_poll_status(host, STATUS_WRITE_OP_DONE);
 		if (stat)
-			return stat;
+			goto transfer_error;
 	}
-	return 0;
+
+transfer_error:
+	sg_miter_stop(&sgm);
+	return stat;
 }
 
 static void mxcmci_datawork(struct work_struct *work)

-- 
2.34.1

[PATCH v2 7/9] mmc: omap: Use sg_miter for PIO

[Linus Walleij]

Use the scatterlist memory iterator instead of just
dereferencing virtual memory using sg_virt().
This make highmem references work properly.

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 | 53 ++++++++++++++++++++++---------------------------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/drivers/mmc/host/omap.c b/drivers/mmc/host/omap.c
index 9fb8995b43a1..088f8ed4fdc4 100644
--- a/drivers/mmc/host/omap.c
+++ b/drivers/mmc/host/omap.c
@@ -148,10 +148,8 @@ struct mmc_omap_host {
 	struct work_struct      send_stop_work;
 	struct mmc_data		*stop_data;
 
+	struct sg_mapping_iter	sg_miter;
 	unsigned int		sg_len;
-	int			sg_idx;
-	u16 *			buffer;
-	u32			buffer_bytes_left;
 	u32			total_bytes_left;
 
 	unsigned		features;
@@ -456,6 +454,8 @@ mmc_omap_xfer_done(struct mmc_omap_host *host, struct mmc_data *data)
 {
 	if (host->dma_in_use)
 		mmc_omap_release_dma(host, data, data->error);
+	else
+		sg_miter_stop(&host->sg_miter);
 
 	host->data = NULL;
 	host->sg_len = 0;
@@ -651,19 +651,6 @@ mmc_omap_cmd_timer(struct timer_list *t)
 	spin_unlock_irqrestore(&host->slot_lock, flags);
 }
 
-/* PIO only */
-static void
-mmc_omap_sg_to_buf(struct mmc_omap_host *host)
-{
-	struct scatterlist *sg;
-
-	sg = host->data->sg + host->sg_idx;
-	host->buffer_bytes_left = sg->length;
-	host->buffer = sg_virt(sg);
-	if (host->buffer_bytes_left > host->total_bytes_left)
-		host->buffer_bytes_left = host->total_bytes_left;
-}
-
 static void
 mmc_omap_clk_timer(struct timer_list *t)
 {
@@ -676,33 +663,37 @@ mmc_omap_clk_timer(struct timer_list *t)
 static void
 mmc_omap_xfer_data(struct mmc_omap_host *host, int write)
 {
+	struct sg_mapping_iter *sgm = &host->sg_miter;
 	int n, nwords;
+	u16 *buffer;
 
-	if (host->buffer_bytes_left == 0) {
-		host->sg_idx++;
-		BUG_ON(host->sg_idx == host->sg_len);
-		mmc_omap_sg_to_buf(host);
+	if (!sg_miter_next(sgm)) {
+		/* This should not happen */
+		dev_err(mmc_dev(host->mmc), "ran out of scatterlist prematurely\n");
+		return;
 	}
+	buffer = sgm->addr;
+
 	n = 64;
-	if (n > host->buffer_bytes_left)
-		n = host->buffer_bytes_left;
+	if (n > sgm->length)
+		n = sgm->length;
+	if (n > host->total_bytes_left)
+		n = host->total_bytes_left;
 
 	/* Round up to handle odd number of bytes to transfer */
 	nwords = DIV_ROUND_UP(n, 2);
 
-	host->buffer_bytes_left -= n;
+	sgm->consumed = n;
 	host->total_bytes_left -= n;
 	host->data->bytes_xfered += n;
 
 	if (write) {
 		__raw_writesw(host->virt_base + OMAP_MMC_REG(host, DATA),
-			      host->buffer, nwords);
+			      buffer, nwords);
 	} else {
 		__raw_readsw(host->virt_base + OMAP_MMC_REG(host, DATA),
-			     host->buffer, nwords);
+			     buffer, nwords);
 	}
-
-	host->buffer += nwords;
 }
 
 #ifdef CONFIG_MMC_DEBUG
@@ -956,6 +947,7 @@ static inline void set_data_timeout(struct mmc_omap_host *host, struct mmc_reque
 static void
 mmc_omap_prepare_data(struct mmc_omap_host *host, struct mmc_request *req)
 {
+	unsigned int miter_flags = SG_MITER_ATOMIC; /* Used from IRQ */
 	struct mmc_data *data = req->data;
 	int i, use_dma = 1, block_size;
 	struct scatterlist *sg;
@@ -990,7 +982,6 @@ mmc_omap_prepare_data(struct mmc_omap_host *host, struct mmc_request *req)
 		}
 	}
 
-	host->sg_idx = 0;
 	if (use_dma) {
 		enum dma_data_direction dma_data_dir;
 		struct dma_async_tx_descriptor *tx;
@@ -1071,7 +1062,11 @@ mmc_omap_prepare_data(struct mmc_omap_host *host, struct mmc_request *req)
 	OMAP_MMC_WRITE(host, BUF, 0x1f1f);
 	host->total_bytes_left = data->blocks * block_size;
 	host->sg_len = sg_len;
-	mmc_omap_sg_to_buf(host);
+	if (data->flags & MMC_DATA_READ)
+		miter_flags |= SG_MITER_TO_SG;
+	else
+		miter_flags |= SG_MITER_FROM_SG;
+	sg_miter_start(&host->sg_miter, data->sg, data->sg_len, miter_flags);
 	host->dma_in_use = 0;
 }
 

-- 
2.34.1

[PATCH v2 8/9] mmc: sdhci-esdhc-mcf: Use sg_miter for swapping

[Linus Walleij]

Use sg_miter iterator instead of sg_virt() and custom code
to loop over the scatterlist. The memory iterator will do
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 | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-mcf.c b/drivers/mmc/host/sdhci-esdhc-mcf.c
index a07f8333cd6b..1909a11fd065 100644
--- a/drivers/mmc/host/sdhci-esdhc-mcf.c
+++ b/drivers/mmc/host/sdhci-esdhc-mcf.c
@@ -299,9 +299,8 @@ static void esdhc_mcf_pltfm_set_bus_width(struct sdhci_host *host, int width)
 static void esdhc_mcf_request_done(struct sdhci_host *host,
 				   struct mmc_request *mrq)
 {
-	struct scatterlist *sg;
+	struct sg_mapping_iter sgm;
 	u32 *buffer;
-	int i;
 
 	if (!mrq->data || !mrq->data->bytes_xfered)
 		goto exit_done;
@@ -313,10 +312,13 @@ static void esdhc_mcf_request_done(struct sdhci_host *host,
 	 * On mcf5441x there is no hw sdma option/flag to select the dma
 	 * 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);
-		esdhc_mcf_buffer_swap32(buffer, sg->length);
+	sg_miter_start(&sgm, mrq->data->sg, mrq->data->sg_len,
+		       SG_MITER_TO_SG | SG_MITER_FROM_SG);
+	while (sg_miter_next(&sgm)) {
+		buffer = sgm.addr;
+		esdhc_mcf_buffer_swap32(buffer, sgm.length);
 	}
+	sg_miter_stop(&sgm);
 
 exit_done:
 	mmc_request_done(host->mmc, mrq);

-- 
2.34.1

Re: [PATCH v2 8/9] mmc: sdhci-esdhc-mcf: Use sg_miter for swapping

[Adrian Hunter]

On 27/01/24 02:19, Linus Walleij wrote:
> Use sg_miter iterator instead of sg_virt() and custom code
> to loop over the scatterlist. The memory iterator will do
> 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 | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-mcf.c b/drivers/mmc/host/sdhci-esdhc-mcf.c
> index a07f8333cd6b..1909a11fd065 100644
> --- a/drivers/mmc/host/sdhci-esdhc-mcf.c
> +++ b/drivers/mmc/host/sdhci-esdhc-mcf.c
> @@ -299,9 +299,8 @@ static void esdhc_mcf_pltfm_set_bus_width(struct sdhci_host *host, int width)
>  static void esdhc_mcf_request_done(struct sdhci_host *host,
>  				   struct mmc_request *mrq)
>  {
> -	struct scatterlist *sg;
> +	struct sg_mapping_iter sgm;
>  	u32 *buffer;
> -	int i;
>  
>  	if (!mrq->data || !mrq->data->bytes_xfered)
>  		goto exit_done;
> @@ -313,10 +312,13 @@ static void esdhc_mcf_request_done(struct sdhci_host *host,
>  	 * On mcf5441x there is no hw sdma option/flag to select the dma
>  	 * 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);
> -		esdhc_mcf_buffer_swap32(buffer, sg->length);
> +	sg_miter_start(&sgm, mrq->data->sg, mrq->data->sg_len,
> +		       SG_MITER_TO_SG | SG_MITER_FROM_SG);

Could be called from atomic context, so probably needs
SG_MITER_ATOMIC

> +	while (sg_miter_next(&sgm)) {
> +		buffer = sgm.addr;
> +		esdhc_mcf_buffer_swap32(buffer, sgm.length);
>  	}
> +	sg_miter_stop(&sgm);
>  
>  exit_done:
>  	mmc_request_done(host->mmc, mrq);
> 

[PATCH v2 9/9] mmc: sh_mmcif: Use sg_miter for PIO

[Linus Walleij]

Use sg_miter iterator instead of sg_virt() and custom code
to loop over the scatterlist. The memory iterator will do
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/sh_mmcif.c | 102 +++++++++++++++++++++++++++-----------------
 1 file changed, 63 insertions(+), 39 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 077d711e964e..1ef6e153e5a3 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -227,14 +227,12 @@ struct sh_mmcif_host {
 	bool dying;
 	long timeout;
 	void __iomem *addr;
-	u32 *pio_ptr;
 	spinlock_t lock;		/* protect sh_mmcif_host::state */
 	enum sh_mmcif_state state;
 	enum sh_mmcif_wait_for wait_for;
 	struct delayed_work timeout_work;
 	size_t blocksize;
-	int sg_idx;
-	int sg_blkidx;
+	struct sg_mapping_iter sg_miter;
 	bool power;
 	bool ccs_enable;		/* Command Completion Signal support */
 	bool clk_ctrl2_enable;
@@ -600,32 +598,17 @@ static int sh_mmcif_error_manage(struct sh_mmcif_host *host)
 	return ret;
 }
 
-static bool sh_mmcif_next_block(struct sh_mmcif_host *host, u32 *p)
-{
-	struct mmc_data *data = host->mrq->data;
-
-	host->sg_blkidx += host->blocksize;
-
-	/* data->sg->length must be a multiple of host->blocksize? */
-	BUG_ON(host->sg_blkidx > data->sg->length);
-
-	if (host->sg_blkidx == data->sg->length) {
-		host->sg_blkidx = 0;
-		if (++host->sg_idx < data->sg_len)
-			host->pio_ptr = sg_virt(++data->sg);
-	} else {
-		host->pio_ptr = p;
-	}
-
-	return host->sg_idx != data->sg_len;
-}
-
 static void sh_mmcif_single_read(struct sh_mmcif_host *host,
 				 struct mmc_request *mrq)
 {
+	struct mmc_data *data = mrq->data;
+
 	host->blocksize = (sh_mmcif_readl(host->addr, MMCIF_CE_BLOCK_SET) &
 			   BLOCK_SIZE_MASK) + 3;
 
+	sg_miter_start(&host->sg_miter, data->sg, data->sg_len,
+		       SG_MITER_ATOMIC | SG_MITER_TO_SG);
+
 	host->wait_for = MMCIF_WAIT_FOR_READ;
 
 	/* buf read enable */
@@ -634,20 +617,32 @@ static void sh_mmcif_single_read(struct sh_mmcif_host *host,
 
 static bool sh_mmcif_read_block(struct sh_mmcif_host *host)
 {
+	struct sg_mapping_iter *sgm = &host->sg_miter;
 	struct device *dev = sh_mmcif_host_to_dev(host);
 	struct mmc_data *data = host->mrq->data;
-	u32 *p = sg_virt(data->sg);
+	u32 *p;
 	int i;
 
 	if (host->sd_error) {
+		sg_miter_stop(sgm);
 		data->error = sh_mmcif_error_manage(host);
 		dev_dbg(dev, "%s(): %d\n", __func__, data->error);
 		return false;
 	}
 
+	if (!sg_miter_next(sgm)) {
+		/* This should not happen on single blocks */
+		sg_miter_stop(sgm);
+		return false;
+	}
+
+	p = sgm->addr;
+
 	for (i = 0; i < host->blocksize / 4; i++)
 		*p++ = sh_mmcif_readl(host->addr, MMCIF_CE_DATA);
 
+	sg_miter_stop(&host->sg_miter);
+
 	/* buffer read end */
 	sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MBUFRE);
 	host->wait_for = MMCIF_WAIT_FOR_READ_END;
@@ -666,34 +661,40 @@ static void sh_mmcif_multi_read(struct sh_mmcif_host *host,
 	host->blocksize = sh_mmcif_readl(host->addr, MMCIF_CE_BLOCK_SET) &
 		BLOCK_SIZE_MASK;
 
+	sg_miter_start(&host->sg_miter, data->sg, data->sg_len,
+		       SG_MITER_ATOMIC | SG_MITER_TO_SG);
+
 	host->wait_for = MMCIF_WAIT_FOR_MREAD;
-	host->sg_idx = 0;
-	host->sg_blkidx = 0;
-	host->pio_ptr = sg_virt(data->sg);
 
 	sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MBUFREN);
 }
 
 static bool sh_mmcif_mread_block(struct sh_mmcif_host *host)
 {
+	struct sg_mapping_iter *sgm = &host->sg_miter;
 	struct device *dev = sh_mmcif_host_to_dev(host);
 	struct mmc_data *data = host->mrq->data;
-	u32 *p = host->pio_ptr;
+	u32 *p;
 	int i;
 
 	if (host->sd_error) {
+		sg_miter_stop(sgm);
 		data->error = sh_mmcif_error_manage(host);
 		dev_dbg(dev, "%s(): %d\n", __func__, data->error);
 		return false;
 	}
 
-	BUG_ON(!data->sg->length);
+	if (!sg_miter_next(sgm)) {
+		sg_miter_stop(sgm);
+		return false;
+	}
+
+	p = sgm->addr;
 
 	for (i = 0; i < host->blocksize / 4; i++)
 		*p++ = sh_mmcif_readl(host->addr, MMCIF_CE_DATA);
 
-	if (!sh_mmcif_next_block(host, p))
-		return false;
+	sgm->consumed = host->blocksize;
 
 	sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MBUFREN);
 
@@ -703,9 +704,14 @@ static bool sh_mmcif_mread_block(struct sh_mmcif_host *host)
 static void sh_mmcif_single_write(struct sh_mmcif_host *host,
 					struct mmc_request *mrq)
 {
+	struct mmc_data *data = mrq->data;
+
 	host->blocksize = (sh_mmcif_readl(host->addr, MMCIF_CE_BLOCK_SET) &
 			   BLOCK_SIZE_MASK) + 3;
 
+	sg_miter_start(&host->sg_miter, data->sg, data->sg_len,
+		       SG_MITER_ATOMIC | SG_MITER_FROM_SG);
+
 	host->wait_for = MMCIF_WAIT_FOR_WRITE;
 
 	/* buf write enable */
@@ -714,20 +720,32 @@ static void sh_mmcif_single_write(struct sh_mmcif_host *host,
 
 static bool sh_mmcif_write_block(struct sh_mmcif_host *host)
 {
+	struct sg_mapping_iter *sgm = &host->sg_miter;
 	struct device *dev = sh_mmcif_host_to_dev(host);
 	struct mmc_data *data = host->mrq->data;
-	u32 *p = sg_virt(data->sg);
+	u32 *p;
 	int i;
 
 	if (host->sd_error) {
+		sg_miter_stop(sgm);
 		data->error = sh_mmcif_error_manage(host);
 		dev_dbg(dev, "%s(): %d\n", __func__, data->error);
 		return false;
 	}
 
+	if (!sg_miter_next(sgm)) {
+		/* This should not happen on single blocks */
+		sg_miter_stop(sgm);
+		return false;
+	}
+
+	p = sgm->addr;
+
 	for (i = 0; i < host->blocksize / 4; i++)
 		sh_mmcif_writel(host->addr, MMCIF_CE_DATA, *p++);
 
+	sg_miter_stop(&host->sg_miter);
+
 	/* buffer write end */
 	sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MDTRANE);
 	host->wait_for = MMCIF_WAIT_FOR_WRITE_END;
@@ -746,34 +764,40 @@ static void sh_mmcif_multi_write(struct sh_mmcif_host *host,
 	host->blocksize = sh_mmcif_readl(host->addr, MMCIF_CE_BLOCK_SET) &
 		BLOCK_SIZE_MASK;
 
+	sg_miter_start(&host->sg_miter, data->sg, data->sg_len,
+		       SG_MITER_ATOMIC | SG_MITER_FROM_SG);
+
 	host->wait_for = MMCIF_WAIT_FOR_MWRITE;
-	host->sg_idx = 0;
-	host->sg_blkidx = 0;
-	host->pio_ptr = sg_virt(data->sg);
 
 	sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MBUFWEN);
 }
 
 static bool sh_mmcif_mwrite_block(struct sh_mmcif_host *host)
 {
+	struct sg_mapping_iter *sgm = &host->sg_miter;
 	struct device *dev = sh_mmcif_host_to_dev(host);
 	struct mmc_data *data = host->mrq->data;
-	u32 *p = host->pio_ptr;
+	u32 *p;
 	int i;
 
 	if (host->sd_error) {
+		sg_miter_stop(sgm);
 		data->error = sh_mmcif_error_manage(host);
 		dev_dbg(dev, "%s(): %d\n", __func__, data->error);
 		return false;
 	}
 
-	BUG_ON(!data->sg->length);
+	if (!sg_miter_next(sgm)) {
+		sg_miter_stop(sgm);
+		return false;
+	}
+
+	p = sgm->addr;
 
 	for (i = 0; i < host->blocksize / 4; i++)
 		sh_mmcif_writel(host->addr, MMCIF_CE_DATA, *p++);
 
-	if (!sh_mmcif_next_block(host, p))
-		return false;
+	sgm->consumed = host->blocksize;
 
 	sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MBUFWEN);
 

-- 
2.34.1

Re: [PATCH v2 9/9] mmc: sh_mmcif: Use sg_miter for PIO

[Geert Uytterhoeven]

 	Hi Linus,

On Sat, 27 Jan 2024, Linus Walleij wrote:
> Use sg_miter iterator instead of sg_virt() and custom code
> to loop over the scatterlist. The memory iterator will do
> 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>

Thanks for your patch, which is now commit 27b57277d9ba3a90 ("mmc:
sh_mmcif: Use sg_miter for PIO") in mmc/next.

I have bisected the following BUGs on R-Mobile APE6 (also seen on
R-Mobile A1 and SH-Mobile AG5) to this commit:

     sh_mobile_sdhi ee120000.mmc: mmc1 base at 0xee120000, max clock rate 12 MHz
     mmc2: new high speed MMC card at address 0001
     sh_mobile_sdhi ee100000.mmc: mmc0 base at 0xee100000, max clock rate 88 MHz
     mmcblk2: mmc2:0001 MMC08G 7.33 GiB
     BUG: sleeping function called from invalid context at kernel/workqueue.c:3347
     in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 35, name: irq/151-ee20000
     preempt_count: 1, expected: 0
     no locks held by irq/151-ee20000/35.
     CPU: 0 PID: 35 Comm: irq/151-ee20000 Not tainted 6.8.0-rc4-ape6evm-00017-g27b57277d9ba #491
     Hardware name: Generic R8A73A4 (Flattened Device Tree)
      unwind_backtrace from show_stack+0x10/0x14
      show_stack from dump_stack_lvl+0x68/0x90
      dump_stack_lvl from __might_resched+0x1ac/0x228
      __might_resched from __flush_work+0x20c/0x2e4
      __flush_work from __cancel_work_timer+0x118/0x198
      __cancel_work_timer from sh_mmcif_irqt+0x38/0x8f8
      sh_mmcif_irqt from irq_thread_fn+0x1c/0x58
      irq_thread_fn from irq_thread+0x10c/0x218
      irq_thread from kthread+0xf0/0x100
      kthread from ret_from_fork+0x14/0x28
     Exception stack(0xf0959fb0 to 0xf0959ff8)
     9fa0:                                     00000000 00000000 00000000 00000000
     9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
     9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
     BUG: scheduling while atomic: irq/151-ee20000/35/0x00000002
     no locks held by irq/151-ee20000/35.
     CPU: 0 PID: 35 Comm: irq/151-ee20000 Tainted: G        W          6.8.0-rc4-ape6evm-00017-g27b57277d9ba #491
     Hardware name: Generic R8A73A4 (Flattened Device Tree)
      unwind_backtrace from show_stack+0x10/0x14
      show_stack from dump_stack_lvl+0x68/0x90
      dump_stack_lvl from __schedule_bug+0x5c/0x7c
      __schedule_bug from __schedule+0xa0/0x9bc
      __schedule from schedule+0x64/0x94
      schedule from irq_thread+0x1dc/0x218
      irq_thread from kthread+0xf0/0x100
      kthread from ret_from_fork+0x14/0x28
     Exception stack(0xf0959fb0 to 0xf0959ff8)
     9fa0:                                     00000000 00000000 00000000 00000000
     9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
     9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
     sh_mmcif ee200000.mmc: Timeout waiting for 2 on CMD18

Reverting this commit fixes the issue.

> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -227,14 +227,12 @@ struct sh_mmcif_host {
> 	bool dying;
> 	long timeout;
> 	void __iomem *addr;
> -	u32 *pio_ptr;
> 	spinlock_t lock;		/* protect sh_mmcif_host::state */
> 	enum sh_mmcif_state state;
> 	enum sh_mmcif_wait_for wait_for;
> 	struct delayed_work timeout_work;
> 	size_t blocksize;
> -	int sg_idx;
> -	int sg_blkidx;
> +	struct sg_mapping_iter sg_miter;
> 	bool power;
> 	bool ccs_enable;		/* Command Completion Signal support */
> 	bool clk_ctrl2_enable;
> @@ -600,32 +598,17 @@ static int sh_mmcif_error_manage(struct sh_mmcif_host *host)
> 	return ret;
> }
>
> -static bool sh_mmcif_next_block(struct sh_mmcif_host *host, u32 *p)
> -{
> -	struct mmc_data *data = host->mrq->data;
> -
> -	host->sg_blkidx += host->blocksize;
> -
> -	/* data->sg->length must be a multiple of host->blocksize? */
> -	BUG_ON(host->sg_blkidx > data->sg->length);
> -
> -	if (host->sg_blkidx == data->sg->length) {
> -		host->sg_blkidx = 0;
> -		if (++host->sg_idx < data->sg_len)
> -			host->pio_ptr = sg_virt(++data->sg);
> -	} else {
> -		host->pio_ptr = p;
> -	}
> -
> -	return host->sg_idx != data->sg_len;
> -}
> -
> static void sh_mmcif_single_read(struct sh_mmcif_host *host,
> 				 struct mmc_request *mrq)
> {
> +	struct mmc_data *data = mrq->data;
> +
> 	host->blocksize = (sh_mmcif_readl(host->addr, MMCIF_CE_BLOCK_SET) &
> 			   BLOCK_SIZE_MASK) + 3;
>
> +	sg_miter_start(&host->sg_miter, data->sg, data->sg_len,
> +		       SG_MITER_ATOMIC | SG_MITER_TO_SG);
> +
> 	host->wait_for = MMCIF_WAIT_FOR_READ;
>
> 	/* buf read enable */
> @@ -634,20 +617,32 @@ static void sh_mmcif_single_read(struct sh_mmcif_host *host,
>
> static bool sh_mmcif_read_block(struct sh_mmcif_host *host)
> {
> +	struct sg_mapping_iter *sgm = &host->sg_miter;
> 	struct device *dev = sh_mmcif_host_to_dev(host);
> 	struct mmc_data *data = host->mrq->data;
> -	u32 *p = sg_virt(data->sg);
> +	u32 *p;
> 	int i;
>
> 	if (host->sd_error) {
> +		sg_miter_stop(sgm);
> 		data->error = sh_mmcif_error_manage(host);
> 		dev_dbg(dev, "%s(): %d\n", __func__, data->error);
> 		return false;
> 	}
>
> +	if (!sg_miter_next(sgm)) {
> +		/* This should not happen on single blocks */
> +		sg_miter_stop(sgm);
> +		return false;
> +	}
> +
> +	p = sgm->addr;
> +
> 	for (i = 0; i < host->blocksize / 4; i++)
> 		*p++ = sh_mmcif_readl(host->addr, MMCIF_CE_DATA);
>
> +	sg_miter_stop(&host->sg_miter);
> +
> 	/* buffer read end */
> 	sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MBUFRE);
> 	host->wait_for = MMCIF_WAIT_FOR_READ_END;
> @@ -666,34 +661,40 @@ static void sh_mmcif_multi_read(struct sh_mmcif_host *host,
> 	host->blocksize = sh_mmcif_readl(host->addr, MMCIF_CE_BLOCK_SET) &
> 		BLOCK_SIZE_MASK;
>
> +	sg_miter_start(&host->sg_miter, data->sg, data->sg_len,
> +		       SG_MITER_ATOMIC | SG_MITER_TO_SG);
> +
> 	host->wait_for = MMCIF_WAIT_FOR_MREAD;
> -	host->sg_idx = 0;
> -	host->sg_blkidx = 0;
> -	host->pio_ptr = sg_virt(data->sg);
>
> 	sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MBUFREN);
> }
>
> static bool sh_mmcif_mread_block(struct sh_mmcif_host *host)
> {
> +	struct sg_mapping_iter *sgm = &host->sg_miter;
> 	struct device *dev = sh_mmcif_host_to_dev(host);
> 	struct mmc_data *data = host->mrq->data;
> -	u32 *p = host->pio_ptr;
> +	u32 *p;
> 	int i;
>
> 	if (host->sd_error) {
> +		sg_miter_stop(sgm);
> 		data->error = sh_mmcif_error_manage(host);
> 		dev_dbg(dev, "%s(): %d\n", __func__, data->error);
> 		return false;
> 	}
>
> -	BUG_ON(!data->sg->length);
> +	if (!sg_miter_next(sgm)) {
> +		sg_miter_stop(sgm);
> +		return false;
> +	}
> +
> +	p = sgm->addr;
>
> 	for (i = 0; i < host->blocksize / 4; i++)
> 		*p++ = sh_mmcif_readl(host->addr, MMCIF_CE_DATA);
>
> -	if (!sh_mmcif_next_block(host, p))
> -		return false;
> +	sgm->consumed = host->blocksize;
>
> 	sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MBUFREN);
>
> @@ -703,9 +704,14 @@ static bool sh_mmcif_mread_block(struct sh_mmcif_host *host)
> static void sh_mmcif_single_write(struct sh_mmcif_host *host,
> 					struct mmc_request *mrq)
> {
> +	struct mmc_data *data = mrq->data;
> +
> 	host->blocksize = (sh_mmcif_readl(host->addr, MMCIF_CE_BLOCK_SET) &
> 			   BLOCK_SIZE_MASK) + 3;
>
> +	sg_miter_start(&host->sg_miter, data->sg, data->sg_len,
> +		       SG_MITER_ATOMIC | SG_MITER_FROM_SG);
> +
> 	host->wait_for = MMCIF_WAIT_FOR_WRITE;
>
> 	/* buf write enable */
> @@ -714,20 +720,32 @@ static void sh_mmcif_single_write(struct sh_mmcif_host *host,
>
> static bool sh_mmcif_write_block(struct sh_mmcif_host *host)
> {
> +	struct sg_mapping_iter *sgm = &host->sg_miter;
> 	struct device *dev = sh_mmcif_host_to_dev(host);
> 	struct mmc_data *data = host->mrq->data;
> -	u32 *p = sg_virt(data->sg);
> +	u32 *p;
> 	int i;
>
> 	if (host->sd_error) {
> +		sg_miter_stop(sgm);
> 		data->error = sh_mmcif_error_manage(host);
> 		dev_dbg(dev, "%s(): %d\n", __func__, data->error);
> 		return false;
> 	}
>
> +	if (!sg_miter_next(sgm)) {
> +		/* This should not happen on single blocks */
> +		sg_miter_stop(sgm);
> +		return false;
> +	}
> +
> +	p = sgm->addr;
> +
> 	for (i = 0; i < host->blocksize / 4; i++)
> 		sh_mmcif_writel(host->addr, MMCIF_CE_DATA, *p++);
>
> +	sg_miter_stop(&host->sg_miter);
> +
> 	/* buffer write end */
> 	sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MDTRANE);
> 	host->wait_for = MMCIF_WAIT_FOR_WRITE_END;
> @@ -746,34 +764,40 @@ static void sh_mmcif_multi_write(struct sh_mmcif_host *host,
> 	host->blocksize = sh_mmcif_readl(host->addr, MMCIF_CE_BLOCK_SET) &
> 		BLOCK_SIZE_MASK;
>
> +	sg_miter_start(&host->sg_miter, data->sg, data->sg_len,
> +		       SG_MITER_ATOMIC | SG_MITER_FROM_SG);
> +
> 	host->wait_for = MMCIF_WAIT_FOR_MWRITE;
> -	host->sg_idx = 0;
> -	host->sg_blkidx = 0;
> -	host->pio_ptr = sg_virt(data->sg);
>
> 	sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MBUFWEN);
> }
>
> static bool sh_mmcif_mwrite_block(struct sh_mmcif_host *host)
> {
> +	struct sg_mapping_iter *sgm = &host->sg_miter;
> 	struct device *dev = sh_mmcif_host_to_dev(host);
> 	struct mmc_data *data = host->mrq->data;
> -	u32 *p = host->pio_ptr;
> +	u32 *p;
> 	int i;
>
> 	if (host->sd_error) {
> +		sg_miter_stop(sgm);
> 		data->error = sh_mmcif_error_manage(host);
> 		dev_dbg(dev, "%s(): %d\n", __func__, data->error);
> 		return false;
> 	}
>
> -	BUG_ON(!data->sg->length);
> +	if (!sg_miter_next(sgm)) {
> +		sg_miter_stop(sgm);
> +		return false;
> +	}
> +
> +	p = sgm->addr;
>
> 	for (i = 0; i < host->blocksize / 4; i++)
> 		sh_mmcif_writel(host->addr, MMCIF_CE_DATA, *p++);
>
> -	if (!sh_mmcif_next_block(host, p))
> -		return false;
> +	sgm->consumed = host->blocksize;
>
> 	sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MBUFWEN);
>

Gr{oetje,eeting}s,

 						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 							    -- Linus Torvalds

Re: [PATCH v2 9/9] mmc: sh_mmcif: Use sg_miter for PIO

[Linus Walleij]

Hi Geert,

sorry for the mess!

On Tue, Feb 20, 2024 at 10:03 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:

>      sh_mobile_sdhi ee120000.mmc: mmc1 base at 0xee120000, max clock rate 12 MHz
>      mmc2: new high speed MMC card at address 0001
>      sh_mobile_sdhi ee100000.mmc: mmc0 base at 0xee100000, max clock rate 88 MHz
>      mmcblk2: mmc2:0001 MMC08G 7.33 GiB

Hey it reads some blocks...

>      BUG: sleeping function called from invalid context at kernel/workqueue.c:3347
>      in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 35, name: irq/151-ee20000
(...)
>       __might_resched from __flush_work+0x20c/0x2e4
>       __flush_work from __cancel_work_timer+0x118/0x198
>       __cancel_work_timer from sh_mmcif_irqt+0x38/0x8f8
>       sh_mmcif_irqt from irq_thread_fn+0x1c/0x58

Actually that is the thread so the message is a bit confusing, the irq thread
isn't atomic.

I wonder if it is caused by this:

> > +     sg_miter_start(&host->sg_miter, data->sg, data->sg_len,
> > +                    SG_MITER_ATOMIC | SG_MITER_TO_SG);

...because I don't need to ask for atomic miter here, since the poll
functions are actually called in process context.

I've sent a patch, can you test?
https://lore.kernel.org/linux-mmc/20240220-fix-sh-mmcif-v1-1-b9d08a787c1f@linaro.org/T/#u

Yours,
Linus Walleij

Re: [PATCH v2 9/9] mmc: sh_mmcif: Use sg_miter for PIO

[Geert Uytterhoeven]

Hi Linus,

On Wed, Feb 21, 2024 at 12:01 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Feb 20, 2024 at 10:03 PM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>
> >      sh_mobile_sdhi ee120000.mmc: mmc1 base at 0xee120000, max clock rate 12 MHz
> >      mmc2: new high speed MMC card at address 0001
> >      sh_mobile_sdhi ee100000.mmc: mmc0 base at 0xee100000, max clock rate 88 MHz
> >      mmcblk2: mmc2:0001 MMC08G 7.33 GiB
>
> Hey it reads some blocks...
>
> >      BUG: sleeping function called from invalid context at kernel/workqueue.c:3347
> >      in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 35, name: irq/151-ee20000
> (...)
> >       __might_resched from __flush_work+0x20c/0x2e4
> >       __flush_work from __cancel_work_timer+0x118/0x198
> >       __cancel_work_timer from sh_mmcif_irqt+0x38/0x8f8
> >       sh_mmcif_irqt from irq_thread_fn+0x1c/0x58
>
> Actually that is the thread so the message is a bit confusing, the irq thread
> isn't atomic.
>
> I wonder if it is caused by this:
>
> > > +     sg_miter_start(&host->sg_miter, data->sg, data->sg_len,
> > > +                    SG_MITER_ATOMIC | SG_MITER_TO_SG);
>
> ...because I don't need to ask for atomic miter here, since the poll
> functions are actually called in process context.
>
> I've sent a patch, can you test?
> https://lore.kernel.org/linux-mmc/20240220-fix-sh-mmcif-v1-1-b9d08a787c1f@linaro.org/T/#u

While that patch fixes the BUG, it does not make the eMMC work fully.
It spews:

    sh_mmcif ee200000.mmc: Timeout waiting for 2 on CMD18

and no or limited data is read ("hd /dev/mmcblk..." blocks after no
or two lines of output).

I still need to revert 27b57277d9ba to restore proper operation.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 9/9] mmc: sh_mmcif: Use sg_miter for PIO

[Linus Walleij]

On Wed, Feb 21, 2024 at 10:50 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:

> > I've sent a patch, can you test?
> > https://lore.kernel.org/linux-mmc/20240220-fix-sh-mmcif-v1-1-b9d08a787c1f@linaro.org/T/#u
>
> While that patch fixes the BUG, it does not make the eMMC work fully.
> It spews:
>
>     sh_mmcif ee200000.mmc: Timeout waiting for 2 on CMD18
>
> and no or limited data is read ("hd /dev/mmcblk..." blocks after no
> or two lines of output).
>
> I still need to revert 27b57277d9ba to restore proper operation.

Halfway there. I looked at the code again and now I think I found the
problem causing CMD18 to time out.

I've send a new 2-patch series:
https://lore.kernel.org/linux-mmc/20240221-fix-sh-mmcif-v2-0-5e521eb25ae4@linaro.org/

Yours,
Linus Walleij

Re: [PATCH v2 9/9] mmc: sh_mmcif: Use sg_miter for PIO

[Geert Uytterhoeven]

Hi Linus,

On Wed, Feb 21, 2024 at 10:25 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Feb 21, 2024 at 10:50 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > > I've sent a patch, can you test?
> > > https://lore.kernel.org/linux-mmc/20240220-fix-sh-mmcif-v1-1-b9d08a787c1f@linaro.org/T/#u
> >
> > While that patch fixes the BUG, it does not make the eMMC work fully.
> > It spews:
> >
> >     sh_mmcif ee200000.mmc: Timeout waiting for 2 on CMD18
> >
> > and no or limited data is read ("hd /dev/mmcblk..." blocks after no
> > or two lines of output).
> >
> > I still need to revert 27b57277d9ba to restore proper operation.
>
> Halfway there. I looked at the code again and now I think I found the
> problem causing CMD18 to time out.
>
> I've send a new 2-patch series:
> https://lore.kernel.org/linux-mmc/20240221-fix-sh-mmcif-v2-0-5e521eb25ae4@linaro.org/

Thanks, life's good again ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds