Re: [PATCH 15/16] net: phy: adin: add ethtool get_stats support
On Tue, 2019-08-06 at 17:46 +0200, Andrew Lunn wrote:
> [External]
>
> On Tue, Aug 06, 2019 at 07:11:57AM +, Ardelean, Alexandru wrote:
> > On Mon, 2019-08-05 at 17:28 +0200, Andrew Lunn wrote:
> > > [External]
> > >
> > > > +struct adin_hw_stat {
> > > > + const char *string;
> > > > +static void adin_get_strings(struct phy_device *phydev, u8 *data)
> > > > +{
> > > > + int i;
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(adin_hw_stats); i++) {
> > > > + memcpy(data + i * ETH_GSTRING_LEN,
> > > > + adin_hw_stats[i].string, ETH_GSTRING_LEN);
> > >
> > > You define string as a char *. So it will be only as long as it should
> > > be. However memcpy always copies ETH_GSTRING_LEN bytes, doing off the
> > > end of the string and into whatever follows.
> > >
> >
> > hmm, will use strlcpy()
> > i blindedly copied memcpy() from some other driver
>
> Hopefully that driver used const char string[ETH_GSTRING_LEN]. Then a
> memcpy is safe. If not, please let me know what driver you copied.
It was an older Marvell PHY driver (marvell.c) ; in version 4.14.
I used that as an initial work-base for writing the driver.
Then I did the conversion to a newer kernel, then I also had to also consider
an older kernel, then I got confused :)
Well, in any case, I am solely considering net-next master (now) for
upstreaming this.
>
> > i'm afraid i don't understand about the snapshot feature you are mentioning;
> > i.e. i don't remember seeing it in other chips;
>
> It is frequency done at the MAC layer for statistics. You tell the
> hardware to snapshot all the statistics. It atomically makes a copy of
> all the statistics into a set of registers. These values are then
> static, and consistent between counters. You can read them out knowing
> they are not going to change.
>
> > regarding the danger that stat->reg1 rolls over, i guess that is
> > possible, but it's a bit hard to guard against;
>
> The normal solution is the read the MSB, the LSB and then the MSB
> again. If the MSB value has changed between the two reads, you know a
> roll over has happened, and you need to do it all again.
hmm; ok
I'll try to look for an existing example for this.
>
> Andrew
Re: [PATCH 15/16] net: phy: adin: add ethtool get_stats support
On Tue, Aug 06, 2019 at 07:11:57AM +, Ardelean, Alexandru wrote:
> On Mon, 2019-08-05 at 17:28 +0200, Andrew Lunn wrote:
> > [External]
> >
> > > +struct adin_hw_stat {
> > > + const char *string;
> > > +static void adin_get_strings(struct phy_device *phydev, u8 *data)
> > > +{
> > > + int i;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(adin_hw_stats); i++) {
> > > + memcpy(data + i * ETH_GSTRING_LEN,
> > > +adin_hw_stats[i].string, ETH_GSTRING_LEN);
> >
> > You define string as a char *. So it will be only as long as it should
> > be. However memcpy always copies ETH_GSTRING_LEN bytes, doing off the
> > end of the string and into whatever follows.
> >
>
> hmm, will use strlcpy()
> i blindedly copied memcpy() from some other driver
Hopefully that driver used const char string[ETH_GSTRING_LEN]. Then a
memcpy is safe. If not, please let me know what driver you copied.
> i'm afraid i don't understand about the snapshot feature you are mentioning;
> i.e. i don't remember seeing it in other chips;
It is frequency done at the MAC layer for statistics. You tell the
hardware to snapshot all the statistics. It atomically makes a copy of
all the statistics into a set of registers. These values are then
static, and consistent between counters. You can read them out knowing
they are not going to change.
> regarding the danger that stat->reg1 rolls over, i guess that is
> possible, but it's a bit hard to guard against;
The normal solution is the read the MSB, the LSB and then the MSB
again. If the MSB value has changed between the two reads, you know a
roll over has happened, and you need to do it all again.
Andrew
Re: [PATCH 15/16] net: phy: adin: add ethtool get_stats support
On Mon, 2019-08-05 at 17:30 +0200, Andrew Lunn wrote:
> [External]
>
> On Mon, Aug 05, 2019 at 07:54:52PM +0300, Alexandru Ardelean wrote:
> > This change implements retrieving all the error counters from the PHY.
> > The PHY supports several error counters/stats. The `Mean Square Errors`
> > status values are only valie when a link is established, and shouldn't be
> > incremented. These values characterize the quality of a signal.
>
> I think you mean accumulated, not incremented?
accumulated sounds better;
> > The rest of the error counters are self-clearing on read.
> > Most of them are reports from the Frame Checker engine that the PHY has.
> >
> > Not retrieving the `LPI Wake Error Count Register` here, since that is used
> > by the PHY framework to check for any EEE errors. And that register is
> > self-clearing when read (as per IEEE spec).
> >
> > Signed-off-by: Alexandru Ardelean
> > ---
> > drivers/net/phy/adin.c | 108 +
> > 1 file changed, 108 insertions(+)
> >
> > diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> > index a1f3456a8504..04896547dac8 100644
> > --- a/drivers/net/phy/adin.c
> > +++ b/drivers/net/phy/adin.c
> > @@ -103,6 +103,32 @@ static struct clause22_mmd_map clause22_mmd_map[] = {
> > { MDIO_MMD_PCS, MDIO_PCS_EEE_WK_ERR,ADIN1300_LPI_WAKE_ERR_CNT_REG },
> > };
> >
> > +struct adin_hw_stat {
> > + const char *string;
> > + u16 reg1;
> > + u16 reg2;
> > + bool do_not_inc;
>
> do_not_accumulate? or reverse its meaning, clear_on_read?
do_not_accumulate works;
there are only 4 regs that need this property set to true
>
>Andrew
Re: [PATCH 15/16] net: phy: adin: add ethtool get_stats support
On Mon, 2019-08-05 at 17:28 +0200, Andrew Lunn wrote:
> [External]
>
> > +struct adin_hw_stat {
> > + const char *string;
> > +static void adin_get_strings(struct phy_device *phydev, u8 *data)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(adin_hw_stats); i++) {
> > + memcpy(data + i * ETH_GSTRING_LEN,
> > + adin_hw_stats[i].string, ETH_GSTRING_LEN);
>
> You define string as a char *. So it will be only as long as it should
> be. However memcpy always copies ETH_GSTRING_LEN bytes, doing off the
> end of the string and into whatever follows.
>
hmm, will use strlcpy()
i blindedly copied memcpy() from some other driver
>
> > + }
> > +}
> > +
> > +static int adin_read_mmd_stat_regs(struct phy_device *phydev,
> > + struct adin_hw_stat *stat,
> > + u32 *val)
> > +{
> > + int ret;
> > +
> > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg1);
> > + if (ret < 0)
> > + return ret;
> > +
> > + *val = (ret & 0x);
> > +
> > + if (stat->reg2 == 0)
> > + return 0;
> > +
> > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2);
> > + if (ret < 0)
> > + return ret;
> > +
> > + *val <<= 16;
> > + *val |= (ret & 0x);
>
> Does the hardware have a snapshot feature? Is there a danger that
> between the two reads stat->reg1 rolls over and you end up with too
> big a value?
i'm afraid i don't understand about the snapshot feature you are mentioning;
i.e. i don't remember seeing it in other chips;
regarding the danger that stat->reg1 rolls over, i guess that is possible, but
it's a bit hard to guard against;
i guess if it ends up in that scenario, [for many counters] things would be
horribly bad, and the chip, or cabling would
be unusable;
not sure if this answer is sufficient/satisfactory;
thanks
>
> Andrew
Re: [PATCH 15/16] net: phy: adin: add ethtool get_stats support
On Mon, Aug 05, 2019 at 07:54:52PM +0300, Alexandru Ardelean wrote:
> This change implements retrieving all the error counters from the PHY.
> The PHY supports several error counters/stats. The `Mean Square Errors`
> status values are only valie when a link is established, and shouldn't be
> incremented. These values characterize the quality of a signal.
I think you mean accumulated, not incremented?
>
> The rest of the error counters are self-clearing on read.
> Most of them are reports from the Frame Checker engine that the PHY has.
>
> Not retrieving the `LPI Wake Error Count Register` here, since that is used
> by the PHY framework to check for any EEE errors. And that register is
> self-clearing when read (as per IEEE spec).
>
> Signed-off-by: Alexandru Ardelean
> ---
> drivers/net/phy/adin.c | 108 +
> 1 file changed, 108 insertions(+)
>
> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> index a1f3456a8504..04896547dac8 100644
> --- a/drivers/net/phy/adin.c
> +++ b/drivers/net/phy/adin.c
> @@ -103,6 +103,32 @@ static struct clause22_mmd_map clause22_mmd_map[] = {
> { MDIO_MMD_PCS, MDIO_PCS_EEE_WK_ERR,ADIN1300_LPI_WAKE_ERR_CNT_REG },
> };
>
> +struct adin_hw_stat {
> + const char *string;
> + u16 reg1;
> + u16 reg2;
> + bool do_not_inc;
do_not_accumulate? or reverse its meaning, clear_on_read?
Andrew
Re: [PATCH 15/16] net: phy: adin: add ethtool get_stats support
> +struct adin_hw_stat {
> + const char *string;
> +static void adin_get_strings(struct phy_device *phydev, u8 *data)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(adin_hw_stats); i++) {
> + memcpy(data + i * ETH_GSTRING_LEN,
> +adin_hw_stats[i].string, ETH_GSTRING_LEN);
You define string as a char *. So it will be only as long as it should
be. However memcpy always copies ETH_GSTRING_LEN bytes, doing off the
end of the string and into whatever follows.
> + }
> +}
> +
> +static int adin_read_mmd_stat_regs(struct phy_device *phydev,
> +struct adin_hw_stat *stat,
> +u32 *val)
> +{
> + int ret;
> +
> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg1);
> + if (ret < 0)
> + return ret;
> +
> + *val = (ret & 0x);
> +
> + if (stat->reg2 == 0)
> + return 0;
> +
> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2);
> + if (ret < 0)
> + return ret;
> +
> + *val <<= 16;
> + *val |= (ret & 0x);
Does the hardware have a snapshot feature? Is there a danger that
between the two reads stat->reg1 rolls over and you end up with too
big a value?
Andrew
[PATCH 15/16] net: phy: adin: add ethtool get_stats support
This change implements retrieving all the error counters from the PHY.
The PHY supports several error counters/stats. The `Mean Square Errors`
status values are only valie when a link is established, and shouldn't be
incremented. These values characterize the quality of a signal.
The rest of the error counters are self-clearing on read.
Most of them are reports from the Frame Checker engine that the PHY has.
Not retrieving the `LPI Wake Error Count Register` here, since that is used
by the PHY framework to check for any EEE errors. And that register is
self-clearing when read (as per IEEE spec).
Signed-off-by: Alexandru Ardelean
---
drivers/net/phy/adin.c | 108 +
1 file changed, 108 insertions(+)
diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index a1f3456a8504..04896547dac8 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -103,6 +103,32 @@ static struct clause22_mmd_map clause22_mmd_map[] = {
{ MDIO_MMD_PCS, MDIO_PCS_EEE_WK_ERR,ADIN1300_LPI_WAKE_ERR_CNT_REG },
};
+struct adin_hw_stat {
+ const char *string;
+ u16 reg1;
+ u16 reg2;
+ bool do_not_inc;
+};
+
+/* Named just like in the datasheet */
+static struct adin_hw_stat adin_hw_stats[] = {
+ { "RxErrCnt", 0x0014, },
+ { "MseA", 0x8402, 0, true },
+ { "MseB", 0x8403, 0, true },
+ { "MseC", 0x8404, 0, true },
+ { "MseD", 0x8405, 0, true },
+ { "FcFrmCnt", 0x940A, 0x940B }, /* FcFrmCntH + FcFrmCntL */
+ { "FcLenErrCnt",0x940C },
+ { "FcAlgnErrCnt", 0x940D },
+ { "FcSymbErrCnt", 0x940E },
+ { "FcOszCnt", 0x940F },
+ { "FcUszCnt", 0x9410 },
+ { "FcOddCnt", 0x9411 },
+ { "FcOddPreCnt",0x9412 },
+ { "FcDribbleBitsCnt", 0x9413 },
+ { "FcFalseCarrierCnt", 0x9414 },
+};
+
/**
* struct adin_priv - ADIN PHY driver private data
* gpiod_reset optional reset GPIO, to be used in soft_reset() cb
@@ -113,6 +139,7 @@ struct adin_priv {
struct gpio_desc*gpiod_reset;
u8 eee_modes;
booledpd_enabled;
+ u64 stats[ARRAY_SIZE(adin_hw_stats)];
};
static int adin_get_phy_internal_mode(struct phy_device *phydev)
@@ -568,6 +595,81 @@ static int adin_reset(struct phy_device *phydev)
return adin_subsytem_soft_reset(phydev);
}
+static int adin_get_sset_count(struct phy_device *phydev)
+{
+ return ARRAY_SIZE(adin_hw_stats);
+}
+
+static void adin_get_strings(struct phy_device *phydev, u8 *data)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(adin_hw_stats); i++) {
+ memcpy(data + i * ETH_GSTRING_LEN,
+ adin_hw_stats[i].string, ETH_GSTRING_LEN);
+ }
+}
+
+static int adin_read_mmd_stat_regs(struct phy_device *phydev,
+ struct adin_hw_stat *stat,
+ u32 *val)
+{
+ int ret;
+
+ ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg1);
+ if (ret < 0)
+ return ret;
+
+ *val = (ret & 0x);
+
+ if (stat->reg2 == 0)
+ return 0;
+
+ ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2);
+ if (ret < 0)
+ return ret;
+
+ *val <<= 16;
+ *val |= (ret & 0x);
+
+ return 0;
+}
+
+static u64 adin_get_stat(struct phy_device *phydev, int i)
+{
+ struct adin_hw_stat *stat = &adin_hw_stats[i];
+ struct adin_priv *priv = phydev->priv;
+ u32 val;
+ int ret;
+
+ if (stat->reg1 > 0x1f) {
+ ret = adin_read_mmd_stat_regs(phydev, stat, &val);
+ if (ret < 0)
+ return (u64)(~0);
+ } else {
+ ret = phy_read(phydev, stat->reg1);
+ if (ret < 0)
+ return (u64)(~0);
+ val = (ret & 0x);
+ }
+
+ if (stat->do_not_inc)
+ priv->stats[i] = val;
+ else
+ priv->stats[i] += val;
+
+ return priv->stats[i];
+}
+
+static void adin_get_stats(struct phy_device *phydev,
+ struct ethtool_stats *stats, u64 *data)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(adin_hw_stats); i++)
+ data[i] = adin_get_stat(phydev, i);
+}
+
static int adin_probe(struct phy_device *phydev)
{
struct device *dev = &phydev->mdio.dev;
@@ -607,6 +709,9 @@ static struct phy_driver adin_driver[] = {
.read_status= adin_read_status,
.ack_interrupt = adin_phy_ack_intr,
.config_intr= adin_phy_config_intr,
+ .get_sset_count = adin_get_sset_count,
+ .get_strings= adin_get_strings,
+ .get_stats = adin_
