Re: [PATCHv3 3/3] m68k: coldfire: fix non-standard readX()/writeX() functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Replacing ColdFire's non-standard I/O accessors breaks eDMA descriptor loading by exposing a hidden endianness bug in `fsl-edma-common.h`, writing little-endian data to big-endian hardware.

Pre-existing issues:
- [High] The eDMA error interrupt handler silently ignores upper channel errors if no lower channel errors are present, and returns IRQ_NONE after successfully handling lower channel errors if no upper channel errors are present.
- [High] Using 32-bit `BIT()` macro with a shift count up to 63 causes undefined behavior, breaking interrupt handling for channels 32-63.
--

commit 90504ddd291ad9040d7cc40def38400a2da51323
Author: Greg Ungerer <gerg@linux-m68k.org>

m68k: coldfire: fix non-standard readX()/writeX() functions

This patch removes the local ColdFire definitions of read/write I/O
access functions and uses the asm-generic versions instead. It also
updates several drivers that previously relied on the non-standard
behavior to use the correct endian-specific accessors or quirks.

[ ... ]

> diff --git a/drivers/dma/mcf-edma-main.c b/drivers/dma/mcf-edma-main.c
> index 9e1c6400c77be..4ed0ce644e37e 100644
> --- a/drivers/dma/mcf-edma-main.c
> +++ b/drivers/dma/mcf-edma-main.c
> @@ -21,9 +21,9 @@ static irqreturn_t mcf_edma_tx_handler(int irq, void *dev_id)
>  	unsigned int ch;
>  	u64 intmap;
>  
> -	intmap = ioread32(regs->inth);
> +	intmap = ioread32be(regs->inth);
>  	intmap <<= 32;
> -	intmap |= ioread32(regs->intl);
> +	intmap |= ioread32be(regs->intl);
>  	if (!intmap)
>  		return IRQ_NONE;

[Severity: High]
This is a pre-existing issue, but does using the 32-bit BIT() macro just
below this block cause undefined behavior for channels 32-63?

In mcf_edma_tx_handler():
	for (ch = 0; ch < mcf_edma->n_chans; ch++) {
		if (intmap & BIT(ch)) {
			iowrite8(EDMA_MASK_CH(ch), regs->cint);

On 32-bit platforms, BIT(ch) evaluates modulo 32. When checking completions
for upper channels (e.g., ch=32), BIT(32) will evaluate to bit 0 instead
of bit 32.

If the wrong bit is checked, the interrupt for channels 32-63 won't be
cleared, leading to an interrupt storm. Should this use BIT_ULL(ch)
since intmap is a u64?

> @@ -43,7 +43,7 @@ static irqreturn_t mcf_edma_err_handler(int irq, void *dev_id)
>  	struct edma_regs *regs = &mcf_edma->regs;
>  	unsigned int err, ch;
>  
> -	err = ioread32(regs->errl);
> +	err = ioread32be(regs->errl);
>  	if (!err)
>  		return IRQ_NONE;

[Severity: High]
This isn't a bug introduced by this patch, but does this error handler 
incorrectly short-circuit if lower channels are error-free?

If a DMA error occurs on channels 32-63 (regs->errh), but not on 0-31
(regs->errl), the handler will hit this first if (!err) check and return
IRQ_NONE.

If it returns IRQ_NONE, the error is never cleared, leaving the IRQ
line asserted. Will this result in an interrupt storm that hangs the
CPU until the kernel permanently disables the IRQ?

[ ... ]

> @@ -203,8 +203,8 @@ static int mcf_edma_probe(struct platform_device *pdev)
>  		edma_write_tcdreg(mcf_chan, cpu_to_le32(0), csr);
>  	}
>  
> -	iowrite32(~0, regs->inth);
> -	iowrite32(~0, regs->intl);
> +	iowrite32be(~0, regs->inth);
> +	iowrite32be(~0, regs->intl);

[Severity: High]
Does removing the non-standard read/write behaviors expose a hidden
endianness bug in how fsl-edma-common.h programs the eDMA descriptors?

The mcf-edma probe function writes TCD registers via edma_write_tcdreg()
in this hunk:
        edma_write_tcdreg(mcf_chan, cpu_to_le32(0), csr);

Under the hood in fsl-edma-common.h, edma_write_tcdreg_c() casts this
little-endian value directly to a native u32 before passing it to
edma_writel() (which maps to iowrite32be on ColdFire).

Previously, the non-standard ColdFire I/O macros lacked byte-swapping,
so the driver's use of iowrite32be() implicitly double-swapped the
incorrectly cast u32 value back to a native Big-Endian value.

Now that the generic writel() corrects this and byte-swaps properly, will
the eDMA controller read garbled (byte-swapped) descriptors and corrupt
memory when DMA transfers are submitted?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907134037.1855408-4-gerg@linux-m68k.org?part=1