netdev - Re: [PATCH net-next v6 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: <ad61c240-eee3-4db4-b03e-de07f3efba12@lunn.ch>
Date: Sun, 24 Aug 2025 17:51:37 +0200
From: Andrew Lunn <andrew@...n.ch>
To: David Yang <mmyangfl@...il.com>
Cc: netdev@...r.kernel.org, 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>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v6 3/3] net: dsa: yt921x: Add support for
 Motorcomm YT921x

> +static int
> +yt921x_intif_read(struct yt921x_priv *priv, int port, int reg, u16 *valp)
> +{
> +	struct device *dev = to_device(priv);
> +	u32 mask;
> +	u32 ctrl;
> +	u32 val;
> +	int res;
> +
> +	res = yt921x_intif_wait(priv);
> +	if (res)
> +		return res;
> +
> +	mask = YT921X_MBUS_CTRL_PORT_M | YT921X_MBUS_CTRL_REG_M |
> +	       YT921X_MBUS_CTRL_OP_M;
> +	ctrl = YT921X_MBUS_CTRL_PORT(port) | YT921X_MBUS_CTRL_REG(reg) |
> +	       YT921X_MBUS_CTRL_READ;
> +	res = yt921x_smi_update_bits(priv, YT921X_INT_MBUS_CTRL, mask, ctrl);
> +	if (res)
> +		return res;
> +	res = yt921x_smi_write(priv, YT921X_INT_MBUS_OP, YT921X_MBUS_OP_START);
> +	if (res)
> +		return res;
> +
> +	res = yt921x_intif_wait(priv);
> +	if (res)
> +		return res;
> +	res = yt921x_smi_read(priv, YT921X_INT_MBUS_DIN, &val);
> +	if (res)
> +		return res;
> +
> +	if ((u16)val != val)
> +		dev_err(dev,
> +			"%s: port %d, reg 0x%x: Expected u16, got 0x%08x\n",
> +			__func__, port, reg, val);
> +	*valp = (u16)val;
> +	return 0;
> +}

...

> +static int yt921x_mbus_int_read(struct mii_bus *mbus, int port, int reg)
> +{
> +	struct yt921x_priv *priv = mbus->priv;
> +	u16 val;
> +	int res;
> +
> +	if (port >= YT921X_PORT_NUM)
> +		return 0xffff;
> +
> +	yt921x_smi_acquire(priv);
> +	res = yt921x_intif_read(priv, port, reg, &val);
> +	yt921x_smi_release(priv);
> +
> +	if (res)
> +		return res;
> +	return val;
> +}
> +
> +static int
> +yt921x_mbus_int_write(struct mii_bus *mbus, int port, int reg, u16 data)
> +{
> +	struct yt921x_priv *priv = mbus->priv;
> +	int res;
> +
> +	if (port >= YT921X_PORT_NUM)
> +		return 0;
> +
> +	yt921x_smi_acquire(priv);
> +	res = yt921x_intif_write(priv, port, reg, data);
> +	yt921x_smi_release(priv);
> +
> +	return res;
> +}

Going back to comment from Russell in an older version:

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

You comment is partially correct, but also wrong. As you can see here,
you hold the lock for a number of read/writes, not just one u32 write
split into two MDIO bus transactions.

They way you currently do locking is error prone.

1) Are you sure you actually hold the lock on all paths?

2) Are you sure you hold the lock long enough for all code which
   requires multiple reads/writes?

The mv88e6xxx driver does things differently:

Every function assigned to struct dsa_switch_ops first takes the lock,
does what needs doing, and then releases the lock just before the
return.

The lowest level read/write function does a mutex_is_locked() to test
that the lock is held. If it is not, it prints an error message.

The first part makes it easy to see the lock is held, and it makes it
clear all operations the driver is doing is covered by the lock, there
is no need worry about two threads racing.

The second part makes bugs about missing locks obvious, an error
message is printed.

Please reconsider your locking. Also, please think about, do you need
a different lock for MDIO, I2C and SPI? Do you need the current
acquire/release abstract?

	Andrew