Re: [net-next v5 3/3] net: dsa: yt921x: Add support for Motorcomm YT921x

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Thu, Aug 21, 2025 at 11:00:35AM +0100, Russell King (Oracle) wrote:
> On Thu, Aug 21, 2025 at 05:25:46PM +0800, Yangfl wrote:
> > On Thu, Aug 21, 2025 at 4:30 PM Russell King (Oracle)
> > <linux@armlinux.org.uk> wrote:
> > > Someone clearly doesn't believe in reading the documentation before
> > > writing code. This also hasn't been tested in any way. Sorry, but
> > > I'm going to put as much effort into this review as you have into
> > > understanding the phylink API, and thus my review ends here.
> > >
> > > NAK.
> > 
> > Sorry I'm quite new here. I don't understand very clearly why a
> > different set of calls is involved in dsa_switch_ops, so I referred to
> > other dsa drivers and made a working driver (at least tested on my
> > device), but I would appreciate it much if you could point it out in
> > an earlier version of series.
> 
> This isn't dsa_switch_ops, but phylink_mac_ops, which are well
> documented in include/linux/phylink.h. Please read the documentation
> found in that header file detailing the phylink_mac_ops methods.
> You'll find a brief overview before the struct, and then in the #if 0
> section, detailed per-method documentation.

Also, the reason I state that it hasn't been tested is because when
your mac_config method is invoked, and print debug information which
includes state->speed and state->duplex, and then go on to use these.
Phylink's sole call path to mac_config() does this:

        /* Stop drivers incorrectly using these */
        linkmode_zero(st.lp_advertising);
        st.speed = SPEED_UNKNOWN;
        st.duplex = DUPLEX_UNKNOWN;
        st.an_complete = false;
        st.link = false;

        phylink_dbg(pl,
                    "%s: mode=%s/%s/%s adv=%*pb pause=%02x\n",
                    __func__, phylink_an_mode_str(pl->act_link_an_mode),
                    phy_modes(st.interface),
                    phy_rate_matching_to_str(st.rate_matching),
                    __ETHTOOL_LINK_MODE_MASK_NBITS, st.advertising,
                    st.pause);

        pl->mac_ops->mac_config(pl->config, pl->act_link_an_mode, &st);

and you would've noticed in your debug print that e.g. state->speed and
state->duplex are both always -1, and thus are not useful. Note also the
debugging that phylink includes.

Note that no other mac_config() implementations refer to state->speed
and state->duplex. The only time drivers _write_ to these is in the
pcs_get_state() method if they support a PCS.

Therefore, I think your code is completely untested.

I'm also concerned about the SMI locking, which looks to me like you
haven't realised that the MDIO bus layer has locking which guarantees
that all invocations of the MDIO bus read* and write* methods are
serialised.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!