Re: [PATCH net-next 2/3] nfp: add support for link auto negotiation

[Jakub Kicinski <kuba@kernel.org>]

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;
>  }