| Message ID | 20240127-mmc-proper-kmap-v2-5-d8e732aa97d1@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: Use proper sg_miter for scatterlists | expand |
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
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
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 >
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; }
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(-)