Re: [PATCH net] net: bcmgenet: unshare the skb before writing the control block

[Nicolai Buchwitz]

Hi Clashiko

On 10.10.2026 13:10, netdev-bot+sashiko@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 (1) · Medium: 1 · 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).
>
> - [Medium] The new skb_share_check() failure branch in bcmgenet_xmit()
>   (drivers/net/ethernet/broadcom/genet/bcmgenet.c:2159-2164) jumps to…
>
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but
>   bcmgenet_timeout()…
>
> --
>
> net: bcmgenet: unshare the skb before writing the control block
>
> bcmgenet_xmit() keeps first_cb, last_cb and bytes_sent in the skb 
> control
> block and reads them back when the transmit completes. A shared skb 
> that
> is resubmitted while it is still in the ring overwrites that state, and
> BQL then stalls the queue. The patch calls skb_share_check() before the
> control block is written, so each transmit works on its own skb.
>
> > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c 
> > b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > index e8908916558b1..f4cfbff49a1b8 100644
> > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > @@ -2153,6 +2153,16 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff 
> > *skb, struct net_device *dev)
> >  		goto out;
> >  	}
> >
> > +	/* We store Tx state in the control block, so the skb must not be
> > +	 * shared, but ether_setup() leaves IFF_TX_SKB_SHARING set.
> > +	 */
> > +	skb = skb_share_check(skb, GFP_ATOMIC);
> > +	if (!skb) {
> > +		BCMGENET_STATS64_INC((&ring->stats64), dropped);
> > +		ret = NETDEV_TX_OK;
> > +		goto out;
>
> [Severity: Medium]
> If the GFP_ATOMIC clone fails on the last skb of an xmit_more batch, do
> the descriptors queued by the earlier calls in that batch get handed to
> the hardware?

No, but that is nothing new. The same goto out is taken by the existing
add_tsb and unmap drops, and the next xmit flushes them anyway. You only
get here on an alloc failure under pktgen, so real traffic probably 
never
hits it. Can be cleaned up separately, maybe even net-next?

> [...]

> [Severity: High]
> This is a pre-existing issue, not one introduced by this patch, but it
> involves the same ring->stats64.syncp that the new dropped increment
> uses.

Agreed, pre-existing. I can fix in another series together with above.

> [...]