Re: [PATCHv2 4/4] 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] The transition to asm-generic readX()/writeX() changes the endianness of ColdFire internal IO accesses, breaking unpatched ColdFire drivers.

Pre-existing issues:
- [High] Undefined behavior when shifting 1 by 32 or more using `BIT(ch)` in `mcf_edma_tx_handler`.
- [High] The error interrupt handler incorrectly skips handling high channels and returns `IRQ_NONE` after servicing low channels.
--

commit 25193db3eba89f780b6dc534f2f72e16f3429571
Author: Greg Ungerer <gerg@linux-m68k.org>

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

Remove the local ColdFire definitions of readb()/readw()/readl() and
writeb()/writew()/writel() and use the asm-generic versions of them.

> diff --git a/arch/m68k/include/asm/io_no.h b/arch/m68k/include/asm/io_no.h
> index 4f0f34b06e37c..2f12f4ed0da54 100644
> --- a/arch/m68k/include/asm/io_no.h
> +++ b/arch/m68k/include/asm/io_no.h

[ ... ]

> -#define writel writel
> -static inline void writel(u32 value, volatile void __iomem *addr)
> -{
> -	if (cf_internalio(addr))
> -		__raw_writel(value, addr);
> -	else
> -		__raw_writel(swab32(value), addr);
> -}

[Severity: High]
Does replacing the local readX()/writeX() with the asm-generic versions
change the endianness of unpatched ColdFire drivers?

The old ColdFire implementation detected internal IO memory using
cf_internalio() and bypassed byte-swapping to provide native big-endian
access. The asm-generic accessors always assume little-endian devices and
perform byte-swapping.

Will unpatched drivers relying on readX()/writeX() for internal IO
(such as spi-coldfire-qspi.c, sdhci-esdhc-mcf.c, or fec_main.c) now read
and write byte-swapped values and malfunction?

[ ... ]

> 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 this shift invoke undefined behavior
for higher channels?

Looking at the loop immediately following this in mcf_edma_tx_handler():

	for (ch = 0; ch < mcf_edma->n_chans; ch++) {
		if (intmap & BIT(ch)) {
			...

On a 32-bit architecture like m68k, BIT(ch) expands to a 32-bit unsigned
long. Since ch can be up to 64, shifting by 32 or more would be undefined,
potentially causing channels 32-63 to be improperly processed or missing
their interrupts entirely.

Should this use BIT_ULL(ch) when testing against the 64-bit intmap?

[ ... ]

> @@ -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 is a pre-existing issue, but does this early return incorrectly skip
handling high channels?

If regs->errl is zero, it returns IRQ_NONE immediately, ignoring any errors
in regs->errh. This could leave errors in higher channels uncleared.

[ ... ]

> @@ -55,7 +55,7 @@ static irqreturn_t mcf_edma_err_handler(int irq, void *dev_id)
>  		}
>  	}
>  
> -	err = ioread32(regs->errh);
> +	err = ioread32be(regs->errh);
>  	if (!err)
>  		return IRQ_NONE;

[Severity: High]
This is a pre-existing issue, but does this return the wrong IRQ status if
low channel errors were already handled?

If errl was non-zero (so some errors were handled) but errh is zero,
returning IRQ_NONE here instead of IRQ_HANDLED might incorrectly signal
to the IRQ subsystem that the interrupt wasn't handled at all.

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