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