linux-kernel - Re: [PATCH net-next v14 4/4] net: dsa: add basic initial driver for MxL862xx switches

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] [day] [month] [year] [list]

Message-ID: <aYfTkEg4ijo_qjqJ@makrotopia.org>
Date: Sun, 8 Feb 2026 00:06:40 +0000
From: Daniel Golle <daniel@...rotopia.org>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, "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>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	Simon Horman <horms@...nel.org>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	Frank Wunderlich <frankwu@....de>, Chad Monroe <chad@...roe.io>,
	Cezary Wilmanski <cezary.wilmanski@...ran.com>,
	Liang Xu <lxu@...linear.com>, John Crispin <john@...ozen.org>
Subject: Re: [PATCH net-next v14 4/4] net: dsa: add basic initial driver for
 MxL862xx switches

On Sat, Feb 07, 2026 at 11:59:02PM +0200, Vladimir Oltean wrote:
> On Sat, Feb 07, 2026 at 03:07:27AM +0000, Daniel Golle wrote:
> > +/* PHY access via firmware relay */
> > +static int mxl862xx_phy_read_mmd(struct mxl862xx_priv *priv, int port,
> > +				 int devadd, int reg)
> > +{
> > +	struct mdio_relay_data param = {
> > +		.phy = port,
> > +		.mmd = devadd,
> > +		.reg = cpu_to_le16(reg),
> > +	};
> > +	int ret;
> > +
> > +	ret = MXL862XX_API_READ(priv, INT_GPHY_READ, param);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return le16_to_cpu(param.data);
> > +}
> > +
> > +static int mxl862xx_phy_write_mmd(struct mxl862xx_priv *priv, int port,
> > +				  int devadd, int reg, u16 data)
> > +{
> > +	struct mdio_relay_data param = {
> > +		.phy = port,
> > +		.mmd = devadd,
> > +		.reg = cpu_to_le16(reg),
> > +		.data = cpu_to_le16(data),
> > +	};
> > +
> > +	return MXL862XX_API_WRITE(priv, INT_GPHY_WRITE, param);
> > +}
> > +
> > +static int mxl862xx_phy_read_mii_bus(struct mii_bus *bus, int port, int regnum)
> > +{
> > +	return mxl862xx_phy_read_mmd(bus->priv, port, 0, regnum);
> > +}
> > +
> > +static int mxl862xx_phy_write_mii_bus(struct mii_bus *bus, int port,
> > +				      int regnum, u16 val)
> > +{
> > +	return mxl862xx_phy_write_mmd(bus->priv, port, 0, regnum, val);
> > +}
> > +
> > +static int mxl862xx_phy_read_c45_mii_bus(struct mii_bus *bus, int port,
> > +					 int devadd, int regnum)
> > +{
> > +	return mxl862xx_phy_read_mmd(bus->priv, port, devadd, regnum);
> > +}
> > +
> > +static int mxl862xx_phy_write_c45_mii_bus(struct mii_bus *bus, int port,
> > +					  int devadd, int regnum, u16 val)
> > +{
> > +	return mxl862xx_phy_write_mmd(bus->priv, port, devadd, regnum, val);
> > +}
> 
> You took inspiration from the wrong place with the mii_bus ops prototypes,
> specifically with the "int port" argument.
> 
> The second argument does not hold the port, it holds the PHY address.
> I.e. in this case:
>                 port@6 {
>                     reg = <6>;
>                     phy-handle = <&phy5>;
>                     phy-mode = "internal";
>                 };
>                 phy5: ethernet-phy@5 {
>                     reg = <5>;
>                 };
> 
> "int port" is 5, not 6.
> 
> Your source of inspiration are the prototypes of an mii_bus used as
> ds->user_mii_bus. We have a different set of requirements there, because
> ds->user_mii_bus exists for the case where the PHY is not described in
> the device tree, so the port index is given as argument and the
> user_mii_bus is responsible for internally translating the port index to
> a PHY address.
> 
> So while the use of "int port" as argument name for these operations is
> justifiable in some cases, it is not applicable to this driver, and will
> be a pitfall for anyone who has to modify or debug this code.

Ack. While not completely correct from the beginning I should have
addressed that and changed the parameter to 'int addr' when I started to
count physical ports from 0 and no longer hide the microcontroller --
from that moment on the PHY address and the port address are no longer
equal.

I will post a follow-up series to address this and also the removal of
the 'label' property from the DT binding example once this has been
merged. Both cases don't justify a "Fixes:"-tag though, but I'll just
try to be fast, so both can still be applied before net-next closes.