Re: [PATCH net 4/6 RESEND] net: dsa: b53: configure PCS negotiation and forced link parameters

[Florian Fainelli]

On 10/26/26 16:47, netdev-bot+sashiko@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
>
> Low priority / nit picks for your consideration:
>
> - [Low] b53_serdes_config() ignores permit_pause_to_mac.
>
> --
>
> net: dsa: b53: configure PCS negotiation and forced link parameters
>
> This commit makes b53_serdes_config() program the advertisement, the BMCR
> autonegotiation enable, the protocol selection and SGMII MAC mode based on
> phylink's negotiation mode. It adds pcs_inband_caps(). It also adds a
> pcs_link_up() handler that forces speed and duplex when in-band negotiation
> is disabled.
>
> > diff --git a/drivers/net/dsa/b53/b53_serdes.c b/drivers/net/dsa/b53/b53_serdes.c
> > index d521050d7c8f7..fc20e211e67e1 100644
> > --- a/drivers/net/dsa/b53/b53_serdes.c
> > +++ b/drivers/net/dsa/b53/b53_serdes.c
>
> [ ... ]
>
> >   static int b53_serdes_config(struct phylink_pcs *pcs, unsigned int neg_mode,
> >   			     phy_interface_t interface,
> >   			     const unsigned long *advertising,
> >   			     bool permit_pause_to_mac)
> >   {
>
> [ ... ]
>
> > +	bmcr = neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED ? BMCR_ANENABLE : 0;
> > +	/* phylink requests Clause 37 restarts separately, but not SGMII. */
> > +	if (bmcr && changed && interface == PHY_INTERFACE_MODE_SGMII)
> > +		bmcr |= BMCR_ANRESTART;
> > +	ret = b53_serdes_modify(dev, lane, B53_SERDES_MII_REG(MII_BMCR),
> > +				SERDES_MII_BLK,
> > +				BMCR_ANENABLE | BMCR_ISOLATE | BMCR_ANRESTART, bmcr);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* The MAC applies phylink's resolved pause policy in mac_link_up(). */
>
> [Severity: Low]
> Is this comment accurate at this point in the series?
>
> b53_serdes_config() ignores permit_pause_to_mac and relies on this comment
> to justify that. However, b53_phylink_mac_link_up() in b53_common.c has no
> MLO_AN_INBAND branch. Its MLO_AN_PHY branch only programs pause on 63xx
> RGMII ports:
>
> drivers/net/dsa/b53/b53_common.c:b53_phylink_mac_link_up() {
>      ...
> 	if (mode == MLO_AN_PHY) {
> 		/* Re-negotiate EEE if it was enabled already */
> 		p->eee_enabled = b53_eee_init(ds, port, phydev);
>
> 		if (is63xx(dev) && in_range(port, B53_63XX_RGMII0, 4)) {
> 			b53_force_port_config(dev, port, speed, duplex,
> 					      tx_pause, rx_pause);
> 			b53_force_link(dev, port, true);
> 		}
>
> 		return;
> 	}
>      ...
> }
>
> As a result, SGMII and 1000BASE-X SerDes ports in in-band or PHY mode never
> get phylink's resolved tx_pause/rx_pause applied to the MAC. This is not a
> change in behavior, since pause was not applied before this patch either.
>
> A later patch in this series, "net: dsa: b53: apply resolved link settings
> to serial port MACs", adds b53_phylink_mac_needs_override(). That helper
> covers SGMII and 1000BASE-X in every mode and calls b53_force_port_config()
> with tx_pause and rx_pause, which makes the comment correct.
>
> Could the comment be added in that later patch, or could this commit
> message note that the MAC side is handled later in the series?

Agreed, the comment was premature at this point in the series. It has 
been removed, and the commit message now explicitly notes that the later 
“apply resolved link settings to serial port MACs” patch handles MAC 
pause policy.
--
Florian