Re: [PATCH 2/3] can: rockchip_canfd: retry the outstanding TX buffer

Hi,

Thank you for pointing this out.

> > diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
> > index d2cbe25c46d6c..c4ecdc9411cc6 100644
> > --- a/drivers/net/can/rockchip/rockchip_canfd-tx.c
> > +++ b/drivers/net/can/rockchip/rockchip_canfd-tx.c
> > @@ -57,8 +57,8 @@ static void rkcanfd_start_xmit_write_cmd(const struct rkcanfd_priv *priv,
> >  
> >  void rkcanfd_xmit_retry(struct rkcanfd_priv *priv)
> >  {
> > - const unsigned int tx_head = rkcanfd_get_tx_head(priv);
> > - const u32 reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_head);
> > + const unsigned int tx_tail = rkcanfd_get_tx_tail(priv);
> > + const u32 reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_tail);
> >  
> >   rkcanfd_start_xmit_write_cmd(priv, reg_cmd);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this call expose a race
> condition on the hardware registers between softirq and hardirq contexts?
> 
> rkcanfd_start_xmit() runs in softirq context and calls
> rkcanfd_start_xmit_write_cmd(), which performs non-atomic register writes
> to RKCANFD_REG_MODE. If a hardware RX interrupt preempts execution between
> these writes, rkcanfd_irq() (hardirq) will call rkcanfd_xmit_retry()
> which also calls rkcanfd_start_xmit_write_cmd().
> 
> The hardirq will restore priv->reg_mode_default, so when the softirq
> resumes, RKCANFD_REG_CMD is written without the required SPACE_RX_MODE
> bit set, bypassing the RK3568 Erratum 12 workaround. Could this lead to
> hardware misbehavior under heavy load?

Yes, the MODE/CMD sequence has a pre-existing serialization problem. On a
device affected by both erratum 6 and erratum 12, `rkcanfd_start_xmit()` can
be interrupted after enabling `SPACE_RX_MODE`. The retry path can then write
its own MODE/CMD/MODE sequence and restore the default mode before the
original path writes CMD. That CMD would consequently be issued without
`SPACE_RX_MODE` enabled.

With the current quirk table this combination applies to RK3568v2. RK3588 is
affected by erratum 6 but not erratum 12, so this specific hardware failure
cannot be reproduced on my RK3588 test board.

PATCH 3/3 already prevents the interleaving: both
`rkcanfd_start_xmit()` and the IRQ retry path call
`rkcanfd_start_xmit_write_cmd()` while holding the same `tx_lock` with IRQs
saved. I will update PATCH 3/3 in v2 to explicitly document that the lock also
protects the MODE/CMD transaction, add a lockdep assertion to
`rkcanfd_start_xmit_write_cmd()`, and include the erratum 12 commit in the
fix description.

Regards,
Cunhao