On Tue, Nov 05, 2024 at 04:26:25PM -0800, Sanman Pradhan wrote:
> Add PCIe hardware statistics support to the fbnic driver. These stats
> provide insight into PCIe transaction performance and error conditions,
> including, read/write and completion TLP counts and DWORD counts and
> debug counters for tag, completion credit and NP credit exhaustion
The second sentence is long and doesn't have a period at the end of
it. Maybe break it up a bit into a set of shorter sentences or a
list or something like that?
> The stats are exposed via ethtool and can be used to monitor PCIe
> performance and debug PCIe issues.
>
> Signed-off-by: Sanman Pradhan <sanman.p211993@xxxxxxxxx>
> ---
> .../device_drivers/ethernet/meta/fbnic.rst | 27 +++++
> drivers/net/ethernet/meta/fbnic/fbnic.h | 1 +
> drivers/net/ethernet/meta/fbnic/fbnic_csr.h | 39 ++++++
> .../net/ethernet/meta/fbnic/fbnic_ethtool.c | 77 +++++++++++-
> .../net/ethernet/meta/fbnic/fbnic_hw_stats.c | 114 ++++++++++++++++++
> .../net/ethernet/meta/fbnic/fbnic_hw_stats.h | 17 +++
> .../net/ethernet/meta/fbnic/fbnic_netdev.c | 3 +
> drivers/net/ethernet/meta/fbnic/fbnic_pci.c | 2 +
> 8 files changed, 278 insertions(+), 2 deletions(-)
[...]
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
> index 1117d5a32867..9f590a42a9df 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
> @@ -6,6 +6,39 @@
[...]
> +
> +#define FBNIC_HW_FIXED_STATS_LEN ARRAY_SIZE(fbnic_gstrings_hw_stats)
> +#define FBNIC_HW_STATS_LEN \
> + (FBNIC_HW_FIXED_STATS_LEN)
Does the above need to be on a separate line? Might be able to fit
in under 80 chars?
> static int
> fbnic_get_ts_info(struct net_device *netdev,
> struct kernel_ethtool_ts_info *tsinfo)
> @@ -51,6 +84,43 @@ static void fbnic_set_counter(u64 *stat, struct fbnic_stat_counter *counter)
> *stat = counter->value;
> }
>
> +static void fbnic_get_strings(struct net_device *dev, u32 sset, u8 *data)
> +{
> + int i;
> +
> + switch (sset) {
> + case ETH_SS_STATS:
> + for (i = 0; i < FBNIC_HW_STATS_LEN; i++)
> + ethtool_puts(&data, fbnic_gstrings_hw_stats[i].string);
> + break;
> + }
> +}
> +
> +static int fbnic_get_sset_count(struct net_device *dev, int sset)
> +{
> + switch (sset) {
> + case ETH_SS_STATS:
> + return FBNIC_HW_STATS_LEN;
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static void fbnic_get_ethtool_stats(struct net_device *dev,
> + struct ethtool_stats *stats, u64 *data)
> +{
> + struct fbnic_net *fbn = netdev_priv(dev);
> + const struct fbnic_stat *stat;
> + int i;
> +
> + fbnic_get_hw_stats(fbn->fbd);
> +
> + for (i = 0; i < FBNIC_HW_STATS_LEN; i++) {
> + stat = &fbnic_gstrings_hw_stats[i];
> + data[i] = *(u64 *)((u8 *)&fbn->fbd->hw_stats + stat->offset);
> + }
> +}
> +
> static void
> fbnic_get_eth_mac_stats(struct net_device *netdev,
> struct ethtool_eth_mac_stats *eth_mac_stats)
> @@ -117,10 +187,13 @@ static void fbnic_get_ts_stats(struct net_device *netdev,
> }
>
> static const struct ethtool_ops fbnic_ethtool_ops = {
> - .get_drvinfo = fbnic_get_drvinfo,
> .get_ts_info = fbnic_get_ts_info,
> - .get_ts_stats = fbnic_get_ts_stats,
> + .get_drvinfo = fbnic_get_drvinfo,
> + .get_strings = fbnic_get_strings,
> + .get_sset_count = fbnic_get_sset_count,
> + .get_ethtool_stats = fbnic_get_ethtool_stats,
> .get_eth_mac_stats = fbnic_get_eth_mac_stats,
> + .get_ts_stats = fbnic_get_ts_stats,
> };
Seems like the two deleted lines were just re-ordered but otherwise
unchanged?
I don't think it's any reason to hold this back, but limiting
changes like that in the future is probably a good idea because it
makes the diff smaller and easier to review.
