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

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.

[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?

[ ... ]

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