[net-next,7/8] ixgbe: add pause frame stats

Report standard pause frame stats. They are already aggregated
in struct ixgbe_hw_stats.

The combination of the registers is suggested as equivalent to
PAUSEMACCtrlFramesTransmitted / PAUSEMACCtrlFramesReceived
by the Intel 82576EB datasheet, I could not find any information
in the HW actually supported by ixgbe.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

On Fri, Sep 11, 2020 at 12:53 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Report standard pause frame stats. They are already aggregated
> in struct ixgbe_hw_stats.
>
> The combination of the registers is suggested as equivalent to
> PAUSEMACCtrlFramesTransmitted / PAUSEMACCtrlFramesReceived
> by the Intel 82576EB datasheet, I could not find any information
> in the HW actually supported by ixgbe.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> index 71ec908266a6..a280aa34ca1d 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> @@ -531,6 +531,16 @@ static int ixgbe_set_link_ksettings(struct net_device *netdev,
>         return err;
>  }
>
> +static void ixgbe_get_pause_stats(struct net_device *netdev,
> +                                 struct ethtool_pause_stats *stats)
> +{
> +       struct ixgbe_adapter *adapter = netdev_priv(netdev);
> +       struct ixgbe_hw_stats *hwstats = &adapter->stats;
> +
> +       stats->tx_pause_frames = hwstats->lxontxc + hwstats->lxofftxc;
> +       stats->rx_pause_frames = hwstats->lxonrxc + hwstats->lxoffrxc;
> +}
> +
>  static void ixgbe_get_pauseparam(struct net_device *netdev,
>                                  struct ethtool_pauseparam *pause)
>  {
> @@ -3546,6 +3556,7 @@ static const struct ethtool_ops ixgbe_ethtool_ops = {
>         .set_eeprom             = ixgbe_set_eeprom,
>         .get_ringparam          = ixgbe_get_ringparam,
>         .set_ringparam          = ixgbe_set_ringparam,
> +       .get_pause_stats        = ixgbe_get_pause_stats,
>         .get_pauseparam         = ixgbe_get_pauseparam,
>         .set_pauseparam         = ixgbe_set_pauseparam,
>         .get_msglevel           = ixgbe_get_msglevel,

So the count for this is simpler in igb than it is for ixgbe. I'm
assuming you want just standard link flow control frames. If so then
this patch is correct. Otherwise if you are wanting to capture
priority flow control data then those are a seperate array of stats
prefixed with a "p" instead of an "l". Otherwise this looks fine to
me.

Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>

On Fri, 11 Sep 2020 14:12:50 -0700 Alexander Duyck wrote:
> On Fri, Sep 11, 2020 at 12:53 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > @@ -3546,6 +3556,7 @@ static const struct ethtool_ops ixgbe_ethtool_ops = {
> >         .set_eeprom             = ixgbe_set_eeprom,
> >         .get_ringparam          = ixgbe_get_ringparam,
> >         .set_ringparam          = ixgbe_set_ringparam,
> > +       .get_pause_stats        = ixgbe_get_pause_stats,
> >         .get_pauseparam         = ixgbe_get_pauseparam,
> >         .set_pauseparam         = ixgbe_set_pauseparam,
> >         .get_msglevel           = ixgbe_get_msglevel,  
> 
> So the count for this is simpler in igb than it is for ixgbe. I'm
> assuming you want just standard link flow control frames. If so then
> this patch is correct. Otherwise if you are wanting to capture
> priority flow control data then those are a seperate array of stats
> prefixed with a "p" instead of an "l". Otherwise this looks fine to
> me.

That's my interpretation, although I haven't found any place the
standard would address this directly. Non-PFC pause has a different
opcode, so I'm reasonably certain this makes sense.

BTW I'm not entirely clear on what "global PFC pause" is either.

Maybe someone can clarify? Mellanox folks?

> Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Thanks!

On Fri, Sep 11, 2020 at 03:13:43PM -0700, Jakub Kicinski wrote:
> On Fri, 11 Sep 2020 14:12:50 -0700 Alexander Duyck wrote:
> > On Fri, Sep 11, 2020 at 12:53 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > @@ -3546,6 +3556,7 @@ static const struct ethtool_ops ixgbe_ethtool_ops = {
> > >         .set_eeprom             = ixgbe_set_eeprom,
> > >         .get_ringparam          = ixgbe_get_ringparam,
> > >         .set_ringparam          = ixgbe_set_ringparam,
> > > +       .get_pause_stats        = ixgbe_get_pause_stats,
> > >         .get_pauseparam         = ixgbe_get_pauseparam,
> > >         .set_pauseparam         = ixgbe_set_pauseparam,
> > >         .get_msglevel           = ixgbe_get_msglevel,  
> > 
> > So the count for this is simpler in igb than it is for ixgbe. I'm
> > assuming you want just standard link flow control frames. If so then
> > this patch is correct. Otherwise if you are wanting to capture
> > priority flow control data then those are a seperate array of stats
> > prefixed with a "p" instead of an "l". Otherwise this looks fine to
> > me.
> 
> That's my interpretation, although I haven't found any place the
> standard would address this directly. Non-PFC pause has a different
> opcode, so I'm reasonably certain this makes sense.
> 
> BTW I'm not entirely clear on what "global PFC pause" is either.
> 
> Maybe someone can clarify? Mellanox folks?

I checked IEEE 802.1Qaz and could not find anything relevant. My only
guess is that it might be a PFC frame with all the priorities set.

Where did you see it?

> 
> > Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> 
> Thanks!
>

On Sun, 13 Sep 2020 12:14:14 +0300 Ido Schimmel wrote:
> On Fri, Sep 11, 2020 at 03:13:43PM -0700, Jakub Kicinski wrote:
> > On Fri, 11 Sep 2020 14:12:50 -0700 Alexander Duyck wrote:  
> > > On Fri, Sep 11, 2020 at 12:53 PM Jakub Kicinski <kuba@kernel.org> wrote:  
> > > > @@ -3546,6 +3556,7 @@ static const struct ethtool_ops ixgbe_ethtool_ops = {
> > > >         .set_eeprom             = ixgbe_set_eeprom,
> > > >         .get_ringparam          = ixgbe_get_ringparam,
> > > >         .set_ringparam          = ixgbe_set_ringparam,
> > > > +       .get_pause_stats        = ixgbe_get_pause_stats,
> > > >         .get_pauseparam         = ixgbe_get_pauseparam,
> > > >         .set_pauseparam         = ixgbe_set_pauseparam,
> > > >         .get_msglevel           = ixgbe_get_msglevel,    
> > > 
> > > So the count for this is simpler in igb than it is for ixgbe. I'm
> > > assuming you want just standard link flow control frames. If so then
> > > this patch is correct. Otherwise if you are wanting to capture
> > > priority flow control data then those are a seperate array of stats
> > > prefixed with a "p" instead of an "l". Otherwise this looks fine to
> > > me.  
> > 
> > That's my interpretation, although I haven't found any place the
> > standard would address this directly. Non-PFC pause has a different
> > opcode, so I'm reasonably certain this makes sense.
> > 
> > BTW I'm not entirely clear on what "global PFC pause" is either.
> > 
> > Maybe someone can clarify? Mellanox folks?  
> 
> I checked IEEE 802.1Qaz and could not find anything relevant. My only
> guess is that it might be a PFC frame with all the priorities set.
> 
> Where did you see it?

I think I saw it in MLX5 and I thought it's something
implementation-specific. But then I noticed 802.1-2018 
has a ieee8021PfcGlobalReqdGroup group.