[PATCH net v2] net: airoha: Add retry mechanism to airoha_qdma_set_trtcm_param()
Leto Liu (刘涛)
Leto.Liu at airoha.com
Thu Jul 2 03:44:35 PDT 2026
First, let me descript the detailed mechanism of hardware QDMA register read/write is as follows:
CPU register reads do not have a “write-fail” issue; a CPU read operation does not fail to execute.
A CPU register write may not take effect as expected, i.e., the value written by the CPU may not be preserved. This is because QDMA performs non-atomic read-modify-write (RMW) operations, and a hardware write can overwrite a CPU write, causing the CPU write to be lost.
rw_done is cleared to 0 when the CPU issues a write request, and is set to 1 after completion. The interval is only a few hardware clock cycles (less than 10 ns).
Regarding write contention:
CPU side (driver/software) HW side (rate-limiter engine)
| |
| write cfg_param register | token-bucket refill tick / packet ingress/egress
| (indirectly writes SRAM) | (updates SRAM)
| |
v v
+-----------------------------------------------------------------------------------------------------------------------------------------+
| HW SRAM |
| |
| HW performs Read-Modify-Write (non-atomic) |
| | |
| | 1. HW reads the old value |
| | 2. CPU write happens first ---> contention point |
| | 3. HW write overwrites with old/mod value ---> CPU value overwritten |
| v |
+-----------------------------------------------------------------------------------------------------------------------------------------+
Best Regards, Leto
> > This is an AI-generated review of your patch. The human sending this
> > email has considered the AI review valid, or at least plausible.
> > Full review at: https://sashiko.dev
> >
> > Simon says: I'd appreciate it if you could look over the first two issues
> > flagged below in the context of this patch. While the third
> > and final issue looks like it is in possible separate follow-up
> > territory.
> >
> > ---
> > net: airoha: Add retry mechanism to airoha_qdma_set_trtcm_param()
> >
> > This patch introduces a retry loop to airoha_qdma_set_trtcm_param() to
> > mitigate bus timeouts when configuring QDMA parameters. It attempts to
> > verify that the configuration is correctly applied to the hardware.
> >
> > > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c
> > > b/drivers/net/ethernet/airoha/airoha_eth.c
> > > index 3370c3df7c107..bb5c0599a4eed 100644
> > > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> > > @@ -2673,14 +2673,30 @@ 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);
> > > + for (i = 0; i < 100; i++) {
> > > + u32 data;
> > >
> > > - 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);
> > > +
> > > + if (read_poll_timeout(airoha_qdma_rr, data,
> > > + data & TRTCM_PARAM_RW_DONE_MASK,
> > > + USEC_PER_MSEC, 10 * USEC_PER_MSEC,
> > > + true, qdma, REG_TRTCM_CFG_PARAM(addr)))
> > > + return -ETIMEDOUT;
> >
> > [Severity: High]
> > If a bus timeout causes the write phase's read_poll_timeout() in
> > airoha_qdma_set_trtcm_param() to fail, this immediately returns
> > -ETIMEDOUT, aborting the 100-iteration retry loop.
> >
> > Doesn't this defeat the mitigation?
Here we are polling the hardware done bit. Based on the hardware behavior described above, a read operation will not fail. In addition, the hardware register write completes within at most 10 ns, while the poll timeout here is configured more than 1 ms. From these two points, we can conclude that this read_poll_timeout will always succeed (i.e., it will return 0). Therefore, adding a return -ETIMEDOUT path here will not cause any issues.
Best Regards, Leto
>
> I agree we just run 'continue' here instead of returing -ETIMEDOUT
No change is needed here, and even if we modify it, it will not cause any issues.
Best Regards, Leto
> >
> > > +
> > > + if (airoha_qdma_get_trtcm_param(qdma, channel, addr, param,
> > > + mode, &data, NULL))
> > > + continue;
> >
> > [Severity: High]
> > If airoha_qdma_get_trtcm_param() times out during the verification
> > phase, it indicates the hardware hasn't finished the read command (DONE bit is still 0).
> >
> > The continue statement then immediately starts the next iteration,
> > writing new commands to DATA_LOW and CFG_PARAM.
> >
> > Can writing to command registers while the hardware state machine is
> > actively busy violate hardware protocols and lead to unrecoverable lockups?
>
> @Brown and @Leto: can you please comment on this case?
>
the hardware register write completes within at most 10 ns, while the poll timeout here is configured more than 1 ms. When airoha_qdma_get_trtcm_param() is called here, the hardware has already completed the previous register write operation. Therefore, the concern mentioned here will definitely not occur, and there will be no issue at this point.
Best Regards, Leto
> >
> > > +
> > > + if (data == val)
> > > + return 0;
> >
> > [Severity: High]
> > This isn't a bug introduced by this patch, but if the bus drops the
> > command write to REG_TRTCM_CFG_PARAM, the hardware never executes the
> > command and the TRTCM_PARAM_RW_DONE_MASK bit remains set from the previous operation.
> >
> > Consequently, read_poll_timeout() returns an immediate false success.
> >
> > If both the set and get command writes are dropped by the bus, the get
> > operation will read back the newly written DATA_LOW register value.
> >
> > Will the data == val check evaluate to true and falsely report success
> > without the hardware ever processing the configuration?
>
> This seems quite a 'pathological' scenario, is it really an issue?
>
> Regards,
> Lorenzo
>
As explained above, the concern here does not apply and there will be no issue.
Best Regards, Leto
> >
> > > + }
> > > +
> > > + return -EBUSY;
> > > }
> > >
> >
> > [ ... ]
> >
> > > static int airoha_qdma_set_trtcm_config(struct airoha_qdma *qdma, int channel,
> > > u32 addr, enum trtcm_mode_type mode,
> > > bool enable, u32 enable_mask)
> > > {
> > > u32 val;
> > >
> > > if (airoha_qdma_get_trtcm_param(qdma, channel, addr, TRTCM_MISC_MODE,
> > > mode, &val, NULL))
> > > return -EINVAL;
> >
> > [Severity: High]
> > Standalone calls to airoha_qdma_get_trtcm_param(), like this one in
> > airoha_qdma_set_trtcm_config(), lack a retry loop entirely.
> >
> > Does this leave read-modify-write cycles fully vulnerable to the same
> > bus timeouts the patch attempts to mitigate?
> >
No change is needed here. Even if the write succeeds but the read-back during verification returns an incorrect value, the verification will fail and the code will proceed to the next retry. A failure would only occur if this happened for 100 consecutive attempts, which has an almost zero probability. Therefore, this code does not affect the final outcome.
That said, your suggestion is a good one, and it is still worthwhile to improve/optimize the read function.
Best Regards, Leto
> > [Severity: High]
> > This isn't a bug introduced by this patch, but the patch mitigates bus
> > timeouts in airoha_qdma_set_trtcm_param() while entirely missing
> > airoha_qdma_set_rl_param().
> >
> > The latter performs the exact same write-and-poll sequence on
> > identical QDMA configuration registers:
> >
> > drivers/net/ethernet/airoha/airoha_eth.c:airoha_qdma_set_rl_param() {
> > ...
> > return read_poll_timeout(..., val & RATE_LIMIT_PARAM_RW_DONE_MASK,
> > ...); }
> >
> > If bus timeouts affect QDMA access broadly as stated in the commit
> > message, are the rate-limiting configuration paths randomly vulnerable to the same failures?
> >
Yes, you are right. Function airoha_qdma_set_rl_param need readback protecting.
Best Regards, Leto
> > [Severity: High]
> > This isn't a bug introduced by this patch, but in
> > airoha_tc_remove_htb_queue(), hardware teardown incorrectly uses queue + 1 instead of queue:
> >
> > drivers/net/ethernet/airoha/airoha_eth.c:airoha_tc_remove_htb_queue() {
> > ...
> > airoha_qdma_set_tx_rate_limit(netdev, queue + 1, 0, 0);
> > ...
> > }
> >
> > Does this leave the removed queue's hardware limits permanently active
> > (resource leak) and inadvertently disable the rate limits for queue +
> > 1, corrupting the QoS state of another active channel?
It has already been fixed, refer to https://github.com/LorenzoBianconi/net-next/commit/d775d542504cbfea2b214149405d97eb00a88dc4
Best Regards, Leto
More information about the linux-arm-kernel
mailing list