[bug report] net: phy: Add support for Aeonsemi AS21xxx PHYs

[bug report] net: phy: Add support for Aeonsemi AS21xxx PHYs

[Dan Carpenter]

Hello Christian Marangi,

Commit 830877d89edc ("net: phy: Add support for Aeonsemi AS21xxx
PHYs") from May 17, 2025 (linux-next), leads to the following Smatch
static checker warning:

	drivers/net/phy/as21xxx.c:561 aeon_ipc_sync_parity()
	error: uninitialized symbol 'ret_sts'.

drivers/net/phy/as21xxx.c
    538 static int aeon_ipc_sync_parity(struct phy_device *phydev,
    539                                 struct as21xxx_priv *priv)
    540 {
    541         u16 ret_sts;
    542         int ret;
    543 
    544         mutex_lock(&priv->ipc_lock);
    545 
    546         /* Send NOP with no parity */
    547         aeon_ipc_noop(phydev, priv, NULL);
    548 
    549         /* Reset packet parity */
    550         priv->parity_status = false;
    551 
    552         /* Send second NOP with no parity */
    553         ret = aeon_ipc_noop(phydev, priv, &ret_sts);
    554 
    555         mutex_unlock(&priv->ipc_lock);
    556 
    557         /* We expect to return -EINVAL */
    558         if (ret != -EINVAL)
    559                 return ret;

Treating -EINVAL as having a special meaning is really risky because lots
of other places return -EINVAL as well.  For example, here if
__phy_write_mmd() returns -EINVAL then "ret_sts" is not initialized.

    560 
--> 561         if ((ret_sts & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_READY) {
    562                 phydev_err(phydev, "Invalid IPC status on sync parity: %x\n",
    563                            ret_sts);
    564                 return -EINVAL;
    565         }
    566 
    567         return 0;
    568 }

regards,
dan carpenter

[bug report] net: phy: Add support for Aeonsemi AS21xxx PHYs

[Dan Carpenter]

Hello Christian Marangi,

Commit 830877d89edc ("net: phy: Add support for Aeonsemi AS21xxx
PHYs") from May 17, 2025 (linux-next), leads to the following Smatch
static checker warning:

	drivers/net/phy/as21xxx.c:561 aeon_ipc_sync_parity()
	error: uninitialized symbol 'ret_sts'.

drivers/net/phy/as21xxx.c
    538 static int aeon_ipc_sync_parity(struct phy_device *phydev,
    539                                 struct as21xxx_priv *priv)
    540 {
    541         u16 ret_sts;
    542         int ret;
    543 
    544         mutex_lock(&priv->ipc_lock);
    545 
    546         /* Send NOP with no parity */
    547         aeon_ipc_noop(phydev, priv, NULL);
    548 
    549         /* Reset packet parity */
    550         priv->parity_status = false;
    551 
    552         /* Send second NOP with no parity */
    553         ret = aeon_ipc_noop(phydev, priv, &ret_sts);
    554 
    555         mutex_unlock(&priv->ipc_lock);
    556 
    557         /* We expect to return -EINVAL */
    558         if (ret != -EINVAL)
    559                 return ret;

There are a bunch of other return -EINVAL situations before we get to
the one we're expecting...

    560 
--> 561         if ((ret_sts & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_READY) {
                     ^^^^^^^
So Smatch complains that if we hit one of those then ret_sts is
uninitialized.  I mean probably it's fine...  But I just always feel
returning -EINVAL as if it has special meaning is a mistake.  There
is a subsystem which make this a core assumption and it doesn't work
but it's too much work to fix at this point.  Probably two days worth of
re-writing probe functions and no one wants to do that.

    562                 phydev_err(phydev, "Invalid IPC status on sync parity: %x\n",
    563                            ret_sts);
    564                 return -EINVAL;
    565         }
    566 
    567         return 0;
    568 }

regards,
dan carpenter