[PATCH net] nfp: correct the output of `ethtool --show-fec <intf>`

[PATCH net] nfp: correct the output of `ethtool --show-fec <intf>`

[Simon Horman]

From: Yu Xiao <yu.xiao@corigine.com>

The output  of `Configured FEC encodings` should display user
configured/requested value, rather than the NIC supported modes
list.

Before this patch, the output is:
 # ethtool --show-fec <intf>
 FEC parameters for <intf>:
 Configured FEC encodings: Auto Off RS BaseR
 Active FEC encoding: None

With this patch, the corrected output is:
 # ethtool --show-fec <intf>
 FEC parameters for <intf>:
 Configured FEC encodings: Auto
 Active FEC encoding: None

Fixes: 0d0870938337 ("nfp: implement ethtool FEC mode settings")
Signed-off-by: Yu Xiao <yu.xiao@corigine.com>
Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c | 4 +++-
 drivers/net/ethernet/netronome/nfp/nfp_port.c        | 1 +
 drivers/net/ethernet/netronome/nfp/nfp_port.h        | 2 ++
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
index df0afd271a21..115acd4b963b 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
@@ -842,8 +842,8 @@ nfp_port_get_fecparam(struct net_device *netdev,
 	if (!nfp_eth_can_support_fec(eth_port))
 		return 0;
 
-	param->fec = nfp_port_fec_nsp_to_ethtool(eth_port->fec_modes_supported);
 	param->active_fec = nfp_port_fec_nsp_to_ethtool(eth_port->fec);
+	param->fec = port->fec_configured;
 
 	return 0;
 }
@@ -873,6 +873,8 @@ nfp_port_set_fecparam(struct net_device *netdev,
 		/* Only refresh if we did something */
 		nfp_net_refresh_port_table(port);
 
+	port->fec_configured = param->fec;
+
 	return err < 0 ? err : 0;
 }
 
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_port.c b/drivers/net/ethernet/netronome/nfp/nfp_port.c
index 4f2308570dcf..73f7bc8add7f 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_port.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_port.c
@@ -189,6 +189,7 @@ int nfp_port_init_phy_port(struct nfp_pf *pf, struct nfp_app *app,
 
 	port->eth_port = &pf->eth_tbl->ports[id];
 	port->eth_id = pf->eth_tbl->ports[id].index;
+	port->fec_configured = ETHTOOL_FEC_NONE;
 	if (pf->mac_stats_mem)
 		port->eth_stats =
 			pf->mac_stats_mem + port->eth_id * NFP_MAC_STATS_SIZE;
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_port.h b/drivers/net/ethernet/netronome/nfp/nfp_port.h
index d1ebe6c72f7f..fc2dfd2d01be 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_port.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_port.h
@@ -50,6 +50,7 @@ enum nfp_port_flags {
  * @eth_id:	for %NFP_PORT_PHYS_PORT port ID in NFP enumeration scheme
  * @eth_forced:	for %NFP_PORT_PHYS_PORT port is forced UP or DOWN, don't change
  * @eth_port:	for %NFP_PORT_PHYS_PORT translated ETH Table port entry
+ * @fec_configured:	for %NFP_PORT_PHYS_PORT configured FEC encodings
  * @eth_stats:	for %NFP_PORT_PHYS_PORT MAC stats if available
  * @pf_id:	for %NFP_PORT_PF_PORT, %NFP_PORT_VF_PORT ID of the PCI PF (0-3)
  * @vf_id:	for %NFP_PORT_VF_PORT ID of the PCI VF within @pf_id
@@ -75,6 +76,7 @@ struct nfp_port {
 			unsigned int eth_id;
 			bool eth_forced;
 			struct nfp_eth_table_port *eth_port;
+			u32 fec_configured;
 			u8 __iomem *eth_stats;
 		};
 		/* NFP_PORT_PF_PORT, NFP_PORT_VF_PORT */
-- 
2.30.2

Re: [PATCH net] nfp: correct the output of `ethtool --show-fec <intf>`

[Jakub Kicinski]

On Mon, 30 May 2022 10:48:42 +0200 Simon Horman wrote:
> The output  of `Configured FEC encodings` should display user
> configured/requested value,

That stands to reason, but when I checked what all drivers do 7 out 
of 10 upstream drivers at the time used it to report supported modes.
At which point it may be better to change the text in ethtool user
space that try to change the meaning of the field..

> rather than the NIC supported modes list.
> 
> Before this patch, the output is:
>  # ethtool --show-fec <intf>
>  FEC parameters for <intf>:
>  Configured FEC encodings: Auto Off RS BaseR
>  Active FEC encoding: None
> 
> With this patch, the corrected output is:
>  # ethtool --show-fec <intf>
>  FEC parameters for <intf>:
>  Configured FEC encodings: Auto
>  Active FEC encoding: None

Re: [PATCH net] nfp: correct the output of `ethtool --show-fec <intf>`

[Yinjun Zhang]

On Mon, May 30, 2022 at 09:32:32PM -0700, Jakub Kicinski wrote:
> On Mon, 30 May 2022 10:48:42 +0200 Simon Horman wrote:
> > The output  of `Configured FEC encodings` should display user
> > configured/requested value,
> 
> That stands to reason, but when I checked what all drivers do 7 out 
> of 10 upstream drivers at the time used it to report supported modes.

It seems you're right. I agree it's OK that nfp driver keep the same with
majority drivers' implementations.

> At which point it may be better to change the text in ethtool user
> space that try to change the meaning of the field..

To adapt to both implementations, "Supported/Configured FEC encodings"
would be a compromise I think.

> 
> > rather than the NIC supported modes list.
> > 
> > Before this patch, the output is:
> >  # ethtool --show-fec <intf>
> >  FEC parameters for <intf>:
> >  Configured FEC encodings: Auto Off RS BaseR
> >  Active FEC encoding: None
> > 
> > With this patch, the corrected output is:
> >  # ethtool --show-fec <intf>
> >  FEC parameters for <intf>:
> >  Configured FEC encodings: Auto
> >  Active FEC encoding: None
> 

Re: [PATCH net] nfp: correct the output of `ethtool --show-fec <intf>`

[Jakub Kicinski]

On Wed, 1 Jun 2022 09:48:25 +0800 Yinjun Zhang wrote:
> On Mon, May 30, 2022 at 09:32:32PM -0700, Jakub Kicinski wrote:
> > On Mon, 30 May 2022 10:48:42 +0200 Simon Horman wrote:  
> > > The output  of `Configured FEC encodings` should display user
> > > configured/requested value,  
> > 
> > That stands to reason, but when I checked what all drivers do 7 out 
> > of 10 upstream drivers at the time used it to report supported modes.  
> 
> It seems you're right. I agree it's OK that nfp driver keep the same with
> majority drivers' implementations.

FWIW this is what I found in my notes:

get:
 s - supported
 c - configured
set:
 1 - single mode
 m - multiple modes

      nfp | ionic | lio | cxgb4 | hns3 | i40e | ice | mlx5 | qede | sfc | bnxt
-----------------------------------------------------------------------------
get |  s  |   s   |  s  |   s   |  s   |  c?  |  s  |  c?  |  s   |  c? |
set |  1  |   1   |  1  |   m   |  m   |  1   |  1  |  m?  |  m   |  m  |

I don't know how accurate that was, hard to tell those things 
by looking only at the kernel driver.

> > At which point it may be better to change the text in ethtool user
> > space that try to change the meaning of the field..  
> 
> To adapt to both implementations, "Supported/Configured FEC encodings"
> would be a compromise I think.

Yup, it should help avoid bug reports. I don't have better ideas :(