On 22/02/24 10:40, Linus Walleij wrote:
> 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?
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 */
