netdev - Re: [PATCH RFC net-next 3/3] 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: <aTnEgjso87YRDlmr@makrotopia.org>
Date: Wed, 10 Dec 2025 19:05:38 +0000
From: Daniel Golle <daniel@...rotopia.org>
To: Andrew Lunn <andrew@...n.ch>
Cc: 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>,
	Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	Frank Wunderlich <frankwu@....de>,
	Avinash Jayaraman <ajayaraman@...linear.com>,
	Bing tao Xu <bxu@...linear.com>, Liang Xu <lxu@...linear.com>,
	Juraj Povazanec <jpovazanec@...linear.com>,
	"Fanni (Fang-Yi) Chan" <fchan@...linear.com>,
	"Benny (Ying-Tsan) Weng" <yweng@...linear.com>,
	"Livia M. Rosu" <lrosu@...linear.com>,
	John Crispin <john@...ozen.org>
Subject: Re: [PATCH RFC net-next 3/3] net: dsa: add basic initial driver for
 MxL862xx switches

On Wed, Dec 10, 2025 at 07:56:13PM +0100, Andrew Lunn wrote:
> > Imho it would be nice to introduce unlock __mdiodev_c45_* helpers in
> > include/linux/mdio.h, ie.
> > 
> > static inline int __mdiodev_c45_read(struct mdio_device *mdiodev, int devad,
> > 				     u16 regnum)
> > {
> > 	return __mdiobus_c45_read(mdiodev->bus, mdiodev->addr, devad, regnum);
> > }
> > 
> > static inline int __mdiodev_c45_write(struct mdio_device *mdiodev, u32 devad,
> > 				      u16 regnum, u16 val)
> > {
> > 	return __mdiobus_c45_write(mdiodev->bus, mdiodev->addr, devad, regnum,
> > 				   val);
> > }
> 
> https://elixir.bootlin.com/linux/v6.18/source/drivers/net/phy/mdio_bus.c#L531

That's __mdiobus_c45_*, but having __mdiodev_c45_* would be nice as
well, see above.

> > > > +	if (result < 0) {
> > > > +		ret = result;
> > > > +		goto out;
> > > > +	}
> > > 
> > > If i'm reading mxl862xx_send_cmd() correct, result is the value of a
> > > register. It seems unlikely this is a Linux error code?
> > 
> > Only someone with insights into the use of error codes by the uC
> > firmware can really answer that. However, as also Russell pointed out,
> > the whole use of s16 here with negative values being interpreted as
> > errors is fishy here, because in the end this is also used to read
> > registers from external MDIO connected PHYs which may return arbitrary
> > 16-bit values...
> > Someone in MaxLinear will need to clarify here.
> 
> It looks wrong, and since different architectures use different error
> code values, it is hard to get right. I would suggest you just return
> EPROTO or EIO and add a netdev_err() to print the value of result.

Ack, makes sense.

> > > > +#define MXL862XX_API_WRITE(dev, cmd, data) \
> > > > +	mxl862xx_api_wrap(dev, cmd, &(data), sizeof((data)), false)
> > > > +#define MXL862XX_API_READ(dev, cmd, data) \
> > > > +	mxl862xx_api_wrap(dev, cmd, &(data), sizeof((data)), true)
> > > 
> > > > +/* 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 = reg & 0xffff,
> > > > +	};
> > > > +	int ret;
> > > > +
> > > > +	ret = MXL862XX_API_READ(priv, INT_GPHY_READ, param);
> > > 
> > > That looks a bit ugly, using a macro as a function name. I would
> > > suggest tiny functions rather than macros. The compiler should do the
> > > right thing.
> > 
> > The thing is that the macro way allows to use MXL862XX_API_* on
> > arbitrary types, such as the packed structs. Using a function would
> > require the type of the parameter to be defined, which would result
> > in a lot of code duplication in this case.
> 
> How many different invocations of these macros are there? For MDIO you
> need two. How many more are there? 

A lot, 80+ in total in the more-or-less complete driver, using 30+
different __packed structs as parameters.

https://github.com/dangowrt/linux/blob/mxl862xx-for-upstream/drivers/net/dsa/mxl862xx/mxl862xx.c

https://github.com/dangowrt/linux/blob/mxl862xx-for-upstream/drivers/net/dsa/mxl862xx/mxl862xx-api.h