linux-kernel - Re: [PATCH 1/1] ethtool : added get_phy_stats,get_strings,get_sset

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5153db09-092f-55b1-a8fc-aa23433c52a0@gmail.com>
Date:   Mon, 27 Mar 2017 09:44:37 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Thomas Scariah <thomasscariah@...il.com>
Cc:     nsekhar@...com, grygorii.strashko@...com, davem@...emloft.net,
        drivshin@...worx.com, mugunthanvnm@...com,
        ivan.khoronzhuk@...aro.org, thomas.scariah@...man.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH 1/1] ethtool : added
 get_phy_stats,get_strings,get_sset_count

Hello,

On 03/27/2017 03:12 AM, Thomas Scariah wrote:
> From: "Scariah, Thomas" <thomas.scariah@...man.com>
> 
>  Added functions to support ethtool to print the phy statistics and error
>  information along with other ethtool statistics. This will help ethtool
>  information to know the error is from physical layer or MAC layer.
>  This is an enahancement for ethtool to accommodate phy statistics

It sounds like your patch should actually be 3 different patches:

- add helper function to the core PHY library
- add statistics support to the Micrel PHY driver
- hook the proper ndo operations in cpsw to allow querying the PHY
driver's statistics

> 
> Signed-off-by: Thomas Scariah <thomasscariah@...il.com>
> ---
>  drivers/net/ethernet/ti/cpsw.c |   16 ++++++++++++-
>  drivers/net/phy/micrel.c       |   50 ++++++++++++++++++++++++++++++++++++++++
>  drivers/net/phy/phy.c          |   23 ++++++++++++++++++
>  include/linux/phy.h            |   14 +++++++++++
>  4 files changed, 102 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 3f48bb9..26d2dc5 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -1001,9 +1001,16 @@ update_return:
>  
>  static int cpsw_get_sset_count(struct net_device *ndev, int sset)
>  {
> +	struct cpsw_priv *priv = netdev_priv(ndev);
> +	int slave_no = cpsw_slave_index(priv);
> +	int count;
> +
>  	switch (sset) {
>  	case ETH_SS_STATS:
> -		return CPSW_STATS_LEN;
> +		count = CPSW_STATS_LEN;
> +		count += phy_ethtool_get_sset_count(priv->slaves[slave_no].phy,
> +						    sset);
> +		return count;
>  	default:
>  		return -EOPNOTSUPP;
>  	}

We already have a way to obtain PHY specific statistics through the
ETH_SS_PHY_STATS string set, cannot we use that here too? It certainly
makes it easier to overlay cpsw statistics with PHY device statistics,
but when Andrew added support for that, AFAIR this was made
intentionally separate.

> @@ -1011,6 +1018,8 @@ static int cpsw_get_sset_count(struct net_device *ndev, int sset)
>  
>  static void cpsw_get_strings(struct net_device *ndev, u32 stringset, u8 *data)
>  {
> +	struct cpsw_priv *priv = netdev_priv(ndev);
> +	int slave_no = cpsw_slave_index(priv);
>  	u8 *p = data;
>  	int i;
>  
> @@ -1021,6 +1030,8 @@ static void cpsw_get_strings(struct net_device *ndev, u32 stringset, u8 *data)
>  			       ETH_GSTRING_LEN);
>  			p += ETH_GSTRING_LEN;
>  		}
> +		phy_ethtool_get_strings(priv->slaves[slave_no].phy,
> +					stringset, p);

Same here.

>  		break;
>  	}
>  }
> @@ -1031,6 +1042,7 @@ static void cpsw_get_ethtool_stats(struct net_device *ndev,
>  	struct cpsw_priv *priv = netdev_priv(ndev);
>  	struct cpdma_chan_stats rx_stats;
>  	struct cpdma_chan_stats tx_stats;
> +	int slave_no = cpsw_slave_index(priv);
>  	u32 val;
>  	u8 *p;
>  	int i, ret;
> @@ -1067,6 +1079,8 @@ static void cpsw_get_ethtool_stats(struct net_device *ndev,
>  		}
>  	}
>  
> +	phy_ethtool_get_stats(priv->slaves[slave_no].phy, stats,
> +			      &data[CPSW_STATS_LEN]);

And here.

>  	pm_runtime_put(&priv->pdev->dev);
>  }
>  


-- 
Florian