Re: [PATCH net-next 1/6] net: dsa: vsc73xx: convert to PHYLINK

[Simon Horman]

On Wed, Jun 21, 2023 at 09:12:56PM +0200, Pawel Dembicki wrote:
> This patch replaces the adjust_link api with the phylink apis that provide
> equivalent functionality.
> 
> The remaining functionality from the adjust_link is now covered in the
> phylink_mac_link_* and phylink_mac_config.
> 
> Removes:
> .adjust_link
> Adds:
> .phylink_get_caps
> .phylink_mac_link_down
> .phylink_mac_link_up
> .phylink_mac_link_down
> 
> Signed-off-by: Pawel Dembicki <paweldembicki@xxxxxxxxx>

...

> +static void vsc73xx_phylink_mac_link_up(struct dsa_switch *ds, int port,
> +					unsigned int mode,
> +					phy_interface_t interface,
> +					struct phy_device *phydev,
> +					int speed, int duplex,
> +					bool tx_pause, bool rx_pause)
> +{
> +	struct vsc73xx *vsc = ds->priv;
> +	u32 val;
>  
> +	switch (speed) {
> +	case SPEED_1000:
>  		/* Set up default for internal port or external RGMII */
> -		if (phydev->interface == PHY_INTERFACE_MODE_RGMII)
> +		if (interface == PHY_INTERFACE_MODE_RGMII)
>  			val = VSC73XX_MAC_CFG_1000M_F_RGMII;
>  		else
>  			val = VSC73XX_MAC_CFG_1000M_F_PHY;
> -		vsc73xx_adjust_enable_port(vsc, port, phydev, val);
> -	} else if (phydev->speed == SPEED_100) {
> -		if (phydev->duplex == DUPLEX_FULL) {
> -			val = VSC73XX_MAC_CFG_100_10M_F_PHY;
> -			dev_dbg(vsc->dev,
> -				"port %d: 100 Mbit full duplex mode\n",
> -				port);
> -		} else {
> -			val = VSC73XX_MAC_CFG_100_10M_H_PHY;
> -			dev_dbg(vsc->dev,
> -				"port %d: 100 Mbit half duplex mode\n",
> -				port);
> -		}
> -		vsc73xx_adjust_enable_port(vsc, port, phydev, val);
> -	} else if (phydev->speed == SPEED_10) {
> -		if (phydev->duplex == DUPLEX_FULL) {
> +		break;
> +	case SPEED_100:
> +	case SPEED_10:
> +		if (duplex == DUPLEX_FULL)
>  			val = VSC73XX_MAC_CFG_100_10M_F_PHY;
> -			dev_dbg(vsc->dev,
> -				"port %d: 10 Mbit full duplex mode\n",
> -				port);
> -		} else {
> +		else
>  			val = VSC73XX_MAC_CFG_100_10M_H_PHY;
> -			dev_dbg(vsc->dev,
> -				"port %d: 10 Mbit half duplex mode\n",
> -				port);
> -		}
> -		vsc73xx_adjust_enable_port(vsc, port, phydev, val);
> -	} else {
> -		dev_err(vsc->dev,
> -			"could not adjust link: unknown speed\n");
> +		break;
>  	}
>  
>  	/* Enable port (forwarding) in the receieve mask */
>  	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
>  			    VSC73XX_RECVMASK, BIT(port), BIT(port));
> +	vsc73xx_adjust_enable_port(vsc, port, val);

Hi Pawel,

GCC 12.3.0 [1] reports that val may now be uninitialised at this point,
and in turn used uninitialised in vsc73xx_adjust_enable_port.

 In function 'vsc73xx_adjust_enable_port',
     inlined from 'vsc73xx_phylink_mac_link_up' at drivers/net/dsa/vitesse-vsc73xx-core.c:891:2:
 drivers/net/dsa/vitesse-vsc73xx-core.c:725:13: warning: 'val' may be used uninitialized [-Wmaybe-uninitialized]
   725 |         val |= VSC73XX_MAC_CFG_RESET;
       |             ^
 drivers/net/dsa/vitesse-vsc73xx-core.c: In function 'vsc73xx_phylink_mac_link_up':
 drivers/net/dsa/vitesse-vsc73xx-core.c:869:13: note: 'val' was declared here
   869 |         u32 val;
       |             ^~~

...