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

From: Simon Horman <simon.horman@corigine.com>
To: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
Cc: Andrew Lunn <andrew@lunn.ch>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Alexander Couzens <lynxis@fe80.eu>,
	AngeloGioacchino Del Regno
	<angelogioacchino.delregno@collabora.com>,
	Cc@web.codeaurora.org:  Claudiu Beznea
	<claudiu.beznea@microchip.com>,
	Daniel Golle <daniel@makrotopia.org>,
	Daniel Machon <daniel.machon@microchip.com>,
	"David S. Miller" <davem@davemloft.net>,
	DENG Qingfang <dqfext@gmail.com>,
	Eric Dumazet <edumazet@google.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Horatiu Vultur <horatiu.vultur@microchip.com>,
	Ioana Ciornei <ioana.ciornei@nxp.com>,
	Jakub Kicinski <kuba@kernel.org>,
	Jose Abreu <Jose.Abreu@synopsys.com>,
	Landen Chao <Landen.Chao@mediatek.com>,
	Lars Povlsen <lars.povlsen@microchip.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org,
	Madalin Bucur <madalin.bucur@nxp.com>,
	Marcin Wojtas <mw@semihalf.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Michal Simek <michal.simek@amd.com>,
	netdev@vger.kernel.org,
	Nicolas Ferre <nicolas.ferre@microchip.com>,
	Paolo Abeni <pabeni@redhat.com>,
	Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>,
	Sean Anderson <sean.anderson@seco.com>,
	Sean Wang <sean.wang@mediatek.com>,
	Steen Hegelund <Steen.Hegelund@microchip.com>,
	Taras Chornyi <taras.chornyi@plvision.eu>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	UNGLinuxDriver@microchip.com, Vladimir Oltean <olteanv@gmail.com>
Subject: Re: [PATCH net-next 01/15] net: phylink: add PCS negotiation mode
Date: Fri, 16 Jun 2023 17:51:06 +0200	[thread overview]
Message-ID: <ZIyE6m3f4ToAf9xg@corigine.com> (raw)
In-Reply-To: <E1qA8De-00EaFA-Ht@rmk-PC.armlinux.org.uk>

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;

...