Re: [PATCH net] net: sparx5: don't reuse the last DCB in fdma xmit

[Daniel Machon]

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 (1) · High: 1 · Medium: 0 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] net: sparx5: unguarded fdma_dcb_advance() in sparx5_fdma_xmit()
>   breaks tx ring
> 
> Pre-existing issues:
> - [Critical] This problem wasn't introduced by this patch, but the patch
>   adds a new way to reach it.
> 
> --
> 
> net: sparx5: don't reuse the last DCB in fdma xmit
> 
> This patch adds a check to sparx5_fdma_xmit() so it rejects the next tx
> DCB when that DCB is fdma->last_dcb. This uses the same !fdma_is_last()
> guard as lan966x and lan969x, so the tail of the chain is never re-added
> and the frame is not lost.
> 
> > diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> > index dbe86f937b21..221b2f1932f6 100644
> > --- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> > @@ -218,7 +218,8 @@ int sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb,
> >       void *virt_addr;
> >
> >       fdma_dcb_advance(fdma);
> > -     if (!fdma_db_is_done(fdma_db_get(fdma, fdma->dcb_index, 0)))
> > +     if (!fdma_db_is_done(fdma_db_get(fdma, fdma->dcb_index, 0)) ||
> > +         fdma_is_last(fdma, fdma_dcb_next_get(fdma)))
> >               return -EINVAL;
> 
> [Severity: High]
> Does this fully fix the drift described in the commit message? The
> underlying problem looks like it is still there: fdma_dcb_advance() runs
> before the done check, and fdma->dcb_index is not restored when
> sparx5_fdma_xmit() returns -EINVAL. The new fdma_is_last() test only
> covers the moment dcb_index lands exactly on last_dcb.
> 
> When the ring is full, a failed xmit leaves dcb_index on the oldest
> in-flight DCB. The next call advances again before checking anything,
> so if the FDMA has completed that DCB in the meantime it is skipped and
> we test a newer DCB that is still in flight. Each failure moves
> dcb_index one step further, so a burst can keep dropping frames while
> completed DCBs sit unused behind dcb_index.
> 
> If dcb_index ends up just behind last_dcb and the FDMA then drains the
> chain, every DCB is done and the ring is idle. The next xmit advances
> onto last_dcb, hits the new fdma_is_last() check and returns -EINVAL,
> which sparx5_port_xmit_impl() treats as a drop:
> 
>         if (ret == -EBUSY)
>                 goto busy;
>         if (ret < 0)
>                 goto drop;
> 
> The lost frame becomes a counted drop, but it is still a drop on an
> empty ring. lan966x_fdma_get_next_dcb() and lan969x_fdma_get_next_dcb()
> don't hit this because they use !fdma_is_last() inside a loop over all
> DCBs and pick another free one, rather than failing on a single index.
> 
> The previous tail is also still unsafe. Say the FDMA has halted on
> last_dcb at index L and dcb_index is at L - 2. The first frame advances
> to L - 1, which is done and not last, so it is linked from
> dcbs[L].nextptr and becomes the new last_dcb. __fdma_dcb_add() does not
> clear the done bit on dcbs[L], so if a second frame comes in before the
> FDMA has fetched dcbs[L].nextptr after the reload, it advances onto L,
> which is still done and no longer last. Re-adding it rewrites
> dcbs[L].nextptr to FDMA_DCB_INVALID_DATA, so dcbs[L - 1] is cut out of
> the chain. Its frame is lost, and since the FDMA never visits it again
> its done bit stays clear, so later xmits that land on it fail too.
> 
> The drift itself dates back to f4aa7e361ae2, but since this patch is
> meant to fix it, would it be cleaner to fix it at the source, by only
> committing the dcb_index advance once the DCB has been accepted (or
> rolling it back on failure)? Then dcb_index always follows last_dcb,
> the next DCB is always the oldest one, and the fdma_is_last() check
> should not be needed. Alternatively, sparx5 could search the ring the
> way lan969x_fdma_get_next_dcb() does.
>

The drift is pre-existing and not what I intended to fix. That said, I think I
can fix both with a simple change where the ring index is computed from the last
index instead.

pw-bot: cr