netdev - Re: [PATCH net-next] eth: fbnic: Add PCIe hardware statistics

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAG4C-On1qMo7+KWDt99A+Lyy29E-qmsxT+gH_5iG2_JpPWR85A@mail.gmail.com>
Date: Wed, 6 Nov 2024 18:14:08 -0800
From: Sanman Pradhan <sanman.p211993@...il.com>
To: Joe Damato <jdamato@...tly.com>, Sanman Pradhan <sanman.p211993@...il.com>, netdev@...r.kernel.org, 
	alexanderduyck@...com, kuba@...nel.org, kernel-team@...a.com, 
	davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com, horms@...nel.org, 
	corbet@....net, mohsin.bashr@...il.com, sanmanpradhan@...a.com, 
	andrew+netdev@...n.ch, vadim.fedorenko@...ux.dev, sdf@...ichev.me, 
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] eth: fbnic: Add PCIe hardware statistics

On Tue, 5 Nov 2024 at 16:49, Joe Damato <jdamato@...tly.com> wrote:
>
> 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@...il.com>
> > ---
> >  .../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?
>
I tried to fit it in under 80 chars, but this seemed to be slightly
more consistent with the out-of-tree driver, so decided to maintain
it.

> >  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.

Yes, I realize that it's just two deleted lines, but I'm trying to
maintain the same order as the order in which ops are defined in the
struct, for readability.
Will try to minimize changes that increase the length of the diffs

Thank you Joe, for reviewing the patch.