linux-kernel - Re: [PATCH net-next v5 2/8] net: ethtool: plumb PHY stats to PHY drivers

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: <95111e20-d08a-42e5-b8cc-801e34d15040@lunn.ch>
Date: Wed, 8 Jan 2025 17:03:25 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Jakub Kicinski <kuba@...nel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Jonathan Corbet <corbet@....net>, kernel@...gutronix.de,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	Simon Horman <horms@...nel.org>,
	Russell King <linux@...linux.org.uk>,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH net-next v5 2/8] net: ethtool: plumb PHY stats to PHY
 drivers

On Wed, Jan 08, 2025 at 10:08:54AM +0100, Oleksij Rempel wrote:
> On Tue, Jan 07, 2025 at 06:02:16PM -0800, Jakub Kicinski wrote:
> > On Mon,  6 Jan 2025 09:32:55 +0100 Oleksij Rempel wrote:
> > > +/**
> > > + * phy_ethtool_get_link_ext_stats - Retrieve extended link statistics for a PHY
> > > + * @phydev: Pointer to the PHY device
> > > + * @link_stats: Pointer to the structure to store extended link statistics
> > > + *
> > > + * Populates the ethtool_link_ext_stats structure with link down event counts
> > > + * and additional driver-specific link statistics, if available.
> > > + */
> > > +static inline void phy_ethtool_get_link_ext_stats(struct phy_device *phydev,
> > > +				    struct ethtool_link_ext_stats *link_stats)
> > > +{
> > > +	link_stats->link_down_events = READ_ONCE(phydev->link_down_events);
> > > +
> > > +	if (!phydev->drv || !phydev->drv->get_link_stats)
> > > +		return;
> > > +
> > > +	mutex_lock(&phydev->lock);
> > > +	phydev->drv->get_link_stats(phydev, link_stats);
> > > +	mutex_unlock(&phydev->lock);
> > > +}
> > 
> > Do these have to be static inlines?
> > 
> > Seemds like it will just bloat the header, and make alignment more
> > painful.
> 
> On one side I need to address the request to handle phydev specific
> thing withing the PHYlib framework. On other side, I can't do it without
> openen a pandora box of build dependencies. It will be a new main-side-quest
> to resolve build dependency of net/ethtool/ and PHYlib. The workaround is to
> put this functions to the header.

Yes, the code is like this because phylib can be a module, and when it
is, you would end up with unresolved symbols if ethtool code is built
in. There are circular dependence as well, if both ethtool and phylib
are module. The inlines help solve this.

However, the number of these inline functions keeps growing. At some
point, we might want a different solution. Maybe phylib needs to
register a structure of ops with ethtool when it loads? Or we give up
with phylib being modular and only allow it to be built in.

	Andrew