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

Simon Horman
2023-06-22T07:34:29Z

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@gmail.com>

...

> +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;
| ^~~

...