On Thu, Feb 22, 2024 at 7:22 AM Adrian Hunter <adrian.hunter@xxxxxxxxx> 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
