| 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
| ||
|
Message-ID: <57a7b3bf-02fa-4b18-bb4b-b11245d3ebfb@intel.com>
Date: Thu, 5 Dec 2024 09:43:34 +0100
From: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
To: 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>
CC: <kernel@...gutronix.de>, <linux-kernel@...r.kernel.org>,
<netdev@...r.kernel.org>, Simon Horman <horms@...nel.org>, Russell King
<linux@...linux.org.uk>, Maxime Chevallier <maxime.chevallier@...tlin.com>,
<linux-doc@...r.kernel.org>
Subject: Re: [PATCH net-next v1 6/7] phy: dp83td510: add statistics support
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
> +
> +#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,
Powered by blists - more mailing lists