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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAAXyoMPjdL=KHyr6XCZv-ODV9=8r1_1iwRatAziYU2mPyREmuA@mail.gmail.com>
Date: Thu, 21 Aug 2025 20:19:15 +0800
From: Yangfl <mmyangfl@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
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 6:14 PM Russell King (Oracle)
<linux@...linux.org.uk> wrote:
>
> 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.
>

Thanks for your help. I didn't notice that since it accidentally
configures autonegotiation for me. Anyway I'll take a closer look at
relevant apis.

> 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.

The device takes two sequential u16 MDIO r/w into one op on its
internal 32b regs, so we need to serialise SMI ops to avoid race
conditions. Strictly speaking only locking the target phyaddr is
needed, but I think it won't hurt to lock the MDIO bus as long as I
don't perform busy wait while holding the bus lock.