Re: [PATCH net v4 5/7] net: stmmac: dwmac4: Use the correct bufzise when the len is exactly 8K

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 2

- [Low] Taken in isolation, the only functional hunk of this patch
  (drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c:477-484,…
- [Low] Commit-message / diff mismatch in "net: stmmac: dwmac4: Use the
  correct bufzise when the len is exactly 8K": the changelog states…

--

net: stmmac: dwmac4: Use the correct bufzise when the len is exactly 8K

This changes the dwmac4 set_16kib_bfsize() comparison from >= BUF_SIZE_8KiB
to > BUF_SIZE_8KiB so that a frame length of exactly 8KiB does not select the
next larger DMA buffer size, and renames the callback parameter in
dwmac4_descs.c, ring_mode.c and the stmmac_mode_ops prototype.

No Link: tags in the commit header.

> net: stmmac: dwmac4: Use the correct bufzise when the len is exactly 8K

[Severity: Low]
This isn't a bug, but the subject has a typo: "bufzise" should be "bufsize".

>     Lets fix the comparison and while at it, rename the parameter from len
>     to mtu.

Does the changelog describe the rename in the right direction?  The diff
renames mtu to len, not len to mtu:

  -static int set_16kib_bfsize(int mtu)
  +static int set_16kib_bfsize(int len)

Same for the ops prototype in hwif.h and for ring_mode.c.  Should this read
"rename the parameter from mtu to len"?

Also, at this commit the caller still passes an MTU, not a frame length.
stmmac_setup_dma_desc() documents "@mtu: MTU to setup the dma queue and buf
with", is called with dev->mtu from stmmac_open() and with new_mtu from
stmmac_change_mtu(), keeps the comment "Returns 0 or BUF_SIZE_16KiB if mtu >
8KiB and dwmac4 or ring mode", and the sibling helper is still
stmmac_set_bfsize(int mtu).  Nothing on that path adds ETH_HLEN, VLAN or FCS
yet.  That unit mismatch is resolved by the following patch in the series
("net: stmmac: size the RX buffers from the frame length, not the MTU"),
which computes len = mtu + ETH_HLEN + 2 * VLAN_HLEN + ETH_FCS_LEN and renames
stmmac_set_bfsize()'s parameter too, but the inverted rename sentence in this
changelog would still be worth correcting on respin.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> index 2994df41ec2c4..c6a8f8d735015 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> @@ -474,11 +474,11 @@ static void dwmac4_set_sarc(struct dma_desc *p, u32 sarc_type)
>  					  sarc_type));
>  }
>  
> -static int set_16kib_bfsize(int mtu)
> +static int set_16kib_bfsize(int len)
>  {
>  	int ret = 0;
>  
> -	if (unlikely(mtu >= BUF_SIZE_8KiB))
> +	if (unlikely(len > BUF_SIZE_8KiB))
>  		ret = BUF_SIZE_16KiB;
>  	return ret;
>  }

[Severity: Low]
Taken on its own, does this hunk change the DMA buffer size the driver ends
up programming?

The only caller is stmmac_setup_dma_desc(), which falls back when the
callback returns anything below BUF_SIZE_16KiB:

	/* Returns 0 or BUF_SIZE_16KiB if mtu > 8KiB and dwmac4 or ring mode */
	bfsize = stmmac_set_16kib_bfsize(priv, mtu);
	if (bfsize < 0)
		bfsize = 0;

	if (bfsize < BUF_SIZE_16KiB)
		bfsize = stmmac_set_bfsize(mtu);

	dma_conf->dma_buf_sz = bfsize;

For the single input whose result changes here (exactly BUF_SIZE_8KiB) the
callback now returns 0, so the fallback runs, and stmmac_set_bfsize() at this
commit still uses the inclusive comparison:

static int stmmac_set_bfsize(int mtu)
{
	int ret;

	if (mtu >= BUF_SIZE_8KiB)
		ret = BUF_SIZE_16KiB;

so dma_buf_sz comes out as BUF_SIZE_16KiB both before and after this patch.
The boundary case is only actually fixed by the next patch in the series
("net: stmmac: size the RX buffers from the frame length, not the MTU"),
which switches stmmac_set_bfsize() to a strict > comparison and feeds both
helpers len = mtu + ETH_HLEN + 2 * VLAN_HLEN + ETH_FCS_LEN.

Given that, is the standalone Fixes: c3efed5ad1b0 tag on this commit right?
Backported alone to stable it would have no effect.  Would it make sense to
either squash the two changes or note the dependency?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> index 9314bcb85c221..857f7562c6c6d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> @@ -540,7 +540,7 @@ struct stmmac_mode_ops {
>  	bool (*is_jumbo_frm)(unsigned int len, bool enh_desc);
>  	int (*jumbo_frm)(struct stmmac_tx_queue *tx_q, struct sk_buff *skb,
>  			 int csum);
> -	int (*set_16kib_bfsize)(int mtu);
> +	int (*set_16kib_bfsize)(int len);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917215339.2022523-1-maxime.chevallier%40bootlin.com