netdev - Re: [PATCH net] net: phy: fix phy_init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 23 Dec 2018 18:07:59 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net] net: phy: fix phy_init_hw fixup handling

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 <hkallweit1@...il.com>

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
>