Re: [PATCH net v4] net: airoha: Add retry mechanism to airoha_qdma_set_trtcm_param()

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 17 Aug 2026 09:50:39 +0200 Christian Marangi wrote:
> CPU accesses QDMA via the bus. 

Maybe it's just me but I have no idea what "the bus" is here.
I'm guessing that the bug is that the CSR write path to 
some SRAM may get starved.

> When multiple modules are using the bus
> simultaneously, CPU access to QDMA may encounter bus timeouts and fails,
> resulting in QDMA configuration failures and potentially causing packet
> transmission issues. In order to mitigate the issue, introduce a retry
> mechanism to airoha_qdma_set_trtcm_param routine in order to ensure the
> configuration is correctly applied to the hardware.
> 
> Fixes: ef1ca9271313b ("net: airoha: Add sched HTB offload support")
> Signed-off-by: Leto Liu (刘涛) <Leto.Liu@airoha.com>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
> Changes v4:
> - Rebase on top of net
> Changes v3:
> - Init data
> - Use continue instead of negative error for poll
> - Add memory block after CFG_PARAM
> 
>  drivers/net/ethernet/airoha/airoha_eth.c | 29 +++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 79418e682f71..302f8c432055 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -2712,14 +2712,31 @@ static int airoha_qdma_set_trtcm_param(struct airoha_qdma *qdma, int channel,
>  		     FIELD_PREP(TRTCM_METER_GROUP_MASK, group) |
>  		     FIELD_PREP(TRTCM_PARAM_INDEX_MASK, idx) |
>  		     FIELD_PREP(TRTCM_PARAM_RATE_TYPE_MASK, mode);
> +	int i;
>  
> -	airoha_qdma_wr(qdma, REG_TRTCM_DATA_LOW(addr), val);
> -	airoha_qdma_wr(qdma, REG_TRTCM_CFG_PARAM(addr), config);

a short two-line comment would help remind why we're doing this loop
here, specifically (and not all register accesses)

> +	for (i = 0; i < 100; i++) {
> +		u32 data = 0;
>  
> -	return read_poll_timeout(airoha_qdma_rr, val,
> -				 val & TRTCM_PARAM_RW_DONE_MASK,
> -				 USEC_PER_MSEC, 10 * USEC_PER_MSEC, true,
> -				 qdma, REG_TRTCM_CFG_PARAM(addr));
> +		airoha_qdma_wr(qdma, REG_TRTCM_DATA_LOW(addr), val);
> +		wmb();
> +		airoha_qdma_wr(qdma, REG_TRTCM_CFG_PARAM(addr), config);
> +		wmb();

The wmb()s look like cargo cult, please drop them or explain what they
do.

> +		if (airoha_qdma_get_trtcm_param(qdma, channel, addr, param,
> +						mode, &data, NULL))
> +			continue;

AI also points out that we're worried about this get timing out here
but other spots in the driver read it without a retry. Is this possibly
also an unnecessary check, like the wmb()?
-- 
pw-bot: cr