On Fri, 2023-02-17 at 15:59 +0100, Alexander Lobakin wrote:

[Some people who received this message don't often get email from
aleksander.lobakin@intel.com. Learn why this is important at
https://aka.ms/LearnAboutSenderIdentification ]

EXTERNAL EMAIL: Do not click links or open attachments unless you
know the content is safe

From: Rakesh Sankaranarayanan <redacted>
Date: Fri, 17 Feb 2023 16:32:07 +0530

quoted

    Add support for ethtool standard device statistics grouping.
Support rmon
    statistics grouping using rmon groups parameter in ethtool
command. rmon
    provides packet size based range grouping. Common mib
parameters are used
    across all KSZ series swtches for packet size statistics,
except for
    KSZ8830. KSZ series have mib counters for packets with size:

[...]

quoted

+void ksz8_get_rmon_stats(struct ksz_device *dev, int port,
+                      struct ethtool_rmon_stats *rmon_stats,
+                      const struct ethtool_rmon_hist_range
**ranges)
+{
+     struct ksz_port_mib *mib;
+     u64 *cnt;

Nit: I guess it can be const since you only read it (in every such
callback)?

This variable is getting updated on every call back after reading from
registers, so we cannot have it as constant.

quoted

+     u8 i;
+
+     mib = &dev->ports[port].mib;
+
+     mutex_lock(&mib->cnt_mutex);
+
+     cnt = &mib->counters[KSZ8_RX_UNDERSIZE];
+     dev->dev_ops->r_mib_pkt(dev, port, KSZ8_RX_UNDERSIZE, NULL,
cnt);
+     rmon_stats->undersize_pkts = *cnt;
+
+     cnt = &mib->counters[KSZ8_RX_OVERSIZE];
+     dev->dev_ops->r_mib_pkt(dev, port, KSZ8_RX_OVERSIZE, NULL,
cnt);
+     rmon_stats->oversize_pkts = *cnt;
+
+     cnt = &mib->counters[KSZ8_RX_FRAGMENTS];
+     dev->dev_ops->r_mib_pkt(dev, port, KSZ8_RX_FRAGMENTS, NULL,
cnt);
+     rmon_stats->fragments = *cnt;
+
+     cnt = &mib->counters[KSZ8_RX_JABBERS];
+     dev->dev_ops->r_mib_pkt(dev, port, KSZ8_RX_JABBERS, NULL,
cnt);
+     rmon_stats->jabbers = *cnt;
+
+     for (i = 0; i < KSZ8_HIST_LEN; i++) {
+             cnt = &mib->counters[KSZ8_RX_64_OR_LESS + i];
+             dev->dev_ops->r_mib_pkt(dev, port,
+                             (KSZ8_RX_64_OR_LESS + i), NULL, cnt);

Weird linewrap. Please align the following lines with the opening
brace
of the first one, e.g.

                dev->dev_ops->r_mib_pkt(dev, port,
                                        KSZ8_RX_64_OR_LESS + i, NULL,
                                        cnt);

BUT I don't see why you need those braces around `macro + i` and
without
them you can fit it into the previous line I believe.

Will update it in next revision

quoted

+             rmon_stats->hist[i] = *cnt;
+     }
+
+     mutex_unlock(&mib->cnt_mutex);
+
+     *ranges = ksz_rmon_ranges;
+}
+
+void ksz9477_get_rmon_stats(struct ksz_device *dev, int port,
+                         struct ethtool_rmon_stats *rmon_stats,
+                         const struct ethtool_rmon_hist_range
**ranges)
+{
+     struct ksz_port_mib *mib;
+     u64 *cnt;
+     u8 i;
+
+     mib = &dev->ports[port].mib;
+
+     mutex_lock(&mib->cnt_mutex);
+
+     cnt = &mib->counters[KSZ9477_RX_UNDERSIZE];
+     dev->dev_ops->r_mib_pkt(dev, port, KSZ9477_RX_UNDERSIZE,
NULL, cnt);
+     rmon_stats->undersize_pkts = *cnt;
+
+     cnt = &mib->counters[KSZ9477_RX_OVERSIZE];
+     dev->dev_ops->r_mib_pkt(dev, port, KSZ9477_RX_OVERSIZE, NULL,
cnt);
+     rmon_stats->oversize_pkts = *cnt;
+
+     cnt = &mib->counters[KSZ9477_RX_FRAGMENTS];
+     dev->dev_ops->r_mib_pkt(dev, port, KSZ9477_RX_FRAGMENTS,
NULL, cnt);
+     rmon_stats->fragments = *cnt;
+
+     cnt = &mib->counters[KSZ9477_RX_JABBERS];
+     dev->dev_ops->r_mib_pkt(dev, port, KSZ9477_RX_JABBERS, NULL,
cnt);
+     rmon_stats->jabbers = *cnt;
+
+     for (i = 0; i < KSZ9477_HIST_LEN; i++) {
+             cnt = &mib->counters[KSZ9477_RX_64_OR_LESS + i];
+             dev->dev_ops->r_mib_pkt(dev, port,
+                             (KSZ9477_RX_64_OR_LESS + i), NULL,
cnt);

(same, and please check all other places in the series)

sure, will update in next version.

quoted

+             rmon_stats->hist[i] = *cnt;
+     }
+
+     mutex_unlock(&mib->cnt_mutex);
+
+     *ranges = ksz_rmon_ranges;
+}

[...]

Thanks,
Olek