Re: [PATCH 2/2] tg3: Fix the TX ring stall

[Michael Chan]

On Wed, Nov 1, 2023 at 9:11 PM <alexey.pakhunov@xxxxxxxxxx> wrote:
>
> Hi,
>
> > Thanks for the patch.  An alternative fix that may be simpler is to
> > add a goto after calling tg3_tso_bug().  Something like this:
> >
> >         tg3_tso_bug();
> >         goto update_tx_mbox;
> > ...
> >
> > update_tx_mbox:
> >         if (!netdev_xmit_more() || netif_xmit_stopped())
> >                 tw32_tx_mbox();
> > ...
>
> Yes, I considered this approach but in the end it seemed more fragile. All
> future updates to tg3_start_xmit() would need to be careful to make sure
> all return paths go through "update_tx_mbox". This is much more
> straightforward with a separate wrapper function.
>
> The sizes of both patches are roughly the same. The wrapper function
> version:
>
>  drivers/net/ethernet/broadcom/tg3.c | 46 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 35 insertions(+), 11 deletions(-)
>
> The goto version touches four different return paths: three tg3_tso_bug()
> calls and the return at the very top of the function:
>
>  drivers/net/ethernet/broadcom/tg3.c | 46 +++++++++++++++++++++++++++++++++-------------
>  1 file changed, 33 insertions(+), 13 deletions(-)
>
> Let me re-test the goto version and resubmit it as v2. Please let me know
> which version of the patch you prefer more.
>

I did not realize the goto version is almost as big.  In that case,
your original version is fine.

You might want to declare the variables in reverse Xmas tree style for
any new code.  This driver is old and most of the existing code does
not follow that style.

Thanks.

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature