Re: [PATCH net-next] net: phy: broadcom: enable jumbo frames on BCM54xx

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] The new unconditional bcm_phy_enable_jumbo() call in
  bcm54xx_config_init() (drivers/net/phy/broadcom.c:543) sets…

--

net: phy: broadcom: enable jumbo frames on BCM54xx

This adds an unconditional bcm_phy_enable_jumbo() call to
bcm54xx_config_init(), which sets the extended packet length bit in the
auxiliary control register and the PCS transmit FIFO elasticity bit in the
extended control register so that jumbo frames are not dropped at 100M.

> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
> index d1a4edb34ad2e..b515e38755041 100644
> --- a/drivers/net/phy/broadcom.c
> +++ b/drivers/net/phy/broadcom.c
> @@ -537,6 +537,13 @@ static int bcm54xx_config_init(struct phy_device *phydev)
>  
>  	bcm54xx_ptp_config_init(phydev);
>  
> +	/* Transmit is limited to 4.5 KB without it, see the BCM54210PE
> +	 * datasheet section 5.4.12.14.
> +	 */
> +	err = bcm_phy_enable_jumbo(phydev);
> +	if (err < 0)
> +		return err;
> +

[Severity: Medium]
This lands directly after bcm54xx_ptp_config_init() has armed the
TimeSync block on PTP-capable parts such as the BCM54210E, whose driver
entry in broadcom_drivers uses this same .config_init.  How does the added
elastic FIFO delay interact with the PHY timestamping path?

bcm_phy_enable_jumbo() in drivers/net/phy/bcm-phy-lib.c ends with:

	/* Enable the elastic FIFO for raising the transmission limit from
	 * 4.5KB to 10KB, at the expense of an additional 16 ns in propagation
	 * latency.
	 */
	return phy_set_bits(phydev, MII_BCM54XX_ECR, MII_BCM54XX_ECR_FIFOE);

If that FIFO sits in the copper PCS transmit path downstream of the 1588
egress capture point, would egress timestamps then be reported
systematically early by that fixed delay?  A link where only one end has
MII_BCM54XX_ECR_FIFOE set would also become asymmetric.

Grepping drivers/net/phy/bcm-phy-ptp.c for latency or compensat returns no
matches, so there appears to be no TX/RX latency correction programmed
anywhere in the PHY PTP driver that could absorb the new delay.

There is also no gating here: no MTU or jumbo condition, no device tree
property and no ethtool knob, and the bit is re-armed on every
config_init() and via bcm54xx_resume().  tg3 sets the same hardware bit
only when tg3_flag(tp, JUMBO_CAPABLE) is set.

Would it make sense to gate the FIFO elasticity bit on an actual
jumbo/large-MTU requirement, or to account for the extra delay in the PTP
path?

>  	/* Acknowledge any left over interrupt and charge the device for
>  	 * wake-up.
>  	 */

Also, the changelog says the elasticity bit raises the transmit limit
"from 4.5 KB to 9 KB", while the in-tree comment on the same register
write in bcm_phy_enable_jumbo() says 4.5KB to 10KB.  Which figure is
intended, and could the two be reconciled?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914094703.1093504-1-nb%40tipi-net.de