All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next v3] dsa: lan9303: Add 3 ethtool stats
@ 2022-11-28 20:55 Jerry Ray
  2022-11-28 21:05 ` Vladimir Oltean
  2022-11-28 23:21 ` Jakub Kicinski
  0 siblings, 2 replies; 10+ messages in thread
From: Jerry Ray @ 2022-11-28 20:55 UTC (permalink / raw)
  To: Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel,
	Jerry Ray

Adding Buffer Manager and Switch Engine counters to the reported
statistics. As these stats are kept by the switch rather than the port
instance, they are indexed differently.

These statistics are maintained by the switch and count the packets
dropped due to buffer limits. Note that the rtnl_link_stats: rx_dropped
statistic does not include dropped packets due to buffer exhaustion and as
such, part of this counter would more appropriately fall under the
rx_missed_errors.

Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
---
v2->v3:
  Isolating this patch to include only the added counters.
  Renamed the new statsistic labels to better identify them.
  Added the SWE Filtered counter to the reported statistics.
  Added comments to explain the added counters.
v1->v2:
  Split patch into 2 pieces.
  Removed the adding of a module number to the driver.
---
 drivers/net/dsa/lan9303-core.c | 56 ++++++++++++++++++++++++++++++----
 1 file changed, 50 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 80f07bd20593..ba3814164194 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -176,11 +176,14 @@
 # define LAN9303_SWE_PORT_MIRROR_DISABLED 0
 #define LAN9303_SWE_INGRESS_PORT_TYPE 0x1847
 #define  LAN9303_SWE_INGRESS_PORT_TYPE_VLAN 3
+#define LAN9303_SWE_FILTERED_CNT_SRC_0 0x1850
 #define LAN9303_BM_CFG 0x1c00
+#define LAN9303_BM_DRP_CNT_SRC_0 0x1c05
 #define LAN9303_BM_EGRSS_PORT_TYPE 0x1c0c
 # define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT2 (BIT(17) | BIT(16))
 # define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT1 (BIT(9) | BIT(8))
 # define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0 (BIT(1) | BIT(0))
+#define LAN9303_BM_RATE_DRP_CNT_SRC_0 0x1c16
 
 #define LAN9303_SWITCH_PORT_REG(port, reg0) (0x400 * (port) + (reg0))
 
@@ -978,10 +981,33 @@ static const struct lan9303_mib_desc lan9303_mib[] = {
 	{ .offset = LAN9303_MAC_TX_LATECOL_0, .name = "TxLateCol", },
 };
 
+/* Buffer Management Statistics (indexed by port) */
+/* Switch Engine Statistics (indexed by port) */
+static const struct lan9303_mib_desc lan9303_switch_mib[] = {
+	{ .offset = LAN9303_BM_DRP_CNT_SRC_0, .name = "TotalDropped", },
+	{ .offset = LAN9303_BM_RATE_DRP_CNT_SRC_0, .name = "LimitDropped", },
+	{ .offset = LAN9303_SWE_FILTERED_CNT_SRC_0, .name = "SweFiltered", },
+};
+
+/* TotalDropped:     This register counts the total number of packets dropped
+ * by the Buffer Manager that were received on the given port. This count
+ * includes packets dropped due to buffer space limits and ingress rate limit
+ * discarding (Red and random Yellow dropping).
+ *
+ * LimitDropped:     This register counts the number of packets received on a
+ * port that were dropped by the Buffer Manager solely due to ingress rate
+ * limiting (discarding packets due to Red and random Yellow dropping).
+ *
+ * SweFiltered:      This counter contains the number of packets filtered by
+ * the Switch Engine at ingress for a given port. The count includes packets
+ * filtered due to broadcast throttling, but does not include packets dropped
+ * due to ingress rate limiting.
+ */
+
 static void lan9303_get_strings(struct dsa_switch *ds, int port,
 				u32 stringset, uint8_t *data)
 {
-	unsigned int u;
+	unsigned int i, u;
 
 	if (stringset != ETH_SS_STATS)
 		return;
@@ -990,26 +1016,44 @@ static void lan9303_get_strings(struct dsa_switch *ds, int port,
 		strncpy(data + u * ETH_GSTRING_LEN, lan9303_mib[u].name,
 			ETH_GSTRING_LEN);
 	}
+	for (i = 0; i < ARRAY_SIZE(lan9303_switch_mib); i++) {
+		strncpy(data + (u + i) * ETH_GSTRING_LEN,
+			lan9303_switch_mib[i].name, ETH_GSTRING_LEN);
+	}
 }
 
 static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
 				      uint64_t *data)
 {
 	struct lan9303 *chip = ds->priv;
-	unsigned int u;
+	unsigned int i, u;
 
 	for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
 		u32 reg;
 		int ret;
 
-		ret = lan9303_read_switch_port(
-			chip, port, lan9303_mib[u].offset, &reg);
-
+		/* Read Port-based MIB stats. */
+		ret = lan9303_read_switch_port(chip, port,
+					       lan9303_mib[u].offset,
+					       &reg);
 		if (ret)
 			dev_warn(chip->dev, "Reading status port %d reg %u failed\n",
 				 port, lan9303_mib[u].offset);
 		data[u] = reg;
 	}
+	for (i = 0; i < ARRAY_SIZE(lan9303_switch_mib); i++) {
+		u32 reg;
+		int ret;
+
+		/* Read Switch stats indexed by port. */
+		ret = lan9303_read_switch_reg(chip,
+					      (lan9303_switch_mib[i].offset +
+					       port), &reg);
+		if (ret)
+			dev_warn(chip->dev, "Reading status port %d reg %u failed\n",
+				 port, lan9303_switch_mib[i].offset);
+		data[i + u] = reg;
+	}
 }
 
 static int lan9303_get_sset_count(struct dsa_switch *ds, int port, int sset)
@@ -1017,7 +1061,7 @@ static int lan9303_get_sset_count(struct dsa_switch *ds, int port, int sset)
 	if (sset != ETH_SS_STATS)
 		return 0;
 
-	return ARRAY_SIZE(lan9303_mib);
+	return ARRAY_SIZE(lan9303_mib) + ARRAY_SIZE(lan9303_switch_mib);
 }
 
 static int lan9303_phy_read(struct dsa_switch *ds, int phy, int regnum)
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next v3] dsa: lan9303: Add 3 ethtool stats
  2022-11-28 20:55 [PATCH net-next v3] dsa: lan9303: Add 3 ethtool stats Jerry Ray
@ 2022-11-28 21:05 ` Vladimir Oltean
  2022-11-30 15:57   ` Jerry.Ray
  2022-11-28 23:21 ` Jakub Kicinski
  1 sibling, 1 reply; 10+ messages in thread
From: Vladimir Oltean @ 2022-11-28 21:05 UTC (permalink / raw)
  To: Jerry Ray
  Cc: Andrew Lunn, Florian Fainelli, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, netdev, linux-kernel

On Mon, Nov 28, 2022 at 02:55:21PM -0600, Jerry Ray wrote:
>  static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
>  				      uint64_t *data)
>  {
>  	struct lan9303 *chip = ds->priv;
> -	unsigned int u;
> +	unsigned int i, u;
>  
>  	for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
>  		u32 reg;
>  		int ret;
>  
> -		ret = lan9303_read_switch_port(
> -			chip, port, lan9303_mib[u].offset, &reg);
> -
> +		/* Read Port-based MIB stats. */
> +		ret = lan9303_read_switch_port(chip, port,
> +					       lan9303_mib[u].offset,
> +					       &reg);

Speaking of unrelated changes...

>  		if (ret)
>  			dev_warn(chip->dev, "Reading status port %d reg %u failed\n",
>  				 port, lan9303_mib[u].offset);

...If lan9303_read_switch_port() fails, should we copy kernel stack
uninitialized memory (reg) to user space?

>  		data[u] = reg;
>  	}
> +	for (i = 0; i < ARRAY_SIZE(lan9303_switch_mib); i++) {
> +		u32 reg;
> +		int ret;
> +
> +		/* Read Switch stats indexed by port. */
> +		ret = lan9303_read_switch_reg(chip,
> +					      (lan9303_switch_mib[i].offset +
> +					       port), &reg);
> +		if (ret)
> +			dev_warn(chip->dev, "Reading status port %d reg %u failed\n",
> +				 port, lan9303_switch_mib[i].offset);

Because the same, existing pattern is also used for new code here.

> +		data[i + u] = reg;
> +	}
>  }
>  
>  static int lan9303_get_sset_count(struct dsa_switch *ds, int port, int sset)
> @@ -1017,7 +1061,7 @@ static int lan9303_get_sset_count(struct dsa_switch *ds, int port, int sset)
>  	if (sset != ETH_SS_STATS)
>  		return 0;
>  
> -	return ARRAY_SIZE(lan9303_mib);
> +	return ARRAY_SIZE(lan9303_mib) + ARRAY_SIZE(lan9303_switch_mib);
>  }
>  
>  static int lan9303_phy_read(struct dsa_switch *ds, int phy, int regnum)
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next v3] dsa: lan9303: Add 3 ethtool stats
  2022-11-28 20:55 [PATCH net-next v3] dsa: lan9303: Add 3 ethtool stats Jerry Ray
  2022-11-28 21:05 ` Vladimir Oltean
@ 2022-11-28 23:21 ` Jakub Kicinski
  2022-11-30 15:51   ` Jerry.Ray
  1 sibling, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2022-11-28 23:21 UTC (permalink / raw)
  To: Jerry Ray
  Cc: Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller,
	Eric Dumazet, Paolo Abeni, netdev, linux-kernel

On Mon, 28 Nov 2022 14:55:21 -0600 Jerry Ray wrote:
> These statistics are maintained by the switch and count the packets
> dropped due to buffer limits. Note that the rtnl_link_stats: rx_dropped
> statistic does not include dropped packets due to buffer exhaustion and as
> such, part of this counter would more appropriately fall under the
> rx_missed_errors.

Why not add them there as well?

Are these drops accounted for in any drop / error statistics within
rtnl_link_stats? 

It's okay to provide implementation specific breakdown via ethtool -S
but user must be able to notice that there are some drops / errors in
the system by looking at standard stats.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH net-next v3] dsa: lan9303: Add 3 ethtool stats
  2022-11-28 23:21 ` Jakub Kicinski
@ 2022-11-30 15:51   ` Jerry.Ray
  2022-11-30 16:52     ` Jakub Kicinski
  0 siblings, 1 reply; 10+ messages in thread
From: Jerry.Ray @ 2022-11-30 15:51 UTC (permalink / raw)
  To: kuba
  Cc: andrew, f.fainelli, olteanv, davem, edumazet, pabeni, netdev,
	linux-kernel

>On Mon, 28 Nov 2022 14:55:21 -0600 Jerry Ray wrote:
>> These statistics are maintained by the switch and count the packets
>> dropped due to buffer limits. Note that the rtnl_link_stats: rx_dropped
>> statistic does not include dropped packets due to buffer exhaustion and as
>> such, part of this counter would more appropriately fall under the
>> rx_missed_errors.
>
>Why not add them there as well?
>
>Are these drops accounted for in any drop / error statistics within
>rtnl_link_stats?
>
>It's okay to provide implementation specific breakdown via ethtool -S
>but user must be able to notice that there are some drops / errors in
>the system by looking at standard stats.
>

The idea here is to provide the statistics as documented in the part
datasheet.  In the future, I'll be looking to add support for the stats64
API and will deal with appropriately sorting the available hardware stats
into the rtnl_link_stats buckets.

Regards,
Jerry.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH net-next v3] dsa: lan9303: Add 3 ethtool stats
  2022-11-28 21:05 ` Vladimir Oltean
@ 2022-11-30 15:57   ` Jerry.Ray
  2022-11-30 16:00     ` Andrew Lunn
  0 siblings, 1 reply; 10+ messages in thread
From: Jerry.Ray @ 2022-11-30 15:57 UTC (permalink / raw)
  To: olteanv
  Cc: andrew, f.fainelli, davem, edumazet, kuba, pabeni, netdev, linux-kernel

>>  static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
>>                                     uint64_t *data)
>>  {
>>       struct lan9303 *chip = ds->priv;
>> -     unsigned int u;
>> +     unsigned int i, u;
>>
>>       for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
>>               u32 reg;
>>               int ret;
>>
>> -             ret = lan9303_read_switch_port(
>> -                     chip, port, lan9303_mib[u].offset, &reg);
>> -
>> +             /* Read Port-based MIB stats. */
>> +             ret = lan9303_read_switch_port(chip, port,
>> +                                            lan9303_mib[u].offset,
>> +                                            &reg);
>
>Speaking of unrelated changes...
>

Cleaning up a warning generated from checkpatch

>>               if (ret)
>>                       dev_warn(chip->dev, "Reading status port %d reg %u failed\n",
>>                                port, lan9303_mib[u].offset);
>
>...If lan9303_read_switch_port() fails, should we copy kernel stack
>uninitialized memory (reg) to user space?
>

Good catch.  I'll zero out the returned stat.

>>               data[u] = reg;
>>       }
>> +     for (i = 0; i < ARRAY_SIZE(lan9303_switch_mib); i++) {
>> +             u32 reg;
>> +             int ret;
>> +
>> +             /* Read Switch stats indexed by port. */
>> +             ret = lan9303_read_switch_reg(chip,
>> +                                           (lan9303_switch_mib[i].offset +
>> +                                            port), &reg);
>> +             if (ret)
>> +                     dev_warn(chip->dev, "Reading status port %d reg %u failed\n",
>> +                              port, lan9303_switch_mib[i].offset);
>
>Because the same, existing pattern is also used for new code here.
>

Ditto

>> +             data[i + u] = reg;
>> +     }
>>  }
>>
>>  static int lan9303_get_sset_count(struct dsa_switch *ds, int port, int sset)
>> @@ -1017,7 +1061,7 @@ static int lan9303_get_sset_count(struct dsa_switch *ds, int port, int sset)
>>       if (sset != ETH_SS_STATS)
>>               return 0;
>>
>> -     return ARRAY_SIZE(lan9303_mib);
>> +     return ARRAY_SIZE(lan9303_mib) + ARRAY_SIZE(lan9303_switch_mib);
>>  }
>>
>>  static int lan9303_phy_read(struct dsa_switch *ds, int phy, int regnum)
>> --
>> 2.17.1
>>
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next v3] dsa: lan9303: Add 3 ethtool stats
  2022-11-30 15:57   ` Jerry.Ray
@ 2022-11-30 16:00     ` Andrew Lunn
  0 siblings, 0 replies; 10+ messages in thread
From: Andrew Lunn @ 2022-11-30 16:00 UTC (permalink / raw)
  To: Jerry.Ray
  Cc: olteanv, f.fainelli, davem, edumazet, kuba, pabeni, netdev, linux-kernel

On Wed, Nov 30, 2022 at 03:57:35PM +0000, Jerry.Ray@microchip.com wrote:
> >>  static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
> >>                                     uint64_t *data)
> >>  {
> >>       struct lan9303 *chip = ds->priv;
> >> -     unsigned int u;
> >> +     unsigned int i, u;
> >>
> >>       for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
> >>               u32 reg;
> >>               int ret;
> >>
> >> -             ret = lan9303_read_switch_port(
> >> -                     chip, port, lan9303_mib[u].offset, &reg);
> >> -
> >> +             /* Read Port-based MIB stats. */
> >> +             ret = lan9303_read_switch_port(chip, port,
> >> +                                            lan9303_mib[u].offset,
> >> +                                            &reg);
> >
> >Speaking of unrelated changes...
> >
> 
> Cleaning up a warning generated from checkpatch

Cleanups a good, but please put them in a patch of their own.

https://www.kernel.org/doc/html/v4.10/process/submitting-patches.html#separate-your-changes

   3) Separate your changes

   Separate each logical change into a separate patch.

Andrew


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next v3] dsa: lan9303: Add 3 ethtool stats
  2022-11-30 15:51   ` Jerry.Ray
@ 2022-11-30 16:52     ` Jakub Kicinski
  2022-11-30 18:12       ` Jerry.Ray
  0 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2022-11-30 16:52 UTC (permalink / raw)
  To: Jerry.Ray
  Cc: andrew, f.fainelli, olteanv, davem, edumazet, pabeni, netdev,
	linux-kernel

On Wed, 30 Nov 2022 15:51:44 +0000 Jerry.Ray@microchip.com wrote:
>> Why not add them there as well?
>>
>> Are these drops accounted for in any drop / error statistics within
>> rtnl_link_stats?
>>
>> It's okay to provide implementation specific breakdown via ethtool -S
>> but user must be able to notice that there are some drops / errors in
>> the system by looking at standard stats.
> 
> The idea here is to provide the statistics as documented in the part
> datasheet.  In the future, I'll be looking to add support for the stats64
> API and will deal with appropriately sorting the available hardware stats
> into the rtnl_link_stats buckets.

Upstream we care about providing reasonably uniform experience across
drivers and vendors. Because I don't know you and therefore don't trust
you to follow up you must do the standard thing in the same patch set,
pretty please.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH net-next v3] dsa: lan9303: Add 3 ethtool stats
  2022-11-30 16:52     ` Jakub Kicinski
@ 2022-11-30 18:12       ` Jerry.Ray
  2022-11-30 18:50         ` Vladimir Oltean
  0 siblings, 1 reply; 10+ messages in thread
From: Jerry.Ray @ 2022-11-30 18:12 UTC (permalink / raw)
  To: kuba
  Cc: andrew, f.fainelli, olteanv, davem, edumazet, pabeni, netdev,
	linux-kernel

>>> Why not add them there as well?
>>>
>>> Are these drops accounted for in any drop / error statistics within
>>> rtnl_link_stats?
>>>
>>> It's okay to provide implementation specific breakdown via ethtool -S
>>> but user must be able to notice that there are some drops / errors in
>>> the system by looking at standard stats.
>>
>> The idea here is to provide the statistics as documented in the part
>> datasheet.  In the future, I'll be looking to add support for the stats64
>> API and will deal with appropriately sorting the available hardware stats
>> into the rtnl_link_stats buckets.
>
>Upstream we care about providing reasonably uniform experience across
>drivers and vendors. Because I don't know you and therefore don't trust
>you to follow up you must do the standard thing in the same patch set,
>pretty please.
>

Won't be able to get to stats64 this cycle.  Looking to migrate to phylink
first.  This is a pretty old driver.

Understand you don't know me - yet.  

Regards,
Jerry.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next v3] dsa: lan9303: Add 3 ethtool stats
  2022-11-30 18:12       ` Jerry.Ray
@ 2022-11-30 18:50         ` Vladimir Oltean
  2022-12-01 15:56           ` Jerry.Ray
  0 siblings, 1 reply; 10+ messages in thread
From: Vladimir Oltean @ 2022-11-30 18:50 UTC (permalink / raw)
  To: Jerry.Ray
  Cc: kuba, andrew, f.fainelli, davem, edumazet, pabeni, netdev, linux-kernel

On Wed, Nov 30, 2022 at 06:12:54PM +0000, Jerry.Ray@microchip.com wrote:
> Won't be able to get to stats64 this cycle.  Looking to migrate to phylink
> first.  This is a pretty old driver.
> 
> Understand you don't know me - yet.

It would be good if you first prepared a bug fix patch for the existing
kernel stack memory leakage, and submit that to the net.git tree.
The net.git is merged back into net-next.git every ~Thursday, and
generally speaking, either you wait for bug fixes to land back into
net-next before you submit new net-next material in the same areas,
or the netdev and linux-next maintainers will have to resolve the merge
conflict between trees manually. Not a huge deal, but it is kind of a
nuisance for backports (to not be able to linearize a series of cherry
picks) and all in all, it's best to organize your work such that you
don't conflict with yourself.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH net-next v3] dsa: lan9303: Add 3 ethtool stats
  2022-11-30 18:50         ` Vladimir Oltean
@ 2022-12-01 15:56           ` Jerry.Ray
  0 siblings, 0 replies; 10+ messages in thread
From: Jerry.Ray @ 2022-12-01 15:56 UTC (permalink / raw)
  To: olteanv
  Cc: kuba, andrew, f.fainelli, davem, edumazet, pabeni, netdev, linux-kernel

>> Won't be able to get to stats64 this cycle.  Looking to migrate to phylink
>> first.  This is a pretty old driver.
>>
>> Understand you don't know me - yet.
>
>It would be good if you first prepared a bug fix patch for the existing
>kernel stack memory leakage, and submit that to the net.git tree.
>The net.git is merged back into net-next.git every ~Thursday, and
>generally speaking, either you wait for bug fixes to land back into
>net-next before you submit new net-next material in the same areas,
>or the netdev and linux-next maintainers will have to resolve the merge
>conflict between trees manually. Not a huge deal, but it is kind of a
>nuisance for backports (to not be able to linearize a series of cherry
>picks) and all in all, it's best to organize your work such that you
>don't conflict with yourself.
>

I'm unaware of a kernel stack memory leak.  Can you point me to a thread?

As for splitting bug fix patches, ...
A comment was made against my patch and I looked to make things consistent
with the existing code.  I failed to see the distinction of addressing the
issue with the patch and addressing the same issue with existing code.
Thanks for explaining the reasoning behind why separating the patches
matters.  I'll try to keep that in mind for future submissions.

Regards,
Jerry.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-12-01 15:57 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-28 20:55 [PATCH net-next v3] dsa: lan9303: Add 3 ethtool stats Jerry Ray
2022-11-28 21:05 ` Vladimir Oltean
2022-11-30 15:57   ` Jerry.Ray
2022-11-30 16:00     ` Andrew Lunn
2022-11-28 23:21 ` Jakub Kicinski
2022-11-30 15:51   ` Jerry.Ray
2022-11-30 16:52     ` Jakub Kicinski
2022-11-30 18:12       ` Jerry.Ray
2022-11-30 18:50         ` Vladimir Oltean
2022-12-01 15:56           ` Jerry.Ray

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.