| Message ID | 1388854031-24142-6-git-send-email-sd@queasysnail.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sat, 2014-01-04 at 17:47 +0100, Sabrina Dubroca wrote: > + __alx_update_hw_stats(hw); > + BUILD_BUG_ON(sizeof(hw->stats) - offsetof(struct alx_hw_stats, rx_ok) < > + ALX_NUM_STATS * sizeof(u64)); I think you should make that != instead of <, otherwise you won't catch all possible differences. johannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[2014-01-06, 10:03:10] Johannes Berg wrote: > On Sat, 2014-01-04 at 17:47 +0100, Sabrina Dubroca wrote: > > > + __alx_update_hw_stats(hw); > > + BUILD_BUG_ON(sizeof(hw->stats) - offsetof(struct alx_hw_stats, rx_ok) < > > + ALX_NUM_STATS * sizeof(u64)); > > I think you should make that != instead of <, otherwise you won't catch > all possible differences. With a !=, BUILD_BUG_ON is triggered if a new field is added at the end of the structure. But adding a field doesn't break the code, though I'm not sure allowing this is useful. The offsetof (and the source in memcpy) also allows to add new fields at the beginning. And the way alx_update_hw_stats is written already includes a kind of check that all fields are present.
On Mon, 2014-01-06 at 11:57 +0100, Sabrina Dubroca wrote: > [2014-01-06, 10:03:10] Johannes Berg wrote: > > On Sat, 2014-01-04 at 17:47 +0100, Sabrina Dubroca wrote: > > > > > + __alx_update_hw_stats(hw); > > > + BUILD_BUG_ON(sizeof(hw->stats) - offsetof(struct alx_hw_stats, rx_ok) < > > > + ALX_NUM_STATS * sizeof(u64)); > > > > I think you should make that != instead of <, otherwise you won't catch > > all possible differences. > > With a !=, BUILD_BUG_ON is triggered if a new field is added at the > end of the structure. That seems reasonable, you'd want to export that field as well? Fields that shouldn't be exported could be added before rx_ok. > But adding a field doesn't break the code, > though I'm not sure allowing this is useful. The offsetof (and the > source in memcpy) also allows to add new fields at the beginning. > > And the way alx_update_hw_stats is written already includes a kind of > check that all fields are present. I was more worried about type mismatches. That's not really a concern with u64 since that's the largest type that really makes sense here, but if the type of some variables changed vs. the ethtool type u64... Maybe I'm overly worried. It seems likely nobody will ever touch this code again :) johannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2014-01-06, 15:11:36 +0100, Johannes Berg wrote: > On Mon, 2014-01-06 at 11:57 +0100, Sabrina Dubroca wrote: > > [2014-01-06, 10:03:10] Johannes Berg wrote: > > > On Sat, 2014-01-04 at 17:47 +0100, Sabrina Dubroca wrote: > > > > > > > + __alx_update_hw_stats(hw); > > > > + BUILD_BUG_ON(sizeof(hw->stats) - offsetof(struct alx_hw_stats, rx_ok) < > > > > + ALX_NUM_STATS * sizeof(u64)); > > > > > > I think you should make that != instead of <, otherwise you won't catch > > > all possible differences. > > > > With a !=, BUILD_BUG_ON is triggered if a new field is added at the > > end of the structure. > > That seems reasonable, you'd want to export that field as well? Fields > that shouldn't be exported could be added before rx_ok. I don't have anything else to add here, I'm just considering what others might want to do. Maybe overthinking. But, yeah, that's the idea. (and fields added to the end of the struct but that don't have a description in alx_gstrings_stats are ignored) > > But adding a field doesn't break the code, > > though I'm not sure allowing this is useful. The offsetof (and the > > source in memcpy) also allows to add new fields at the beginning. > > > > And the way alx_update_hw_stats is written already includes a kind of > > check that all fields are present. > > I was more worried about type mismatches. That's not really a concern > with u64 since that's the largest type that really makes sense here, but > if the type of some variables changed vs. the ethtool type u64... Add "all stats fields must be u64" to the comment before the struct? > Maybe I'm overly worried. It seems likely nobody will ever touch this > code again :) :)
On Mon, 2014-01-06 at 15:56 +0100, Sabrina Dubroca wrote: > > I was more worried about type mismatches. That's not really a concern > > with u64 since that's the largest type that really makes sense here, but > > if the type of some variables changed vs. the ethtool type u64... > > Add "all stats fields must be u64" to the comment before the struct? I guess that's OK. I can live with either of these solutions really, I have no plans to change this driver ever again (though I might need WOL soon ... *sigh*) johannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/atheros/alx/ethtool.c b/drivers/net/ethernet/atheros/alx/ethtool.c index 45b3650..62aea08 100644 --- a/drivers/net/ethernet/atheros/alx/ethtool.c +++ b/drivers/net/ethernet/atheros/alx/ethtool.c @@ -46,6 +46,66 @@ #include "reg.h" #include "hw.h" +/* The order of these strings must match the order of the fields in + * struct alx_hw_stats + * See hw.h + */ +static const char alx_gstrings_stats[][ETH_GSTRING_LEN] = { + "rx_packets", + "rx_bcast_packets", + "rx_mcast_packets", + "rx_pause_packets", + "rx_ctrl_packets", + "rx_fcs_errors", + "rx_length_errors", + "rx_bytes", + "rx_runt_packets", + "rx_fragments", + "rx_64B_or_less_packets", + "rx_65B_to_127B_packets", + "rx_128B_to_255B_packets", + "rx_256B_to_511B_packets", + "rx_512B_to_1023B_packets", + "rx_1024B_to_1518B_packets", + "rx_1519B_to_mtu_packets", + "rx_oversize_packets", + "rx_rxf_ov_drop_packets", + "rx_rrd_ov_drop_packets", + "rx_align_errors", + "rx_bcast_bytes", + "rx_mcast_bytes", + "rx_address_errors", + "tx_packets", + "tx_bcast_packets", + "tx_mcast_packets", + "tx_pause_packets", + "tx_exc_defer_packets", + "tx_ctrl_packets", + "tx_defer_packets", + "tx_bytes", + "tx_64B_or_less_packets", + "tx_65B_to_127B_packets", + "tx_128B_to_255B_packets", + "tx_256B_to_511B_packets", + "tx_512B_to_1023B_packets", + "tx_1024B_to_1518B_packets", + "tx_1519B_to_mtu_packets", + "tx_single_collision", + "tx_multiple_collisions", + "tx_late_collision", + "tx_abort_collision", + "tx_underrun", + "tx_trd_eop", + "tx_length_errors", + "tx_trunc_packets", + "tx_bcast_bytes", + "tx_mcast_bytes", + "tx_update", +}; + +#define ALX_NUM_STATS ARRAY_SIZE(alx_gstrings_stats) + + static u32 alx_get_supported_speeds(struct alx_hw *hw) { u32 supported = SUPPORTED_10baseT_Half | @@ -201,6 +261,44 @@ static void alx_set_msglevel(struct net_device *netdev, u32 data) alx->msg_enable = data; } +static void alx_get_ethtool_stats(struct net_device *netdev, + struct ethtool_stats *estats, u64 *data) +{ + struct alx_priv *alx = netdev_priv(netdev); + struct alx_hw *hw = &alx->hw; + + spin_lock(&alx->stats_lock); + + __alx_update_hw_stats(hw); + BUILD_BUG_ON(sizeof(hw->stats) - offsetof(struct alx_hw_stats, rx_ok) < + ALX_NUM_STATS * sizeof(u64)); + memcpy(data, &hw->stats.rx_ok, ALX_NUM_STATS * sizeof(u64)); + + spin_unlock(&alx->stats_lock); +} + +static void alx_get_strings(struct net_device *netdev, u32 stringset, u8 *buf) +{ + switch (stringset) { + case ETH_SS_STATS: + memcpy(buf, &alx_gstrings_stats, sizeof(alx_gstrings_stats)); + break; + default: + WARN_ON(1); + break; + } +} + +static int alx_get_sset_count(struct net_device *netdev, int sset) +{ + switch (sset) { + case ETH_SS_STATS: + return ALX_NUM_STATS; + default: + return -EINVAL; + } +} + const struct ethtool_ops alx_ethtool_ops = { .get_settings = alx_get_settings, .set_settings = alx_set_settings, @@ -209,4 +307,7 @@ const struct ethtool_ops alx_ethtool_ops = { .get_msglevel = alx_get_msglevel, .set_msglevel = alx_set_msglevel, .get_link = ethtool_op_get_link, + .get_strings = alx_get_strings, + .get_sset_count = alx_get_sset_count, + .get_ethtool_stats = alx_get_ethtool_stats, }; diff --git a/drivers/net/ethernet/atheros/alx/hw.h b/drivers/net/ethernet/atheros/alx/hw.h index 3780773..de0bdaf 100644 --- a/drivers/net/ethernet/atheros/alx/hw.h +++ b/drivers/net/ethernet/atheros/alx/hw.h @@ -381,7 +381,11 @@ struct alx_rrd { ALX_ISR_RX_Q6 | \ ALX_ISR_RX_Q7) -/* Statistics counters collected by the MAC */ +/* Statistics counters collected by the MAC + * + * The order of the fields must match the strings in alx_gstrings_stats + * See ethtool.c + */ struct alx_hw_stats { /* rx */ u64 rx_ok;
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> --- drivers/net/ethernet/atheros/alx/ethtool.c | 101 +++++++++++++++++++++++++++++ drivers/net/ethernet/atheros/alx/hw.h | 6 +- 2 files changed, 106 insertions(+), 1 deletion(-)