Re: [PATCH net 6/6 RESEND] net: dsa: b53: apply resolved link settings to serial port MACs

[Florian Fainelli <florian.fainelli@broadcom.com>]

On 10/26/26 16:47, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 (2) · 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] b53_force_port_config() uses the IMP-register bit layout when
>    it writes the per-port GMII override register.
> - [Medium] net: dsa: b53: b53_force_port_config() clears GMII_PO_EN on
>    non-IMP ports
> 
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but
>    b53_srab_request_grant() in drivers/net/dsa/b53/b53_srab.c checks the…
> - [Medium] This problem wasn't introduced by this patch, but
>    b53_force_link() and b53_force_port_config() ignore the return value…
> 
> --
> 
> net: dsa: b53: apply resolved link settings to serial port MACs
> 
> With this commit, the b53 phylink link_up/link_down callbacks use the MAC
> override registers for SGMII and 1000BASE-X ports in every negotiation
> mode, not only for fixed-link. At link-up the MAC gets the speed, duplex
> and pause that phylink resolved, and at link-down the MAC link is forced
> down.
> 
>> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
>> index b04f9d5f05256..172ada124cbf1 100644
>> --- a/drivers/net/dsa/b53/b53_common.c
>> +++ b/drivers/net/dsa/b53/b53_common.c
>> @@ -1600,6 +1600,17 @@ static void b53_phylink_mac_config(struct phylink_config *config,
>>   	}
>>   }
>>   
>> +static bool b53_phylink_mac_needs_override(struct b53_device *dev, int port,
>> +					   unsigned int mode,
>> +					   phy_interface_t interface)
>> +{
>> +	return mode == MLO_AN_FIXED ||
>> +	       interface == PHY_INTERFACE_MODE_SGMII ||
>> +	       interface == PHY_INTERFACE_MODE_1000BASEX ||
>> +	       (mode == MLO_AN_PHY && is63xx(dev) &&
>> +		in_range(port, B53_63XX_RGMII0, 4));
>> +}
>> +
> 
> [ ... ]
> 
>> @@ -1633,29 +1636,22 @@ static void b53_phylink_mac_link_up(struct phylink_config *config,
> 
> [ ... ]
> 
>> +	/* Force flow control on BCM5301x's CPU port */
>> +	if (mode == MLO_AN_FIXED && is5301x(dev) && dsa_is_cpu_port(ds, port)) {
>> +		tx_pause = true;
>> +		rx_pause = true;
>>   	}
>> +
>> +	/* PCS negotiation supplies state; the MAC applies phylink's policy. */
>> +	b53_force_port_config(dev, port, speed, duplex, tx_pause, rx_pause);
>> +	b53_force_link(dev, port, true);
> 
> [Severity: Medium]
> b53_phylink_mac_needs_override() now returns true for SGMII and
> 1000BASE-X in every mode. That means the BCM58xx SerDes ports (ports 4
> and 5, see b53_srab_serdes_map_lane()) reach b53_force_port_config()
> on every link-up. Those ports are never dev->imp_port, so the helper
> programs B53_GMII_PORT_OVERRIDE_CTRL(port) with val = GMII_PO_EN.
> 
> Does the 2G handling in b53_force_port_config() use the right bit for
> that register? In b53_regs.h, bits 6 and 7 are swapped between the IMP
> override register and the GMII override register:
> 
> 	PORT_OVERRIDE_SPEED_2000M	BIT(6)
> 	PORT_OVERRIDE_EN		BIT(7)
> 
> 	GMII_PO_EN			BIT(6)
> 	GMII_PO_SPEED_2000M		BIT(7)
> 
> After reg |= GMII_PO_EN, the is5301x(dev) || is58xx(dev) branch does
> reg &= ~PORT_OVERRIDE_SPEED_2000M. That clears BIT(6), which is the
> override enable bit that was just set. GMII_PO_SPEED_2000M is never
> cleared, and the case 2000 path sets BIT(6) rather than BIT(7).
> 
> As a result, the b53_write8() at the end of b53_force_port_config()
> goes out with GMII_PO_EN clear. The MAC briefly returns to autonomous
> hardware control until b53_force_link() sets the enable bit again in a
> second write. If the bootloader left BIT(7) set, the MAC also stays
> forced to 2G even though phylink resolved 1000. That contradicts the
> commit message's claim that the settings passed by phylink are applied
> at link-up.
> 
> The mask itself predates this patch and already affected non-IMP
> fixed-link ports on 5301x/58xx. However, this patch is what routes the
> SerDes ports through it in in-band and PHY modes, and it relies on the
> helper to apply phylink's resolved state. Should the non-IMP path use
> GMII_PO_SPEED_2000M here, for example by picking the 2G bit alongside
> off and val?
> 
> [Severity: Medium]
> b53_phylink_mac_needs_override() now returns true for SGMII and 1000BASE-X
> in any mode. That means the BCM58xx SerDes ports (ports 5 and 4, mapped to
> lanes 0 and 1 by b53_srab_serdes_map_lane()) now reach
> b53_force_port_config() in in-band and PHY modes too.
> 
> These ports are not dev->imp_port, so they use the GMII override register.
> Does b53_force_port_config() use the right bit layout for that register?
> 
> drivers/net/dsa/b53/b53_common.c:b53_force_port_config() {
>      ...
> 	} else {
> 		off = B53_GMII_PORT_OVERRIDE_CTRL(port);
> 		val = GMII_PO_EN;
> 	}
> 
> 	b53_read8(dev, B53_CTRL_PAGE, off, &reg);
> 	reg |= val;
>      ...
> 	reg &= ~(0x3 << GMII_PO_SPEED_S);
> 	if (is5301x(dev) || is58xx(dev))
> 		reg &= ~PORT_OVERRIDE_SPEED_2000M;
>      ...
> }
> 
> In b53_regs.h the two registers have the enable bit and the 2G bit in
> swapped positions:
> 
> #define   PORT_OVERRIDE_SPEED_2000M	BIT(6)
> #define   PORT_OVERRIDE_EN		BIT(7)
> ...
> #define   GMII_PO_EN			BIT(6)
> #define   GMII_PO_SPEED_2000M		BIT(7)
> 
> So on the GMII register, clearing PORT_OVERRIDE_SPEED_2000M also clears
> the GMII_PO_EN bit that was just set. GMII_PO_SPEED_2000M is never
> cleared.
> 
> This seems to have three effects:
> 
> First, the write from b53_force_port_config() goes out with the override
> disabled. The MAC briefly goes back to autonomous control until
> b53_force_link() sets GMII_PO_EN again.
> 
> Second, if firmware or the bootloader left BIT(7) set, the MAC stays forced
> to 2G while phylink resolved 1000. The driver never clears that bit, and
> its case 2000 sets BIT(6) on this register. Would that contradict the
> commit message's "Apply the settings passed by phylink at link-up"?
> 
> Third, for an unknown speed, b53_force_port_config() calls dev_err() and
> returns without writing anything. b53_phylink_mac_link_up() still calls
> b53_force_link(dev, port, true), which forces the link up with the old
> speed, duplex and pause bits. For SGMII and 1000BASE-X this may not be
> reachable in practice.
> 
> The wrong mask in b53_force_port_config() is older code and already
> affected non-IMP fixed-link ports on 5301x/58xx. This patch is what sends
> the SerDes ports through it and relies on it to apply phylink's settings.
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but b53_force_port_config() and
> b53_force_link() both ignore the return value of b53_read8() before they
> modify reg and write it back:
> 
> 	b53_read8(dev, B53_CTRL_PAGE, off, &reg);
> 	reg |= val;
> 
> b53_srab_read8() returns without setting *val when
> b53_srab_request_grant() or b53_srab_op() fails.
> 
> Can reg then hold uninitialized stack data that gets written to
> B53_GMII_PORT_OVERRIDE_CTRL(port) with the enable and link bits set?
> 
> Neither helper reports a failure. If the write in b53_force_port_config()
> fails but the next b53_force_link(dev, port, true) succeeds, the MAC is
> forced up with its previous settings. Likewise, a failed write at link-down
> can leave the MAC forced up.
> 
> This patch adds the SerDes ports in in-band and PHY modes to the users of
> these helpers.
> 
> [Severity: Medium]
> This is a pre-existing issue and was not introduced by this patch. On SRAB
> switches these override accesses go through b53_srab_read8() and
> b53_srab_write8(), and both call b53_srab_request_grant() first. Is the
> timeout check in that function using the right limit?
> 
> drivers/net/dsa/b53/b53_srab.c:b53_srab_request_grant() {
>      ...
> 	for (i = 0; i < 20; i++) {
> 		ctrls = readl(regs + B53_SRAB_CTRLS);
> 		if (ctrls & B53_SRAB_CTRLS_RCAGNT)
> 			break;
> 		usleep_range(10, 100);
> 	}
> 	if (WARN_ON(i == 5))
> 		return -EIO;
>      ...
> }
> 
> If the grant never arrives, the loop exits with i == 20 and the function
> returns 0. The register access then goes ahead without the grant.
> 
> If the grant arrives on the sixth poll, i == 5. A WARN fires and -EIO is
> returned even though the grant is held, so the access is skipped.
> 
> Should this check be against 20 instead?

Agreed. The two override-bit findings describe the same underlying bug. 
Separate prerequisite fixes now select the correct speed bit for each 
register, check override register-access errors, and prevent forcing 
link up after failed configuration or an unsupported speed.

The pre-existing SRAB timeout check is also fixed separately.

pw-bot: cr
-- 
Florian