[PATCH net-next v1 0/7] Introduce unified and structured PHY

[PATCH net-next v1 0/7] Introduce unified and structured PHY

[Oleksij Rempel]

This patch set introduces a unified and well-structured interface for
reporting PHY statistics. Instead of relying on arbitrary strings in PHY
drivers, this interface provides a consistent and structured way to
expose PHY statistics to userspace via ethtool.

The initial groundwork for this effort was laid by Jakub Kicinski, who
contributed patches to plumb PHY statistics to drivers and added support
for structured statistics in ethtool. Building on Jakub's work, I tested
the implementation with several PHYs, addressed a few issues, and added
support for statistics in two specific PHY drivers.

Jakub Kicinski (2):
  net: ethtool: plumb PHY stats to PHY drivers
  net: ethtool: add support for structured PHY statistics

Oleksij Rempel (5):
  phy: replace bitwise flag definitions with BIT() macro
  phy: introduce optional polling interface for PHY statistics
  ethtool: add helper to prevent invalid statistics exposure to
    userspace
  phy: dp83td510: add statistics support
  phy: dp83tg720: add statistics support

 Documentation/networking/ethtool-netlink.rst |   1 +
 drivers/net/phy/dp83td510.c                  |  98 ++++++++++++-
 drivers/net/phy/dp83tg720.c                  | 147 ++++++++++++++++++-
 drivers/net/phy/phy.c                        |  15 ++
 include/linux/ethtool.h                      |  39 +++++
 include/linux/phy.h                          |  27 +++-
 include/uapi/linux/ethtool.h                 |   2 +
 include/uapi/linux/ethtool_netlink.h         |  15 ++
 net/ethtool/linkstate.c                      |  25 +++-
 net/ethtool/netlink.h                        |   1 +
 net/ethtool/stats.c                          |  58 ++++++++
 net/ethtool/strset.c                         |   5 +
 12 files changed, 423 insertions(+), 10 deletions(-)

--
2.39.5

[PATCH net-next v1 1/7] net: ethtool: plumb PHY stats to PHY drivers

[Oleksij Rempel]

From: Jakub Kicinski <kuba@kernel.org>

Feed the existing IEEE PHY counter struct (which currently
only has one entry) and link stats into the PHY driver.
The MAC driver can override the value if it somehow has a better
idea of PHY stats. Since the stats are "undefined" at input
the drivers can't += the values, so we should be safe from
double-counting.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 include/linux/phy.h     | 10 ++++++++++
 net/ethtool/linkstate.c | 25 ++++++++++++++++++++++---
 net/ethtool/stats.c     | 19 +++++++++++++++++++
 3 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 563c46205685..523195c724b5 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1090,6 +1090,16 @@ struct phy_driver {
 	int (*cable_test_get_status)(struct phy_device *dev, bool *finished);
 
 	/* Get statistics from the PHY using ethtool */
+	/**
+	 * @get_phy_stats: Get well known statistics.
+	 * @get_link_stats: Get well known link statistics.
+	 * The input structure is not zero-initialized and the implementation
+	 * must only set statistics which are actually collected by the device.
+	 */
+	void (*get_phy_stats)(struct phy_device *dev,
+			      struct ethtool_eth_phy_stats *eth_stats);
+	void (*get_link_stats)(struct phy_device *dev,
+			       struct ethtool_link_ext_stats *link_stats);
 	/** @get_sset_count: Number of statistic counters */
 	int (*get_sset_count)(struct phy_device *dev);
 	/** @get_strings: Names of the statistic counters */
diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c
index 34d76e87847d..8d3a38cc3d48 100644
--- a/net/ethtool/linkstate.c
+++ b/net/ethtool/linkstate.c
@@ -94,6 +94,27 @@ static int linkstate_get_link_ext_state(struct net_device *dev,
 	return 0;
 }
 
+static void
+ethtool_get_phydev_stats(struct net_device *dev,
+			 struct linkstate_reply_data *data)
+{
+	struct phy_device *phydev = dev->phydev;
+
+	if (!phydev)
+		return;
+
+	if (dev->phydev)
+		data->link_stats.link_down_events =
+			READ_ONCE(dev->phydev->link_down_events);
+
+	if (!phydev->drv || !phydev->drv->get_link_stats)
+		return;
+
+	mutex_lock(&phydev->lock);
+	phydev->drv->get_link_stats(phydev, &data->link_stats);
+	mutex_unlock(&phydev->lock);
+}
+
 static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
 				  struct ethnl_reply_data *reply_base,
 				  const struct genl_info *info)
@@ -127,9 +148,7 @@ static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
 			   sizeof(data->link_stats) / 8);
 
 	if (req_base->flags & ETHTOOL_FLAG_STATS) {
-		if (dev->phydev)
-			data->link_stats.link_down_events =
-				READ_ONCE(dev->phydev->link_down_events);
+		ethtool_get_phydev_stats(dev, data);
 
 		if (dev->ethtool_ops->get_link_ext_stats)
 			dev->ethtool_ops->get_link_ext_stats(dev,
diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
index 912f0c4fff2f..cf802b1cda6f 100644
--- a/net/ethtool/stats.c
+++ b/net/ethtool/stats.c
@@ -1,5 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 
+#include <linux/phy.h>
+
 #include "netlink.h"
 #include "common.h"
 #include "bitset.h"
@@ -112,6 +114,19 @@ static int stats_parse_request(struct ethnl_req_info *req_base,
 	return 0;
 }
 
+static void
+ethtool_get_phydev_stats(struct net_device *dev, struct stats_reply_data *data)
+{
+	struct phy_device *phydev = dev->phydev;
+
+	if (!phydev || !phydev->drv || !phydev->drv->get_phy_stats)
+		return;
+
+	mutex_lock(&phydev->lock);
+	phydev->drv->get_phy_stats(phydev, &data->phy_stats);
+	mutex_unlock(&phydev->lock);
+}
+
 static int stats_prepare_data(const struct ethnl_req_info *req_base,
 			      struct ethnl_reply_data *reply_base,
 			      const struct genl_info *info)
@@ -145,6 +160,10 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
 	data->ctrl_stats.src = src;
 	data->rmon_stats.src = src;
 
+	if (test_bit(ETHTOOL_STATS_ETH_PHY, req_info->stat_mask) &&
+	    src == ETHTOOL_MAC_STATS_SRC_AGGREGATE)
+		ethtool_get_phydev_stats(dev, data);
+
 	if (test_bit(ETHTOOL_STATS_ETH_PHY, req_info->stat_mask) &&
 	    dev->ethtool_ops->get_eth_phy_stats)
 		dev->ethtool_ops->get_eth_phy_stats(dev, &data->phy_stats);
-- 
2.39.5

Re: [PATCH net-next v1 1/7] net: ethtool: plumb PHY stats to PHY drivers

[Maxime Chevallier]

Hi Oleksij,

On Tue,  3 Dec 2024 08:56:15 +0100
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> From: Jakub Kicinski <kuba@kernel.org>
> 
> Feed the existing IEEE PHY counter struct (which currently
> only has one entry) and link stats into the PHY driver.
> The MAC driver can override the value if it somehow has a better
> idea of PHY stats. Since the stats are "undefined" at input
> the drivers can't += the values, so we should be safe from
> double-counting.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

[...] 

> +static void
> +ethtool_get_phydev_stats(struct net_device *dev,
> +			 struct linkstate_reply_data *data)
> +{
> +	struct phy_device *phydev = dev->phydev;
> +
> +	if (!phydev)
> +		return;
> +
> +	if (dev->phydev)
> +		data->link_stats.link_down_events =
> +			READ_ONCE(dev->phydev->link_down_events);
> +
> +	if (!phydev->drv || !phydev->drv->get_link_stats)
> +		return;
> +
> +	mutex_lock(&phydev->lock);
> +	phydev->drv->get_link_stats(phydev, &data->link_stats);
> +	mutex_unlock(&phydev->lock);
> +}
> +
>  static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
>  				  struct ethnl_reply_data *reply_base,
>  				  const struct genl_info *info)
> @@ -127,9 +148,7 @@ static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
>  			   sizeof(data->link_stats) / 8);
>  
>  	if (req_base->flags & ETHTOOL_FLAG_STATS) {
> -		if (dev->phydev)
> -			data->link_stats.link_down_events =
> -				READ_ONCE(dev->phydev->link_down_events);
> +		ethtool_get_phydev_stats(dev, data);

I'm sorry to bother you with my multi-phy stuff, but I'd like to avoid
directly accessing netdev->phydev at least in the netlink code.

Could it be possible for you to pass a phydev directly to the
ethtool_get_phydev_stats() helper you're creating ? That way, you could
get the stats from other phydevs on the link if userspace passed a phy
index in the netlink header. You'd get the phydev that way :

phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_LINKSTATE_HEADER,], info->extack);

This is what's done in the pse-pd, plca and cabletest netlink code that
deals with phydevs.

Note that this helper will fallback to netdev->phydev if user didn't
pass any PHY index, which I expect to be what people do most of the
time. However should the netdev have more than 1 PHY, we would be able
to get the far-away PHY's stats :)

>  
>  		if (dev->ethtool_ops->get_link_ext_stats)
>  			dev->ethtool_ops->get_link_ext_stats(dev,
> diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
> index 912f0c4fff2f..cf802b1cda6f 100644
> --- a/net/ethtool/stats.c
> +++ b/net/ethtool/stats.c
> @@ -1,5 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  
> +#include <linux/phy.h>
> +
>  #include "netlink.h"
>  #include "common.h"
>  #include "bitset.h"
> @@ -112,6 +114,19 @@ static int stats_parse_request(struct ethnl_req_info *req_base,
>  	return 0;
>  }
>  
> +static void
> +ethtool_get_phydev_stats(struct net_device *dev, struct stats_reply_data *data)
> +{
> +	struct phy_device *phydev = dev->phydev;
> +
> +	if (!phydev || !phydev->drv || !phydev->drv->get_phy_stats)
> +		return;
> +
> +	mutex_lock(&phydev->lock);
> +	phydev->drv->get_phy_stats(phydev, &data->phy_stats);
> +	mutex_unlock(&phydev->lock);
> +}
> +
>  static int stats_prepare_data(const struct ethnl_req_info *req_base,
>  			      struct ethnl_reply_data *reply_base,
>  			      const struct genl_info *info)
> @@ -145,6 +160,10 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
>  	data->ctrl_stats.src = src;
>  	data->rmon_stats.src = src;
>  
> +	if (test_bit(ETHTOOL_STATS_ETH_PHY, req_info->stat_mask) &&
> +	    src == ETHTOOL_MAC_STATS_SRC_AGGREGATE)
> +		ethtool_get_phydev_stats(dev, data);

Same here :)

Thanks,

Maxime

Re: [PATCH net-next v1 1/7] net: ethtool: plumb PHY stats to PHY drivers

[Mateusz Polchlopek]

On 12/3/2024 8:56 AM, Oleksij Rempel wrote:
> From: Jakub Kicinski <kuba@kernel.org>
> 
> Feed the existing IEEE PHY counter struct (which currently
> only has one entry) and link stats into the PHY driver.
> The MAC driver can override the value if it somehow has a better
> idea of PHY stats. Since the stats are "undefined" at input
> the drivers can't += the values, so we should be safe from
> double-counting.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>   include/linux/phy.h     | 10 ++++++++++
>   net/ethtool/linkstate.c | 25 ++++++++++++++++++++++---
>   net/ethtool/stats.c     | 19 +++++++++++++++++++
>   3 files changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 563c46205685..523195c724b5 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1090,6 +1090,16 @@ struct phy_driver {
>   	int (*cable_test_get_status)(struct phy_device *dev, bool *finished);
>   
>   	/* Get statistics from the PHY using ethtool */
> +	/**
> +	 * @get_phy_stats: Get well known statistics.
> +	 * @get_link_stats: Get well known link statistics.
> +	 * The input structure is not zero-initialized and the implementation
> +	 * must only set statistics which are actually collected by the device.
> +	 */
> +	void (*get_phy_stats)(struct phy_device *dev,
> +			      struct ethtool_eth_phy_stats *eth_stats);
> +	void (*get_link_stats)(struct phy_device *dev,
> +			       struct ethtool_link_ext_stats *link_stats);
>   	/** @get_sset_count: Number of statistic counters */
>   	int (*get_sset_count)(struct phy_device *dev);
>   	/** @get_strings: Names of the statistic counters */
> diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c
> index 34d76e87847d..8d3a38cc3d48 100644
> --- a/net/ethtool/linkstate.c
> +++ b/net/ethtool/linkstate.c
> @@ -94,6 +94,27 @@ static int linkstate_get_link_ext_state(struct net_device *dev,
>   	return 0;
>   }
>   
> +static void
> +ethtool_get_phydev_stats(struct net_device *dev,
> +			 struct linkstate_reply_data *data)
> +{
> +	struct phy_device *phydev = dev->phydev;
> +
> +	if (!phydev)
> +		return;
> +
> +	if (dev->phydev)
> +		data->link_stats.link_down_events =
> +			READ_ONCE(dev->phydev->link_down_events);

Maybe silly questions but... Why to use dev->phydev when you created
*phydev pointer at the top of the function? Moreover, is that `if`
necessary? I understand that it will be always true as negative
scenario you handle in the first if? Or do I misunderstand something?

> +
> +	if (!phydev->drv || !phydev->drv->get_link_stats)
> +		return;
> +
> +	mutex_lock(&phydev->lock);
> +	phydev->drv->get_link_stats(phydev, &data->link_stats);
> +	mutex_unlock(&phydev->lock);
> +}
> +
>   static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
>   				  struct ethnl_reply_data *reply_base,
>   				  const struct genl_info *info)
> @@ -127,9 +148,7 @@ static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
>   			   sizeof(data->link_stats) / 8);
>   
>   	if (req_base->flags & ETHTOOL_FLAG_STATS) {
> -		if (dev->phydev)
> -			data->link_stats.link_down_events =
> -				READ_ONCE(dev->phydev->link_down_events);
> +		ethtool_get_phydev_stats(dev, data);
>   
>   		if (dev->ethtool_ops->get_link_ext_stats)
>   			dev->ethtool_ops->get_link_ext_stats(dev,
> diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
> index 912f0c4fff2f..cf802b1cda6f 100644
> --- a/net/ethtool/stats.c
> +++ b/net/ethtool/stats.c
> @@ -1,5 +1,7 @@
>   // SPDX-License-Identifier: GPL-2.0-only
>   
> +#include <linux/phy.h>
> +
>   #include "netlink.h"
>   #include "common.h"
>   #include "bitset.h"
> @@ -112,6 +114,19 @@ static int stats_parse_request(struct ethnl_req_info *req_base,
>   	return 0;
>   }
>   
> +static void
> +ethtool_get_phydev_stats(struct net_device *dev, struct stats_reply_data *data)
> +{

Nitpick, not big deal but you have the same function names in both
files. I see that they are static and there should not be conflict
but maybe is it worth to add `_phy_` or `_link_` in the name of
the function? Like:

ethtool_get_phydev_phy_stats
or
ethtool_get_phydev_link_stats

?

> +	struct phy_device *phydev = dev->phydev;
> +
> +	if (!phydev || !phydev->drv || !phydev->drv->get_phy_stats)
> +		return;
> +
> +	mutex_lock(&phydev->lock);
> +	phydev->drv->get_phy_stats(phydev, &data->phy_stats);
> +	mutex_unlock(&phydev->lock);
> +}
> +
>   static int stats_prepare_data(const struct ethnl_req_info *req_base,
>   			      struct ethnl_reply_data *reply_base,
>   			      const struct genl_info *info)
> @@ -145,6 +160,10 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
>   	data->ctrl_stats.src = src;
>   	data->rmon_stats.src = src;
>   
> +	if (test_bit(ETHTOOL_STATS_ETH_PHY, req_info->stat_mask) &&
> +	    src == ETHTOOL_MAC_STATS_SRC_AGGREGATE)
> +		ethtool_get_phydev_stats(dev, data);
> +
>   	if (test_bit(ETHTOOL_STATS_ETH_PHY, req_info->stat_mask) &&
>   	    dev->ethtool_ops->get_eth_phy_stats)
>   		dev->ethtool_ops->get_eth_phy_stats(dev, &data->phy_stats);

Re: [PATCH net-next v1 1/7] net: ethtool: plumb PHY stats to PHY drivers

[Russell King (Oracle)]

On Tue, Dec 03, 2024 at 08:56:15AM +0100, Oleksij Rempel wrote:
> From: Jakub Kicinski <kuba@kernel.org>
> 
> Feed the existing IEEE PHY counter struct (which currently
> only has one entry) and link stats into the PHY driver.
> The MAC driver can override the value if it somehow has a better
> idea of PHY stats. Since the stats are "undefined" at input
> the drivers can't += the values, so we should be safe from
> double-counting.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  include/linux/phy.h     | 10 ++++++++++
>  net/ethtool/linkstate.c | 25 ++++++++++++++++++++++---
>  net/ethtool/stats.c     | 19 +++++++++++++++++++
>  3 files changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 563c46205685..523195c724b5 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1090,6 +1090,16 @@ struct phy_driver {
>  	int (*cable_test_get_status)(struct phy_device *dev, bool *finished);
>  
>  	/* Get statistics from the PHY using ethtool */
> +	/**
> +	 * @get_phy_stats: Get well known statistics.
> +	 * @get_link_stats: Get well known link statistics.
> +	 * The input structure is not zero-initialized and the implementation
> +	 * must only set statistics which are actually collected by the device.

Eh what? This states to me that the structure is not initialised, but
drivers should not write to all members unless they support the
statistic.

Doesn't this mean we end up returning uninitialised data to userspace?
If the structure is not initialised, how does core code know which
statistics the driver has set to avoid returning uninitialised data?

Also, one comment per function pointer please.

> +	 */
> +	void (*get_phy_stats)(struct phy_device *dev,
> +			      struct ethtool_eth_phy_stats *eth_stats);
> +	void (*get_link_stats)(struct phy_device *dev,
> +			       struct ethtool_link_ext_stats *link_stats);
>  	/** @get_sset_count: Number of statistic counters */
>  	int (*get_sset_count)(struct phy_device *dev);
>  	/** @get_strings: Names of the statistic counters */
> diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c
> index 34d76e87847d..8d3a38cc3d48 100644
> --- a/net/ethtool/linkstate.c
> +++ b/net/ethtool/linkstate.c
> @@ -94,6 +94,27 @@ static int linkstate_get_link_ext_state(struct net_device *dev,
>  	return 0;
>  }
>  
> +static void
> +ethtool_get_phydev_stats(struct net_device *dev,
> +			 struct linkstate_reply_data *data)
> +{
> +	struct phy_device *phydev = dev->phydev;
> +
> +	if (!phydev)
> +		return;
> +
> +	if (dev->phydev)
> +		data->link_stats.link_down_events =
> +			READ_ONCE(dev->phydev->link_down_events);
> +
> +	if (!phydev->drv || !phydev->drv->get_link_stats)
> +		return;
> +
> +	mutex_lock(&phydev->lock);
> +	phydev->drv->get_link_stats(phydev, &data->link_stats);
> +	mutex_unlock(&phydev->lock);

I don't like the idea of code outside of phylib fiddling around with
phy_device members. Please make the bulk of this a function in phylib,
and then do:

	if (phydev)
		phy_ethtool_get_stats(phydev, &data->link_stats);

However, at that point it's probably not worth having the separate
function, and it might as well be placed in linkstate_prepare_data().

> +}
> +
>  static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
>  				  struct ethnl_reply_data *reply_base,
>  				  const struct genl_info *info)
> @@ -127,9 +148,7 @@ static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
>  			   sizeof(data->link_stats) / 8);
>  
>  	if (req_base->flags & ETHTOOL_FLAG_STATS) {
> -		if (dev->phydev)
> -			data->link_stats.link_down_events =
> -				READ_ONCE(dev->phydev->link_down_events);
> +		ethtool_get_phydev_stats(dev, data);
>  
>  		if (dev->ethtool_ops->get_link_ext_stats)
>  			dev->ethtool_ops->get_link_ext_stats(dev,
> diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
> index 912f0c4fff2f..cf802b1cda6f 100644
> --- a/net/ethtool/stats.c
> +++ b/net/ethtool/stats.c
> @@ -1,5 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  
> +#include <linux/phy.h>
> +
>  #include "netlink.h"
>  #include "common.h"
>  #include "bitset.h"
> @@ -112,6 +114,19 @@ static int stats_parse_request(struct ethnl_req_info *req_base,
>  	return 0;
>  }
>  
> +static void
> +ethtool_get_phydev_stats(struct net_device *dev, struct stats_reply_data *data)
> +{
> +	struct phy_device *phydev = dev->phydev;
> +
> +	if (!phydev || !phydev->drv || !phydev->drv->get_phy_stats)
> +		return;
> +
> +	mutex_lock(&phydev->lock);
> +	phydev->drv->get_phy_stats(phydev, &data->phy_stats);
> +	mutex_unlock(&phydev->lock);

Ditto.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v1 1/7] net: ethtool: plumb PHY stats to PHY drivers

[Jakub Kicinski]

On Thu, 5 Dec 2024 11:57:33 +0000 Russell King (Oracle) wrote:
> > +	 * The input structure is not zero-initialized and the implementation
> > +	 * must only set statistics which are actually collected by the device.  
> 
> Eh what? This states to me that the structure is not initialised, but
> drivers should not write to all members unless they support the
> statistic.
> 
> Doesn't this mean we end up returning uninitialised data to userspace?
> If the structure is not initialised, how does core code know which
> statistics the driver has set to avoid returning uninitialised data?

It's not zero-initialized. Meaning it's initialized to a special magic
value that the core then checks for to decide if the driver actually
reported something.

Maybe this:

 * Drivers must not zero out statistics which they don't report.
 * Core will initialize members to ETHTOOL_STAT_NOT_SET and check
 * for this value to report to user space only the stats actually
 * supported by the device.

IDK how to phrase this better..

Re: [PATCH net-next v1 1/7] net: ethtool: plumb PHY stats to PHY drivers

[Russell King (Oracle)]

On Thu, Dec 05, 2024 at 05:19:09PM -0800, Jakub Kicinski wrote:
> On Thu, 5 Dec 2024 11:57:33 +0000 Russell King (Oracle) wrote:
> > > +	 * The input structure is not zero-initialized and the implementation
> > > +	 * must only set statistics which are actually collected by the device.  
> > 
> > Eh what? This states to me that the structure is not initialised, but
> > drivers should not write to all members unless they support the
> > statistic.
> > 
> > Doesn't this mean we end up returning uninitialised data to userspace?
> > If the structure is not initialised, how does core code know which
> > statistics the driver has set to avoid returning uninitialised data?
> 
> It's not zero-initialized. Meaning it's initialized to a special magic
> value that the core then checks for to decide if the driver actually
> reported something.
> 
> Maybe this:
> 
>  * Drivers must not zero out statistics which they don't report.
>  * Core will initialize members to ETHTOOL_STAT_NOT_SET and check
>  * for this value to report to user space only the stats actually
>  * supported by the device.
> 
> IDK how to phrase this better..

Maybe:

 * The input structure is pre-initialised with ETHTOOL_STAT_NOT_SET and
 * the implementation must only change implemented statistics.

?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v1 1/7] net: ethtool: plumb PHY stats to PHY drivers

[Jakub Kicinski]

On Fri, 6 Dec 2024 09:11:32 +0000 Russell King (Oracle) wrote:
> Maybe:
> 
>  * The input structure is pre-initialised with ETHTOOL_STAT_NOT_SET and
>  * the implementation must only change implemented statistics.

Yup, that's better!

FWIW I think my brain goes to talking about zero-init because for
per-queue or per-cpu stats some drivers do:

	for each q:
		struct->stat += q->stat;

without first setting to 0. And it _seems_ fine since NOT_SET is -1,
and the off-by-one is hard to spot. But for PHY stats this sort of
iteration is very unlikely.

Re: [PATCH net-next v1 1/7] net: ethtool: plumb PHY stats to PHY drivers

[Simon Horman]

On Tue, Dec 03, 2024 at 08:56:15AM +0100, Oleksij Rempel wrote:
> From: Jakub Kicinski <kuba@kernel.org>
> 
> Feed the existing IEEE PHY counter struct (which currently
> only has one entry) and link stats into the PHY driver.
> The MAC driver can override the value if it somehow has a better
> idea of PHY stats. Since the stats are "undefined" at input
> the drivers can't += the values, so we should be safe from
> double-counting.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  include/linux/phy.h     | 10 ++++++++++
>  net/ethtool/linkstate.c | 25 ++++++++++++++++++++++---
>  net/ethtool/stats.c     | 19 +++++++++++++++++++
>  3 files changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 563c46205685..523195c724b5 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1090,6 +1090,16 @@ struct phy_driver {
>  	int (*cable_test_get_status)(struct phy_device *dev, bool *finished);
>  
>  	/* Get statistics from the PHY using ethtool */
> +	/**
> +	 * @get_phy_stats: Get well known statistics.
> +	 * @get_link_stats: Get well known link statistics.
> +	 * The input structure is not zero-initialized and the implementation
> +	 * must only set statistics which are actually collected by the device.
> +	 */
> +	void (*get_phy_stats)(struct phy_device *dev,
> +			      struct ethtool_eth_phy_stats *eth_stats);

nit: There should be a kernel doc for @get_link_stats here.

     Flagged by ./scripts/kernel-doc -none

> +	void (*get_link_stats)(struct phy_device *dev,
> +			       struct ethtool_link_ext_stats *link_stats);
>  	/** @get_sset_count: Number of statistic counters */
>  	int (*get_sset_count)(struct phy_device *dev);
>  	/** @get_strings: Names of the statistic counters */

...

[PATCH net-next v1 2/7] net: ethtool: add support for structured PHY statistics

[Oleksij Rempel]

From: Jakub Kicinski <kuba@kernel.org>

Introduce a new way to report PHY statistics in a structured and
standardized format using the netlink API. This new method does not
replace the old driver-specific stats, which can still be accessed with
`ethtool -S <eth name>`. The structured stats are available with
`ethtool -S <eth name> --all-groups`.

This new method makes it easier to diagnose problems by organizing stats
in a consistent and documented way.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 Documentation/networking/ethtool-netlink.rst |  1 +
 include/linux/ethtool.h                      | 23 +++++++++++
 include/linux/phy.h                          |  3 +-
 include/uapi/linux/ethtool.h                 |  2 +
 include/uapi/linux/ethtool_netlink.h         | 15 +++++++
 net/ethtool/netlink.h                        |  1 +
 net/ethtool/stats.c                          | 43 +++++++++++++++++++-
 net/ethtool/strset.c                         |  5 +++
 8 files changed, 90 insertions(+), 3 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index b25926071ece..3bc6003185a1 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -1611,6 +1611,7 @@ the ``ETHTOOL_A_STATS_GROUPS`` bitset. Currently defined values are:
  ETHTOOL_STATS_ETH_PHY  eth-phy  Basic IEEE 802.3 PHY statistics (30.3.2.1.*)
  ETHTOOL_STATS_ETH_CTRL eth-ctrl Basic IEEE 802.3 MAC Ctrl statistics (30.3.3.*)
  ETHTOOL_STATS_RMON     rmon     RMON (RFC 2819) statistics
+ ETHTOOL_STATS_PHY      phy      Additional PHY statistics, not defined by IEEE
  ====================== ======== ===============================================
 
 Each group should have a corresponding ``ETHTOOL_A_STATS_GRP`` in the reply.
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index b8b935b52603..b0ed740ca749 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -412,6 +412,29 @@ struct ethtool_eth_phy_stats {
 	);
 };
 
+/**
+ * struct ethtool_phy_stats - PHY-level statistics counters
+ * @rx_packets: Total successfully received frames
+ * @rx_bytes: Total successfully received bytes
+ * @rx_errors: Total received frames with errors (e.g., CRC errors)
+ * @tx_packets: Total successfully transmitted frames
+ * @tx_bytes: Total successfully transmitted bytes
+ * @tx_errors: Total transmitted frames with errors
+ *
+ * This structure provides a standardized interface for reporting
+ * PHY-level statistics counters. It is designed to expose statistics
+ * commonly provided by PHYs but not explicitly defined in the IEEE
+ * 802.3 standard.
+ */
+struct ethtool_phy_stats {
+	u64 rx_packets;
+	u64 rx_bytes;
+	u64 rx_errors;
+	u64 tx_packets;
+	u64 tx_bytes;
+	u64 tx_errors;
+};
+
 /* Basic IEEE 802.3 MAC Ctrl statistics (30.3.3.*), not otherwise exposed
  * via a more targeted API.
  */
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 523195c724b5..20a0d43ab5d4 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1097,7 +1097,8 @@ struct phy_driver {
 	 * must only set statistics which are actually collected by the device.
 	 */
 	void (*get_phy_stats)(struct phy_device *dev,
-			      struct ethtool_eth_phy_stats *eth_stats);
+			      struct ethtool_eth_phy_stats *eth_stats,
+			      struct ethtool_phy_stats *stats);
 	void (*get_link_stats)(struct phy_device *dev,
 			       struct ethtool_link_ext_stats *link_stats);
 	/** @get_sset_count: Number of statistic counters */
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 7e1b3820f91f..d1089b88efc7 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -681,6 +681,7 @@ enum ethtool_link_ext_substate_module {
  * @ETH_SS_STATS_ETH_MAC: names of IEEE 802.3 MAC statistics
  * @ETH_SS_STATS_ETH_CTRL: names of IEEE 802.3 MAC Control statistics
  * @ETH_SS_STATS_RMON: names of RMON statistics
+ * @ETH_SS_STATS_PHY: names of PHY(dev) statistics
  *
  * @ETH_SS_COUNT: number of defined string sets
  */
@@ -706,6 +707,7 @@ enum ethtool_stringset {
 	ETH_SS_STATS_ETH_MAC,
 	ETH_SS_STATS_ETH_CTRL,
 	ETH_SS_STATS_RMON,
+	ETH_SS_STATS_PHY,
 
 	/* add new constants above here */
 	ETH_SS_COUNT
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 283305f6b063..dc332f8aa3a6 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -820,6 +820,7 @@ enum {
 	ETHTOOL_STATS_ETH_MAC,
 	ETHTOOL_STATS_ETH_CTRL,
 	ETHTOOL_STATS_RMON,
+	ETHTOOL_STATS_PHY,
 
 	/* add new constants above here */
 	__ETHTOOL_STATS_CNT
@@ -935,6 +936,20 @@ enum {
 	ETHTOOL_A_STATS_RMON_MAX = (__ETHTOOL_A_STATS_RMON_CNT - 1)
 };
 
+enum {
+	/* Basic packet counters if PHY has separate counters from the MAC */
+	ETHTOOL_A_STATS_PHY_RX_PKTS,
+	ETHTOOL_A_STATS_PHY_RX_BYTES,
+	ETHTOOL_A_STATS_PHY_RX_ERRORS,
+	ETHTOOL_A_STATS_PHY_TX_PKTS,
+	ETHTOOL_A_STATS_PHY_TX_BYTES,
+	ETHTOOL_A_STATS_PHY_TX_ERRORS,
+
+	/* add new constants above here */
+	__ETHTOOL_A_STATS_PHY_CNT,
+	ETHTOOL_A_STATS_PHY_MAX = (__ETHTOOL_A_STATS_PHY_CNT - 1)
+};
+
 /* MODULE */
 
 enum {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 203b08eb6c6f..3a6ecdcb5b8c 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -505,5 +505,6 @@ extern const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING
 extern const char stats_eth_mac_names[__ETHTOOL_A_STATS_ETH_MAC_CNT][ETH_GSTRING_LEN];
 extern const char stats_eth_ctrl_names[__ETHTOOL_A_STATS_ETH_CTRL_CNT][ETH_GSTRING_LEN];
 extern const char stats_rmon_names[__ETHTOOL_A_STATS_RMON_CNT][ETH_GSTRING_LEN];
+extern const char stats_phy_names[__ETHTOOL_A_STATS_PHY_CNT][ETH_GSTRING_LEN];
 
 #endif /* _NET_ETHTOOL_NETLINK_H */
diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
index cf802b1cda6f..ec3b0a2e01e8 100644
--- a/net/ethtool/stats.c
+++ b/net/ethtool/stats.c
@@ -22,6 +22,7 @@ struct stats_reply_data {
 		struct ethtool_eth_mac_stats	mac_stats;
 		struct ethtool_eth_ctrl_stats	ctrl_stats;
 		struct ethtool_rmon_stats	rmon_stats;
+		struct ethtool_phy_stats	phydev_stats;
 	);
 	const struct ethtool_rmon_hist_range	*rmon_ranges;
 };
@@ -34,6 +35,7 @@ const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN] = {
 	[ETHTOOL_STATS_ETH_MAC]			= "eth-mac",
 	[ETHTOOL_STATS_ETH_CTRL]		= "eth-ctrl",
 	[ETHTOOL_STATS_RMON]			= "rmon",
+	[ETHTOOL_STATS_PHY]			= "phydev",
 };
 
 const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN] = {
@@ -78,6 +80,15 @@ const char stats_rmon_names[__ETHTOOL_A_STATS_RMON_CNT][ETH_GSTRING_LEN] = {
 	[ETHTOOL_A_STATS_RMON_JABBER]		= "etherStatsJabbers",
 };
 
+const char stats_phy_names[__ETHTOOL_A_STATS_PHY_CNT][ETH_GSTRING_LEN] = {
+	[ETHTOOL_A_STATS_PHY_RX_PKTS]		= "RxFrames",
+	[ETHTOOL_A_STATS_PHY_RX_BYTES]		= "RxOctets",
+	[ETHTOOL_A_STATS_PHY_RX_ERRORS]		= "RxErrors",
+	[ETHTOOL_A_STATS_PHY_TX_PKTS]		= "TxFrames",
+	[ETHTOOL_A_STATS_PHY_TX_BYTES]		= "TxOctets",
+	[ETHTOOL_A_STATS_PHY_TX_ERRORS]		= "TxErrors",
+};
+
 const struct nla_policy ethnl_stats_get_policy[ETHTOOL_A_STATS_SRC + 1] = {
 	[ETHTOOL_A_STATS_HEADER]	=
 		NLA_POLICY_NESTED(ethnl_header_policy),
@@ -123,7 +134,8 @@ ethtool_get_phydev_stats(struct net_device *dev, struct stats_reply_data *data)
 		return;
 
 	mutex_lock(&phydev->lock);
-	phydev->drv->get_phy_stats(phydev, &data->phy_stats);
+	phydev->drv->get_phy_stats(phydev, &data->phy_stats,
+				   &data->phydev_stats);
 	mutex_unlock(&phydev->lock);
 }
 
@@ -160,7 +172,8 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
 	data->ctrl_stats.src = src;
 	data->rmon_stats.src = src;
 
-	if (test_bit(ETHTOOL_STATS_ETH_PHY, req_info->stat_mask) &&
+	if ((test_bit(ETHTOOL_STATS_PHY, req_info->stat_mask) ||
+	     test_bit(ETHTOOL_STATS_ETH_PHY, req_info->stat_mask)) &&
 	    src == ETHTOOL_MAC_STATS_SRC_AGGREGATE)
 		ethtool_get_phydev_stats(dev, data);
 
@@ -213,6 +226,10 @@ static int stats_reply_size(const struct ethnl_req_info *req_base,
 			nla_total_size(4)) *	/* _A_STATS_GRP_HIST_BKT_HI */
 			ETHTOOL_RMON_HIST_MAX * 2;
 	}
+	if (test_bit(ETHTOOL_STATS_PHY, req_info->stat_mask)) {
+		n_stats += sizeof(struct ethtool_phy_stats) / sizeof(u64);
+		n_grps++;
+	}
 
 	len += n_grps * (nla_total_size(0) + /* _A_STATS_GRP */
 			 nla_total_size(4) + /* _A_STATS_GRP_ID */
@@ -266,6 +283,25 @@ static int stats_put_phy_stats(struct sk_buff *skb,
 	return 0;
 }
 
+static int stats_put_phydev_stats(struct sk_buff *skb,
+				  const struct stats_reply_data *data)
+{
+	if (stat_put(skb, ETHTOOL_A_STATS_PHY_RX_PKTS,
+		     data->phydev_stats.rx_packets) ||
+	    stat_put(skb, ETHTOOL_A_STATS_PHY_RX_BYTES,
+		     data->phydev_stats.rx_bytes) ||
+	    stat_put(skb, ETHTOOL_A_STATS_PHY_RX_ERRORS,
+		     data->phydev_stats.rx_errors) ||
+	    stat_put(skb, ETHTOOL_A_STATS_PHY_TX_PKTS,
+		     data->phydev_stats.tx_packets) ||
+	    stat_put(skb, ETHTOOL_A_STATS_PHY_TX_BYTES,
+		     data->phydev_stats.tx_bytes) ||
+	    stat_put(skb, ETHTOOL_A_STATS_PHY_TX_ERRORS,
+		     data->phydev_stats.tx_errors))
+		return -EMSGSIZE;
+	return 0;
+}
+
 static int stats_put_mac_stats(struct sk_buff *skb,
 			       const struct stats_reply_data *data)
 {
@@ -442,6 +478,9 @@ static int stats_fill_reply(struct sk_buff *skb,
 	if (!ret && test_bit(ETHTOOL_STATS_RMON, req_info->stat_mask))
 		ret = stats_put_stats(skb, data, ETHTOOL_STATS_RMON,
 				      ETH_SS_STATS_RMON, stats_put_rmon_stats);
+	if (!ret && test_bit(ETHTOOL_STATS_PHY, req_info->stat_mask))
+		ret = stats_put_stats(skb, data, ETHTOOL_STATS_PHY,
+				      ETH_SS_STATS_PHY, stats_put_phydev_stats);
 
 	return ret;
 }
diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index b3382b3cf325..818cf01f0911 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -105,6 +105,11 @@ static const struct strset_info info_template[] = {
 		.count		= __ETHTOOL_A_STATS_RMON_CNT,
 		.strings	= stats_rmon_names,
 	},
+	[ETH_SS_STATS_PHY] = {
+		.per_dev	= false,
+		.count		= __ETHTOOL_A_STATS_PHY_CNT,
+		.strings	= stats_phy_names,
+	},
 };
 
 struct strset_req_info {
-- 
2.39.5

Re: [PATCH net-next v1 2/7] net: ethtool: add support for structured PHY statistics

[Russell King (Oracle)]

On Tue, Dec 03, 2024 at 08:56:16AM +0100, Oleksij Rempel wrote:
> From: Jakub Kicinski <kuba@kernel.org>
> 
> Introduce a new way to report PHY statistics in a structured and
> standardized format using the netlink API. This new method does not
> replace the old driver-specific stats, which can still be accessed with
> `ethtool -S <eth name>`. The structured stats are available with
> `ethtool -S <eth name> --all-groups`.
> 
> This new method makes it easier to diagnose problems by organizing stats
> in a consistent and documented way.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
...
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 523195c724b5..20a0d43ab5d4 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1097,7 +1097,8 @@ struct phy_driver {
>  	 * must only set statistics which are actually collected by the device.
>  	 */
>  	void (*get_phy_stats)(struct phy_device *dev,
> -			      struct ethtool_eth_phy_stats *eth_stats);
> +			      struct ethtool_eth_phy_stats *eth_stats,
> +			      struct ethtool_phy_stats *stats);

So we introduce a new function pointer in patch 1, and then change its
prototype in patch 2... wouldn't it be better to introduce the new
structure in patch 1, and then the function pointer in the next patch?

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH net-next v1 3/7] phy: replace bitwise flag definitions with BIT() macro

[Oleksij Rempel]

Convert the PHY flag definitions to use the BIT() macro instead of
hexadecimal values. This improves readability and maintainability.

No functional changes are introduced by this modification.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 include/linux/phy.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 20a0d43ab5d4..a6c47b0675af 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -86,11 +86,11 @@ extern const int phy_10gbit_features_array[1];
 #define PHY_POLL		-1
 #define PHY_MAC_INTERRUPT	-2
 
-#define PHY_IS_INTERNAL		0x00000001
-#define PHY_RST_AFTER_CLK_EN	0x00000002
-#define PHY_POLL_CABLE_TEST	0x00000004
-#define PHY_ALWAYS_CALL_SUSPEND	0x00000008
-#define MDIO_DEVICE_IS_PHY	0x80000000
+#define PHY_IS_INTERNAL		BIT(0)
+#define PHY_RST_AFTER_CLK_EN	BIT(1)
+#define PHY_POLL_CABLE_TEST	BIT(2)
+#define PHY_ALWAYS_CALL_SUSPEND	BIT(3)
+#define MDIO_DEVICE_IS_PHY	BIT(31)
 
 /**
  * enum phy_interface_t - Interface Mode definitions
-- 
2.39.5

RE: [PATCH net-next v1 3/7] phy: replace bitwise flag definitions with BIT() macro

[David Laight]

From: Oleksij Rempel
> Sent: 03 December 2024 07:56
> 
> Convert the PHY flag definitions to use the BIT() macro instead of
> hexadecimal values. This improves readability and maintainability.
> 
> No functional changes are introduced by this modification.

Are you absolutely sure.
You are changing the type of the constants from 'signed int' to
'unsigned long' and that can easily have unexpected consequences.
Especially since MDIO_DEVICE_IS_PHY was negative.

	David

> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  include/linux/phy.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 20a0d43ab5d4..a6c47b0675af 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -86,11 +86,11 @@ extern const int phy_10gbit_features_array[1];
>  #define PHY_POLL		-1
>  #define PHY_MAC_INTERRUPT	-2
> 
> -#define PHY_IS_INTERNAL		0x00000001
> -#define PHY_RST_AFTER_CLK_EN	0x00000002
> -#define PHY_POLL_CABLE_TEST	0x00000004
> -#define PHY_ALWAYS_CALL_SUSPEND	0x00000008
> -#define MDIO_DEVICE_IS_PHY	0x80000000
> +#define PHY_IS_INTERNAL		BIT(0)
> +#define PHY_RST_AFTER_CLK_EN	BIT(1)
> +#define PHY_POLL_CABLE_TEST	BIT(2)
> +#define PHY_ALWAYS_CALL_SUSPEND	BIT(3)
> +#define MDIO_DEVICE_IS_PHY	BIT(31)
> 
>  /**
>   * enum phy_interface_t - Interface Mode definitions
> --
> 2.39.5
> 

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH net-next v1 3/7] phy: replace bitwise flag definitions with BIT() macro

[Oleksij Rempel]

On Thu, Dec 05, 2024 at 02:50:32AM +0000, David Laight wrote:
> From: Oleksij Rempel
> > Sent: 03 December 2024 07:56
> > 
> > Convert the PHY flag definitions to use the BIT() macro instead of
> > hexadecimal values. This improves readability and maintainability.
> > 
> > No functional changes are introduced by this modification.
> 
> Are you absolutely sure.
> You are changing the type of the constants from 'signed int' to
> 'unsigned long' and that can easily have unexpected consequences.
> Especially since MDIO_DEVICE_IS_PHY was negative.

In current kernel code following flags are assigned to u32 variable: 

> > -#define PHY_IS_INTERNAL		0x00000001
> > -#define PHY_RST_AFTER_CLK_EN	0x00000002
> > -#define PHY_POLL_CABLE_TEST	0x00000004
> > -#define PHY_ALWAYS_CALL_SUSPEND	0x00000008

phydrv->flags (u32)

This one is assigned to an int:
> > -#define MDIO_DEVICE_IS_PHY	0x80000000

mdiodrv->flags (int)

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v1 3/7] phy: replace bitwise flag definitions with BIT() macro

[Russell King (Oracle)]

On Tue, Dec 03, 2024 at 08:56:17AM +0100, Oleksij Rempel wrote:
> Convert the PHY flag definitions to use the BIT() macro instead of
> hexadecimal values. This improves readability and maintainability.

Maybe only readability. One can argue that changing them introduces the
possibility of conflicts when porting patches which adds maintenance
burden.

However, this change looks fine to me:

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v1 3/7] phy: replace bitwise flag definitions with BIT() macro

[Russell King (Oracle)]

On Thu, Dec 05, 2024 at 12:02:19PM +0000, Russell King (Oracle) wrote:
> On Tue, Dec 03, 2024 at 08:56:17AM +0100, Oleksij Rempel wrote:
> > Convert the PHY flag definitions to use the BIT() macro instead of
> > hexadecimal values. This improves readability and maintainability.
> 
> Maybe only readability. One can argue that changing them introduces the
> possibility of conflicts when porting patches which adds maintenance
> burden.

Thinking a bit more about this, we can do much better to improve
readability. The question that stands out right now is "but what
are these flags used for?" and their definition doesn't make any
hint.

The PHY_* constants are for struct phy_driver.flags, and I think
this needs to be documented. I think that's a much more important
detail here that would massively improve readability way beyond
simply changing them to be defined in terms of BIT().

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH net-next v1 4/7] phy: introduce optional polling interface for PHY statistics

[Oleksij Rempel]

Add an optional polling interface for PHY statistics to simplify driver
implementation. Drivers can request the PHYlib to handle the polling task by
explicitly setting the `PHY_POLL_STATS` flag in their driver configuration.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/phy.c | 15 +++++++++++++++
 include/linux/phy.h   |  6 ++++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 0d20b534122b..b10ee9223fc9 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1346,6 +1346,18 @@ static int phy_enable_interrupts(struct phy_device *phydev)
 	return phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED);
 }
 
+/**
+ * phy_update_stats - update the PHY statistics
+ * @phydev: target phy_device struct
+ */
+static int phy_update_stats(struct phy_device *phydev)
+{
+	if (!phydev->drv->update_stats)
+		return 0;
+
+	return phydev->drv->update_stats(phydev);
+}
+
 /**
  * phy_request_interrupt - request and enable interrupt for a PHY device
  * @phydev: target phy_device struct
@@ -1415,6 +1427,9 @@ static enum phy_state_work _phy_state_machine(struct phy_device *phydev)
 	case PHY_RUNNING:
 		err = phy_check_link_status(phydev);
 		func = &phy_check_link_status;
+
+		if (!err)
+			err = phy_update_stats(phydev);
 		break;
 	case PHY_CABLETEST:
 		err = phydev->drv->cable_test_get_status(phydev, &finished);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index a6c47b0675af..21cd44d177d2 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -90,6 +90,7 @@ extern const int phy_10gbit_features_array[1];
 #define PHY_RST_AFTER_CLK_EN	BIT(1)
 #define PHY_POLL_CABLE_TEST	BIT(2)
 #define PHY_ALWAYS_CALL_SUSPEND	BIT(3)
+#define PHY_POLL_STATS		BIT(4)
 #define MDIO_DEVICE_IS_PHY	BIT(31)
 
 /**
@@ -1101,6 +1102,8 @@ struct phy_driver {
 			      struct ethtool_phy_stats *stats);
 	void (*get_link_stats)(struct phy_device *dev,
 			       struct ethtool_link_ext_stats *link_stats);
+	int (*update_stats)(struct phy_device *dev);
+
 	/** @get_sset_count: Number of statistic counters */
 	int (*get_sset_count)(struct phy_device *dev);
 	/** @get_strings: Names of the statistic counters */
@@ -1591,6 +1594,9 @@ static inline bool phy_polling_mode(struct phy_device *phydev)
 		if (phydev->drv->flags & PHY_POLL_CABLE_TEST)
 			return true;
 
+	if (phydev->drv->update_stats && phydev->drv->flags & PHY_POLL_STATS)
+		return true;
+
 	return phydev->irq == PHY_POLL;
 }
 
-- 
2.39.5

Re: [PATCH net-next v1 4/7] phy: introduce optional polling interface for PHY statistics

[Mateusz Polchlopek]

On 12/3/2024 8:56 AM, Oleksij Rempel wrote:
> Add an optional polling interface for PHY statistics to simplify driver
> implementation. Drivers can request the PHYlib to handle the polling task by
> explicitly setting the `PHY_POLL_STATS` flag in their driver configuration.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>   drivers/net/phy/phy.c | 15 +++++++++++++++
>   include/linux/phy.h   |  6 ++++++
>   2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 0d20b534122b..b10ee9223fc9 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1346,6 +1346,18 @@ static int phy_enable_interrupts(struct phy_device *phydev)
>   	return phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED);
>   }
>   
> +/**
> + * phy_update_stats - update the PHY statistics
> + * @phydev: target phy_device struct
> + */

As this is newly intoduced function I would love to see the full
kdoc header, with information what the function returns, like here:

https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation

> +static int phy_update_stats(struct phy_device *phydev)
> +{
> +	if (!phydev->drv->update_stats)
> +		return 0;
> +
> +	return phydev->drv->update_stats(phydev);
> +}
> +
>   /**
>    * phy_request_interrupt - request and enable interrupt for a PHY device
>    * @phydev: target phy_device struct
> @@ -1415,6 +1427,9 @@ static enum phy_state_work _phy_state_machine(struct phy_device *phydev)
>   	case PHY_RUNNING:
>   		err = phy_check_link_status(phydev);
>   		func = &phy_check_link_status;
> +
> +		if (!err)
> +			err = phy_update_stats(phydev);
>   		break;
>   	case PHY_CABLETEST:
>   		err = phydev->drv->cable_test_get_status(phydev, &finished);
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index a6c47b0675af..21cd44d177d2 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -90,6 +90,7 @@ extern const int phy_10gbit_features_array[1];
>   #define PHY_RST_AFTER_CLK_EN	BIT(1)
>   #define PHY_POLL_CABLE_TEST	BIT(2)
>   #define PHY_ALWAYS_CALL_SUSPEND	BIT(3)
> +#define PHY_POLL_STATS		BIT(4)
>   #define MDIO_DEVICE_IS_PHY	BIT(31)
>   
>   /**
> @@ -1101,6 +1102,8 @@ struct phy_driver {
>   			      struct ethtool_phy_stats *stats);
>   	void (*get_link_stats)(struct phy_device *dev,
>   			       struct ethtool_link_ext_stats *link_stats);
> +	int (*update_stats)(struct phy_device *dev);
> +
>   	/** @get_sset_count: Number of statistic counters */
>   	int (*get_sset_count)(struct phy_device *dev);
>   	/** @get_strings: Names of the statistic counters */
> @@ -1591,6 +1594,9 @@ static inline bool phy_polling_mode(struct phy_device *phydev)
>   		if (phydev->drv->flags & PHY_POLL_CABLE_TEST)
>   			return true;
>   
> +	if (phydev->drv->update_stats && phydev->drv->flags & PHY_POLL_STATS)
> +		return true;
> +
>   	return phydev->irq == PHY_POLL;
>   }
>   

Re: [PATCH net-next v1 4/7] phy: introduce optional polling interface for PHY statistics

[Russell King (Oracle)]

On Thu, Dec 05, 2024 at 09:14:08AM +0100, Mateusz Polchlopek wrote:
> On 12/3/2024 8:56 AM, Oleksij Rempel wrote:
> > Add an optional polling interface for PHY statistics to simplify driver
> > implementation. Drivers can request the PHYlib to handle the polling task by
> > explicitly setting the `PHY_POLL_STATS` flag in their driver configuration.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >   drivers/net/phy/phy.c | 15 +++++++++++++++
> >   include/linux/phy.h   |  6 ++++++
> >   2 files changed, 21 insertions(+)
> > 
> > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > index 0d20b534122b..b10ee9223fc9 100644
> > --- a/drivers/net/phy/phy.c
> > +++ b/drivers/net/phy/phy.c
> > @@ -1346,6 +1346,18 @@ static int phy_enable_interrupts(struct phy_device *phydev)
> >   	return phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED);
> >   }
> > +/**
> > + * phy_update_stats - update the PHY statistics
> > + * @phydev: target phy_device struct
> > + */
> 
> As this is newly intoduced function I would love to see the full
> kdoc header, with information what the function returns, like here:
> 
> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation

As it's an internal phylib function, I don't think there's any need for
kernel-doc unless it's something more complex. It's obvious what the
function itself is doing.

What would be more helpful is to properly document the "update_stats"
method, since that is what PHY drivers are going to implement. Yes, I
know kernel-doc isn't good at that, but look at phylink.h to see how
to do it.

> > @@ -1591,6 +1594,9 @@ static inline bool phy_polling_mode(struct phy_device *phydev)
> >   		if (phydev->drv->flags & PHY_POLL_CABLE_TEST)
> >   			return true;
> > +	if (phydev->drv->update_stats && phydev->drv->flags & PHY_POLL_STATS)
> > +		return true;

Is there a case where ->update_stats would be implemented but we
wouldn't have PHY_POLL_STATS set?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v1 4/7] phy: introduce optional polling interface for PHY statistics

[Oleksij Rempel]

On Thu, Dec 05, 2024 at 12:09:55PM +0000, Russell King (Oracle) wrote:
> On Thu, Dec 05, 2024 at 09:14:08AM +0100, Mateusz Polchlopek wrote:
> > On 12/3/2024 8:56 AM, Oleksij Rempel wrote:
> > > Add an optional polling interface for PHY statistics to simplify driver
> > > implementation. Drivers can request the PHYlib to handle the polling task by
> > > explicitly setting the `PHY_POLL_STATS` flag in their driver configuration.
> > > 
> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > ---
> > >   drivers/net/phy/phy.c | 15 +++++++++++++++
> > >   include/linux/phy.h   |  6 ++++++
> > >   2 files changed, 21 insertions(+)
> > > 
> > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > > index 0d20b534122b..b10ee9223fc9 100644
> > > --- a/drivers/net/phy/phy.c
> > > +++ b/drivers/net/phy/phy.c
> > > @@ -1346,6 +1346,18 @@ static int phy_enable_interrupts(struct phy_device *phydev)
> > >   	return phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED);
> > >   }
> > > +/**
> > > + * phy_update_stats - update the PHY statistics
> > > + * @phydev: target phy_device struct
> > > + */
> > 
> > As this is newly intoduced function I would love to see the full
> > kdoc header, with information what the function returns, like here:
> > 
> > https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
> 
> As it's an internal phylib function, I don't think there's any need for
> kernel-doc unless it's something more complex. It's obvious what the
> function itself is doing.
> 
> What would be more helpful is to properly document the "update_stats"
> method, since that is what PHY drivers are going to implement. Yes, I
> know kernel-doc isn't good at that, but look at phylink.h to see how
> to do it.

Ok, i'll send a preparation patch to make it consequently for all
callbacks in this struct.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH net-next v1 5/7] ethtool: add helper to prevent invalid statistics exposure to userspace

[Oleksij Rempel]

Introduce a new helper function, `ethtool_stat_add`, to update 64-bit
statistics with proper handling of the reserved value
`ETHTOOL_STAT_NOT_SET`. This ensures that statistics remain valid and
are always reported to userspace, even if the driver accidentally sets
`ETHTOOL_STAT_NOT_SET` during an update.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 include/linux/ethtool.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index b0ed740ca749..657bd69ddaf7 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -371,6 +371,22 @@ static inline void ethtool_stats_init(u64 *stats, unsigned int n)
 		stats[n] = ETHTOOL_STAT_NOT_SET;
 }
 
+/**
+ * ethtool_stat_add - Add a value to a u64 statistic with wraparound handling
+ * @stat: Pointer to the statistic to update
+ * @value: Value to add to the statistic
+ *
+ * Adds the specified value to a u64 statistic. If the result of the addition
+ * equals the reserved value (`ETHTOOL_STAT_NOT_SET`), it increments the result
+ * by 1 to avoid the reserved value.
+ */
+static inline void ethtool_stat_add(u64 *stat, u64 value)
+{
+	*stat += value;
+	if (*stat == ETHTOOL_STAT_NOT_SET)
+		(*stat)++;
+}
+
 /* Basic IEEE 802.3 MAC statistics (30.3.1.1.*), not otherwise exposed
  * via a more targeted API.
  */
-- 
2.39.5

Re: [PATCH net-next v1 5/7] ethtool: add helper to prevent invalid statistics exposure to userspace

[Mateusz Polchlopek]

On 12/3/2024 8:56 AM, Oleksij Rempel wrote:
> Introduce a new helper function, `ethtool_stat_add`, to update 64-bit
> statistics with proper handling of the reserved value
> `ETHTOOL_STAT_NOT_SET`. This ensures that statistics remain valid and
> are always reported to userspace, even if the driver accidentally sets
> `ETHTOOL_STAT_NOT_SET` during an update.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>   include/linux/ethtool.h | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index b0ed740ca749..657bd69ddaf7 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -371,6 +371,22 @@ static inline void ethtool_stats_init(u64 *stats, unsigned int n)
>   		stats[n] = ETHTOOL_STAT_NOT_SET;
>   }
>   
> +/**
> + * ethtool_stat_add - Add a value to a u64 statistic with wraparound handling
> + * @stat: Pointer to the statistic to update
> + * @value: Value to add to the statistic
> + *
> + * Adds the specified value to a u64 statistic. If the result of the addition
> + * equals the reserved value (`ETHTOOL_STAT_NOT_SET`), it increments the result
> + * by 1 to avoid the reserved value.
> + */
> +static inline void ethtool_stat_add(u64 *stat, u64 value)
> +{
> +	*stat += value;
> +	if (*stat == ETHTOOL_STAT_NOT_SET)
> +		(*stat)++;
> +}
> +
>   /* Basic IEEE 802.3 MAC statistics (30.3.1.1.*), not otherwise exposed
>    * via a more targeted API.
>    */

The code of function looks good

Reviewed-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>

[PATCH net-next v1 6/7] phy: dp83td510: add statistics support

[Oleksij Rempel]

Add support for reporting PHY statistics in the DP83TD510 driver. This
includes cumulative tracking of transmit/receive packet counts, and
error counts. Implemented functions to update and provide statistics via
ethtool, with optional polling support enabled through `PHY_POLL_STATS`.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/dp83td510.c | 98 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 97 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/dp83td510.c b/drivers/net/phy/dp83td510.c
index 92aa3a2b9744..08d61a6a8c61 100644
--- a/drivers/net/phy/dp83td510.c
+++ b/drivers/net/phy/dp83td510.c
@@ -34,6 +34,24 @@
 #define DP83TD510E_CTRL_HW_RESET		BIT(15)
 #define DP83TD510E_CTRL_SW_RESET		BIT(14)
 
+#define DP83TD510E_PKT_STAT_1			0x12b
+#define DP83TD510E_TX_PKT_CNT_15_0_MASK		GENMASK(15, 0)
+
+#define DP83TD510E_PKT_STAT_2			0x12c
+#define DP83TD510E_TX_PKT_CNT_31_16_MASK	GENMASK(15, 0)
+
+#define DP83TD510E_PKT_STAT_3			0x12d
+#define DP83TD510E_TX_ERR_PKT_CNT_MASK		GENMASK(15, 0)
+
+#define DP83TD510E_PKT_STAT_4			0x12e
+#define DP83TD510E_RX_PKT_CNT_15_0_MASK		GENMASK(15, 0)
+
+#define DP83TD510E_PKT_STAT_5			0x12f
+#define DP83TD510E_RX_PKT_CNT_31_16_MASK	GENMASK(15, 0)
+
+#define DP83TD510E_PKT_STAT_6			0x130
+#define DP83TD510E_RX_ERR_PKT_CNT_MASK		GENMASK(15, 0)
+
 #define DP83TD510E_AN_STAT_1			0x60c
 #define DP83TD510E_MASTER_SLAVE_RESOL_FAIL	BIT(15)
 
@@ -58,8 +76,16 @@ static const u16 dp83td510_mse_sqi_map[] = {
 	0x0000  /* 24dB =< SNR */
 };
 
+struct dp83td510_stats {
+	u64 tx_pkt_cnt;
+	u64 tx_err_pkt_cnt;
+	u64 rx_pkt_cnt;
+	u64 rx_err_pkt_cnt;
+};
+
 struct dp83td510_priv {
 	bool alcd_test_active;
+	struct dp83td510_stats stats;
 };
 
 /* Time Domain Reflectometry (TDR) Functionality of DP83TD510 PHY
@@ -177,6 +203,74 @@ struct dp83td510_priv {
 #define DP83TD510E_ALCD_COMPLETE			BIT(15)
 #define DP83TD510E_ALCD_CABLE_LENGTH			GENMASK(10, 0)
 
+/**
+ * dp83td510_update_stats - Update the PHY statistics for the DP83TD510 PHY.
+ * @phydev: Pointer to the phy_device structure.
+ *
+ * The function reads the PHY statistics registers and updates the statistics
+ * structure.
+ *
+ * Returns: 0 on success or a negative error code on failure.
+ */
+static int dp83td510_update_stats(struct phy_device *phydev)
+{
+	struct dp83td510_priv *priv = phydev->priv;
+	u64 count;
+	int ret;
+
+	/* DP83TD510E_PKT_STAT_1 to DP83TD510E_PKT_STAT_6 registers are cleared
+	 * after reading them in a sequence. A reading of this register not in
+	 * sequence will prevent them from being cleared.
+	 */
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_1);
+	if (ret < 0)
+		return ret;
+	count = FIELD_GET(DP83TD510E_TX_PKT_CNT_15_0_MASK, ret);
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_2);
+	if (ret < 0)
+		return ret;
+	count |= (u64)FIELD_GET(DP83TD510E_TX_PKT_CNT_31_16_MASK, ret) << 16;
+	ethtool_stat_add(&priv->stats.tx_pkt_cnt, count);
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_3);
+	if (ret < 0)
+		return ret;
+	count = FIELD_GET(DP83TD510E_TX_ERR_PKT_CNT_MASK, ret);
+	ethtool_stat_add(&priv->stats.tx_err_pkt_cnt, count);
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_4);
+	if (ret < 0)
+		return ret;
+	count = FIELD_GET(DP83TD510E_RX_PKT_CNT_15_0_MASK, ret);
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_5);
+	if (ret < 0)
+		return ret;
+	count |= (u64)FIELD_GET(DP83TD510E_RX_PKT_CNT_31_16_MASK, ret) << 16;
+	ethtool_stat_add(&priv->stats.rx_pkt_cnt, count);
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_6);
+	if (ret < 0)
+		return ret;
+	count = FIELD_GET(DP83TD510E_RX_ERR_PKT_CNT_MASK, ret);
+	ethtool_stat_add(&priv->stats.rx_err_pkt_cnt, count);
+
+	return 0;
+}
+
+static void dp83td510_get_phy_stats(struct phy_device *phydev,
+				    struct ethtool_eth_phy_stats *eth_stats,
+				    struct ethtool_phy_stats *stats)
+{
+	struct dp83td510_priv *priv = phydev->priv;
+
+	stats->tx_packets = priv->stats.tx_pkt_cnt;
+	stats->tx_errors = priv->stats.tx_err_pkt_cnt;
+	stats->rx_packets = priv->stats.rx_pkt_cnt;
+	stats->rx_errors = priv->stats.rx_err_pkt_cnt;
+}
+
 static int dp83td510_config_intr(struct phy_device *phydev)
 {
 	int ret;
@@ -588,7 +682,7 @@ static struct phy_driver dp83td510_driver[] = {
 	PHY_ID_MATCH_MODEL(DP83TD510E_PHY_ID),
 	.name		= "TI DP83TD510E",
 
-	.flags          = PHY_POLL_CABLE_TEST,
+	.flags          = PHY_POLL_CABLE_TEST | PHY_POLL_STATS,
 	.probe		= dp83td510_probe,
 	.config_aneg	= dp83td510_config_aneg,
 	.read_status	= dp83td510_read_status,
@@ -599,6 +693,8 @@ static struct phy_driver dp83td510_driver[] = {
 	.get_sqi_max	= dp83td510_get_sqi_max,
 	.cable_test_start = dp83td510_cable_test_start,
 	.cable_test_get_status = dp83td510_cable_test_get_status,
+	.get_phy_stats	= dp83td510_get_phy_stats,
+	.update_stats	= dp83td510_update_stats,
 
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
-- 
2.39.5

Re: [PATCH net-next v1 6/7] phy: dp83td510: add statistics support

[Mateusz Polchlopek]

On 12/3/2024 8:56 AM, Oleksij Rempel wrote:
> Add support for reporting PHY statistics in the DP83TD510 driver. This
> includes cumulative tracking of transmit/receive packet counts, and
> error counts. Implemented functions to update and provide statistics via
> ethtool, with optional polling support enabled through `PHY_POLL_STATS`.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>   drivers/net/phy/dp83td510.c | 98 ++++++++++++++++++++++++++++++++++++-
>   1 file changed, 97 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/dp83td510.c b/drivers/net/phy/dp83td510.c
> index 92aa3a2b9744..08d61a6a8c61 100644
> --- a/drivers/net/phy/dp83td510.c
> +++ b/drivers/net/phy/dp83td510.c
> @@ -34,6 +34,24 @@
>   #define DP83TD510E_CTRL_HW_RESET		BIT(15)
>   #define DP83TD510E_CTRL_SW_RESET		BIT(14)
>   
> +#define DP83TD510E_PKT_STAT_1			0x12b
> +#define DP83TD510E_TX_PKT_CNT_15_0_MASK		GENMASK(15, 0)
> +
> +#define DP83TD510E_PKT_STAT_2			0x12c
> +#define DP83TD510E_TX_PKT_CNT_31_16_MASK	GENMASK(15, 0)

Shouldn't it be GENMASK(31, 16) ? If not then I think that macro
name is a little bit misleading

> +
> +#define DP83TD510E_PKT_STAT_3			0x12d
> +#define DP83TD510E_TX_ERR_PKT_CNT_MASK		GENMASK(15, 0)
> +
> +#define DP83TD510E_PKT_STAT_4			0x12e
> +#define DP83TD510E_RX_PKT_CNT_15_0_MASK		GENMASK(15, 0)
> +
> +#define DP83TD510E_PKT_STAT_5			0x12f
> +#define DP83TD510E_RX_PKT_CNT_31_16_MASK	GENMASK(15, 0)

Same as above

> +
> +#define DP83TD510E_PKT_STAT_6			0x130
> +#define DP83TD510E_RX_ERR_PKT_CNT_MASK		GENMASK(15, 0)
> +
>   #define DP83TD510E_AN_STAT_1			0x60c
>   #define DP83TD510E_MASTER_SLAVE_RESOL_FAIL	BIT(15)
>   
> @@ -58,8 +76,16 @@ static const u16 dp83td510_mse_sqi_map[] = {
>   	0x0000  /* 24dB =< SNR */
>   };
>   
> +struct dp83td510_stats {
> +	u64 tx_pkt_cnt;
> +	u64 tx_err_pkt_cnt;
> +	u64 rx_pkt_cnt;
> +	u64 rx_err_pkt_cnt;
> +};
> +
>   struct dp83td510_priv {
>   	bool alcd_test_active;
> +	struct dp83td510_stats stats;
>   };
>   
>   /* Time Domain Reflectometry (TDR) Functionality of DP83TD510 PHY
> @@ -177,6 +203,74 @@ struct dp83td510_priv {
>   #define DP83TD510E_ALCD_COMPLETE			BIT(15)
>   #define DP83TD510E_ALCD_CABLE_LENGTH			GENMASK(10, 0)
>   
> +/**
> + * dp83td510_update_stats - Update the PHY statistics for the DP83TD510 PHY.
> + * @phydev: Pointer to the phy_device structure.
> + *
> + * The function reads the PHY statistics registers and updates the statistics
> + * structure.
> + *
> + * Returns: 0 on success or a negative error code on failure.

Typo, it should be 'Return:' not 'Returns:'

> + */
> +static int dp83td510_update_stats(struct phy_device *phydev)
> +{
> +	struct dp83td510_priv *priv = phydev->priv;
> +	u64 count;
> +	int ret;
> +
> +	/* DP83TD510E_PKT_STAT_1 to DP83TD510E_PKT_STAT_6 registers are cleared
> +	 * after reading them in a sequence. A reading of this register not in
> +	 * sequence will prevent them from being cleared.
> +	 */
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_1);
> +	if (ret < 0)
> +		return ret;
> +	count = FIELD_GET(DP83TD510E_TX_PKT_CNT_15_0_MASK, ret);
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_2);
> +	if (ret < 0)
> +		return ret;
> +	count |= (u64)FIELD_GET(DP83TD510E_TX_PKT_CNT_31_16_MASK, ret) << 16;

Ah... here you do shift. I think it would be better to just define

#define DP83TD510E_TX_PKT_CNT_31_16_MASK	GENMASK(31, 16)

instead of shifting, what do you think ?

> +	ethtool_stat_add(&priv->stats.tx_pkt_cnt, count);
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_3);
> +	if (ret < 0)
> +		return ret;
> +	count = FIELD_GET(DP83TD510E_TX_ERR_PKT_CNT_MASK, ret);
> +	ethtool_stat_add(&priv->stats.tx_err_pkt_cnt, count);
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_4);
> +	if (ret < 0)
> +		return ret;
> +	count = FIELD_GET(DP83TD510E_RX_PKT_CNT_15_0_MASK, ret);
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_5);
> +	if (ret < 0)
> +		return ret;
> +	count |= (u64)FIELD_GET(DP83TD510E_RX_PKT_CNT_31_16_MASK, ret) << 16;
> +	ethtool_stat_add(&priv->stats.rx_pkt_cnt, count);
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_6);
> +	if (ret < 0)
> +		return ret;
> +	count = FIELD_GET(DP83TD510E_RX_ERR_PKT_CNT_MASK, ret);
> +	ethtool_stat_add(&priv->stats.rx_err_pkt_cnt, count);
> +
> +	return 0;
> +}
> +
> +static void dp83td510_get_phy_stats(struct phy_device *phydev,
> +				    struct ethtool_eth_phy_stats *eth_stats,
> +				    struct ethtool_phy_stats *stats)
> +{
> +	struct dp83td510_priv *priv = phydev->priv;
> +
> +	stats->tx_packets = priv->stats.tx_pkt_cnt;
> +	stats->tx_errors = priv->stats.tx_err_pkt_cnt;
> +	stats->rx_packets = priv->stats.rx_pkt_cnt;
> +	stats->rx_errors = priv->stats.rx_err_pkt_cnt;
> +}
> +
>   static int dp83td510_config_intr(struct phy_device *phydev)
>   {
>   	int ret;
> @@ -588,7 +682,7 @@ static struct phy_driver dp83td510_driver[] = {
>   	PHY_ID_MATCH_MODEL(DP83TD510E_PHY_ID),
>   	.name		= "TI DP83TD510E",
>   
> -	.flags          = PHY_POLL_CABLE_TEST,
> +	.flags          = PHY_POLL_CABLE_TEST | PHY_POLL_STATS,
>   	.probe		= dp83td510_probe,
>   	.config_aneg	= dp83td510_config_aneg,
>   	.read_status	= dp83td510_read_status,
> @@ -599,6 +693,8 @@ static struct phy_driver dp83td510_driver[] = {
>   	.get_sqi_max	= dp83td510_get_sqi_max,
>   	.cable_test_start = dp83td510_cable_test_start,
>   	.cable_test_get_status = dp83td510_cable_test_get_status,
> +	.get_phy_stats	= dp83td510_get_phy_stats,
> +	.update_stats	= dp83td510_update_stats,
>   
>   	.suspend	= genphy_suspend,
>   	.resume		= genphy_resume,

Re: [PATCH net-next v1 6/7] phy: dp83td510: add statistics support

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3552 bytes --]

On 05.12.2024 09:43:34, Mateusz Polchlopek wrote:
> 
> 
> On 12/3/2024 8:56 AM, Oleksij Rempel wrote:
> > Add support for reporting PHY statistics in the DP83TD510 driver. This
> > includes cumulative tracking of transmit/receive packet counts, and
> > error counts. Implemented functions to update and provide statistics via
> > ethtool, with optional polling support enabled through `PHY_POLL_STATS`.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >   drivers/net/phy/dp83td510.c | 98 ++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 97 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/phy/dp83td510.c b/drivers/net/phy/dp83td510.c
> > index 92aa3a2b9744..08d61a6a8c61 100644
> > --- a/drivers/net/phy/dp83td510.c
> > +++ b/drivers/net/phy/dp83td510.c
> > @@ -34,6 +34,24 @@
> >   #define DP83TD510E_CTRL_HW_RESET		BIT(15)
> >   #define DP83TD510E_CTRL_SW_RESET		BIT(14)
> > +#define DP83TD510E_PKT_STAT_1			0x12b
> > +#define DP83TD510E_TX_PKT_CNT_15_0_MASK		GENMASK(15, 0)
> > +
> > +#define DP83TD510E_PKT_STAT_2			0x12c
> > +#define DP83TD510E_TX_PKT_CNT_31_16_MASK	GENMASK(15, 0)
> 
> Shouldn't it be GENMASK(31, 16) ? If not then I think that macro
> name is a little bit misleading

Yes, the name may be a bit misleading...

[...]

> > + */
> > +static int dp83td510_update_stats(struct phy_device *phydev)
> > +{
> > +	struct dp83td510_priv *priv = phydev->priv;
> > +	u64 count;
> > +	int ret;
> > +
> > +	/* DP83TD510E_PKT_STAT_1 to DP83TD510E_PKT_STAT_6 registers are cleared
> > +	 * after reading them in a sequence. A reading of this register not in
> > +	 * sequence will prevent them from being cleared.
> > +	 */
> > +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_1);
> > +	if (ret < 0)
> > +		return ret;
> > +	count = FIELD_GET(DP83TD510E_TX_PKT_CNT_15_0_MASK, ret);
> > +
> > +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_2);
> > +	if (ret < 0)
> > +		return ret;
> > +	count |= (u64)FIELD_GET(DP83TD510E_TX_PKT_CNT_31_16_MASK, ret) << 16;
> 
> Ah... here you do shift. I think it would be better to just define
> 
> #define DP83TD510E_TX_PKT_CNT_31_16_MASK	GENMASK(31, 16)

No. This would not be the same.

The current code takes the lower 16 bit of "ret" and shifts it left 16
bits.

As far as I understand the code DP83TD510E_PKT_STAT_1 contain the lower
16 bits, while DP83TD510E_PKT_STAT_2 contain the upper 16 bits.

DP83TD510E_PKT_STAT_1 gives 0x????aaaa
DP83TD510E_PKT_STAT_2 gives 0x????bbbb

count will be 0xbbbbaaaa

This raises another question: Are these values latched?

If not you can get funny results if DP83TD510E_PKT_STAT_1 rolls over. On
unlatched MMIO busses you first read the upper part, then the lower,
then the upper again and loop if the value of the upper part changed in
between. Not sure how much overhead this means for the slow busses.

Consult the doc of the chip if you can read both in one go and if the
chip latches these values for that access mode.

> instead of shifting, what do you think ?

nope - If you don't want to shift, you can use a combination of
FIELD_GET() (to extract the relevant 16 bits) and FIELD_PREP() to shift.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net-next v1 6/7] phy: dp83td510: add statistics support

[Mateusz Polchlopek]

On 12/5/2024 10:01 AM, Marc Kleine-Budde wrote:
> On 05.12.2024 09:43:34, Mateusz Polchlopek wrote:
>>
>>
>> On 12/3/2024 8:56 AM, Oleksij Rempel wrote:
>>> Add support for reporting PHY statistics in the DP83TD510 driver. This
>>> includes cumulative tracking of transmit/receive packet counts, and
>>> error counts. Implemented functions to update and provide statistics via
>>> ethtool, with optional polling support enabled through `PHY_POLL_STATS`.
>>>
>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>>> ---
>>>    drivers/net/phy/dp83td510.c | 98 ++++++++++++++++++++++++++++++++++++-
>>>    1 file changed, 97 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/phy/dp83td510.c b/drivers/net/phy/dp83td510.c
>>> index 92aa3a2b9744..08d61a6a8c61 100644
>>> --- a/drivers/net/phy/dp83td510.c
>>> +++ b/drivers/net/phy/dp83td510.c
>>> @@ -34,6 +34,24 @@
>>>    #define DP83TD510E_CTRL_HW_RESET		BIT(15)
>>>    #define DP83TD510E_CTRL_SW_RESET		BIT(14)
>>> +#define DP83TD510E_PKT_STAT_1			0x12b
>>> +#define DP83TD510E_TX_PKT_CNT_15_0_MASK		GENMASK(15, 0)
>>> +
>>> +#define DP83TD510E_PKT_STAT_2			0x12c
>>> +#define DP83TD510E_TX_PKT_CNT_31_16_MASK	GENMASK(15, 0)
>>
>> Shouldn't it be GENMASK(31, 16) ? If not then I think that macro
>> name is a little bit misleading
> 
> Yes, the name may be a bit misleading...
> 
> [...]
> 
>>> + */
>>> +static int dp83td510_update_stats(struct phy_device *phydev)
>>> +{
>>> +	struct dp83td510_priv *priv = phydev->priv;
>>> +	u64 count;
>>> +	int ret;
>>> +
>>> +	/* DP83TD510E_PKT_STAT_1 to DP83TD510E_PKT_STAT_6 registers are cleared
>>> +	 * after reading them in a sequence. A reading of this register not in
>>> +	 * sequence will prevent them from being cleared.
>>> +	 */
>>> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_1);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +	count = FIELD_GET(DP83TD510E_TX_PKT_CNT_15_0_MASK, ret);
>>> +
>>> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_2);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +	count |= (u64)FIELD_GET(DP83TD510E_TX_PKT_CNT_31_16_MASK, ret) << 16;
>>
>> Ah... here you do shift. I think it would be better to just define
>>
>> #define DP83TD510E_TX_PKT_CNT_31_16_MASK	GENMASK(31, 16)
> 
> No. This would not be the same.
> 
> The current code takes the lower 16 bit of "ret" and shifts it left 16
> bits.
> 
> As far as I understand the code DP83TD510E_PKT_STAT_1 contain the lower
> 16 bits, while DP83TD510E_PKT_STAT_2 contain the upper 16 bits.
> 
> DP83TD510E_PKT_STAT_1 gives 0x????aaaa
> DP83TD510E_PKT_STAT_2 gives 0x????bbbb
> 
> count will be 0xbbbbaaaa
> 
> This raises another question: Are these values latched?
> 
> If not you can get funny results if DP83TD510E_PKT_STAT_1 rolls over. On
> unlatched MMIO busses you first read the upper part, then the lower,
> then the upper again and loop if the value of the upper part changed in
> between. Not sure how much overhead this means for the slow busses.
> 
> Consult the doc of the chip if you can read both in one go and if the
> chip latches these values for that access mode.
> 
>> instead of shifting, what do you think ?
> 
> nope - If you don't want to shift, you can use a combination of
> FIELD_GET() (to extract the relevant 16 bits) and FIELD_PREP() to shift.
> 
> regards,
> Marc
> 

Okay, thanks Marc for an explanation! Now I understand it better

Re: [PATCH net-next v1 6/7] phy: dp83td510: add statistics support

[Oleksij Rempel]

On Thu, Dec 05, 2024 at 10:01:10AM +0100, Marc Kleine-Budde wrote:
> On 05.12.2024 09:43:34, Mateusz Polchlopek wrote:
> > 
> > 
> > On 12/3/2024 8:56 AM, Oleksij Rempel wrote:
> > > Add support for reporting PHY statistics in the DP83TD510 driver. This
> > > includes cumulative tracking of transmit/receive packet counts, and
> > > error counts. Implemented functions to update and provide statistics via
> > > ethtool, with optional polling support enabled through `PHY_POLL_STATS`.
> > > 
> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > ---
> > >   drivers/net/phy/dp83td510.c | 98 ++++++++++++++++++++++++++++++++++++-
> > >   1 file changed, 97 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/phy/dp83td510.c b/drivers/net/phy/dp83td510.c
> > > index 92aa3a2b9744..08d61a6a8c61 100644
> > > --- a/drivers/net/phy/dp83td510.c
> > > +++ b/drivers/net/phy/dp83td510.c
> > > @@ -34,6 +34,24 @@
> > >   #define DP83TD510E_CTRL_HW_RESET		BIT(15)
> > >   #define DP83TD510E_CTRL_SW_RESET		BIT(14)
> > > +#define DP83TD510E_PKT_STAT_1			0x12b
> > > +#define DP83TD510E_TX_PKT_CNT_15_0_MASK		GENMASK(15, 0)
> > > +
> > > +#define DP83TD510E_PKT_STAT_2			0x12c
> > > +#define DP83TD510E_TX_PKT_CNT_31_16_MASK	GENMASK(15, 0)
> > 
> > Shouldn't it be GENMASK(31, 16) ? If not then I think that macro
> > name is a little bit misleading
> 
> Yes, the name may be a bit misleading...

The naming is done according to the chip datasheet. This is preferable
way to name defines.

> [...]
> 
> > > + */
> > > +static int dp83td510_update_stats(struct phy_device *phydev)
> > > +{
> > > +	struct dp83td510_priv *priv = phydev->priv;
> > > +	u64 count;
> > > +	int ret;
> > > +
> > > +	/* DP83TD510E_PKT_STAT_1 to DP83TD510E_PKT_STAT_6 registers are cleared
> > > +	 * after reading them in a sequence. A reading of this register not in
> > > +	 * sequence will prevent them from being cleared.
> > > +	 */

this comment is relevant for the following question by Marc.

> > > +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_1);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +	count = FIELD_GET(DP83TD510E_TX_PKT_CNT_15_0_MASK, ret);
> > > +
> > > +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_2);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +	count |= (u64)FIELD_GET(DP83TD510E_TX_PKT_CNT_31_16_MASK, ret) << 16;
> > 
> > Ah... here you do shift. I think it would be better to just define
> > 
> > #define DP83TD510E_TX_PKT_CNT_31_16_MASK	GENMASK(31, 16)
> 
> No. This would not be the same.
> 
> The current code takes the lower 16 bit of "ret" and shifts it left 16
> bits.
> 
> As far as I understand the code DP83TD510E_PKT_STAT_1 contain the lower
> 16 bits, while DP83TD510E_PKT_STAT_2 contain the upper 16 bits.
> 
> DP83TD510E_PKT_STAT_1 gives 0x????aaaa
> DP83TD510E_PKT_STAT_2 gives 0x????bbbb
> 
> count will be 0xbbbbaaaa
> 
> This raises another question: Are these values latched?
> 
> If not you can get funny results if DP83TD510E_PKT_STAT_1 rolls over. On
> unlatched MMIO busses you first read the upper part, then the lower,
> then the upper again and loop if the value of the upper part changed in
> between. Not sure how much overhead this means for the slow busses.
> 
> Consult the doc of the chip if you can read both in one go and if the
> chip latches these values for that access mode.

It is not documented, what is documented is that PKT_STAT_1 to
PKT_STAT_3 should be read in sequence to trigger auto clear function of
this registers. If chip do not latches these values, we will have
additional problem - some counts will be lost in the PKT_STAT_1/2 till we with
PKT_STAT_3 will be done.

With other words, I'll do more testing and add corresponding comments in
the code..
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v1 6/7] phy: dp83td510: add statistics support

[Russell King (Oracle)]

On Tue, Dec 03, 2024 at 08:56:20AM +0100, Oleksij Rempel wrote:
> Add support for reporting PHY statistics in the DP83TD510 driver. This
> includes cumulative tracking of transmit/receive packet counts, and
> error counts. Implemented functions to update and provide statistics via
> ethtool, with optional polling support enabled through `PHY_POLL_STATS`.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/phy/dp83td510.c | 98 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 97 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/dp83td510.c b/drivers/net/phy/dp83td510.c
> index 92aa3a2b9744..08d61a6a8c61 100644
> --- a/drivers/net/phy/dp83td510.c
> +++ b/drivers/net/phy/dp83td510.c
> @@ -34,6 +34,24 @@
>  #define DP83TD510E_CTRL_HW_RESET		BIT(15)
>  #define DP83TD510E_CTRL_SW_RESET		BIT(14)
>  
> +#define DP83TD510E_PKT_STAT_1			0x12b
> +#define DP83TD510E_TX_PKT_CNT_15_0_MASK		GENMASK(15, 0)
> +
> +#define DP83TD510E_PKT_STAT_2			0x12c
> +#define DP83TD510E_TX_PKT_CNT_31_16_MASK	GENMASK(15, 0)
> +
> +#define DP83TD510E_PKT_STAT_3			0x12d
> +#define DP83TD510E_TX_ERR_PKT_CNT_MASK		GENMASK(15, 0)
> +
> +#define DP83TD510E_PKT_STAT_4			0x12e
> +#define DP83TD510E_RX_PKT_CNT_15_0_MASK		GENMASK(15, 0)
> +
> +#define DP83TD510E_PKT_STAT_5			0x12f
> +#define DP83TD510E_RX_PKT_CNT_31_16_MASK	GENMASK(15, 0)
> +
> +#define DP83TD510E_PKT_STAT_6			0x130
> +#define DP83TD510E_RX_ERR_PKT_CNT_MASK		GENMASK(15, 0)

I'm not sure I like this pattern of _MASK here. Why not call these
registers e.g. DP83TD510E_RX_PKT_CNT_31_16 ? Given that the full
register value is used, I don't see the need for _MASK and the
FIELD_GET()s, which just add extra complexity to the code and
reduce readability.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH net-next v1 7/7] phy: dp83tg720: add statistics support

[Oleksij Rempel]

Add support for reporting PHY statistics in the DP83TG720 driver. This includes
cumulative tracking of link loss events, transmit/receive packet counts, and
error counts. Implemented functions to update and provide statistics via
ethtool, with optional polling support enabled through `PHY_POLL_STATS`.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/dp83tg720.c | 147 +++++++++++++++++++++++++++++++++++-
 1 file changed, 146 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/dp83tg720.c b/drivers/net/phy/dp83tg720.c
index 0ef4d7dba065..f56659d41b31 100644
--- a/drivers/net/phy/dp83tg720.c
+++ b/drivers/net/phy/dp83tg720.c
@@ -51,6 +51,9 @@
 /* Register 0x0405: Unknown Register */
 #define DP83TG720S_UNKNOWN_0405			0x405
 
+#define DP83TG720S_LINK_QUAL_3			0x547
+#define DP83TG720S_LINK_LOSS_CNT_MASK		GENMASK(15, 10)
+
 /* Register 0x0576: TDR Master Link Down Control */
 #define DP83TG720S_TDR_MASTER_LINK_DOWN		0x576
 
@@ -60,6 +63,24 @@
 /* In RGMII mode, Enable or disable the internal delay for TXD */
 #define DP83TG720S_RGMII_TX_CLK_SEL		BIT(0)
 
+#define DP83TG720S_PKT_STAT_1			0x639
+#define DP83TG720S_TX_PKT_CNT_15_0_MASK		GENMASK(15, 0)
+
+#define DP83TG720S_PKT_STAT_2			0x63a
+#define DP83TG720S_TX_PKT_CNT_31_16_MASK	GENMASK(15, 0)
+
+#define DP83TG720S_PKT_STAT_3			0x63b
+#define DP83TG720S_TX_ERR_PKT_CNT_MASK		GENMASK(15, 0)
+
+#define DP83TG720S_PKT_STAT_4			0x63c
+#define DP83TG720S_RX_PKT_CNT_15_0_MASK		GENMASK(15, 0)
+
+#define DP83TG720S_PKT_STAT_5			0x63d
+#define DP83TG720S_RX_PKT_CNT_31_16_MASK	GENMASK(15, 0)
+
+#define DP83TG720S_PKT_STAT_6			0x63e
+#define DP83TG720S_RX_ERR_PKT_CNT_MASK		GENMASK(15, 0)
+
 /* Register 0x083F: Unknown Register */
 #define DP83TG720S_UNKNOWN_083F			0x83f
 
@@ -69,6 +90,102 @@
 
 #define DP83TG720_SQI_MAX			7
 
+struct dp83tg720_stats {
+	u64 link_loss_cnt;
+	u64 tx_pkt_cnt;
+	u64 tx_err_pkt_cnt;
+	u64 rx_pkt_cnt;
+	u64 rx_err_pkt_cnt;
+};
+
+struct dp83tg720_priv {
+	struct dp83tg720_stats stats;
+};
+
+/**
+ * dp83tg720_update_stats - Update the PHY statistics for the DP83TD510 PHY.
+ * @phydev: Pointer to the phy_device structure.
+ *
+ * The function reads the PHY statistics registers and updates the statistics
+ * structure.
+ *
+ * Returns: 0 on success or a negative error code on failure.
+ */
+static int dp83tg720_update_stats(struct phy_device *phydev)
+{
+	struct dp83tg720_priv *priv = phydev->priv;
+	u64 count;
+	int ret;
+
+	/* Read the link loss count */
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TG720S_LINK_QUAL_3);
+	if (ret < 0)
+		return ret;
+	count = FIELD_GET(DP83TG720S_LINK_LOSS_CNT_MASK, ret);
+	ethtool_stat_add(&priv->stats.link_loss_cnt, count);
+
+	/* Read frame statistics */
+	/* DP83TG720S_PKT_STAT_1 to DP83TG720S_PKT_STAT_6 registers are cleared
+	 * after reading them in a sequence. A reading of this register not in
+	 * sequence will prevent them from being cleared.
+	 */
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TG720S_PKT_STAT_1);
+	if (ret < 0)
+		return ret;
+	count = FIELD_GET(DP83TG720S_TX_PKT_CNT_15_0_MASK, ret);
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TG720S_PKT_STAT_2);
+	if (ret < 0)
+		return ret;
+	count |= (u64)FIELD_GET(DP83TG720S_TX_PKT_CNT_31_16_MASK, ret) << 16;
+	ethtool_stat_add(&priv->stats.tx_pkt_cnt, count);
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TG720S_PKT_STAT_3);
+	if (ret < 0)
+		return ret;
+	count = FIELD_GET(DP83TG720S_TX_ERR_PKT_CNT_MASK, ret);
+	ethtool_stat_add(&priv->stats.tx_err_pkt_cnt, count);
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TG720S_PKT_STAT_4);
+	if (ret < 0)
+		return ret;
+	count = FIELD_GET(DP83TG720S_RX_PKT_CNT_15_0_MASK, ret);
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TG720S_PKT_STAT_5);
+	if (ret < 0)
+		return ret;
+	count |= (u64)FIELD_GET(DP83TG720S_RX_PKT_CNT_31_16_MASK, ret) << 16;
+	ethtool_stat_add(&priv->stats.rx_pkt_cnt, count);
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TG720S_PKT_STAT_6);
+	if (ret < 0)
+		return ret;
+	count = FIELD_GET(DP83TG720S_RX_ERR_PKT_CNT_MASK, ret);
+	ethtool_stat_add(&priv->stats.rx_err_pkt_cnt, count);
+
+	return 0;
+}
+
+static void dp83tg720_get_link_stats(struct phy_device *phydev,
+				     struct ethtool_link_ext_stats *link_stats)
+{
+	struct dp83tg720_priv *priv = phydev->priv;
+
+	link_stats->link_down_events = priv->stats.link_loss_cnt;
+}
+
+static void dp83tg720_get_phy_stats(struct phy_device *phydev,
+				    struct ethtool_eth_phy_stats *eth_stats,
+				    struct ethtool_phy_stats *stats)
+{
+	struct dp83tg720_priv *priv = phydev->priv;
+
+	stats->tx_packets = priv->stats.tx_pkt_cnt;
+	stats->tx_errors = priv->stats.tx_err_pkt_cnt;
+	stats->rx_packets = priv->stats.rx_pkt_cnt;
+	stats->rx_errors = priv->stats.rx_err_pkt_cnt;
+}
+
 /**
  * dp83tg720_cable_test_start - Start the cable test for the DP83TG720 PHY.
  * @phydev: Pointer to the phy_device structure.
@@ -182,6 +299,11 @@ static int dp83tg720_cable_test_get_status(struct phy_device *phydev,
 
 	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A, stat);
 
+	/* save the current stats before resetting the PHY */
+	ret = dp83tg720_update_stats(phydev);
+	if (ret)
+		return ret;
+
 	return phy_init_hw(phydev);
 }
 
@@ -217,6 +339,11 @@ static int dp83tg720_read_status(struct phy_device *phydev)
 	phy_sts = phy_read(phydev, DP83TG720S_MII_REG_10);
 	phydev->link = !!(phy_sts & DP83TG720S_LINK_STATUS);
 	if (!phydev->link) {
+		/* save the current stats before resetting the PHY */
+		ret = dp83tg720_update_stats(phydev);
+		if (ret)
+			return ret;
+
 		/* According to the "DP83TC81x, DP83TG72x Software
 		 * Implementation Guide", the PHY needs to be reset after a
 		 * link loss or if no link is created after at least 100ms.
@@ -341,12 +468,27 @@ static int dp83tg720_config_init(struct phy_device *phydev)
 	return genphy_c45_pma_baset1_read_master_slave(phydev);
 }
 
+static int dp83tg720_probe(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct dp83tg720_priv *priv;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	phydev->priv = priv;
+
+	return 0;
+}
+
 static struct phy_driver dp83tg720_driver[] = {
 {
 	PHY_ID_MATCH_MODEL(DP83TG720S_PHY_ID),
 	.name		= "TI DP83TG720S",
 
-	.flags          = PHY_POLL_CABLE_TEST,
+	.flags          = PHY_POLL_CABLE_TEST | PHY_POLL_STATS,
+	.probe		= dp83tg720_probe,
 	.config_aneg	= dp83tg720_config_aneg,
 	.read_status	= dp83tg720_read_status,
 	.get_features	= genphy_c45_pma_read_ext_abilities,
@@ -355,6 +497,9 @@ static struct phy_driver dp83tg720_driver[] = {
 	.get_sqi_max	= dp83tg720_get_sqi_max,
 	.cable_test_start = dp83tg720_cable_test_start,
 	.cable_test_get_status = dp83tg720_cable_test_get_status,
+	.get_link_stats	= dp83tg720_get_link_stats,
+	.get_phy_stats	= dp83tg720_get_phy_stats,
+	.update_stats	= dp83tg720_update_stats,
 
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
-- 
2.39.5

Re: [PATCH net-next v1 0/7] Introduce unified and structured PHY

[Russell King (Oracle)]

Hi Oleksij,

As this comment applies to several of your patches, I'm replying to the
series.

Please use the prefix "net: phy: " for phylib patches to disambiguate
them from phy (drivers/phy) patches in the kernel change log.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!