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

From: Simon Horman <[email-masked]>
Date: Tue, 13 Jun 2023 17:55:11 +0200

On Tue, Jun 13, 2023 at 03:37:32PM +0100, Russell King (Oracle) wrote:

...

> @@ -1149,12 +1159,20 @@ static int phylink_change_inband_advert(struct phylink *pl)

Hi Russell,

A bit further above in this function is the following condition:

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

So it is anticipated that pl->pcs may be NULL,
but the function will continue unless
pl->config->legacy_pre_march2020 is true.

>  		    __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: this indentation is incorrect.

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

But Smatch flags that here pl->pcs is dereferenced unconditionally.

> +		neg_mode = pl->pcs_neg_mode;
> +
>  	/* 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;