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:

quoted

On Mon,  6 Jan 2025 09:32:55 +0100 Oleksij Rempel wrote:

quoted

+/**
+ * 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