| Message ID | 20220530084842.21258-1-simon.horman@corigine.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] nfp: correct the output of `ethtool --show-fec <intf>` | expand |
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
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 >
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 :(
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 */