[net-next,v4] dsa: lan9303: Add 3 ethtool stats
Commit Message
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.
Migrating to phylink will be a pre-requisite for adding the stats64 API,
at which point the rtnl_link_stats will come into play.
Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
---
v3->v4:
added returning stat of 0 if there is a device read failure.
removed unrelated change.
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 | 57 +++++++++++++++++++++++++++++++---
1 file changed, 53 insertions(+), 4 deletions(-)
Comments
Hi Jerry,
On Wed, Nov 30, 2022 at 02:08:04PM -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;
>
> for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
> u32 reg;
> int ret;
>
> ret = lan9303_read_switch_port(
> chip, port, lan9303_mib[u].offset, ®);
>
> - if (ret)
> + if (ret) {
> dev_warn(chip->dev, "Reading status port %d reg %u failed\n",
> port, lan9303_mib[u].offset);
> + reg = 0;
> + }
This part of the change still is unrelated and affects existing code.
Bug fixes to existing code are submitted as separate patches. In some
kernel trees, they are at the very least tagged with a Fixes: tag and
put before other development work. In netdev, they are sent to a different
git tree (net.git) which eventually lands in a different set of branches
than net-next.git. You need to not mix bug fixes with development code.
Andrew also suggested that you separate each logical change into a
separate patch.
This, plus the fact that Jakub asked you to also provide standardized
counters, not just free-form ones, which you found it ok to disregard.
I hope that only a misunderstanding is involved, because if it isn't,
then Jakub will know you, alright, but as the person who disregards
review feedback and expects that it'll just disappear. I think Jakub
has pretty solid grounds to not expect that you'll come back with what
has been requested.
Sorry, this patch has a NACK from me at least until you come back with
some clarifications, and split the change.
> data[u] = reg;
> }
>> static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
>> uint64_t *data)
>> {
>> struct lan9303 *chip = ds->priv;
>> unsigned int 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, ®);
>>
>> - if (ret)
>> + if (ret) {
>> dev_warn(chip->dev, "Reading status port %d reg %u failed\n",
>> port, lan9303_mib[u].offset);
>> + reg = 0;
>> + }
>
>This part of the change still is unrelated and affects existing code.
>Bug fixes to existing code are submitted as separate patches. In some
>kernel trees, they are at the very least tagged with a Fixes: tag and
>put before other development work. In netdev, they are sent to a different
>git tree (net.git) which eventually lands in a different set of branches
>than net-next.git. You need to not mix bug fixes with development code.
>Andrew also suggested that you separate each logical change into a
>separate patch.
>
>This, plus the fact that Jakub asked you to also provide standardized
>counters, not just free-form ones, which you found it ok to disregard.
>
>I hope that only a misunderstanding is involved, because if it isn't,
>then Jakub will know you, alright, but as the person who disregards
>review feedback and expects that it'll just disappear. I think Jakub
>has pretty solid grounds to not expect that you'll come back with what
>has been requested.
>
I was hoping to get at least this change upstreamed this cycle and address
the standardized counters down the road. get_stats64 will require PHYLINK.
I replied as such and was hoping to get the benefit of the doubt.
I have a patch set under internal review for migrating to phylink and adding
support for the port_max_mtu api.
I have been asked to, and have plans for, adding VLAN support and then adding
Hardware Timestamping support for the LAN9354 variant. I was hoping to show
some progress with this patchset, but if I need to pull it and pick it up
later then I'll shuffle things around. I wasn't disregarding Jakub's request
and apologize if was viewed that way. I'm simply trying to generate a correct
patchset on something relatively simple before moving on to more complicated
patchsets.
>Sorry, this patch has a NACK from me at least until you come back with
>some clarifications, and split the change.
>
>> data[u] = reg;
>> }
My plan is to correct the issues pointed out in this patchset, but not extend
it to cover standardized counters. get_stats64 is a separate beast requiring
a background thread to keep the stats stored in the driver accumulated and
up to date. The current patch simply reads a few more registers from the
hardware when asked.
Regards,
Jerry.
@@ -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,49 @@ 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;
+ /* Read Port-based MIB stats. */
ret = lan9303_read_switch_port(
chip, port, lan9303_mib[u].offset, ®);
- if (ret)
+ if (ret) {
dev_warn(chip->dev, "Reading status port %d reg %u failed\n",
port, lan9303_mib[u].offset);
+ reg = 0;
+ }
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), ®);
+
+ if (ret) {
+ dev_warn(chip->dev, "Reading status port %d reg %u failed\n",
+ port, lan9303_switch_mib[i].offset + port);
+ reg = 0;
+ }
+ data[i + u] = reg;
+ }
}
static int lan9303_get_sset_count(struct dsa_switch *ds, int port, int sset)
@@ -1017,7 +1066,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)