| Message ID | 1515704105-10043-1-git-send-email-andrew@lunn.ch |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: phy: Have __phy_modify return 0 on success | expand |
On 01/11/2018 12:55 PM, Andrew Lunn wrote: > __phy_modify would return the old value of the register before it was > modified. Thus on success, it does not return 0, but a positive value. > Thus functions using phy_modify, which is a wrapper around > __phy_modify, can start returning > 0 on success, rather than 0. As a > result, breakage has been noticed in various places, where 0 was > assumed. > > Code inspection does not find any current location where the return of > the old value is currently used. phy_restore_page() does actually use the old value returned by __phy_modify(), but treats > 0 and == 0 the same way so it is technically used, just not as a > 0 quantity it seems. Russell, are there out of tree call sites (e.g: in your "phy" branch) that we are going to be breaking if we accept this change? So have __phy_modify return 0 on > success. When there is a real need for the old value, either a new > accessor can be added, or an additional parameter passed. > > Fixes: 2b74e5be17d2 ("net: phy: add phy_modify() accessor") You would probably want to tag it with: Fixes: fea23fb591cc ("net: phy: convert read-modify-write to phy_modify()") as well, because that seems to be the problem Niklas and Geert encountered. > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > > Geert, Niklas > > Please can you test this and let me know if it fixes the problems you > see. > > drivers/net/phy/phy-core.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c > index e75989ce8850..36cad6b3b96d 100644 > --- a/drivers/net/phy/phy-core.c > +++ b/drivers/net/phy/phy-core.c > @@ -336,16 +336,15 @@ EXPORT_SYMBOL(phy_write_mmd); > */ > int __phy_modify(struct phy_device *phydev, u32 regnum, u16 mask, u16 set) > { > - int ret, res; > + int ret; > > ret = __phy_read(phydev, regnum); > - if (ret >= 0) { > - res = __phy_write(phydev, regnum, (ret & ~mask) | set); > - if (res < 0) > - ret = res; > - } > + if (ret < 0) > + return ret; > > - return ret; > + ret = __phy_write(phydev, regnum, (ret & ~mask) | set); > + > + return ret < 0 ? ret: 0; > } > EXPORT_SYMBOL_GPL(__phy_modify); > >
Hello! On 1/11/2018 11:55 PM, Andrew Lunn wrote: > __phy_modify would return the old value of the register before it was > modified. Thus on success, it does not return 0, but a positive value. > Thus functions using phy_modify, which is a wrapper around > __phy_modify, can start returning > 0 on success, rather than 0. As a > result, breakage has been noticed in various places, where 0 was > assumed. > > Code inspection does not find any current location where the return of > the old value is currently used. So have __phy_modify return 0 on > success. When there is a real need for the old value, either a new > accessor can be added, or an additional parameter passed. > > Fixes: 2b74e5be17d2 ("net: phy: add phy_modify() accessor") > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > > Geert, Niklas > > Please can you test this and let me know if it fixes the problems you > see. > > drivers/net/phy/phy-core.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c > index e75989ce8850..36cad6b3b96d 100644 > --- a/drivers/net/phy/phy-core.c > +++ b/drivers/net/phy/phy-core.c > @@ -336,16 +336,15 @@ EXPORT_SYMBOL(phy_write_mmd); > */ > int __phy_modify(struct phy_device *phydev, u32 regnum, u16 mask, u16 set) > { > - int ret, res; > + int ret; > > ret = __phy_read(phydev, regnum); > - if (ret >= 0) { > - res = __phy_write(phydev, regnum, (ret & ~mask) | set); > - if (res < 0) > - ret = res; > - } > + if (ret < 0) > + return ret; > > - return ret; > + ret = __phy_write(phydev, regnum, (ret & ~mask) | set); > + > + return ret < 0 ? ret: 0; Need another space, before ':'... > } > EXPORT_SYMBOL_GPL(__phy_modify); > MBR, Sergei
Hi Andrew, On Thu, Jan 11, 2018 at 9:55 PM, Andrew Lunn <andrew@lunn.ch> wrote: > __phy_modify would return the old value of the register before it was > modified. Thus on success, it does not return 0, but a positive value. > Thus functions using phy_modify, which is a wrapper around > __phy_modify, can start returning > 0 on success, rather than 0. As a > result, breakage has been noticed in various places, where 0 was > assumed. > > Code inspection does not find any current location where the return of > the old value is currently used. So have __phy_modify return 0 on > success. When there is a real need for the old value, either a new > accessor can be added, or an additional parameter passed. > > Fixes: 2b74e5be17d2 ("net: phy: add phy_modify() accessor") > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > > Geert, Niklas > > Please can you test this and let me know if it fixes the problems you > see. Yes it does, thanks! Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> I'm a bit worried about users already relying on the new return value, though. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Thu, Jan 11, 2018 at 05:29:22PM -0800, Florian Fainelli wrote: > On 01/11/2018 12:55 PM, Andrew Lunn wrote: > > __phy_modify would return the old value of the register before it was > > modified. Thus on success, it does not return 0, but a positive value. > > Thus functions using phy_modify, which is a wrapper around > > __phy_modify, can start returning > 0 on success, rather than 0. As a > > result, breakage has been noticed in various places, where 0 was > > assumed. > > > > Code inspection does not find any current location where the return of > > the old value is currently used. > > phy_restore_page() does actually use the old value returned by > __phy_modify(), Hi Florian int phy_restore_page(struct phy_device *phydev, int oldpage, int ret) { int r; if (oldpage >= 0) { r = __phy_write_page(phydev, oldpage); /* Propagate the operation return code if the page write * was successful. */ if (ret >= 0 && r < 0) ret = r; } else { /* Propagate the phy page selection error code */ ret = oldpage; } mutex_unlock(&phydev->mdio.bus->mdio_lock); return ret; } Ah! I see it now. The value of ret parameter can be what phy_modify() returned. If ret is not an error, and __phy_write_page() returned an error, use r, the error from __phy_write_page(). As you say, the actual value is not used, just an indication of if it represents an error. So this change is O.K. Andrew
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c index e75989ce8850..36cad6b3b96d 100644 --- a/drivers/net/phy/phy-core.c +++ b/drivers/net/phy/phy-core.c @@ -336,16 +336,15 @@ EXPORT_SYMBOL(phy_write_mmd); */ int __phy_modify(struct phy_device *phydev, u32 regnum, u16 mask, u16 set) { - int ret, res; + int ret; ret = __phy_read(phydev, regnum); - if (ret >= 0) { - res = __phy_write(phydev, regnum, (ret & ~mask) | set); - if (res < 0) - ret = res; - } + if (ret < 0) + return ret; - return ret; + ret = __phy_write(phydev, regnum, (ret & ~mask) | set); + + return ret < 0 ? ret: 0; } EXPORT_SYMBOL_GPL(__phy_modify);
__phy_modify would return the old value of the register before it was modified. Thus on success, it does not return 0, but a positive value. Thus functions using phy_modify, which is a wrapper around __phy_modify, can start returning > 0 on success, rather than 0. As a result, breakage has been noticed in various places, where 0 was assumed. Code inspection does not find any current location where the return of the old value is currently used. So have __phy_modify return 0 on success. When there is a real need for the old value, either a new accessor can be added, or an additional parameter passed. Fixes: 2b74e5be17d2 ("net: phy: add phy_modify() accessor") Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- Geert, Niklas Please can you test this and let me know if it fixes the problems you see. drivers/net/phy/phy-core.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)