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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aKbxdaDFMe2Fqnxu@shell.armlinux.org.uk>
Date: Thu, 21 Aug 2025 11:14:13 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Yangfl <mmyangfl@...il.com>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
	Vladimir Oltean <olteanv@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Simon Horman <horms@...nel.org>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [net-next v5 3/3] net: dsa: yt921x: Add support for Motorcomm
 YT921x

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@...linux.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!