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

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

[Heiner Kallweit]

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>
---
 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;
 
-	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

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

[Andrew Lunn]

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
> 

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

[Heiner Kallweit]

On 23.12.2018 18:07, Andrew Lunn wrote:
> 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?
> 
Right, the current behavior has been there forever. I'm not aware of
any concrete case. I went for "net" because the current code could
break a device.

> 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.
> 
Would be fine with me too. Maybe David can advise whether a fix for
a potential issue w/o known case qualifies for net.

>> ---
>>  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.
>   
Agree. I'll provide a v2 then for either net or net-next (once it's
open again).

> 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
>>
> 

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

[Andrew Lunn]

On Sun, Dec 23, 2018 at 06:23:18PM +0100, Heiner Kallweit wrote:
> On 23.12.2018 18:07, Andrew Lunn wrote:
> > 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?
> > 
> Right, the current behavior has been there forever. I'm not aware of
> any concrete case. I went for "net" because the current code could
> break a device.

Hi Heiner

https://www.kernel.org/doc/html/v4.15/process/stable-kernel-rules.html

The 4th bullet point:

o It must fix a real bug that bothers people (not a, “This could be a
  problem...” type thing).

    Andrew

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

[Heiner Kallweit]

On 23.12.2018 18:28, Andrew Lunn wrote:
> On Sun, Dec 23, 2018 at 06:23:18PM +0100, Heiner Kallweit wrote:
>> On 23.12.2018 18:07, Andrew Lunn wrote:
>>> 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?
>>>
>> Right, the current behavior has been there forever. I'm not aware of
>> any concrete case. I went for "net" because the current code could
>> break a device.
> 
> Hi Heiner
> 
> https://www.kernel.org/doc/html/v4.15/process/stable-kernel-rules.html
> 
> The 4th bullet point:
> 
> o It must fix a real bug that bothers people (not a, “This could be a
>   problem...” type thing).
> 
OK, this answers the question. Thanks.

>     Andrew
> 
Heiner

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

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Sun, 23 Dec 2018 15:00:26 +0100

> 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>

As per the discussion, let's defer this to when net-next opens back up.

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

[Heiner Kallweit]

On 24.12.2018 23:23, David Miller wrote:
> From: Heiner Kallweit <hkallweit1@gmail.com>
> Date: Sun, 23 Dec 2018 15:00:26 +0100
> 
>> 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>
> 
> As per the discussion, let's defer this to when net-next opens back up.
> 
For my understanding, are you going to pick up deferred patches like this
when net-next opens or should I resubmit?

Thanks

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

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Tue, 25 Dec 2018 09:30:45 +0100

> On 24.12.2018 23:23, David Miller wrote:
>> From: Heiner Kallweit <hkallweit1@gmail.com>
>> Date: Sun, 23 Dec 2018 15:00:26 +0100
>> 
>>> 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>
>> 
>> As per the discussion, let's defer this to when net-next opens back up.
>> 
> For my understanding, are you going to pick up deferred patches like this
> when net-next opens or should I resubmit?

You always need to resubmit, it is the only sane way for me to keep track
of things.

Thanks for asking.