| Message ID | 20231119151258.20201-1-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: phy: correctly check soft_reset ret ONLY if defined for PHY | expand |
On Sun, Nov 19, 2023 at 04:12:58PM +0100, Christian Marangi wrote: > soft_reset call for phy_init_hw had multiple revision across the years > and the implementation goes back to 2014. Originally was a simple call > to write the generic PHY reset BIT, it was then moved to a dedicated > function. It was then added the option for PHY driver to define their > own special way to reset the PHY. Till this change, checking for ret was > correct as it was always filled by either the generic reset or the > custom implementation. This changed tho with commit 6e2d85ec0559 ("net: > phy: Stop with excessive soft reset"), as the generic reset call to PHY > was dropped but the ret check was never made entirely optional and > dependent whether soft_reset was defined for the PHY driver or not. > > Luckly nothing was ever added before the soft_reset call so the ret > check (in the case where a PHY didn't had soft_reset defined) although > wrong, never caused problems as ret was init 0 at the start of > phy_init_hw. > > To prevent any kind of problem and to make the function cleaner and more > robust, correctly move the ret check if the soft_reset section making it > optional and needed only with the function defined. I think this should target net-next, not net. It does not appear to be an problem which actually affects somebody using stable kernels. The change itself looks O.K. Andrew
On Sun, Nov 19, 2023 at 05:24:00PM +0100, Andrew Lunn wrote: > On Sun, Nov 19, 2023 at 04:12:58PM +0100, Christian Marangi wrote: > > soft_reset call for phy_init_hw had multiple revision across the years > > and the implementation goes back to 2014. Originally was a simple call > > to write the generic PHY reset BIT, it was then moved to a dedicated > > function. It was then added the option for PHY driver to define their > > own special way to reset the PHY. Till this change, checking for ret was > > correct as it was always filled by either the generic reset or the > > custom implementation. This changed tho with commit 6e2d85ec0559 ("net: > > phy: Stop with excessive soft reset"), as the generic reset call to PHY > > was dropped but the ret check was never made entirely optional and > > dependent whether soft_reset was defined for the PHY driver or not. > > > > Luckly nothing was ever added before the soft_reset call so the ret > > check (in the case where a PHY didn't had soft_reset defined) although > > wrong, never caused problems as ret was init 0 at the start of > > phy_init_hw. > > > > To prevent any kind of problem and to make the function cleaner and more > > robust, correctly move the ret check if the soft_reset section making it > > optional and needed only with the function defined. > > I think this should target net-next, not net. It does not appear to be > an problem which actually affects somebody using stable kernels. > > The change itself looks O.K. > Ok to resubmit or should I wait 24h? (asking as it's a very simple change) Also is the stable Cc ok? (that was the main reason I added the net tag to this)
On Sun, Nov 19, 2023 at 06:55:47PM +0100, Christian Marangi wrote: > On Sun, Nov 19, 2023 at 05:24:00PM +0100, Andrew Lunn wrote: > > On Sun, Nov 19, 2023 at 04:12:58PM +0100, Christian Marangi wrote: > > > soft_reset call for phy_init_hw had multiple revision across the years > > > and the implementation goes back to 2014. Originally was a simple call > > > to write the generic PHY reset BIT, it was then moved to a dedicated > > > function. It was then added the option for PHY driver to define their > > > own special way to reset the PHY. Till this change, checking for ret was > > > correct as it was always filled by either the generic reset or the > > > custom implementation. This changed tho with commit 6e2d85ec0559 ("net: > > > phy: Stop with excessive soft reset"), as the generic reset call to PHY > > > was dropped but the ret check was never made entirely optional and > > > dependent whether soft_reset was defined for the PHY driver or not. > > > > > > Luckly nothing was ever added before the soft_reset call so the ret > > > check (in the case where a PHY didn't had soft_reset defined) although > > > wrong, never caused problems as ret was init 0 at the start of > > > phy_init_hw. > > > > > > To prevent any kind of problem and to make the function cleaner and more > > > robust, correctly move the ret check if the soft_reset section making it > > > optional and needed only with the function defined. > > > > I think this should target net-next, not net. It does not appear to be > > an problem which actually affects somebody using stable kernels. > > > > The change itself looks O.K. > > > > Ok to resubmit or should I wait 24h? (asking as it's a very simple > change) Please wait 24 hours. > Also is the stable Cc ok? > (that was the main reason I added the net tag to this) No drop the Cc: Stable. Your description of the problem does not fit the rules for stable. Andrew
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 2ce74593d6e4..478126f6b5bc 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1235,14 +1235,13 @@ int phy_init_hw(struct phy_device *phydev) if (phydev->drv->soft_reset) { ret = phydev->drv->soft_reset(phydev); + if (ret < 0) + return ret; + /* see comment in genphy_soft_reset for an explanation */ - if (!ret) - phydev->suspended = 0; + phydev->suspended = 0; } - if (ret < 0) - return ret; - ret = phy_scan_fixups(phydev); if (ret < 0) return ret;
soft_reset call for phy_init_hw had multiple revision across the years and the implementation goes back to 2014. Originally was a simple call to write the generic PHY reset BIT, it was then moved to a dedicated function. It was then added the option for PHY driver to define their own special way to reset the PHY. Till this change, checking for ret was correct as it was always filled by either the generic reset or the custom implementation. This changed tho with commit 6e2d85ec0559 ("net: phy: Stop with excessive soft reset"), as the generic reset call to PHY was dropped but the ret check was never made entirely optional and dependent whether soft_reset was defined for the PHY driver or not. Luckly nothing was ever added before the soft_reset call so the ret check (in the case where a PHY didn't had soft_reset defined) although wrong, never caused problems as ret was init 0 at the start of phy_init_hw. To prevent any kind of problem and to make the function cleaner and more robust, correctly move the ret check if the soft_reset section making it optional and needed only with the function defined. Cc: stable@vger.kernel.org # v5.8+ Fixes: 6e2d85ec0559 ("net: phy: Stop with excessive soft reset") Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- drivers/net/phy/phy_device.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)