| Message ID | 20220510142247.16071-1-wanjiabing@vivo.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net] net: phy: mscc: Add error check when __phy_read() failed | expand |
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
> 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
Quoting Andrew Lunn (2022-05-10 17:08:07) > > 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. Right. On top of that there are other calls to __phy_read in this driver not checking the returned value. Plus all __phy_write calls. If that was found by code inspection I would suggest to improve the whole driver and not this function alone. Thanks, Antoine
On 2022/5/10 23:33, Antoine Tenart wrote: > Quoting Andrew Lunn (2022-05-10 17:08:07) >> 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. > Right. On top of that there are other calls to __phy_read in this driver > not checking the returned value. Plus all __phy_write calls. If that was > found by code inspection I would suggest to improve the whole driver and > not this function alone. OK, I'll try to improve the whole driver and send a patch set for this. Thanks, Wan Jiabing
On Tue, May 10, 2022 at 10:22:45PM +0800, Wan Jiabing wrote: > 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") > Signed-off-by: Wan Jiabing <wanjiabing@vivo.com> > --- > Changelog: > v2: > - Sort variable declaration and add a detailed comment. > --- > drivers/net/phy/mscc/mscc_macsec.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/phy/mscc/mscc_macsec.c b/drivers/net/phy/mscc/mscc_macsec.c > index b7b2521c73fb..58ad11a697b6 100644 > --- a/drivers/net/phy/mscc/mscc_macsec.c > +++ b/drivers/net/phy/mscc/mscc_macsec.c > @@ -22,9 +22,9 @@ > 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; > + > failed: > phy_restore_page(phydev, rc, rc); > > - return (val_h << 16) | val_l; > + return ret; > } This is still wrong - phy_restore_page() can fail to retore the page. It's rather unfortunate that you need to return a u32, where the high values become negative ints, which means you can't use phy_restore_page() as it's supposed to be used. If you fail to read from the PHY, is returning zero acceptable? I think what you should be doing at the very least is: rc = phy_select_page(phydev, MSCC_PHY_PAGE_MACSEC); if (rc < 0) goto failed; rc = __phy_write(phydev, MSCC_EXT_PAGE_MACSEC_20, ...); if (rc < 0) goto failed; ... rc = __phy_write(phydev, MSCC_EXT_PAGE_MACSEC_19, ...); if (rc < 0) goto failed; ... do { val = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_19); if (val < 0) { rc = val; goto failed; } } while (...); val_l = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_17); if (val_l < 0) { rc = val_l; goto failed; } val_h = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_18); if (val_h < 0) rc = val_h; failed: rc = phy_restore_page(phgydev, rc, 0); return rc < 0 ? 0 : val_h << 16 | val_l; Which means that if any of the PHY IO functions fail at any point, this returns zero.
diff --git a/drivers/net/phy/mscc/mscc_macsec.c b/drivers/net/phy/mscc/mscc_macsec.c index b7b2521c73fb..58ad11a697b6 100644 --- a/drivers/net/phy/mscc/mscc_macsec.c +++ b/drivers/net/phy/mscc/mscc_macsec.c @@ -22,9 +22,9 @@ 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; + failed: phy_restore_page(phydev, rc, rc); - return (val_h << 16) | val_l; + return ret; } static void vsc8584_macsec_phy_write(struct phy_device *phydev,
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") Signed-off-by: Wan Jiabing <wanjiabing@vivo.com> --- Changelog: v2: - Sort variable declaration and add a detailed comment. --- drivers/net/phy/mscc/mscc_macsec.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)