From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Lunn Subject: Re: [PATCH net] net: phy: fix phy_init_hw fixup handling Date: Sun, 23 Dec 2018 18:07:59 +0100 Message-ID: <20181223170759.GC2791@lunn.ch> References: <07a7d513-cf5c-99b5-c5ac-337bed2d00dd@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Florian Fainelli , David Miller , "netdev@vger.kernel.org" To: Heiner Kallweit Return-path: Received: from vps0.lunn.ch ([185.16.172.187]:56246 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725497AbeLWRIE (ORCPT ); Sun, 23 Dec 2018 12:08:04 -0500 Content-Disposition: inline In-Reply-To: <07a7d513-cf5c-99b5-c5ac-337bed2d00dd@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: On Sun, Dec 23, 2018 at 03:00:26PM +0100, Heiner Kallweit wrote: > Currently we return immediately if callback config_init isn't defined. > This prevents the fixups from being executed. I see no dependency > between fixups and config_init, therefore change the function to > run the fixups also if config_init isn't defined. > > Fixes: 2f5cb43406d0 ("phylib: Properly reinitialize PHYs after hibernation") > Signed-off-by: Heiner Kallweit Hi Heiner Is this a real fix? It seems like it has been like this forever. Do you know of a PHY which is actually broken? I think the change does make sense, i just don't know if it should be a fix and included in stable. It might be better to wait until net-next opens again. > --- > drivers/net/phy/phy_device.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index e10ac6075..07b1e6751 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -1035,20 +1035,22 @@ int phy_init_hw(struct phy_device *phydev) > /* Deassert the reset signal */ > phy_device_reset(phydev, 0); > > - if (!phydev->drv || !phydev->drv->config_init) > + if (!phydev->drv) > return 0; > > if (phydev->drv->soft_reset) > ret = phydev->drv->soft_reset(phydev); > - > - if (ret < 0) > + if (ret) > return ret; > > ret = phy_scan_fixups(phydev); > - if (ret < 0) > + if (ret) > return ret; These changes from < 0 to any value other than zero should be in a separate patch. This is particularly true for a fix, where we want fixes to be as small as possible. Statistics show fixes more often break stuff than normal development, because they get less testing. So we don't really want to make such a change in a fix for stable. Thanks Andrew > - return phydev->drv->config_init(phydev); > + if (phydev->drv->config_init) > + ret = phydev->drv->config_init(phydev); > + > + return ret; > } > EXPORT_SYMBOL(phy_init_hw); > > -- > 2.20.1 >