| Message ID | 20240222-fix-sdhci-esdhc-mcf-v1-1-fb87e04ca575@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: sdhci-esdhc-mcf: Flag if the sg_miter is atomic | expand |
On 22/02/24 02:00, Linus Walleij wrote: > The sg_miter used to loop over the returned sglist from a > transfer in the esdhc subdriver for SDHCI needs to know if > it is being used in process or atomic context. > > This creates a problem because we cannot unconditionally > add SG_MITER_ATOMIC to the miter, as that can create > scheduling while atomic dmesg splats. > > Bit the bullet and make the .request_done() callback in > struct sdhci_ops aware of whether it is called from atomic > context or not, and only add the flag when actually called > from atomic context. > > sdhci_request_done() is always called from process context, > either as a work or as part of the threaded interrupt handler, > so this will pass false for is_atomic, and the one case when > we are actually calling .request_done() from an atomic context > is in sdhci_irq(). > > Fixes: e8a167b84886 ("mmc: sdhci-esdhc-mcf: Use sg_miter for swapping") I notice, in fact, that the driver is already using a bounce buffer always: static int sdhci_esdhc_mcf_probe(struct platform_device *pdev) ... if (!host->bounce_buffer) { dev_err(&pdev->dev, "bounce buffer not allocated"); err = -ENOMEM; goto cleanup; } ... Doesn't that mean the original patch is not needed? > Reported-by: Adrian Hunter <adrian.hunter@intel.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/mmc/host/sdhci-esdhc-mcf.c | 8 ++++++-- > drivers/mmc/host/sdhci-pxav2.c | 2 +- > drivers/mmc/host/sdhci-sprd.c | 3 ++- > drivers/mmc/host/sdhci.c | 4 ++-- > drivers/mmc/host/sdhci.h | 3 ++- > 5 files changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-esdhc-mcf.c b/drivers/mmc/host/sdhci-esdhc-mcf.c > index 1909a11fd065..7b0686701c6e 100644 > --- a/drivers/mmc/host/sdhci-esdhc-mcf.c > +++ b/drivers/mmc/host/sdhci-esdhc-mcf.c > @@ -297,8 +297,10 @@ 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 mmc_request *mrq, > + bool is_atomic) > { > + unsigned int miter_flags = SG_MITER_TO_SG | SG_MITER_FROM_SG; > struct sg_mapping_iter sgm; > u32 *buffer; > > @@ -312,8 +314,10 @@ 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. > */ > + if (is_atomic) > + miter_flags |= SG_MITER_ATOMIC; > sg_miter_start(&sgm, mrq->data->sg, mrq->data->sg_len, > - SG_MITER_TO_SG | SG_MITER_FROM_SG); > + miter_flags); > while (sg_miter_next(&sgm)) { > buffer = sgm.addr; > esdhc_mcf_buffer_swap32(buffer, sgm.length); > diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c > index b75cbea88b40..c1b49c2feea8 100644 > --- a/drivers/mmc/host/sdhci-pxav2.c > +++ b/drivers/mmc/host/sdhci-pxav2.c > @@ -120,7 +120,7 @@ static u32 pxav1_irq(struct sdhci_host *host, u32 intmask) > return intmask; > } > > -static void pxav1_request_done(struct sdhci_host *host, struct mmc_request *mrq) > +static void pxav1_request_done(struct sdhci_host *host, struct mmc_request *mrq, bool is_atomic) > { > u16 tmp; > struct sdhci_pxav2_host *pxav2_host; > diff --git a/drivers/mmc/host/sdhci-sprd.c b/drivers/mmc/host/sdhci-sprd.c > index bed57a1c64b5..a2b244e13451 100644 > --- a/drivers/mmc/host/sdhci-sprd.c > +++ b/drivers/mmc/host/sdhci-sprd.c > @@ -411,7 +411,8 @@ static unsigned int sdhci_sprd_get_ro(struct sdhci_host *host) > } > > static void sdhci_sprd_request_done(struct sdhci_host *host, > - struct mmc_request *mrq) > + struct mmc_request *mrq, > + bool is_atomic) > { > /* Validate if the request was from software queue firstly. */ > if (mmc_hsq_finalize_request(host->mmc, mrq)) > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index c79f73459915..3c6a9ba573cc 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -3184,7 +3184,7 @@ static bool sdhci_request_done(struct sdhci_host *host) > spin_unlock_irqrestore(&host->lock, flags); > > if (host->ops->request_done) > - host->ops->request_done(host, mrq); > + host->ops->request_done(host, mrq, false); > else > mmc_request_done(host->mmc, mrq); > > @@ -3639,7 +3639,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id) > continue; > > if (host->ops->request_done) > - host->ops->request_done(host, mrqs_done[i]); > + host->ops->request_done(host, mrqs_done[i], true); > else > mmc_request_done(host->mmc, mrqs_done[i]); > } > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h > index a20864fc0641..c2ff327e2c96 100644 > --- a/drivers/mmc/host/sdhci.h > +++ b/drivers/mmc/host/sdhci.h > @@ -666,7 +666,8 @@ struct sdhci_ops { > struct mmc_data *data, > unsigned int length); > void (*request_done)(struct sdhci_host *host, > - struct mmc_request *mrq); > + struct mmc_request *mrq, > + bool is_atomic); > void (*dump_vendor_regs)(struct sdhci_host *host); > }; > > > --- > base-commit: 2d5c7b7eb345249cb34d42cbc2b97b4c57ea944e > change-id: 20240222-fix-sdhci-esdhc-mcf-aa062c072800 > > Best regards,
On Thu, Feb 22, 2024 at 7:22 AM Adrian Hunter <adrian.hunter@intel.com> wrote: > On 22/02/24 02:00, Linus Walleij wrote: > > The sg_miter used to loop over the returned sglist from a > > transfer in the esdhc subdriver for SDHCI needs to know if > > it is being used in process or atomic context. > > > > This creates a problem because we cannot unconditionally > > add SG_MITER_ATOMIC to the miter, as that can create > > scheduling while atomic dmesg splats. > > > > Bit the bullet and make the .request_done() callback in > > struct sdhci_ops aware of whether it is called from atomic > > context or not, and only add the flag when actually called > > from atomic context. > > > > sdhci_request_done() is always called from process context, > > either as a work or as part of the threaded interrupt handler, > > so this will pass false for is_atomic, and the one case when > > we are actually calling .request_done() from an atomic context > > is in sdhci_irq(). > > > > Fixes: e8a167b84886 ("mmc: sdhci-esdhc-mcf: Use sg_miter for swapping") > > I notice, in fact, that the driver is already using a bounce > buffer always: > > static int sdhci_esdhc_mcf_probe(struct platform_device *pdev) > ... > if (!host->bounce_buffer) { > dev_err(&pdev->dev, "bounce buffer not allocated"); > err = -ENOMEM; > goto cleanup; > } > ... > > Doesn't that mean the original patch is not needed? TBH I just followed the pattern to handle sglists everywhere the same way. I looked closer at it: on the write path what you say is definately correct: we copy the data to the bounce buffer and byte swap it in esdhc_mcf_copy_to_bounce_buffer() and that buffer is a GFP_KERNEL allocation so it will be in lowmem. As we can see in sdhci_pre_dma_transfer() this is however as the name suggests only copying *to* the bounce buffer on mem->device transfers using DMA, i.e. when writing blocks. (Small writes is where we saw the big win with this bounce buffer when we wrote the code.) In the case of incoming data, reading blocks, DMA will read data into the sglist locations, which are some physical memory. This could very well be in highmem, especially for prefetching. Then at the end of a read esdhc_mcf_request_done() is called to byteswap incoming data, and if that is in highmem we need this sg miter walking code. So I think this code is needed, at least theoretically. Then whether ColdFire m5441x will use highmem is another question. I don't know anything about the ColdFire memory configurations so I converted it on a "better safe than sorry"-basis. arch/m68k/include/asm/mcf_pgalloc.h makes an effort to avoid putting page tables into highmem, and I suppose that is for a reason so this device can actually have highmem? Yours, Linus Walleij
On 22/02/24 10:40, Linus Walleij wrote: > On Thu, Feb 22, 2024 at 7:22 AM Adrian Hunter <adrian.hunter@intel.com> wrote: >> On 22/02/24 02:00, Linus Walleij wrote: >>> The sg_miter used to loop over the returned sglist from a >>> transfer in the esdhc subdriver for SDHCI needs to know if >>> it is being used in process or atomic context. >>> >>> This creates a problem because we cannot unconditionally >>> add SG_MITER_ATOMIC to the miter, as that can create >>> scheduling while atomic dmesg splats. >>> >>> Bit the bullet and make the .request_done() callback in >>> struct sdhci_ops aware of whether it is called from atomic >>> context or not, and only add the flag when actually called >>> from atomic context. >>> >>> sdhci_request_done() is always called from process context, >>> either as a work or as part of the threaded interrupt handler, >>> so this will pass false for is_atomic, and the one case when >>> we are actually calling .request_done() from an atomic context >>> is in sdhci_irq(). >>> >>> Fixes: e8a167b84886 ("mmc: sdhci-esdhc-mcf: Use sg_miter for swapping") >> >> I notice, in fact, that the driver is already using a bounce >> buffer always: >> >> static int sdhci_esdhc_mcf_probe(struct platform_device *pdev) >> ... >> if (!host->bounce_buffer) { >> dev_err(&pdev->dev, "bounce buffer not allocated"); >> err = -ENOMEM; >> goto cleanup; >> } >> ... >> >> Doesn't that mean the original patch is not needed? > > TBH I just followed the pattern to handle sglists everywhere the same > way. > > I looked closer at it: on the write path what you say is definately correct: > we copy the data to the bounce buffer and byte swap it in > esdhc_mcf_copy_to_bounce_buffer() and that buffer is a GFP_KERNEL > allocation so it will be in lowmem. > > As we can see in sdhci_pre_dma_transfer() this is however as the name > suggests only copying *to* the bounce buffer on mem->device transfers > using DMA, i.e. when writing blocks. (Small writes is where we saw the > big win with this bounce buffer when we wrote the code.) > > In the case of incoming data, reading blocks, DMA will read data into the > sglist locations, which are some physical memory. This could very well > be in highmem, especially for prefetching. Then at the end of a read > esdhc_mcf_request_done() is called to byteswap incoming data, and > if that is in highmem we need this sg miter walking code. > > So I think this code is needed, at least theoretically. > > Then whether ColdFire m5441x will use highmem is another > question. I don't know anything about the ColdFire memory configurations > so I converted it on a "better safe than sorry"-basis. > arch/m68k/include/asm/mcf_pgalloc.h makes an effort to avoid putting > page tables into highmem, and I suppose that is for a reason so this device > can actually have highmem? Dunno But passing around is_atomic seems ugly And esdhc_mcf_buffer_swap32() doesn't sleep, so there should not be a problem using kmap_atomic() As an aside, gotta wonder why there is not: #define SG_MITER_LOCAL_PAGE (1 << 3) /* use kmap_local_page */
On Wed, Feb 28, 2024 at 9:36 AM Adrian Hunter <adrian.hunter@intel.com> wrote: > But passing around is_atomic seems ugly > > And esdhc_mcf_buffer_swap32() doesn't sleep, so there should not > be a problem using kmap_atomic() Aha, I'll send a simple patch just making the iteration atomic so we don't overwork this. > As an aside, gotta wonder why there is not: > > #define SG_MITER_LOCAL_PAGE (1 << 3) /* use kmap_local_page */ That beats me, but Christoph probably has a good answer. Yours, Linus Walleij
On Wed, Feb 28, 2024 at 09:59:12AM +0100, Linus Walleij wrote: > Aha, I'll send a simple patch just making the iteration atomic > so we don't overwork this. > > > As an aside, gotta wonder why there is not: > > > > #define SG_MITER_LOCAL_PAGE (1 << 3) /* use kmap_local_page */ > > That beats me, but Christoph probably has a good answer. Probably just because no one got to it yet.
diff --git a/drivers/mmc/host/sdhci-esdhc-mcf.c b/drivers/mmc/host/sdhci-esdhc-mcf.c index 1909a11fd065..7b0686701c6e 100644 --- a/drivers/mmc/host/sdhci-esdhc-mcf.c +++ b/drivers/mmc/host/sdhci-esdhc-mcf.c @@ -297,8 +297,10 @@ 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 mmc_request *mrq, + bool is_atomic) { + unsigned int miter_flags = SG_MITER_TO_SG | SG_MITER_FROM_SG; struct sg_mapping_iter sgm; u32 *buffer; @@ -312,8 +314,10 @@ 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. */ + if (is_atomic) + miter_flags |= SG_MITER_ATOMIC; sg_miter_start(&sgm, mrq->data->sg, mrq->data->sg_len, - SG_MITER_TO_SG | SG_MITER_FROM_SG); + miter_flags); while (sg_miter_next(&sgm)) { buffer = sgm.addr; esdhc_mcf_buffer_swap32(buffer, sgm.length); diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c index b75cbea88b40..c1b49c2feea8 100644 --- a/drivers/mmc/host/sdhci-pxav2.c +++ b/drivers/mmc/host/sdhci-pxav2.c @@ -120,7 +120,7 @@ static u32 pxav1_irq(struct sdhci_host *host, u32 intmask) return intmask; } -static void pxav1_request_done(struct sdhci_host *host, struct mmc_request *mrq) +static void pxav1_request_done(struct sdhci_host *host, struct mmc_request *mrq, bool is_atomic) { u16 tmp; struct sdhci_pxav2_host *pxav2_host; diff --git a/drivers/mmc/host/sdhci-sprd.c b/drivers/mmc/host/sdhci-sprd.c index bed57a1c64b5..a2b244e13451 100644 --- a/drivers/mmc/host/sdhci-sprd.c +++ b/drivers/mmc/host/sdhci-sprd.c @@ -411,7 +411,8 @@ static unsigned int sdhci_sprd_get_ro(struct sdhci_host *host) } static void sdhci_sprd_request_done(struct sdhci_host *host, - struct mmc_request *mrq) + struct mmc_request *mrq, + bool is_atomic) { /* Validate if the request was from software queue firstly. */ if (mmc_hsq_finalize_request(host->mmc, mrq)) diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index c79f73459915..3c6a9ba573cc 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -3184,7 +3184,7 @@ static bool sdhci_request_done(struct sdhci_host *host) spin_unlock_irqrestore(&host->lock, flags); if (host->ops->request_done) - host->ops->request_done(host, mrq); + host->ops->request_done(host, mrq, false); else mmc_request_done(host->mmc, mrq); @@ -3639,7 +3639,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id) continue; if (host->ops->request_done) - host->ops->request_done(host, mrqs_done[i]); + host->ops->request_done(host, mrqs_done[i], true); else mmc_request_done(host->mmc, mrqs_done[i]); } diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h index a20864fc0641..c2ff327e2c96 100644 --- a/drivers/mmc/host/sdhci.h +++ b/drivers/mmc/host/sdhci.h @@ -666,7 +666,8 @@ struct sdhci_ops { struct mmc_data *data, unsigned int length); void (*request_done)(struct sdhci_host *host, - struct mmc_request *mrq); + struct mmc_request *mrq, + bool is_atomic); void (*dump_vendor_regs)(struct sdhci_host *host); };
The sg_miter used to loop over the returned sglist from a transfer in the esdhc subdriver for SDHCI needs to know if it is being used in process or atomic context. This creates a problem because we cannot unconditionally add SG_MITER_ATOMIC to the miter, as that can create scheduling while atomic dmesg splats. Bit the bullet and make the .request_done() callback in struct sdhci_ops aware of whether it is called from atomic context or not, and only add the flag when actually called from atomic context. sdhci_request_done() is always called from process context, either as a work or as part of the threaded interrupt handler, so this will pass false for is_atomic, and the one case when we are actually calling .request_done() from an atomic context is in sdhci_irq(). Fixes: e8a167b84886 ("mmc: sdhci-esdhc-mcf: Use sg_miter for swapping") Reported-by: Adrian Hunter <adrian.hunter@intel.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/mmc/host/sdhci-esdhc-mcf.c | 8 ++++++-- drivers/mmc/host/sdhci-pxav2.c | 2 +- drivers/mmc/host/sdhci-sprd.c | 3 ++- drivers/mmc/host/sdhci.c | 4 ++-- drivers/mmc/host/sdhci.h | 3 ++- 5 files changed, 13 insertions(+), 7 deletions(-) --- base-commit: 2d5c7b7eb345249cb34d42cbc2b97b4c57ea944e change-id: 20240222-fix-sdhci-esdhc-mcf-aa062c072800 Best regards,