commit 1211ce53077164e0d34641d0ca5fb4d4a7574498
("net: phy: resume/suspend PHYs on attach/detach")
introduced a feature to suspend PHYs when entering halted state.
Unfortunately, not all bootloaders properly power-up PHYs on reset
and fail to access ethernet because the PHY is still powered down.
Therefore, we add a boolean module parameter suspend_halted with
default value of true. Disabling that parameter prevents PHYs from
being suspended when entering halted state.
Signed-off-by: Sebastian Hesselbarth <[email protected]>
Reported-by: Andrew Lunn <[email protected]>
---
Andrew, can you please re-test if disabling the feature does work on
your board? I tried a bunch of mine, but none failed to power-up the
PHY in u-boot.
Cc: David Miller <[email protected]>
Cc: Florian Fainelli <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
drivers/net/phy/phy_device.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 4b03e63639b7..671eea0eb5db 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -40,6 +40,10 @@ MODULE_DESCRIPTION("PHY library");
MODULE_AUTHOR("Andy Fleming");
MODULE_LICENSE("GPL");
+static bool suspend_halted = true;
+module_param(suspend_halted, bool, 0644);
+MODULE_PARM_DESC(suspend_halted, "Suspend PHYs when entering halted state.");
+
void phy_device_free(struct phy_device *phydev)
{
put_device(&phydev->dev);
@@ -685,6 +689,12 @@ int phy_suspend(struct phy_device *phydev)
struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
struct ethtool_wolinfo wol;
+ /* Some bootloaders do not power-up PHYs properly after reset,
+ * allow to disable the suspend halted PHYs feature.
+ */
+ if (!suspend_halted)
+ return -ENOSYS;
+
/* If the device has WOL enabled, we cannot suspend the PHY */
wol.cmd = ETHTOOL_GWOL;
phy_ethtool_get_wol(phydev, &wol);
--
1.8.5.3
Hi Sebastian,
2014-02-23 8:58 GMT-08:00 Sebastian Hesselbarth
<[email protected]>:
> commit 1211ce53077164e0d34641d0ca5fb4d4a7574498
> ("net: phy: resume/suspend PHYs on attach/detach")
> introduced a feature to suspend PHYs when entering halted state.
>
> Unfortunately, not all bootloaders properly power-up PHYs on reset
> and fail to access ethernet because the PHY is still powered down.
>
> Therefore, we add a boolean module parameter suspend_halted with
> default value of true. Disabling that parameter prevents PHYs from
> being suspended when entering halted state.
>
> Signed-off-by: Sebastian Hesselbarth <[email protected]>
> Reported-by: Andrew Lunn <[email protected]>
> ---
> Andrew, can you please re-test if disabling the feature does work on
> your board? I tried a bunch of mine, but none failed to power-up the
> PHY in u-boot.
Would be good to get Andrew's testing on this just to make sure it
solves his problem. Otherwise:
Acked-by: Florian Fainelli <[email protected]>
>
> Cc: David Miller <[email protected]>
> Cc: Florian Fainelli <[email protected]>
> Cc: Andrew Lunn <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/net/phy/phy_device.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 4b03e63639b7..671eea0eb5db 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -40,6 +40,10 @@ MODULE_DESCRIPTION("PHY library");
> MODULE_AUTHOR("Andy Fleming");
> MODULE_LICENSE("GPL");
>
> +static bool suspend_halted = true;
> +module_param(suspend_halted, bool, 0644);
> +MODULE_PARM_DESC(suspend_halted, "Suspend PHYs when entering halted state.");
> +
> void phy_device_free(struct phy_device *phydev)
> {
> put_device(&phydev->dev);
> @@ -685,6 +689,12 @@ int phy_suspend(struct phy_device *phydev)
> struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
> struct ethtool_wolinfo wol;
>
> + /* Some bootloaders do not power-up PHYs properly after reset,
> + * allow to disable the suspend halted PHYs feature.
> + */
> + if (!suspend_halted)
> + return -ENOSYS;
> +
> /* If the device has WOL enabled, we cannot suspend the PHY */
> wol.cmd = ETHTOOL_GWOL;
> phy_ethtool_get_wol(phydev, &wol);
> --
> 1.8.5.3
>
--
Florian
On Sun, Feb 23, 2014 at 05:58:39PM +0100, Sebastian Hesselbarth wrote:
> commit 1211ce53077164e0d34641d0ca5fb4d4a7574498
> ("net: phy: resume/suspend PHYs on attach/detach")
> introduced a feature to suspend PHYs when entering halted state.
>
> Unfortunately, not all bootloaders properly power-up PHYs on reset
> and fail to access ethernet because the PHY is still powered down.
>
> Therefore, we add a boolean module parameter suspend_halted with
> default value of true. Disabling that parameter prevents PHYs from
> being suspended when entering halted state.
Hi Sebastian
Am i doing something silly here?
root@qnap:/sys/module/libphy/parameters# cat suspend_halted
Y
root@qnap:/sys/module/libphy/parameters# echo N > suspend_halted
root@qnap:/sys/module/libphy/parameters# cat suspend_halted
N
root@qnap:/sys/module/libphy/parameters# reboot
...
...
Net: egiga0 [PRIME]
Hit any key to stop autoboot: 0
Send Cmd : 0x68 to UART1
egiga0 no link
Using egiga0 device
TFTP from server 10.0.0.1; our IP address is 10.0.0.2
Filename 'uImage-qnap'.
Load address: 0x800000
Loading: T T
Does not seem to work.
Andrew
2014-02-24 11:15 GMT-08:00 Andrew Lunn <[email protected]>:
> On Sun, Feb 23, 2014 at 05:58:39PM +0100, Sebastian Hesselbarth wrote:
>> commit 1211ce53077164e0d34641d0ca5fb4d4a7574498
>> ("net: phy: resume/suspend PHYs on attach/detach")
>> introduced a feature to suspend PHYs when entering halted state.
>>
>> Unfortunately, not all bootloaders properly power-up PHYs on reset
>> and fail to access ethernet because the PHY is still powered down.
>>
>> Therefore, we add a boolean module parameter suspend_halted with
>> default value of true. Disabling that parameter prevents PHYs from
>> being suspended when entering halted state.
>
> Hi Sebastian
>
> Am i doing something silly here?
Could the PHY be already suspended during your initial boot? If that
is the case, the first time we all phy_suspend() the flag is true, we
suspend it, and we never wake it again despite changing
suspend_halted. Does it work better if you specify this module
parameter on the initial kernel boot command-line?
>
> root@qnap:/sys/module/libphy/parameters# cat suspend_halted
> Y
> root@qnap:/sys/module/libphy/parameters# echo N > suspend_halted
> root@qnap:/sys/module/libphy/parameters# cat suspend_halted
> N
> root@qnap:/sys/module/libphy/parameters# reboot
> ...
> ...
>
> Net: egiga0 [PRIME]
> Hit any key to stop autoboot: 0
> Send Cmd : 0x68 to UART1
> egiga0 no link
> Using egiga0 device
> TFTP from server 10.0.0.1; our IP address is 10.0.0.2
> Filename 'uImage-qnap'.
> Load address: 0x800000
> Loading: T T
>
> Does not seem to work.
>
> Andrew
--
Florian
On Mon, Feb 24, 2014 at 11:37:15AM -0800, Florian Fainelli wrote:
> 2014-02-24 11:15 GMT-08:00 Andrew Lunn <[email protected]>:
> > On Sun, Feb 23, 2014 at 05:58:39PM +0100, Sebastian Hesselbarth wrote:
> >> commit 1211ce53077164e0d34641d0ca5fb4d4a7574498
> >> ("net: phy: resume/suspend PHYs on attach/detach")
> >> introduced a feature to suspend PHYs when entering halted state.
> >>
> >> Unfortunately, not all bootloaders properly power-up PHYs on reset
> >> and fail to access ethernet because the PHY is still powered down.
> >>
> >> Therefore, we add a boolean module parameter suspend_halted with
> >> default value of true. Disabling that parameter prevents PHYs from
> >> being suspended when entering halted state.
> >
> > Hi Sebastian
> >
> > Am i doing something silly here?
>
> Could the PHY be already suspended during your initial boot?
No. I tftpboot.
> If that
> is the case, the first time we all phy_suspend() the flag is true, we
> suspend it, and we never wake it again despite changing
> suspend_halted. Does it work better if you specify this module
> parameter on the initial kernel boot command-line?
I tried that as well, after i emailed. Makes no difference, no working
Ethernet.
Andrew
From: Florian Fainelli <[email protected]>
Date: Mon, 24 Feb 2014 10:20:10 -0800
> Hi Sebastian,
>
> 2014-02-23 8:58 GMT-08:00 Sebastian Hesselbarth
> <[email protected]>:
>> commit 1211ce53077164e0d34641d0ca5fb4d4a7574498
>> ("net: phy: resume/suspend PHYs on attach/detach")
>> introduced a feature to suspend PHYs when entering halted state.
>>
>> Unfortunately, not all bootloaders properly power-up PHYs on reset
>> and fail to access ethernet because the PHY is still powered down.
>>
>> Therefore, we add a boolean module parameter suspend_halted with
>> default value of true. Disabling that parameter prevents PHYs from
>> being suspended when entering halted state.
>>
>> Signed-off-by: Sebastian Hesselbarth <[email protected]>
>> Reported-by: Andrew Lunn <[email protected]>
>> ---
>> Andrew, can you please re-test if disabling the feature does work on
>> your board? I tried a bunch of mine, but none failed to power-up the
>> PHY in u-boot.
>
> Would be good to get Andrew's testing on this just to make sure it
> solves his problem. Otherwise:
>
> Acked-by: Florian Fainelli <[email protected]>
I disagree with using a module parameter for this.
Figure out the devices that cannot do this properly, and add
an internal flag that this driver sets.
Module parameters are terrible.
On 02/25/2014 12:05 AM, David Miller wrote:
> From: Florian Fainelli <[email protected]>
> Date: Mon, 24 Feb 2014 10:20:10 -0800
>
>> Hi Sebastian,
>>
>> 2014-02-23 8:58 GMT-08:00 Sebastian Hesselbarth
>> <[email protected]>:
>>> commit 1211ce53077164e0d34641d0ca5fb4d4a7574498
>>> ("net: phy: resume/suspend PHYs on attach/detach")
>>> introduced a feature to suspend PHYs when entering halted state.
>>>
>>> Unfortunately, not all bootloaders properly power-up PHYs on reset
>>> and fail to access ethernet because the PHY is still powered down.
>>>
>>> Therefore, we add a boolean module parameter suspend_halted with
>>> default value of true. Disabling that parameter prevents PHYs from
>>> being suspended when entering halted state.
>>>
>>> Signed-off-by: Sebastian Hesselbarth <[email protected]>
>>> Reported-by: Andrew Lunn <[email protected]>
>>> ---
>>> Andrew, can you please re-test if disabling the feature does work on
>>> your board? I tried a bunch of mine, but none failed to power-up the
>>> PHY in u-boot.
>>
>> Would be good to get Andrew's testing on this just to make sure it
>> solves his problem. Otherwise:
>>
>> Acked-by: Florian Fainelli <[email protected]>
>
> I disagree with using a module parameter for this.
>
> Figure out the devices that cannot do this properly, and add
> an internal flag that this driver sets.
Hmm, as it seems to be a bootloader issue, it will be quite
impossible to determine if a board is affected or not.
I am still trying to get any of my boards to mis-behave the same
way to figure out what is really causing it.
We do still have 2-3 weeks to find a proper fix, don't we?
> Module parameters are terrible.
Maybe. If you prefer, I can remove the module param and leave
the sysfs entry?
Sebastian
On 02/24/2014 08:15 PM, Andrew Lunn wrote:
> On Sun, Feb 23, 2014 at 05:58:39PM +0100, Sebastian Hesselbarth wrote:
>> commit 1211ce53077164e0d34641d0ca5fb4d4a7574498
>> ("net: phy: resume/suspend PHYs on attach/detach")
>> introduced a feature to suspend PHYs when entering halted state.
>>
>> Unfortunately, not all bootloaders properly power-up PHYs on reset
>> and fail to access ethernet because the PHY is still powered down.
>>
>> Therefore, we add a boolean module parameter suspend_halted with
>> default value of true. Disabling that parameter prevents PHYs from
>> being suspended when entering halted state.
>
> Am i doing something silly here?
>
> root@qnap:/sys/module/libphy/parameters# cat suspend_halted
> Y
> root@qnap:/sys/module/libphy/parameters# echo N > suspend_halted
> root@qnap:/sys/module/libphy/parameters# cat suspend_halted
> N
> root@qnap:/sys/module/libphy/parameters# reboot
Just to be sure, can you ifconfig up the interface before reboot?
The PHY could already be powered-down, if the interface is down.
> ...
> ...
>
> Net: egiga0 [PRIME]
> Hit any key to stop autoboot: 0
> Send Cmd : 0x68 to UART1
> egiga0 no link
> Using egiga0 device
> TFTP from server 10.0.0.1; our IP address is 10.0.0.2
> Filename 'uImage-qnap'.
> Load address: 0x800000
> Loading: T T
>
> Does not seem to work.
I tested the patch on Topkick (w/ 88E1318S, 0x0141 0x0e90) which
bootloader is also affected.
Both, libphy.suspend_halted=0 as kernel boot parameter and sysfs
procedure above successfully prevent the PHY from being powered
down. After reboot, u-boot tftpboot works as expected.
Sebastian
On Tue, Feb 25, 2014 at 11:38:52PM +0100, Sebastian Hesselbarth wrote:
> On 02/24/2014 08:15 PM, Andrew Lunn wrote:
> >On Sun, Feb 23, 2014 at 05:58:39PM +0100, Sebastian Hesselbarth wrote:
> >>commit 1211ce53077164e0d34641d0ca5fb4d4a7574498
> >> ("net: phy: resume/suspend PHYs on attach/detach")
> >>introduced a feature to suspend PHYs when entering halted state.
> >>
> >>Unfortunately, not all bootloaders properly power-up PHYs on reset
> >>and fail to access ethernet because the PHY is still powered down.
> >>
> >>Therefore, we add a boolean module parameter suspend_halted with
> >>default value of true. Disabling that parameter prevents PHYs from
> >>being suspended when entering halted state.
> >
> >Am i doing something silly here?
> >
> >root@qnap:/sys/module/libphy/parameters# cat suspend_halted
> >Y
> >root@qnap:/sys/module/libphy/parameters# echo N > suspend_halted
> >root@qnap:/sys/module/libphy/parameters# cat suspend_halted
> >N
> >root@qnap:/sys/module/libphy/parameters# reboot
>
> Just to be sure, can you ifconfig up the interface before reboot?
> The PHY could already be powered-down, if the interface is down.
>
> >...
> >...
> >
> >Net: egiga0 [PRIME]
> >Hit any key to stop autoboot: 0
> >Send Cmd : 0x68 to UART1
> >egiga0 no link
> >Using egiga0 device
> >TFTP from server 10.0.0.1; our IP address is 10.0.0.2
> >Filename 'uImage-qnap'.
> >Load address: 0x800000
> >Loading: T T
> >
> >Does not seem to work.
>
> I tested the patch on Topkick (w/ 88E1318S, 0x0141 0x0e90) which
> bootloader is also affected.
>
> Both, libphy.suspend_halted=0 as kernel boot parameter and sysfs
> procedure above successfully prevent the PHY from being powered
> down. After reboot, u-boot tftpboot works as expected.
Hi Sebastian
I tested again, and it seems like i made an error. This patch does
actually fix the problem.
The u-boot on this device is somewhat broken, when it comes to
networking and tftpboot. It seems like if the auto negotiation is not
complete before the TFTP starts, it never works. There are no
retransmits of the RRQ message. If i ^C it and start again, it does
work.
As to the comment from davem about not using a kernel parameter. How
about turning it all around. Put a boolean parameter into DT PHY node
to indicate when it is safe to power down an idle phy?
Andrew
2014-02-26 10:21 GMT-08:00 Andrew Lunn <[email protected]>:
> On Tue, Feb 25, 2014 at 11:38:52PM +0100, Sebastian Hesselbarth wrote:
>> On 02/24/2014 08:15 PM, Andrew Lunn wrote:
>> >On Sun, Feb 23, 2014 at 05:58:39PM +0100, Sebastian Hesselbarth wrote:
>> >>commit 1211ce53077164e0d34641d0ca5fb4d4a7574498
>> >> ("net: phy: resume/suspend PHYs on attach/detach")
>> >>introduced a feature to suspend PHYs when entering halted state.
>> >>
>> >>Unfortunately, not all bootloaders properly power-up PHYs on reset
>> >>and fail to access ethernet because the PHY is still powered down.
>> >>
>> >>Therefore, we add a boolean module parameter suspend_halted with
>> >>default value of true. Disabling that parameter prevents PHYs from
>> >>being suspended when entering halted state.
>> >
>> >Am i doing something silly here?
>> >
>> >root@qnap:/sys/module/libphy/parameters# cat suspend_halted
>> >Y
>> >root@qnap:/sys/module/libphy/parameters# echo N > suspend_halted
>> >root@qnap:/sys/module/libphy/parameters# cat suspend_halted
>> >N
>> >root@qnap:/sys/module/libphy/parameters# reboot
>>
>> Just to be sure, can you ifconfig up the interface before reboot?
>> The PHY could already be powered-down, if the interface is down.
>>
>> >...
>> >...
>> >
>> >Net: egiga0 [PRIME]
>> >Hit any key to stop autoboot: 0
>> >Send Cmd : 0x68 to UART1
>> >egiga0 no link
>> >Using egiga0 device
>> >TFTP from server 10.0.0.1; our IP address is 10.0.0.2
>> >Filename 'uImage-qnap'.
>> >Load address: 0x800000
>> >Loading: T T
>> >
>> >Does not seem to work.
>>
>> I tested the patch on Topkick (w/ 88E1318S, 0x0141 0x0e90) which
>> bootloader is also affected.
>>
>> Both, libphy.suspend_halted=0 as kernel boot parameter and sysfs
>> procedure above successfully prevent the PHY from being powered
>> down. After reboot, u-boot tftpboot works as expected.
>
> Hi Sebastian
>
> I tested again, and it seems like i made an error. This patch does
> actually fix the problem.
>
> The u-boot on this device is somewhat broken, when it comes to
> networking and tftpboot. It seems like if the auto negotiation is not
> complete before the TFTP starts, it never works. There are no
> retransmits of the RRQ message. If i ^C it and start again, it does
> work.
Sounds familiar, I saw that on other platforms as well, but never
really found the time to get that fix.
>
> As to the comment from davem about not using a kernel parameter. How
> about turning it all around. Put a boolean parameter into DT PHY node
> to indicate when it is safe to power down an idle phy?
Ah ah, nice try, but I do not think this belongs in DT, this is purely
a software issue here, powering up/down the PHY itself works as
expected.
How about we have this knob a sysfs parameter? This should be easy
enough to tweak at runtime and debug in case thing go wrong.
--
Florian
> > As to the comment from davem about not using a kernel parameter. How
> > about turning it all around. Put a boolean parameter into DT PHY node
> > to indicate when it is safe to power down an idle phy?
>
> Ah ah, nice try, but I do not think this belongs in DT, this is purely
> a software issue here, powering up/down the PHY itself works as
> expected.
>
> How about we have this knob a sysfs parameter? This should be easy
> enough to tweak at runtime and debug in case thing go wrong.
With a kernel parameter i can place it into the bootargs of the chosen
node in DT. Solves the problem for everybody with a QNAP box. Same
goes for topkick and any other board which has a broken u-boot. A
sysfs parameter needs setting from user space, so it is not something
we can solve within the kernel.
Andrew
2014-02-26 11:10 GMT-08:00 Andrew Lunn <[email protected]>:
>> > As to the comment from davem about not using a kernel parameter. How
>> > about turning it all around. Put a boolean parameter into DT PHY node
>> > to indicate when it is safe to power down an idle phy?
>>
>> Ah ah, nice try, but I do not think this belongs in DT, this is purely
>> a software issue here, powering up/down the PHY itself works as
>> expected.
>>
>> How about we have this knob a sysfs parameter? This should be easy
>> enough to tweak at runtime and debug in case thing go wrong.
>
> With a kernel parameter i can place it into the bootargs of the chosen
> node in DT. Solves the problem for everybody with a QNAP box. Same
> goes for topkick and any other board which has a broken u-boot. A
> sysfs parameter needs setting from user space, so it is not something
> we can solve within the kernel.
David objected to a module parameter, a kernel parameter is not too
different right?
The only case we need to handle is when the interface is brought down,
suspend_halted=true will also power down the PHY, you reboot into
u-boot, and you attempt a network boot right after that, in that case
the PHY interface is still powered down and this does not work.
That could be worked around by putting the interface up again before
you reboot into u-boot right, that specific logic being gated by
reading the board model. Agreed, you need to duplicate that workaround
in all affected user-space....
--
Florian
> The only case we need to handle is when the interface is brought down,
> suspend_halted=true will also power down the PHY, you reboot into
> u-boot, and you attempt a network boot right after that, in that case
> the PHY interface is still powered down and this does not work.
Correct. And since my device uses dhclient, the interface is always
put down on reboot when it releases the lease.
> That could be worked around by putting the interface up again before
> you reboot into u-boot right, that specific logic being gated by
> reading the board model. Agreed, you need to duplicate that workaround
> in all affected user-space....
I wonder how many other systems are broken? Are we considering this a
regression? Should this feature to turned off by default, and a sysfs
knob used to enable it? That is the safe option.
Andrew