linux-kernel - Re: [net-next PATCH v12 09/13] mfd: an8855: Add support for Airoha AN8855 Switch MFD

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: <49d2cfed-cb31-438d-9736-6d8e0bb24041@lunn.ch>
Date: Fri, 14 Mar 2025 20:34:58 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Lee Jones <lee@...nel.org>
Cc: Christian Marangi <ansuelsmth@...il.com>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Vladimir Oltean <olteanv@...il.com>,
	Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org, netdev@...r.kernel.org,
	upstream@...oha.com
Subject: Re: [net-next PATCH v12 09/13] mfd: an8855: Add support for Airoha
 AN8855 Switch MFD

> > +static int an8855_mii_set_page(struct an8855_mfd_priv *priv, u8 phy_id,
> > +			       u8 page) __must_hold(&priv->bus->mdio_lock)
> > +{
> > +	struct mii_bus *bus = priv->bus;
> > +	int ret;
> > +
> > +	ret = __mdiobus_write(bus, phy_id, AN8855_PHY_SELECT_PAGE, page);
> 
> Calling functions with '__' is a red flag.

In this case, it is correct. It was probably a bad decision to call
this __mdiobus_write() in the first place, but we were not expecting
it to be used in real driver code, just in core code, where calls to
it are wrapped with a mutex lock/unlock.

But drivers started to need to perform multiple read/write operations
in an atomic sequence, so they started doing there own taking of the
lock, and using these unlocked low level helpers.

We should probably rename __mdiobus_write() to _mdiobus_write() to
avoid compiler namespace issues.

> > +	if (ret < 0)
> > +		dev_err_ratelimited(&bus->dev,
> > +				    "failed to set an8855 mii page\n");
> 
> Use 100-chars if it avoids these kind of line breaks.

I _guess_ the code is following networking coding style, even though
it is outside of normal networking directories. netdev keeps with the
old 80 limit.

> > +static int an8855_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> > +{
> > +	struct an8855_mfd_priv *priv = ctx;
> > +	struct mii_bus *bus = priv->bus;
> > +	u16 addr = priv->switch_addr;
> > +	int ret;
> > +
> > +	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> 
> guard()?

netdev people consider guard() too magical. scoped_guard() is however
O.K.

> > +	return ret < 0 ? ret : 0;
> 
> Doesn't the caller already expect possible >0 results?

It should not happen. Networking has a very small number of functions
which do return 1 on a different sort of success. Such functions are
generally called foo_changed(), e.g. mdiobus_modify_changed(). This
will do a read/modify/write. If it did not need to modify anything,
because of the current value, it will return 0. If something did
actually change, it returns 1. If something did actually change, you
sometimes need to take further actions. But many callers don't care,
so we have wrappers e.g. mdiobus_modify() calls
mdiobus_modify_changed() and then converts 0 or 1 to just 0. I'm
guessing this code copied such a helper when it should not of done.

	Andrew