Re: [PATCH] crypto: talitos: replace in_be32/out_be32 with ioread32be/iowrite32be

[Simon Richter]

Hi,

On 6/4/26 4:33 AM, Rosen Penev wrote:

> Convert ppc4xx-specific in_be32/out_be32 and the setbits32/clrbits32
> macros to the portable ioread32be/iowrite32be helpers.

It doesn't do that. The setbits32/clrbits32 macros are unchanged.

If they had been adapted, there would have been no need to inline the 
macro definition before substituting the IO accessors.

This inlining makes the code harder to read, because it consists of 
nested function calls (which have very specific and annoying indentation 
requirements that you're not following), and also duplicates the address 
calculation.

> Add COMPILE_TEST for extra compile coverage.

> Assisted-by: opencode:big-pickle

I suspect these two lines are related in a horrible way, and this code 
has only been compile-tested on the wrong architecture as part of the 
feedback loop. COMPILE_TEST is not necessary for compile-testing with a 
cross compiler and an appropriate defconfig for the target platform.

> -		setbits32(priv->chan[ch].reg + TALITOS_CCCR_LO,
> -			  TALITOS1_CCCR_LO_RESET);
> +		iowrite32be(ioread32be(priv->chan[ch].reg + TALITOS_CCCR_LO) |
> +				    (TALITOS1_CCCR_LO_RESET),
> +			    priv->chan[ch].reg + TALITOS_CCCR_LO);

Wrong formatting, and either the parentheses around 
TALITOS1_CCCR_LO_RESET are unnecessary here, or

> -		while ((in_be32(priv->chan[ch].reg + TALITOS_CCCR_LO) &
> -			TALITOS1_CCCR_LO_RESET) && --timeout)
> +		while ((ioread32be(priv->chan[ch].reg + TALITOS_CCCR_LO) &
> +			TALITOS1_CCCR_LO_RESET) &&
> +		       --timeout)

also needs them. The former is correct (macro definitions need to 
include parentheses if using them inside a calculation would give you 
unexpected operator precedence.

>   	/* set 36-bit addressing, done writeback enable and done IRQ enable */
> -	setbits32(priv->chan[ch].reg + TALITOS_CCCR_LO, TALITOS_CCCR_LO_EAE |
> -		  TALITOS_CCCR_LO_CDWE | TALITOS_CCCR_LO_CDIE);
> +	iowrite32be(ioread32be(priv->chan[ch].reg + TALITOS_CCCR_LO) |
> +			    (TALITOS_CCCR_LO_EAE | TALITOS_CCCR_LO_CDWE | TALITOS_CCCR_LO_CDIE),
> +		    priv->chan[ch].reg + TALITOS_CCCR_LO);

These parentheses are likewise unnecessary. It's a big OR, no need to 
group them.

\> +#define DEF_TALITOS1_DONE(name, ch_done_mask) 
                         \
> +	static void talitos1_done_##name(unsigned long data)                                  \

Inconsistent backslashes, and does not improve the horribleness that was 
there before, only makes it longer and harder to read.

>   	if (!desc_hdr)
> -		desc_hdr = cpu_to_be32(in_be32(priv->chan[ch].reg + TALITOS_DESCBUF));
> +		desc_hdr = cpu_to_be32(ioread32be(priv->chan[ch].reg + TALITOS_DESCBUF));

Likewise, this is bad and unreadable in the current state, and this 
patch does nothing to improve that.

> -		dev_err(dev, "AFEUISR 0x%08x_%08x\n",
> -			in_be32(priv->reg_afeu + TALITOS_EUISR),
> -			in_be32(priv->reg_afeu + TALITOS_EUISR_LO));
> +		dev_err(dev, "AFEUISR 0x%08x_%08x\n", ioread32be(priv->reg_afeu + TALITOS_EUISR),
> +			ioread32be(priv->reg_afeu + TALITOS_EUISR_LO));

You can probably see how the formatting is worse than before.

I'm not going to bother checking if this introduces any bugs, as I have 
the strong suspicion that I would be the first person reading this code.

   Simon