Re: [PATCH v3 3/4] spi: amlogic: spisg: Fix the incorrect keep_ss of the last descriptor

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The DMA descriptors are defined using native integer types and lack endianness conversions.
--

--- Patch [3]: [PATCH v3 3/4] spi: amlogic: spisg: Fix the incorrect keep_ss of the last descriptor ---
commit b9d6f07e79dcd1e9c5b73de8be6904945ef9d97b
Author: Sunny Luo <sunny.luo@amlogic.com>

spi: amlogic: spisg: Fix the incorrect keep_ss of the last descriptor

This commit fixes an issue where CFG_KEEP_SS was unconditionally
cleared on the last descriptor, causing the cs_change setting to be
ignored for the final transfer. It records the cs_change value and
applies it correctly, even when a null descriptor is inserted for
cs-hold delay.

> diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
> index a6123975a37cf..2d66f00dccd45 100644
> --- a/drivers/spi/spi-amlogic-spisg.c
> +++ b/drivers/spi/spi-amlogic-spisg.c
[ ... ]
> @@ -539,13 +542,17 @@ static int aml_spisg_transfer_one_message(struct spi_controller *ctlr,
>  				       xfer->effective_speed_hz);
>  	}
>  
> -	if (cs_hold_in_sclk)
> +	if (cs_hold_in_sclk) {
>  		/* additional null-descriptor to achieve the cs-hold delay */
>  		aml_spisg_setup_null_desc(spisg, desc, cs_hold_in_sclk);
> -	else
>  		desc--;
> +		desc->cfg_bus |= FIELD_PREP(CFG_KEEP_SS, 1);
> +		desc++;
> +	} else {
> +		desc--;
> +	}
>  
> -	desc->cfg_bus |= FIELD_PREP(CFG_KEEP_SS, 0);
> +	FIELD_MODIFY(CFG_KEEP_SS, &desc->cfg_bus, last_xfer_keep_ss);

[Severity: High]
This is a pre-existing issue, but since this code modifies a hardware-facing
DMA descriptor in aml_spisg_transfer_one_message(), will these native integer
assignments cause failures on big-endian architectures?

Looking at drivers/spi/spi-amlogic-spisg.c, struct spisg_descriptor defines
these fields using native integer types:

struct spisg_descriptor {
	u32				cfg_start;
	u32				cfg_bus;
	u64				tx_paddr;
	u64				rx_paddr;
};

And native values are directly assigned to them without endianness conversion,
such as desc->tx_paddr = paddr in aml_spisg_setup_transfer(), or via
FIELD_PREP() and FIELD_MODIFY() here.

On big-endian CPU configurations, won't the hardware receive byte-swapped
values for physical addresses and configuration fields, potentially leading
to DMA faults or memory corruption? Should these hardware descriptors be
defined with explicit endian types (like __le32 and __le64) and updated
using conversions like cpu_to_le32()?

>  	desc->cfg_start |= FIELD_PREP(CFG_EOC, 1);
>  
>  	/* some tolerances */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-a9-spisg-v3-0-a15da3f70029@amlogic.com?part=3