[bug report] net: phylink: use legacy_pre_march2020

[bug report] net: phylink: use legacy_pre_march2020

[Dan Carpenter]

Hello Russell King (Oracle),

This is a semi-automatic email about new static checker warnings.

The patch 001f4261fe4d: "net: phylink: use legacy_pre_march2020" from 
Dec 9, 2021, leads to the following Smatch complaint:

    drivers/net/phy/phylink.c:823 phylink_change_inband_advert()
    error: we previously assumed 'pl->pcs_ops' could be null (see line 806)

drivers/net/phy/phylink.c
   805	
   806		if (!pl->pcs_ops && pl->config->legacy_pre_march2020) {
                    ^^^^^^^^^^^^
Should this be an ||?

   807			/* Legacy method */
   808			phylink_mac_config(pl, &pl->link_config);
   809			phylink_mac_pcs_an_restart(pl);
   810			return 0;
   811		}
   812	
   813		phylink_dbg(pl, "%s: mode=%s/%s adv=%*pb pause=%02x\n", __func__,
   814			    phylink_an_mode_str(pl->cur_link_an_mode),
   815			    phy_modes(pl->link_config.interface),
   816			    __ETHTOOL_LINK_MODE_MASK_NBITS, pl->link_config.advertising,
   817			    pl->link_config.pause);
   818	
   819		/* Modern PCS-based method; update the advert at the PCS, and
   820		 * restart negotiation if the pcs_config() helper indicates that
   821		 * the programmed advertisement has changed.
   822		 */
   823		ret = pl->pcs_ops->pcs_config(pl->pcs, pl->cur_link_an_mode,
                      ^^^^^^^^^^^^^
Unchecked dereference.

   824					      pl->link_config.interface,
   825					      pl->link_config.advertising,

regards,
dan carpenter

Re: [bug report] net: phylink: use legacy_pre_march2020

[Russell King (Oracle)]

On Wed, Dec 15, 2021 at 02:35:39PM +0300, Dan Carpenter wrote:
> Hello Russell King (Oracle),
> 
> This is a semi-automatic email about new static checker warnings.
> 
> The patch 001f4261fe4d: "net: phylink: use legacy_pre_march2020" from 
> Dec 9, 2021, leads to the following Smatch complaint:
> 
>     drivers/net/phy/phylink.c:823 phylink_change_inband_advert()
>     error: we previously assumed 'pl->pcs_ops' could be null (see line 806)
> 
> drivers/net/phy/phylink.c
>    805	
>    806		if (!pl->pcs_ops && pl->config->legacy_pre_march2020) {
>                     ^^^^^^^^^^^^
> Should this be an ||?

No, the intention is correct, even though it looks a little weird.

We only call phylink_change_inband_advert() if we are in in-band mode,
and in-band mode for non-legacy users requires a PCS. Consequently, if
legacy_pre_march2020 is clear, we have no PCS, and we reach here, then
something is definitely broken.

There isn't an easy way to detect this condition earlier, but we could
do something like:

	if (WARN_ON(!pl->pcs_ops))
		return -EINVAL;

which should be sufficiently noisy for people to do something about.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!