2022-05-10 17:20:46

by Antoine Tenart

Subject: Re: [PATCH v2 net] net: phy: mscc: Add error check when __phy_read() failed

Hello,

Quoting Wan Jiabing (2022-05-10 16:22:45)
> Calling __phy_read() might return a negative error code. Use 'int'
> to declare variables which call __phy_read() and also add error check
> for them.
>
> The numerous callers of vsc8584_macsec_phy_read() don't expect it to
> fail. So don't return the error code from __phy_read(), but also don't
> return random values if it does fail.
>
> Fixes: fa164e40c53b ("net: phy: mscc: split the driver into separate files")

Does this fix an actual issue or was this found by code inspection? If
that is not fixing a real issue I don't think it should go to stable
trees.

Also this is not the right commit, the __phy_read call was introduced
before splitting the file.

> static u32 vsc8584_macsec_phy_read(struct phy_device *phydev,
> enum macsec_bank bank, u32 reg)
> {
> - u32 val, val_l = 0, val_h = 0;
> + int rc, val, val_l, val_h;
> unsigned long deadline;
> - int rc;
> + u32 ret = 0;
>
> rc = phy_select_page(phydev, MSCC_PHY_PAGE_MACSEC);
> if (rc < 0)
> @@ -47,15 +47,20 @@ static u32 vsc8584_macsec_phy_read(struct phy_device *phydev,
> deadline = jiffies + msecs_to_jiffies(PROC_CMD_NCOMPLETED_TIMEOUT_MS);
> do {
> val = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_19);
> + if (val < 0)
> + goto failed;
> } while (time_before(jiffies, deadline) && !(val & MSCC_PHY_MACSEC_19_CMD));
>
> val_l = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_17);
> val_h = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_18);
>
> + if (val_l > 0 && val_h > 0)
> + ret = (val_h << 16) | val_l;

Both values have to be non-0 for the function to return a value? I
haven't checked but I would assume it is valid to have one of the two
being 0.

> failed:
> phy_restore_page(phydev, rc, rc);
>
> - return (val_h << 16) | val_l;
> + return ret;
> }

Thanks,
Antoine

2022-05-10 23:35:27

by Andrew Lunn

Subject: Re: [PATCH v2 net] net: phy: mscc: Add error check when __phy_read() failed

> Does this fix an actual issue or was this found by code inspection? If
> that is not fixing a real issue I don't think it should go to stable
> trees.

You are probably right about stable vs net-next. With the old code, a
bad read will result in random return values and bad things are likely
to happen. With this change, 0 will be returned, and hopefully less
bad things will happen.

But i doubt this impacts real users. MDIO tends to either work or not
work at all. And not working is pretty noticeable, and nobody has
reported issues.

So, lets drop the fixes tag, and submit to net-next.

Andrew