| Message ID | 20220414122250.158113-8-clement.leger@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | add support for Renesas RZ/N1 ethernet subsystem devices | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/cc_maintainers | success | CCed 9 of 9 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/checkpatch | warning | CHECK: spinlock_t definition without comment |
| netdev/kdoc | success | Errors and warnings before: 2 this patch: 2 |
| netdev/source_inline | success | Was 0 now: 0 |
On Thu, Apr 14, 2022 at 02:22:45PM +0200, Clément Léger wrote: > Add per-port statistics. This support requries to add a stat lock since > statistics are stored in two 32 bits registers, the hi part one being > global and latched when accessing the lo part. > > Signed-off-by: Clément Léger <clement.leger@bootlin.com> > --- I think for new drivers Jakub will also want to see the more specific and less free-form get_stats64, get_eth_mac_stats, get_eth_phy_stats, get_eth_ctrl_stats ops implemented. Your counters should map nicely over these. > drivers/net/dsa/rzn1_a5psw.c | 101 +++++++++++++++++++++++++++++++++++ > drivers/net/dsa/rzn1_a5psw.h | 2 + > 2 files changed, 103 insertions(+) > > diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c > index 5bee999f7050..7ab7d9054427 100644 > --- a/drivers/net/dsa/rzn1_a5psw.c > +++ b/drivers/net/dsa/rzn1_a5psw.c > @@ -16,6 +16,59 @@ > > #include "rzn1_a5psw.h" > > +struct a5psw_stats { > + u16 offset; > + const char *name; > +}; > + > +#define STAT_DESC(_offset, _name) {.offset = _offset, .name = _name} > + > +static const struct a5psw_stats a5psw_stats[] = { > + STAT_DESC(0x868, "aFrameTransmitted"), > + STAT_DESC(0x86C, "aFrameReceived"), > + STAT_DESC(0x870, "aFrameCheckSequenceErrors"), > + STAT_DESC(0x874, "aAlignmentErrors"), > + STAT_DESC(0x878, "aOctetsTransmitted"), > + STAT_DESC(0x87C, "aOctetsReceived"), > + STAT_DESC(0x880, "aTxPAUSEMACCtrlFrames"), > + STAT_DESC(0x884, "aRxPAUSEMACCtrlFrames"), What does the "a" stand for? > + /* If */ > + STAT_DESC(0x888, "ifInErrors"), > + STAT_DESC(0x88C, "ifOutErrors"), > + STAT_DESC(0x890, "ifInUcastPkts"), > + STAT_DESC(0x894, "ifInMulticastPkts"), > + STAT_DESC(0x898, "ifInBroadcastPkts"), > + STAT_DESC(0x89C, "ifOutDiscards"), > + STAT_DESC(0x8A0, "ifOutUcastPkts"), > + STAT_DESC(0x8A4, "ifOutMulticastPkts"), > + STAT_DESC(0x8A8, "ifOutBroadcastPkts"), > + /* Ether */ > + STAT_DESC(0x8AC, "etherStatsDropEvents"), > + STAT_DESC(0x8B0, "etherStatsOctets"), > + STAT_DESC(0x8B4, "etherStatsPkts"), > + STAT_DESC(0x8B8, "etherStatsUndersizePkts"), > + STAT_DESC(0x8BC, "etherStatsetherStatsOversizePktsDropEvents"), "etherStats" is duplicated here. > + STAT_DESC(0x8C0, "etherStatsPkts64Octets"), > + STAT_DESC(0x8C4, "etherStatsPkts65to127Octets"), > + STAT_DESC(0x8C8, "etherStatsPkts128to255Octets"), > + STAT_DESC(0x8CC, "etherStatsPkts256to511Octets"), > + STAT_DESC(0x8D0, "etherStatsPkts512to1023Octets"), > + STAT_DESC(0x8D4, "etherStatsPkts1024to1518Octets"), > + STAT_DESC(0x8D8, "etherStatsPkts1519toXOctets"), > + STAT_DESC(0x8DC, "etherStatsJabbers"), > + STAT_DESC(0x8E0, "etherStatsFragments"), > + > + STAT_DESC(0x8E8, "VLANReceived"), > + STAT_DESC(0x8EC, "VLANTransmitted"), > + > + STAT_DESC(0x910, "aDeferred"), > + STAT_DESC(0x914, "aMultipleCollisions"), > + STAT_DESC(0x918, "aSingleCollisions"), > + STAT_DESC(0x91C, "aLateCollisions"), > + STAT_DESC(0x920, "aExcessiveCollisions"), > + STAT_DESC(0x924, "aCarrierSenseErrors"), > +}; > + > static void a5psw_reg_writel(struct a5psw *a5psw, int offset, u32 value) > { > writel(value, a5psw->base + offset); > @@ -316,6 +369,50 @@ static void a5psw_port_fast_age(struct dsa_switch *ds, int port) > a5psw_port_fdb_flush(a5psw, port); > } > > +static void a5psw_get_strings(struct dsa_switch *ds, int port, u32 stringset, > + uint8_t *data) > +{ > + unsigned int u; > + > + if (stringset != ETH_SS_STATS) > + return; > + > + for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) { > + strncpy(data + u * ETH_GSTRING_LEN, a5psw_stats[u].name, > + ETH_GSTRING_LEN); > + } > +} > + > +static void a5psw_get_ethtool_stats(struct dsa_switch *ds, int port, > + uint64_t *data) > +{ > + struct a5psw *a5psw = ds->priv; > + u32 reg_lo, reg_hi; > + unsigned int u; > + > + for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) { > + /* A5PSW_STATS_HIWORD is global and thus, access must be > + * exclusive > + */ > + spin_lock(&a5psw->stats_lock); > + reg_lo = a5psw_reg_readl(a5psw, a5psw_stats[u].offset + > + A5PSW_PORT_OFFSET(port)); > + /* A5PSW_STATS_HIWORD is latched on stat read */ > + reg_hi = a5psw_reg_readl(a5psw, A5PSW_STATS_HIWORD); > + > + data[u] = ((u64)reg_hi << 32) | reg_lo; > + spin_unlock(&a5psw->stats_lock); > + } > +} > + > +static int a5psw_get_sset_count(struct dsa_switch *ds, int port, int sset) > +{ > + if (sset != ETH_SS_STATS) > + return 0; > + > + return ARRAY_SIZE(a5psw_stats); > +} > + > static int a5psw_setup(struct dsa_switch *ds) > { > struct a5psw *a5psw = ds->priv; > @@ -395,6 +492,9 @@ const struct dsa_switch_ops a5psw_switch_ops = { > .phylink_mac_link_up = a5psw_phylink_mac_link_up, > .port_change_mtu = a5psw_port_change_mtu, > .port_max_mtu = a5psw_port_max_mtu, > + .get_sset_count = a5psw_get_sset_count, > + .get_strings = a5psw_get_strings, > + .get_ethtool_stats = a5psw_get_ethtool_stats, > .set_ageing_time = a5psw_set_ageing_time, > .port_bridge_join = a5psw_port_bridge_join, > .port_bridge_leave = a5psw_port_bridge_leave, > @@ -580,6 +680,7 @@ static int a5psw_probe(struct platform_device *pdev) > return -ENOMEM; > > a5psw->dev = dev; > + spin_lock_init(&a5psw->stats_lock); > spin_lock_init(&a5psw->lk_lock); > spin_lock_init(&a5psw->reg_lock); > a5psw->base = devm_platform_ioremap_resource(pdev, 0); > diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h > index 2d96a2afbc3a..b34ea549e936 100644 > --- a/drivers/net/dsa/rzn1_a5psw.h > +++ b/drivers/net/dsa/rzn1_a5psw.h > @@ -177,6 +177,7 @@ > * @mdio_freq: MDIO bus frequency requested > * @pcs: Array of PCS connected to the switch ports (not for the CPU) > * @ds: DSA switch struct > + * @stats_lock: lock to access statistics (shared HI counter) > * @lk_lock: Lock for the lookup table > * @reg_lock: Lock for register read-modify-write operation > * @flooding_ports: List of ports that should be flooded > @@ -190,6 +191,7 @@ struct a5psw { > u32 mdio_freq; > struct phylink_pcs *pcs[A5PSW_PORTS_NUM - 1]; > struct dsa_switch ds; > + spinlock_t stats_lock; > spinlock_t lk_lock; > spinlock_t reg_lock; > u32 flooding_ports; > -- > 2.34.1 >
On Thu, Apr 14, 2022 at 02:22:45PM +0200, Clément Léger wrote: > Add per-port statistics. This support requries to add a stat lock since > statistics are stored in two 32 bits registers, the hi part one being > global and latched when accessing the lo part. > > Signed-off-by: Clément Léger <clement.leger@bootlin.com> > --- > drivers/net/dsa/rzn1_a5psw.c | 101 +++++++++++++++++++++++++++++++++++ > drivers/net/dsa/rzn1_a5psw.h | 2 + > 2 files changed, 103 insertions(+) > > diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c > index 5bee999f7050..7ab7d9054427 100644 > --- a/drivers/net/dsa/rzn1_a5psw.c > +++ b/drivers/net/dsa/rzn1_a5psw.c > @@ -16,6 +16,59 @@ > > #include "rzn1_a5psw.h" > > +struct a5psw_stats { > + u16 offset; > + const char *name; > +}; > + > +#define STAT_DESC(_offset, _name) {.offset = _offset, .name = _name} > + > +static const struct a5psw_stats a5psw_stats[] = { > + STAT_DESC(0x868, "aFrameTransmitted"), > + STAT_DESC(0x86C, "aFrameReceived"), > + STAT_DESC(0x8BC, "etherStatsetherStatsOversizePktsDropEvents"), > +}; > +static void a5psw_get_strings(struct dsa_switch *ds, int port, u32 stringset, > + uint8_t *data) > +{ > + unsigned int u; > + > + if (stringset != ETH_SS_STATS) > + return; > + > + for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) { > + strncpy(data + u * ETH_GSTRING_LEN, a5psw_stats[u].name, > + ETH_GSTRING_LEN); > + } The kernel strncpy() is like the user space one. It does not add a NULL if the string is longer than ETH_GSTRING_LEN and it needs to truncate. So there is a danger here. What you find most drivers do is struct a5psw_stats { u16 offset; const char name[ETH_GSTRING_LEN]; }; You should then get a compiler warning/error if you string is ever longer than allowed. And use memcpy() rather than strcpy(), which is faster anyway. But you do use up a bit more memory. > +static void a5psw_get_ethtool_stats(struct dsa_switch *ds, int port, > + uint64_t *data) > +{ > + struct a5psw *a5psw = ds->priv; > + u32 reg_lo, reg_hi; > + unsigned int u; > + > + for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) { > + /* A5PSW_STATS_HIWORD is global and thus, access must be > + * exclusive > + */ Could you explain that a bit more. The RTNL lock will prevent two parallel calls to this function. > + spin_lock(&a5psw->stats_lock); > + reg_lo = a5psw_reg_readl(a5psw, a5psw_stats[u].offset + > + A5PSW_PORT_OFFSET(port)); > + /* A5PSW_STATS_HIWORD is latched on stat read */ > + reg_hi = a5psw_reg_readl(a5psw, A5PSW_STATS_HIWORD); > + > + data[u] = ((u64)reg_hi << 32) | reg_lo; > + spin_unlock(&a5psw->stats_lock); > + } > +} Andrew
Le Fri, 15 Apr 2022 01:16:11 +0200, Andrew Lunn <andrew@lunn.ch> a écrit : > On Thu, Apr 14, 2022 at 02:22:45PM +0200, Clément Léger wrote: > > Add per-port statistics. This support requries to add a stat lock since > > statistics are stored in two 32 bits registers, the hi part one being > > global and latched when accessing the lo part. > > > > Signed-off-by: Clément Léger <clement.leger@bootlin.com> > > --- > > drivers/net/dsa/rzn1_a5psw.c | 101 +++++++++++++++++++++++++++++++++++ > > drivers/net/dsa/rzn1_a5psw.h | 2 + > > 2 files changed, 103 insertions(+) > > > > diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c > > index 5bee999f7050..7ab7d9054427 100644 > > --- a/drivers/net/dsa/rzn1_a5psw.c > > +++ b/drivers/net/dsa/rzn1_a5psw.c > > @@ -16,6 +16,59 @@ > > > > #include "rzn1_a5psw.h" > > > > +struct a5psw_stats { > > + u16 offset; > > + const char *name; > > +}; > > + > > +#define STAT_DESC(_offset, _name) {.offset = _offset, .name = _name} > > + > > +static const struct a5psw_stats a5psw_stats[] = { > > + STAT_DESC(0x868, "aFrameTransmitted"), > > + STAT_DESC(0x86C, "aFrameReceived"), > > + STAT_DESC(0x8BC, "etherStatsetherStatsOversizePktsDropEvents"), > > > +}; > > > > +static void a5psw_get_strings(struct dsa_switch *ds, int port, u32 stringset, > > + uint8_t *data) > > +{ > > + unsigned int u; > > + > > + if (stringset != ETH_SS_STATS) > > + return; > > + > > + for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) { > > + strncpy(data + u * ETH_GSTRING_LEN, a5psw_stats[u].name, > > + ETH_GSTRING_LEN); > > + } > > The kernel strncpy() is like the user space one. It does not add a > NULL if the string is longer than ETH_GSTRING_LEN and it needs to > truncate. So there is a danger here. > > What you find most drivers do is > > struct a5psw_stats { > u16 offset; > const char name[ETH_GSTRING_LEN]; > }; > > You should then get a compiler warning/error if you string is ever > longer than allowed. And use memcpy() rather than strcpy(), which is > faster anyway. But you do use up a bit more memory. Acked. > > > +static void a5psw_get_ethtool_stats(struct dsa_switch *ds, int port, > > + uint64_t *data) > > +{ > > + struct a5psw *a5psw = ds->priv; > > + u32 reg_lo, reg_hi; > > + unsigned int u; > > + > > + for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) { > > + /* A5PSW_STATS_HIWORD is global and thus, access must be > > + * exclusive > > + */ > > Could you explain that a bit more. The RTNL lock will prevent two > parallel calls to this function. Ok, I wasn't sure of the locking applicable here.
Le Thu, 14 Apr 2022 20:34:44 +0300, Vladimir Oltean <olteanv@gmail.com> a écrit : > On Thu, Apr 14, 2022 at 02:22:45PM +0200, Clément Léger wrote: > > Add per-port statistics. This support requries to add a stat lock since > > statistics are stored in two 32 bits registers, the hi part one being > > global and latched when accessing the lo part. > > > > Signed-off-by: Clément Léger <clement.leger@bootlin.com> > > --- > > I think for new drivers Jakub will also want to see the more specific > and less free-form get_stats64, get_eth_mac_stats, get_eth_phy_stats, > get_eth_ctrl_stats ops implemented. Your counters should map nicely over > these. Ok, I'll implement these callbacks ! > > > drivers/net/dsa/rzn1_a5psw.c | 101 +++++++++++++++++++++++++++++++++++ > > drivers/net/dsa/rzn1_a5psw.h | 2 + > > 2 files changed, 103 insertions(+) > > > > diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c > > index 5bee999f7050..7ab7d9054427 100644 > > --- a/drivers/net/dsa/rzn1_a5psw.c > > +++ b/drivers/net/dsa/rzn1_a5psw.c > > @@ -16,6 +16,59 @@ > > > > #include "rzn1_a5psw.h" > > > > +struct a5psw_stats { > > + u16 offset; > > + const char *name; > > +}; > > + > > +#define STAT_DESC(_offset, _name) {.offset = _offset, .name = _name} > > + > > +static const struct a5psw_stats a5psw_stats[] = { > > + STAT_DESC(0x868, "aFrameTransmitted"), > > + STAT_DESC(0x86C, "aFrameReceived"), > > + STAT_DESC(0x870, "aFrameCheckSequenceErrors"), > > + STAT_DESC(0x874, "aAlignmentErrors"), > > + STAT_DESC(0x878, "aOctetsTransmitted"), > > + STAT_DESC(0x87C, "aOctetsReceived"), > > + STAT_DESC(0x880, "aTxPAUSEMACCtrlFrames"), > > + STAT_DESC(0x884, "aRxPAUSEMACCtrlFrames"), > > What does the "a" stand for? That's a mystery :/ I tried to be a normal person and copy/pasted these from the datasheet ;)
> > > +static void a5psw_get_ethtool_stats(struct dsa_switch *ds, int port, > > > + uint64_t *data) > > > +{ > > > + struct a5psw *a5psw = ds->priv; > > > + u32 reg_lo, reg_hi; > > > + unsigned int u; > > > + > > > + for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) { > > > + /* A5PSW_STATS_HIWORD is global and thus, access must be > > > + * exclusive > > > + */ > > > > Could you explain that a bit more. The RTNL lock will prevent two > > parallel calls to this function. > > Ok, I wasn't sure of the locking applicable here. In general, RTNL protects you for any user space management like operation on the driver. In this case, if you look in net/ethtool, you will find the IOCTL handler code takes RTNL before calling into the main IOCTL dispatcher. If you want to be paranoid/document the assumption, you can add an ASSERT_RTNL(). The semantics for some of the other statistics Vladimir requested can be slightly different. One of them is in atomic context, because a spinlock is held. But i don't remember if RTNL is also held. This is less of an issue for your switch, since it uses MMIO, however many switches need to perform blocking IO over MDIO, SPI, IC2 etc to get stats, which you cannot do in atomic context. So they end up returning cached values. Look in the mailing list for past discussion for details. Andrew
Le Fri, 15 Apr 2022 15:37:38 +0200, Andrew Lunn <andrew@lunn.ch> a écrit : > > > > +static void a5psw_get_ethtool_stats(struct dsa_switch *ds, int port, > > > > + uint64_t *data) > > > > +{ > > > > + struct a5psw *a5psw = ds->priv; > > > > + u32 reg_lo, reg_hi; > > > > + unsigned int u; > > > > + > > > > + for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) { > > > > + /* A5PSW_STATS_HIWORD is global and thus, access must be > > > > + * exclusive > > > > + */ > > > > > > Could you explain that a bit more. The RTNL lock will prevent two > > > parallel calls to this function. > > > > Ok, I wasn't sure of the locking applicable here. > > In general, RTNL protects you for any user space management like > operation on the driver. In this case, if you look in net/ethtool, you > will find the IOCTL handler code takes RTNL before calling into the > main IOCTL dispatcher. If you want to be paranoid/document the > assumption, you can add an ASSERT_RTNL(). Ok, I'll look at the call stack in details to see what locking is applied. > > The semantics for some of the other statistics Vladimir requested can > be slightly different. One of them is in atomic context, because a > spinlock is held. But i don't remember if RTNL is also held. This is > less of an issue for your switch, since it uses MMIO, however many > switches need to perform blocking IO over MDIO, SPI, IC2 etc to get > stats, which you cannot do in atomic context. So they end up returning > cached values. > > Look in the mailing list for past discussion for details. Ok, thanks, > > Andrew
diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c index 5bee999f7050..7ab7d9054427 100644 --- a/drivers/net/dsa/rzn1_a5psw.c +++ b/drivers/net/dsa/rzn1_a5psw.c @@ -16,6 +16,59 @@ #include "rzn1_a5psw.h" +struct a5psw_stats { + u16 offset; + const char *name; +}; + +#define STAT_DESC(_offset, _name) {.offset = _offset, .name = _name} + +static const struct a5psw_stats a5psw_stats[] = { + STAT_DESC(0x868, "aFrameTransmitted"), + STAT_DESC(0x86C, "aFrameReceived"), + STAT_DESC(0x870, "aFrameCheckSequenceErrors"), + STAT_DESC(0x874, "aAlignmentErrors"), + STAT_DESC(0x878, "aOctetsTransmitted"), + STAT_DESC(0x87C, "aOctetsReceived"), + STAT_DESC(0x880, "aTxPAUSEMACCtrlFrames"), + STAT_DESC(0x884, "aRxPAUSEMACCtrlFrames"), + /* If */ + STAT_DESC(0x888, "ifInErrors"), + STAT_DESC(0x88C, "ifOutErrors"), + STAT_DESC(0x890, "ifInUcastPkts"), + STAT_DESC(0x894, "ifInMulticastPkts"), + STAT_DESC(0x898, "ifInBroadcastPkts"), + STAT_DESC(0x89C, "ifOutDiscards"), + STAT_DESC(0x8A0, "ifOutUcastPkts"), + STAT_DESC(0x8A4, "ifOutMulticastPkts"), + STAT_DESC(0x8A8, "ifOutBroadcastPkts"), + /* Ether */ + STAT_DESC(0x8AC, "etherStatsDropEvents"), + STAT_DESC(0x8B0, "etherStatsOctets"), + STAT_DESC(0x8B4, "etherStatsPkts"), + STAT_DESC(0x8B8, "etherStatsUndersizePkts"), + STAT_DESC(0x8BC, "etherStatsetherStatsOversizePktsDropEvents"), + STAT_DESC(0x8C0, "etherStatsPkts64Octets"), + STAT_DESC(0x8C4, "etherStatsPkts65to127Octets"), + STAT_DESC(0x8C8, "etherStatsPkts128to255Octets"), + STAT_DESC(0x8CC, "etherStatsPkts256to511Octets"), + STAT_DESC(0x8D0, "etherStatsPkts512to1023Octets"), + STAT_DESC(0x8D4, "etherStatsPkts1024to1518Octets"), + STAT_DESC(0x8D8, "etherStatsPkts1519toXOctets"), + STAT_DESC(0x8DC, "etherStatsJabbers"), + STAT_DESC(0x8E0, "etherStatsFragments"), + + STAT_DESC(0x8E8, "VLANReceived"), + STAT_DESC(0x8EC, "VLANTransmitted"), + + STAT_DESC(0x910, "aDeferred"), + STAT_DESC(0x914, "aMultipleCollisions"), + STAT_DESC(0x918, "aSingleCollisions"), + STAT_DESC(0x91C, "aLateCollisions"), + STAT_DESC(0x920, "aExcessiveCollisions"), + STAT_DESC(0x924, "aCarrierSenseErrors"), +}; + static void a5psw_reg_writel(struct a5psw *a5psw, int offset, u32 value) { writel(value, a5psw->base + offset); @@ -316,6 +369,50 @@ static void a5psw_port_fast_age(struct dsa_switch *ds, int port) a5psw_port_fdb_flush(a5psw, port); } +static void a5psw_get_strings(struct dsa_switch *ds, int port, u32 stringset, + uint8_t *data) +{ + unsigned int u; + + if (stringset != ETH_SS_STATS) + return; + + for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) { + strncpy(data + u * ETH_GSTRING_LEN, a5psw_stats[u].name, + ETH_GSTRING_LEN); + } +} + +static void a5psw_get_ethtool_stats(struct dsa_switch *ds, int port, + uint64_t *data) +{ + struct a5psw *a5psw = ds->priv; + u32 reg_lo, reg_hi; + unsigned int u; + + for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) { + /* A5PSW_STATS_HIWORD is global and thus, access must be + * exclusive + */ + spin_lock(&a5psw->stats_lock); + reg_lo = a5psw_reg_readl(a5psw, a5psw_stats[u].offset + + A5PSW_PORT_OFFSET(port)); + /* A5PSW_STATS_HIWORD is latched on stat read */ + reg_hi = a5psw_reg_readl(a5psw, A5PSW_STATS_HIWORD); + + data[u] = ((u64)reg_hi << 32) | reg_lo; + spin_unlock(&a5psw->stats_lock); + } +} + +static int a5psw_get_sset_count(struct dsa_switch *ds, int port, int sset) +{ + if (sset != ETH_SS_STATS) + return 0; + + return ARRAY_SIZE(a5psw_stats); +} + static int a5psw_setup(struct dsa_switch *ds) { struct a5psw *a5psw = ds->priv; @@ -395,6 +492,9 @@ const struct dsa_switch_ops a5psw_switch_ops = { .phylink_mac_link_up = a5psw_phylink_mac_link_up, .port_change_mtu = a5psw_port_change_mtu, .port_max_mtu = a5psw_port_max_mtu, + .get_sset_count = a5psw_get_sset_count, + .get_strings = a5psw_get_strings, + .get_ethtool_stats = a5psw_get_ethtool_stats, .set_ageing_time = a5psw_set_ageing_time, .port_bridge_join = a5psw_port_bridge_join, .port_bridge_leave = a5psw_port_bridge_leave, @@ -580,6 +680,7 @@ static int a5psw_probe(struct platform_device *pdev) return -ENOMEM; a5psw->dev = dev; + spin_lock_init(&a5psw->stats_lock); spin_lock_init(&a5psw->lk_lock); spin_lock_init(&a5psw->reg_lock); a5psw->base = devm_platform_ioremap_resource(pdev, 0); diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h index 2d96a2afbc3a..b34ea549e936 100644 --- a/drivers/net/dsa/rzn1_a5psw.h +++ b/drivers/net/dsa/rzn1_a5psw.h @@ -177,6 +177,7 @@ * @mdio_freq: MDIO bus frequency requested * @pcs: Array of PCS connected to the switch ports (not for the CPU) * @ds: DSA switch struct + * @stats_lock: lock to access statistics (shared HI counter) * @lk_lock: Lock for the lookup table * @reg_lock: Lock for register read-modify-write operation * @flooding_ports: List of ports that should be flooded @@ -190,6 +191,7 @@ struct a5psw { u32 mdio_freq; struct phylink_pcs *pcs[A5PSW_PORTS_NUM - 1]; struct dsa_switch ds; + spinlock_t stats_lock; spinlock_t lk_lock; spinlock_t reg_lock; u32 flooding_ports;
Add per-port statistics. This support requries to add a stat lock since statistics are stored in two 32 bits registers, the hi part one being global and latched when accessing the lo part. Signed-off-by: Clément Léger <clement.leger@bootlin.com> --- drivers/net/dsa/rzn1_a5psw.c | 101 +++++++++++++++++++++++++++++++++++ drivers/net/dsa/rzn1_a5psw.h | 2 + 2 files changed, 103 insertions(+)