| Message ID | 20220921121235.169761-3-simon.horman@corigine.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | nfp: support FEC mode reporting and auto-neg | expand |
On Wed, 21 Sep 2022 14:12:34 +0200 Simon Horman wrote: > From: Yinjun Zhang <yinjun.zhang@corigine.com> > > Report the auto negotiation capability if it's supported > in management firmware, and advertise it if it's enabled. > > Changing FEC to mode other than auto or changing port > speed is not allowed when autoneg is enabled. And FEC mode > is enforced into auto mode when enabling link autoneg. > > The ethtool <intf> command displays the auto-neg capability: > if (cmd->base.speed != SPEED_UNKNOWN) { > - u32 speed = cmd->base.speed / eth_port->lanes; > + if (req_aneg) { > + netdev_err(netdev, "Speed changing is not allowed when working on autoneg mode.\n"); > + err = -EINVAL; > + goto err_bad_set; > + } else { > + u32 speed = cmd->base.speed / eth_port->lanes; > > - err = __nfp_eth_set_speed(nsp, speed); > + err = __nfp_eth_set_speed(nsp, speed); > + if (err) > + goto err_bad_set; > + } Please refactor this to avoid the extra indentation > + } > + > + if (req_aneg && nfp_eth_can_support_fec(eth_port) && eth_port->fec != NFP_FEC_AUTO_BIT) { > + err = __nfp_eth_set_fec(nsp, NFP_FEC_AUTO_BIT); > if (err) > goto err_bad_set; > + if (eth_port->supp_aneg && eth_port->aneg == NFP_ANEG_AUTO && fec != NFP_FEC_AUTO_BIT) { > + netdev_err(netdev, "Only auto mode is allowed when link autoneg is enabled.\n"); > + return -EINVAL; > + } Autoneg and AUTO fec are two completely different things. There was a long thread on AUTO recently.. :( > snprintf(hwinfo, sizeof(hwinfo), "sp_indiff=%d", sp_indiff); > err = nfp_nsp_hwinfo_set(nsp, hwinfo, sizeof(hwinfo)); > - if (err) > + if (err) { > + /* Not a fatal error, no need to return error to stop driver from loading */ > nfp_warn(pf->cpp, "HWinfo(sp_indiff=%d) set failed: %d\n", sp_indiff, err); > + err = 0; This should be a separate commit, it seems > > nfp_nsp_close(nsp); > return err; > @@ -331,7 +334,23 @@ static int nfp_net_pf_cfg_nsp(struct nfp_pf *pf, bool sp_indiff) > > static int nfp_net_pf_init_nsp(struct nfp_pf *pf) > { > - return nfp_net_pf_cfg_nsp(pf, pf->sp_indiff); > + int err; > + > + err = nfp_net_pf_cfg_nsp(pf, pf->sp_indiff); > + if (!err) { > + struct nfp_port *port; > + > + /* The eth ports need be refreshed after nsp is configured, > + * since the eth table state may change, e.g. aneg_supp field. No idea why, tho > + * Only `CHANGED` bit is set here in case nsp needs some time > + * to process the configuration. I can't parse what this is saying but doesn't look good > + */ > + list_for_each_entry(port, &pf->ports, port_list) > + if (__nfp_port_get_eth_port(port)) > + set_bit(NFP_PORT_CHANGED, &port->flags); > + } > + > + return err; > }
On Thu, 22 Sep 2022 18:00:40 -0700 Jakub Kicinski wrote: > On Wed, 21 Sep 2022 14:12:34 +0200 Simon Horman wrote: > > From: Yinjun Zhang <yinjun.zhang@corigine.com> > > > > Report the auto negotiation capability if it's supported > > in management firmware, and advertise it if it's enabled. > > > > Changing FEC to mode other than auto or changing port > > speed is not allowed when autoneg is enabled. And FEC mode > > is enforced into auto mode when enabling link autoneg. > > > > The ethtool <intf> command displays the auto-neg capability: > > > + goto err_bad_set; > > + } > > Please refactor this to avoid the extra indentation Will do. > > > + if (eth_port->supp_aneg && eth_port->aneg == NFP_ANEG_AUTO && > fec != NFP_FEC_AUTO_BIT) { > > + netdev_err(netdev, "Only auto mode is allowed when link > autoneg is enabled.\n"); > > + return -EINVAL; > > + } > > Autoneg and AUTO fec are two completely different things. > There was a long thread on AUTO recently.. :( Yes, you're right, will remove this limitation. > > > + /* Not a fatal error, no need to return error to stop driver > from loading */ > > nfp_warn(pf->cpp, "HWinfo(sp_indiff=%d) set failed: %d\n", > sp_indiff, err); > > + err = 0; > > This should be a separate commit, it seems Will separate it. > > > > > nfp_nsp_close(nsp); > > return err; > > @@ -331,7 +334,23 @@ static int nfp_net_pf_cfg_nsp(struct nfp_pf *pf, > bool sp_indiff) > > > > static int nfp_net_pf_init_nsp(struct nfp_pf *pf) > > { > > - return nfp_net_pf_cfg_nsp(pf, pf->sp_indiff); > > + int err; > > + > > + err = nfp_net_pf_cfg_nsp(pf, pf->sp_indiff); > > + if (!err) { > > + struct nfp_port *port; > > + > > + /* The eth ports need be refreshed after nsp is configured, > > + * since the eth table state may change, e.g. aneg_supp field. > > No idea why, tho > > > + * Only `CHANGED` bit is set here in case nsp needs some > time > > + * to process the configuration. > > I can't parse what this is saying but doesn't look good I think this comment is clear enough. In previous ` nfp_net_pf_cfg_nsp`, hwinfo "sp_indiff" is configured into Management firmware(NSP), and it decides if autoneg is supported or not and updates eth table accordingly. And only `CHANGED` flag is set here so that with some delay driver can get the updated eth table instead of stale info. > > > + */ > > + list_for_each_entry(port, &pf->ports, port_list) > > + if (__nfp_port_get_eth_port(port)) > > + set_bit(NFP_PORT_CHANGED, &port->flags); > > + } > > + > > + return err; > > }
On Fri, 23 Sep 2022 04:37:58 +0000 Yinjun Zhang wrote: > > I can't parse what this is saying but doesn't look good > > I think this comment is clear enough. In previous ` > nfp_net_pf_cfg_nsp`, hwinfo "sp_indiff" is configured into Management > firmware(NSP), and it decides if autoneg is supported or not and > updates eth table accordingly. And only `CHANGED` flag is set here so > that with some delay driver can get the updated eth table instead of > stale info. Why is the sp_indif thing configured at the nfp_main layer, before the eth table is read? Doing this inside nfp_net_main seems like the wrong layering to me.
On Fri, Sep 23, 2022 at 06:21:14AM -0700, Jakub Kicinski wrote: > On Fri, 23 Sep 2022 04:37:58 +0000 Yinjun Zhang wrote: > > > I can't parse what this is saying but doesn't look good > > > > I think this comment is clear enough. In previous ` > > nfp_net_pf_cfg_nsp`, hwinfo "sp_indiff" is configured into Management > > firmware(NSP), and it decides if autoneg is supported or not and > > updates eth table accordingly. And only `CHANGED` flag is set here so > > that with some delay driver can get the updated eth table instead of > > stale info. > > Why is the sp_indif thing configured at the nfp_main layer, before > the eth table is read? Doing this inside nfp_net_main seems like > the wrong layering to me. Because the value of sp_indiff depends on the loaded application firmware, please ref to previous commit: 2b88354d37ca ("nfp: check if application firmware is indifferent to port speed")
On Fri, 23 Sep 2022 23:41:57 +0800 Yinjun Zhang wrote: > > Why is the sp_indif thing configured at the nfp_main layer, before > > the eth table is read? Doing this inside nfp_net_main seems like > > the wrong layering to me. > > Because the value of sp_indiff depends on the loaded application > firmware, please ref to previous commit: > 2b88354d37ca ("nfp: check if application firmware is indifferent to port speed") AFAICT you check if it's flower, you can check that in the main code, the app id is just a symbol, right?
On Fri, Sep 23, 2022 at 05:24:10PM -0700, Jakub Kicinski wrote: > On Fri, 23 Sep 2022 23:41:57 +0800 Yinjun Zhang wrote: > > > Why is the sp_indif thing configured at the nfp_main layer, before > > > the eth table is read? Doing this inside nfp_net_main seems like > > > the wrong layering to me. > > > > Because the value of sp_indiff depends on the loaded application > > firmware, please ref to previous commit: > > 2b88354d37ca ("nfp: check if application firmware is indifferent to port speed") > > AFAICT you check if it's flower, you can check that in the main code, > the app id is just a symbol, right? Not only check if it's flower, but also check if it's sp_indiff when it's not flower by parsing the tlv caps.
On Sat, 24 Sep 2022 10:45:30 +0800 Yinjun Zhang wrote: > On Fri, Sep 23, 2022 at 05:24:10PM -0700, Jakub Kicinski wrote: > > > Because the value of sp_indiff depends on the loaded application > > > firmware, please ref to previous commit: > > > 2b88354d37ca ("nfp: check if application firmware is indifferent to port speed") > > > > AFAICT you check if it's flower, you can check that in the main code, > > the app id is just a symbol, right? > > Not only check if it's flower, but also check if it's sp_indiff when > it's not flower by parsing the tlv caps. Seems bogus. The speed independence is a property of the whole FW image, you record it in the pf structure.
On Mon, Sep 26, 2022 at 09:25:47AM -0700, Jakub Kicinski wrote: > On Sat, 24 Sep 2022 10:45:30 +0800 Yinjun Zhang wrote: > > On Fri, Sep 23, 2022 at 05:24:10PM -0700, Jakub Kicinski wrote: > > > > Because the value of sp_indiff depends on the loaded application > > > > firmware, please ref to previous commit: > > > > 2b88354d37ca ("nfp: check if application firmware is indifferent to port speed") > > > > > > AFAICT you check if it's flower, you can check that in the main code, > > > the app id is just a symbol, right? > > > > Not only check if it's flower, but also check if it's sp_indiff when > > it's not flower by parsing the tlv caps. > > Seems bogus. The speed independence is a property of the whole FW image, > you record it in the pf structure. It's indeed a per-fw property, but we don't have existing way to expose per-fw capabilities to driver currently, so use per-vnic tlv caps here. Maybe define a new fw symbol is a choice, but my concern is it's not visible to netvf driver. Any suggestion is welcomed.
On Tue, 27 Sep 2022 09:13:53 +0800 Yinjun Zhang wrote: > On Mon, Sep 26, 2022 at 09:25:47AM -0700, Jakub Kicinski wrote: > > On Sat, 24 Sep 2022 10:45:30 +0800 Yinjun Zhang wrote: > > > Not only check if it's flower, but also check if it's sp_indiff when > > > it's not flower by parsing the tlv caps. > > > > Seems bogus. The speed independence is a property of the whole FW image, > > you record it in the pf structure. > > It's indeed a per-fw property, but we don't have existing way to expose > per-fw capabilities to driver currently, so use per-vnic tlv caps here. > Maybe define a new fw symbol is a choice, but my concern is it's not > visible to netvf driver. > Any suggestion is welcomed. Why not put an rtsym with the value in the FW? That'd be my first go-to way of communicating information about the FW as a whole.
On Mon, Sep 26, 2022 at 06:38:40PM -0700, Jakub Kicinski wrote: > On Tue, 27 Sep 2022 09:13:53 +0800 Yinjun Zhang wrote: > > On Mon, Sep 26, 2022 at 09:25:47AM -0700, Jakub Kicinski wrote: > > > On Sat, 24 Sep 2022 10:45:30 +0800 Yinjun Zhang wrote: > > > > Not only check if it's flower, but also check if it's sp_indiff when > > > > it's not flower by parsing the tlv caps. > > > > > > Seems bogus. The speed independence is a property of the whole FW image, > > > you record it in the pf structure. > > > > It's indeed a per-fw property, but we don't have existing way to expose > > per-fw capabilities to driver currently, so use per-vnic tlv caps here. > > Maybe define a new fw symbol is a choice, but my concern is it's not > > visible to netvf driver. > > Any suggestion is welcomed. > > Why not put an rtsym with the value in the FW? That'd be my first > go-to way of communicating information about the FW as a whole. Like I said, the VF driver cannot read the rtsym, so it's not a perfect way as exposing a per-FW property. But for this case, it's OK since VF doesn't need this info. I'll try this way. Thanks.
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c index d50af23642a2..00aacc48a7a2 100644 --- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c @@ -290,8 +290,13 @@ nfp_net_get_link_ksettings(struct net_device *netdev, if (eth_port) { ethtool_link_ksettings_add_link_mode(cmd, supported, Pause); ethtool_link_ksettings_add_link_mode(cmd, advertising, Pause); - cmd->base.autoneg = eth_port->aneg != NFP_ANEG_DISABLED ? - AUTONEG_ENABLE : AUTONEG_DISABLE; + if (eth_port->supp_aneg) { + ethtool_link_ksettings_add_link_mode(cmd, supported, Autoneg); + if (eth_port->aneg == NFP_ANEG_AUTO) { + ethtool_link_ksettings_add_link_mode(cmd, advertising, Autoneg); + cmd->base.autoneg = AUTONEG_ENABLE; + } + } nfp_net_set_fec_link_mode(eth_port, cmd); } @@ -327,6 +332,7 @@ static int nfp_net_set_link_ksettings(struct net_device *netdev, const struct ethtool_link_ksettings *cmd) { + bool req_aneg = (cmd->base.autoneg == AUTONEG_ENABLE); struct nfp_eth_table_port *eth_port; struct nfp_port *port; struct nfp_nsp *nsp; @@ -346,16 +352,36 @@ nfp_net_set_link_ksettings(struct net_device *netdev, if (IS_ERR(nsp)) return PTR_ERR(nsp); - err = __nfp_eth_set_aneg(nsp, cmd->base.autoneg == AUTONEG_ENABLE ? - NFP_ANEG_AUTO : NFP_ANEG_DISABLED); + if (req_aneg && !eth_port->supp_aneg) { + netdev_warn(netdev, "Autoneg is not supported.\n"); + err = -EOPNOTSUPP; + goto err_bad_set; + } + + err = __nfp_eth_set_aneg(nsp, req_aneg ? NFP_ANEG_AUTO : NFP_ANEG_DISABLED); if (err) goto err_bad_set; + if (cmd->base.speed != SPEED_UNKNOWN) { - u32 speed = cmd->base.speed / eth_port->lanes; + if (req_aneg) { + netdev_err(netdev, "Speed changing is not allowed when working on autoneg mode.\n"); + err = -EINVAL; + goto err_bad_set; + } else { + u32 speed = cmd->base.speed / eth_port->lanes; - err = __nfp_eth_set_speed(nsp, speed); + err = __nfp_eth_set_speed(nsp, speed); + if (err) + goto err_bad_set; + } + } + + if (req_aneg && nfp_eth_can_support_fec(eth_port) && eth_port->fec != NFP_FEC_AUTO_BIT) { + err = __nfp_eth_set_fec(nsp, NFP_FEC_AUTO_BIT); if (err) goto err_bad_set; + + netdev_info(netdev, "FEC is enforced into auto mode when autoneg is enabled.\n"); } err = nfp_eth_config_commit_end(nsp); @@ -1021,6 +1047,11 @@ nfp_port_set_fecparam(struct net_device *netdev, if (fec < 0) return fec; + if (eth_port->supp_aneg && eth_port->aneg == NFP_ANEG_AUTO && fec != NFP_FEC_AUTO_BIT) { + netdev_err(netdev, "Only auto mode is allowed when link autoneg is enabled.\n"); + return -EINVAL; + } + err = nfp_eth_set_fec(port->app->cpp, eth_port->index, fec); if (!err) /* Only refresh if we did something */ diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c index e2d4c487e8de..2c0279dcf299 100644 --- a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c @@ -322,8 +322,11 @@ static int nfp_net_pf_cfg_nsp(struct nfp_pf *pf, bool sp_indiff) snprintf(hwinfo, sizeof(hwinfo), "sp_indiff=%d", sp_indiff); err = nfp_nsp_hwinfo_set(nsp, hwinfo, sizeof(hwinfo)); - if (err) + if (err) { + /* Not a fatal error, no need to return error to stop driver from loading */ nfp_warn(pf->cpp, "HWinfo(sp_indiff=%d) set failed: %d\n", sp_indiff, err); + err = 0; + } nfp_nsp_close(nsp); return err; @@ -331,7 +334,23 @@ static int nfp_net_pf_cfg_nsp(struct nfp_pf *pf, bool sp_indiff) static int nfp_net_pf_init_nsp(struct nfp_pf *pf) { - return nfp_net_pf_cfg_nsp(pf, pf->sp_indiff); + int err; + + err = nfp_net_pf_cfg_nsp(pf, pf->sp_indiff); + if (!err) { + struct nfp_port *port; + + /* The eth ports need be refreshed after nsp is configured, + * since the eth table state may change, e.g. aneg_supp field. + * Only `CHANGED` bit is set here in case nsp needs some time + * to process the configuration. + */ + list_for_each_entry(port, &pf->ports, port_list) + if (__nfp_port_get_eth_port(port)) + set_bit(NFP_PORT_CHANGED, &port->flags); + } + + return err; } static void nfp_net_pf_clean_nsp(struct nfp_pf *pf) diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.h b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.h index 52465670a01e..e045b6fb5fde 100644 --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.h +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.h @@ -174,6 +174,7 @@ struct nfp_eth_table { bool enabled; bool tx_enabled; bool rx_enabled; + bool supp_aneg; bool override_changed; @@ -218,6 +219,7 @@ void nfp_eth_config_cleanup_end(struct nfp_nsp *nsp); int __nfp_eth_set_aneg(struct nfp_nsp *nsp, enum nfp_eth_aneg mode); int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int speed); int __nfp_eth_set_split(struct nfp_nsp *nsp, unsigned int lanes); +int __nfp_eth_set_fec(struct nfp_nsp *nsp, enum nfp_eth_fec mode); /** * struct nfp_nsp_identify - NSP static information diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c index 18ba7629cdc2..8084d52ade46 100644 --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c @@ -27,6 +27,7 @@ #define NSP_ETH_PORT_PHYLABEL GENMASK_ULL(59, 54) #define NSP_ETH_PORT_FEC_SUPP_BASER BIT_ULL(60) #define NSP_ETH_PORT_FEC_SUPP_RS BIT_ULL(61) +#define NSP_ETH_PORT_SUPP_ANEG BIT_ULL(63) #define NSP_ETH_PORT_LANES_MASK cpu_to_le64(NSP_ETH_PORT_LANES) @@ -178,6 +179,7 @@ nfp_eth_port_translate(struct nfp_nsp *nsp, const union eth_table_entry *src, return; dst->act_fec = FIELD_GET(NSP_ETH_STATE_ACT_FEC, state); + dst->supp_aneg = FIELD_GET(NSP_ETH_PORT_SUPP_ANEG, port); } static void @@ -564,7 +566,7 @@ int __nfp_eth_set_aneg(struct nfp_nsp *nsp, enum nfp_eth_aneg mode) * * Return: 0 or -ERRNO. */ -static int __nfp_eth_set_fec(struct nfp_nsp *nsp, enum nfp_eth_fec mode) +int __nfp_eth_set_fec(struct nfp_nsp *nsp, enum nfp_eth_fec mode) { return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_STATE, NSP_ETH_STATE_FEC, mode,