Re: [PATCH net] net: phy: fix phy_init_hw fixup handling

[Andrew Lunn <andrew@lunn.ch>]

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@gmail.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
>