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

Message-ID: <d5ea5bee-40c5-43f5-9238-ced5ca1904b7@lunn.ch>
Date: Wed, 10 Dec 2025 19:56:13 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Daniel Golle <daniel@...rotopia.org>
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

> 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

> static int mxl862xx_reg_read(struct mxl862xx_priv *priv, u32 addr)
> {
> 	return __mdiodev_c45_read(priv->mdiodev, MDIO_MMD_VEND1, addr);
> }
> 
> static int mxl862xx_reg_write(struct mxl862xx_priv *priv, u32 addr, u16 data)
> {
> 	return __mdiodev_c45_write(priv->mdiodev, MDIO_MMD_VEND1, addr, data);
> }
> 
> static int mxl862xx_ctrl_read(struct mxl862xx_priv *priv)
> {
> 	return mxl862xx_reg_read(priv, MXL862XX_MMD_REG_CTRL);
> }
> 
> static int mxl862xx_busy_wait(struct mxl862xx_priv *priv)
> {
> 	int val;
> 
> 	return readx_poll_timeout(mxl862xx_ctrl_read, priv, val,
> 				  !(val & CTRL_BUSY_MASK), 15, 10000);
> }
> 
> Do you agree?

This part, yes.

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

> > > +#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? 

     Andrew