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: <20241205-satisfied-gerbil-of-cookies-471293-mkl@pengutronix.de>
Date: Thu, 5 Dec 2024 10:01:10 +0100
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
Cc: Oleksij Rempel <o.rempel@...gutronix.de>, 
	"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 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...

[...]

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)