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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <Z1GHczyyzDFkV592@pengutronix.de>
Date: Thu, 5 Dec 2024 11:58:59 +0100
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: Marc Kleine-Budde <mkl@...gutronix.de>
Cc: Mateusz Polchlopek <mateusz.polchlopek@...el.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Jonathan Corbet <corbet@....net>, kernel@...gutronix.de,
	linux-doc@...r.kernel.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	Simon Horman <horms@...nel.org>,
	Russell King <linux@...linux.org.uk>
Subject: Re: [PATCH net-next v1 6/7] phy: dp83td510: add statistics support

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@...gutronix.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 |