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

[Nicolai Buchwitz <nb@tipi-net.de>]

On 15.9.2026 11:47, netdev-bot+sashiko@kernel.org wrote:
> 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?

Regarding mtu guard:
tg3 gates on JUMBO_CAPABLE, a device capability rather than the 
configured MTU.

Also phylib does not have a MTU hook without any further changes. 
config_init()
runs on attach and resume, not on an MTU change. bcm7xxx.c enables it 
unconditionally
for the very same reason.

Regarding PTP compensation:
egress timestamping happens at the MDI, so the transmit FIFO sits ahead 
of the
capture point and its 16 ns does not end up in the timestamp. So AFAIU 
nothing
to compensate here.

> 
>>  	/* 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?

The datasheet gives 4.5 KB to 9 KB for the elasticity bit alone and 10 
KB
for copper mode with both bits set. The existing comment in
bcm_phy_enable_jumbo() attributes the 10 KB to that one bit.

I can send a separate patch for the existing comment if wanted?

Thanks,
Nicolai