Re: [PATCH net-next 01/15] net: phylink: add PCS negotiation mode

[Simon Horman <simon.horman@corigine.com>]

On Fri, Jun 16, 2023 at 01:06:22PM +0100, Russell King (Oracle) wrote:

Hi Russell,

some minor feedback from my side.

> @@ -1149,12 +1159,20 @@ static int phylink_change_inband_advert(struct phylink *pl)
>  		    __ETHTOOL_LINK_MODE_MASK_NBITS, pl->link_config.advertising,
>  		    pl->link_config.pause);
>  
> +	/* Recompute the PCS neg mode */
> +	pl->pcs_neg_mode = phylink_pcs_neg_mode(pl->cur_link_an_mode,
> +					pl->link_config.interface,
> +					pl->link_config.advertising);

nit: the indentation of the above two lines seems off.

> +
> +	neg_mode = pl->cur_link_an_mode;
> +	if (pl->pcs->neg_mode)
> +		neg_mode = pl->pcs_neg_mode;
> +

Smatch is unhappy that previously it was thought that
pl->pcs could be NULL.

I assume it is taking into account the following, which appears slightly
above this hunk:

	if (!pl->pcs && pl->config->legacy_pre_march2020) {
		...
		return 0;
	}

Could it be the case that pl->pcs is NULL and
pl->config->legacy_pre_march2020 is false?



>  	/* Modern PCS-based method; update the advert at the PCS, and
>  	 * restart negotiation if the pcs_config() helper indicates that
>  	 * the programmed advertisement has changed.
>  	 */
> -	ret = phylink_pcs_config(pl->pcs, pl->cur_link_an_mode,
> -				 &pl->link_config,
> +	ret = phylink_pcs_config(pl->pcs, neg_mode, &pl->link_config,
>  				 !!(pl->link_config.pause & MLO_PAUSE_AN));
>  	if (ret < 0)
>  		return ret;

...