[PATCH] net: phy: add suspend_halted module param

[PATCH] net: phy: add suspend_halted module param

[Sebastian Hesselbarth]

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 <sebastian.hesselbarth@gmail.com>
Reported-by: Andrew Lunn <andrew@lunn.ch>
---
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 <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: netdev at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org
---
 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

[PATCH] net: phy: add suspend_halted module param

[Florian Fainelli]

Hi Sebastian,

2014-02-23 8:58 GMT-08:00 Sebastian Hesselbarth
<sebastian.hesselbarth@gmail.com>:
> 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 <sebastian.hesselbarth@gmail.com>
> Reported-by: Andrew Lunn <andrew@lunn.ch>
> ---
> 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 <f.fainelli@gmail.com>

>
> Cc: David Miller <davem@davemloft.net>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: netdev at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: linux-kernel at vger.kernel.org
> ---
>  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

[PATCH] net: phy: add suspend_halted module param

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Mon, 24 Feb 2014 10:20:10 -0800

> Hi Sebastian,
> 
> 2014-02-23 8:58 GMT-08:00 Sebastian Hesselbarth
> <sebastian.hesselbarth@gmail.com>:
>> 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 <sebastian.hesselbarth@gmail.com>
>> Reported-by: Andrew Lunn <andrew@lunn.ch>
>> ---
>> 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 <f.fainelli@gmail.com>

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.

[PATCH] net: phy: add suspend_halted module param

[Sebastian Hesselbarth]

On 02/25/2014 12:05 AM, David Miller wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Mon, 24 Feb 2014 10:20:10 -0800
>
>> Hi Sebastian,
>>
>> 2014-02-23 8:58 GMT-08:00 Sebastian Hesselbarth
>> <sebastian.hesselbarth@gmail.com>:
>>> 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 <sebastian.hesselbarth@gmail.com>
>>> Reported-by: Andrew Lunn <andrew@lunn.ch>
>>> ---
>>> 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 <f.fainelli@gmail.com>
>
> 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

[PATCH] net: phy: add suspend_halted module param

[Andrew Lunn]

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 at qnap:/sys/module/libphy/parameters# cat suspend_halted 
Y
root at qnap:/sys/module/libphy/parameters# echo N > suspend_halted 
root at qnap:/sys/module/libphy/parameters# cat suspend_halted 
N
root at 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

[PATCH] net: phy: add suspend_halted module param

[Florian Fainelli]

2014-02-24 11:15 GMT-08:00 Andrew Lunn <andrew@lunn.ch>:
> 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 at qnap:/sys/module/libphy/parameters# cat suspend_halted
> Y
> root at qnap:/sys/module/libphy/parameters# echo N > suspend_halted
> root at qnap:/sys/module/libphy/parameters# cat suspend_halted
> N
> root at 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

[PATCH] net: phy: add suspend_halted module param

[Andrew Lunn]

On Mon, Feb 24, 2014 at 11:37:15AM -0800, Florian Fainelli wrote:
> 2014-02-24 11:15 GMT-08:00 Andrew Lunn <andrew@lunn.ch>:
> > 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

[PATCH] net: phy: add suspend_halted module param

[Sebastian Hesselbarth]

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 at qnap:/sys/module/libphy/parameters# cat suspend_halted
> Y
> root at qnap:/sys/module/libphy/parameters# echo N > suspend_halted
> root at qnap:/sys/module/libphy/parameters# cat suspend_halted
> N
> root at 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

[PATCH] net: phy: add suspend_halted module param

[Andrew Lunn]

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 at qnap:/sys/module/libphy/parameters# cat suspend_halted
> >Y
> >root at qnap:/sys/module/libphy/parameters# echo N > suspend_halted
> >root at qnap:/sys/module/libphy/parameters# cat suspend_halted
> >N
> >root at 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

[PATCH] net: phy: add suspend_halted module param

[Florian Fainelli]

2014-02-26 10:21 GMT-08:00 Andrew Lunn <andrew@lunn.ch>:
> 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 at qnap:/sys/module/libphy/parameters# cat suspend_halted
>> >Y
>> >root at qnap:/sys/module/libphy/parameters# echo N > suspend_halted
>> >root at qnap:/sys/module/libphy/parameters# cat suspend_halted
>> >N
>> >root at 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

[PATCH] net: phy: add suspend_halted module param

[Andrew Lunn]

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

[PATCH] net: phy: add suspend_halted module param

[Florian Fainelli]

2014-02-26 11:10 GMT-08:00 Andrew Lunn <andrew@lunn.ch>:
>> > 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

[PATCH] net: phy: add suspend_halted module param

[Andrew Lunn]

> 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

[PATCH] net: phy: Add sysfs attribute to prevent PHY suspend

[Sebastian Hesselbarth]

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, this adds code and documentation for a sysfs attribute to
allow/deny PHYs to be suspended on a per-PHY basis. Disabling that
attribute prevents PHYs from being suspended when entering halted state.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Reported-by: Andrew Lunn <andrew@lunn.ch>
---
David,

I know I am late, but I still consider this as a fix for v3.14, therefore
this is based on v3.14-rc1. If you are already done with taking fixes for
v3.14, I can, of course, rebase this upon net-next.

Cc: David Miller <davem@davemloft.net>
Cc: Rob Landley <rob@landley.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: netdev at vger.kernel.org
Cc: linux-doc at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org
---
 Documentation/ABI/testing/sysfs-bus-mdio | 10 ++++++++++
 drivers/net/phy/mdio_bus.c               | 26 ++++++++++++++++++++++++++
 drivers/net/phy/phy_device.c             |  5 +++++
 include/linux/phy.h                      |  2 ++
 4 files changed, 43 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-mdio b/Documentation/ABI/testing/sysfs-bus-mdio
index 6349749ebc29..e85a3d350cb1 100644
--- a/Documentation/ABI/testing/sysfs-bus-mdio
+++ b/Documentation/ABI/testing/sysfs-bus-mdio
@@ -7,3 +7,13 @@ Description:
 		by the device during bus enumeration, encoded in hexadecimal.
 		This ID is used to match the device with the appropriate
 		driver.
+
+What:		/sys/bus/mdio_bus/devices/.../phy_allow_suspend
+Date:		March 2014
+KernelVersion:	3.14
+Contact:	netdev at vger.kernel.org
+Description:
+		This attribute contains a boolean parameter to allow (1) or
+		deny (0) MDIO bus attached PHYs to be suspended. Some
+		bootloaders fail to properly resume a suspended PHY, so this
+		can be used to prevent the PHY from being suspended.
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 71e49000fbf3..811d35185596 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -432,8 +432,34 @@ phy_id_show(struct device *dev, struct device_attribute *attr, char *buf)
 }
 static DEVICE_ATTR_RO(phy_id);
 
+static ssize_t phy_allow_suspend_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+
+	return sprintf(buf, "%d\n", phydev->allow_suspend);
+}
+
+static ssize_t phy_allow_suspend_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+	bool val;
+	int ret;
+
+	ret = strtobool(buf, &val);
+	if (ret < 0)
+		return ret;
+
+	phydev->allow_suspend = val;
+
+	return count;
+}
+static DEVICE_ATTR_RW(phy_allow_suspend);
+
 static struct attribute *mdio_dev_attrs[] = {
 	&dev_attr_phy_id.attr,
+	&dev_attr_phy_allow_suspend.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(mdio_dev);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 4b03e63639b7..1c83d34d848b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -173,6 +173,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id,
 	dev->phy_id = phy_id;
 	if (c45_ids)
 		dev->c45_ids = *c45_ids;
+	dev->allow_suspend = true;
 	dev->bus = bus;
 	dev->dev.parent = bus->parent;
 	dev->dev.bus = &mdio_bus_type;
@@ -685,6 +686,10 @@ int phy_suspend(struct phy_device *phydev)
 	struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
 	struct ethtool_wolinfo wol;
 
+	/* Do not suspend PHYs, if user disabled it */
+	if (!phydev->allow_suspend)
+		return -ENOSYS;
+
 	/* If the device has WOL enabled, we cannot suspend the PHY */
 	wol.cmd = ETHTOOL_GWOL;
 	phy_ethtool_get_wol(phydev, &wol);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 565188ca328f..c472e750c023 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -272,6 +272,7 @@ struct phy_c45_device_ids {
  * c45_ids: 802.3-c45 Device Identifers if is_c45.
  * is_c45:  Set to true if this phy uses clause 45 addressing.
  * is_internal: Set to true if this phy is internal to a MAC.
+ * allow_suspend: Set to false to prevent PHY suspend.
  * state: state of the PHY for management purposes
  * dev_flags: Device-specific flags used by the PHY driver.
  * addr: Bus address of PHY
@@ -308,6 +309,7 @@ struct phy_device {
 	struct phy_c45_device_ids c45_ids;
 	bool is_c45;
 	bool is_internal;
+	bool allow_suspend;
 
 	enum phy_state state;
 
-- 
1.8.5.3

[PATCH] net: phy: Add sysfs attribute to prevent PHY suspend

[Florian Fainelli]

2014-03-07 3:34 GMT-08:00 Sebastian Hesselbarth
<sebastian.hesselbarth@gmail.com>:
> 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, this adds code and documentation for a sysfs attribute to
> allow/deny PHYs to be suspended on a per-PHY basis. Disabling that
> attribute prevents PHYs from being suspended when entering halted state.
>
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Reported-by: Andrew Lunn <andrew@lunn.ch>

Looks good to me, thanks Sebastian!

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

> ---
> David,
>
> I know I am late, but I still consider this as a fix for v3.14, therefore
> this is based on v3.14-rc1. If you are already done with taking fixes for
> v3.14, I can, of course, rebase this upon net-next.
>
> Cc: David Miller <davem@davemloft.net>
> Cc: Rob Landley <rob@landley.net>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: netdev at vger.kernel.org
> Cc: linux-doc at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: linux-kernel at vger.kernel.org
> ---
>  Documentation/ABI/testing/sysfs-bus-mdio | 10 ++++++++++
>  drivers/net/phy/mdio_bus.c               | 26 ++++++++++++++++++++++++++
>  drivers/net/phy/phy_device.c             |  5 +++++
>  include/linux/phy.h                      |  2 ++
>  4 files changed, 43 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-mdio b/Documentation/ABI/testing/sysfs-bus-mdio
> index 6349749ebc29..e85a3d350cb1 100644
> --- a/Documentation/ABI/testing/sysfs-bus-mdio
> +++ b/Documentation/ABI/testing/sysfs-bus-mdio
> @@ -7,3 +7,13 @@ Description:
>                 by the device during bus enumeration, encoded in hexadecimal.
>                 This ID is used to match the device with the appropriate
>                 driver.
> +
> +What:          /sys/bus/mdio_bus/devices/.../phy_allow_suspend
> +Date:          March 2014
> +KernelVersion: 3.14
> +Contact:       netdev at vger.kernel.org
> +Description:
> +               This attribute contains a boolean parameter to allow (1) or
> +               deny (0) MDIO bus attached PHYs to be suspended. Some
> +               bootloaders fail to properly resume a suspended PHY, so this
> +               can be used to prevent the PHY from being suspended.
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 71e49000fbf3..811d35185596 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -432,8 +432,34 @@ phy_id_show(struct device *dev, struct device_attribute *attr, char *buf)
>  }
>  static DEVICE_ATTR_RO(phy_id);
>
> +static ssize_t phy_allow_suspend_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       struct phy_device *phydev = to_phy_device(dev);
> +
> +       return sprintf(buf, "%d\n", phydev->allow_suspend);
> +}
> +
> +static ssize_t phy_allow_suspend_store(struct device *dev,
> +               struct device_attribute *attr, const char *buf, size_t count)
> +{
> +       struct phy_device *phydev = to_phy_device(dev);
> +       bool val;
> +       int ret;
> +
> +       ret = strtobool(buf, &val);
> +       if (ret < 0)
> +               return ret;
> +
> +       phydev->allow_suspend = val;
> +
> +       return count;
> +}
> +static DEVICE_ATTR_RW(phy_allow_suspend);
> +
>  static struct attribute *mdio_dev_attrs[] = {
>         &dev_attr_phy_id.attr,
> +       &dev_attr_phy_allow_suspend.attr,
>         NULL,
>  };
>  ATTRIBUTE_GROUPS(mdio_dev);
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 4b03e63639b7..1c83d34d848b 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -173,6 +173,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id,
>         dev->phy_id = phy_id;
>         if (c45_ids)
>                 dev->c45_ids = *c45_ids;
> +       dev->allow_suspend = true;
>         dev->bus = bus;
>         dev->dev.parent = bus->parent;
>         dev->dev.bus = &mdio_bus_type;
> @@ -685,6 +686,10 @@ int phy_suspend(struct phy_device *phydev)
>         struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
>         struct ethtool_wolinfo wol;
>
> +       /* Do not suspend PHYs, if user disabled it */
> +       if (!phydev->allow_suspend)
> +               return -ENOSYS;
> +
>         /* If the device has WOL enabled, we cannot suspend the PHY */
>         wol.cmd = ETHTOOL_GWOL;
>         phy_ethtool_get_wol(phydev, &wol);
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 565188ca328f..c472e750c023 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -272,6 +272,7 @@ struct phy_c45_device_ids {
>   * c45_ids: 802.3-c45 Device Identifers if is_c45.
>   * is_c45:  Set to true if this phy uses clause 45 addressing.
>   * is_internal: Set to true if this phy is internal to a MAC.
> + * allow_suspend: Set to false to prevent PHY suspend.
>   * state: state of the PHY for management purposes
>   * dev_flags: Device-specific flags used by the PHY driver.
>   * addr: Bus address of PHY
> @@ -308,6 +309,7 @@ struct phy_device {
>         struct phy_c45_device_ids c45_ids;
>         bool is_c45;
>         bool is_internal;
> +       bool allow_suspend;
>
>         enum phy_state state;
>
> --
> 1.8.5.3
>



-- 
Florian

[PATCH] net: phy: Add sysfs attribute to prevent PHY suspend

[David Miller]

From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Date: Fri,  7 Mar 2014 12:34:52 +0100

> 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, this adds code and documentation for a sysfs attribute to
> allow/deny PHYs to be suspended on a per-PHY basis. Disabling that
> attribute prevents PHYs from being suspended when entering halted state.
> 
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Reported-by: Andrew Lunn <andrew@lunn.ch>

I know you won't like what I have to say, but I want to see a solution
without this sysfs knob.

First of all, you obviously have a way to end up having the sysfs knob
get set on the appropriate systems.

Therefore, you obviously have some way to propagate the same piece of
information into the kernel somehow during boot time.

For example, via a device tree property or similar.

Please pursue an avenue such as that.  This sysfs thing, it's a user
facing interface we'd have to live with forever.

[PATCH] net: phy: Add sysfs attribute to prevent PHY suspend

[Sebastian Hesselbarth]

On 03/10/2014 12:12 AM, David Miller wrote:
> From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Date: Fri,  7 Mar 2014 12:34:52 +0100
>
>> 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, this adds code and documentation for a sysfs attribute to
>> allow/deny PHYs to be suspended on a per-PHY basis. Disabling that
>> attribute prevents PHYs from being suspended when entering halted state.
>>
>> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> Reported-by: Andrew Lunn <andrew@lunn.ch>
>
> I know you won't like what I have to say, but I want to see a solution
> without this sysfs knob.
>
> First of all, you obviously have a way to end up having the sysfs knob
> get set on the appropriate systems.
>
> Therefore, you obviously have some way to propagate the same piece of
> information into the kernel somehow during boot time.
>
> For example, via a device tree property or similar.

There is no way to determine if a bootloader is broken or not. The
sysfs knob allows to provide a use case based decision. Of course, we
can invent some freaky device tree property but that the DT maintainers
will not like either.

This is not an issue with broken HW but SW. The PHY is fine, you can
suspend and resume it perfectly in Linux. But the bootloader fails to
properly initialize it on a warm boot. You could update the bootloader
and the issue disappears.

> Please pursue an avenue such as that.  This sysfs thing, it's a user
> facing interface we'd have to live with forever.

If you want me to try a devicetree property for the issue, we also
create ABI and we will have to live with it forever.

I am open for suggestions, but I have a bad feeling about a "broken
bootloader" DT property.

Sebastian

[PATCH] net: phy: Add sysfs attribute to prevent PHY suspend

[David Miller]

From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Date: Mon, 10 Mar 2014 00:25:24 +0100

> There is no way to determine if a bootloader is broken or not. The
> sysfs knob allows to provide a use case based decision. Of course,
> we can invent some freaky device tree property but that the DT
> maintainers will not like either.

My point is that whatever mechanism is used to "decide" that the sysfs
knob gets set, can also be used to "decide" that a DT property is
instantiated in the device tree.

[PATCH] net: phy: Add sysfs attribute to prevent PHY suspend

[Sebastian Hesselbarth]

On 03/10/2014 01:30 AM, David Miller wrote:
> From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Date: Mon, 10 Mar 2014 00:25:24 +0100
>
>> There is no way to determine if a bootloader is broken or not. The
>> sysfs knob allows to provide a use case based decision. Of course,
>> we can invent some freaky device tree property but that the DT
>> maintainers will not like either.
>
> My point is that whatever mechanism is used to "decide" that the sysfs
> knob gets set, can also be used to "decide" that a DT property is
> instantiated in the device tree.

The mechanism is manual, no automatic way to determine it. I understand
your point, but DT maintainers will argue here that DT is to describe HW
not SW. And a badly written bootloader initialization routine for a PHY
is SW.

Also, this will force us to maintain two sets of DT files for each
affected board: one for those with broken bootloader and one for those
with an updated, fixed bootloader. And of course, the broken bootloaders
are from pre-DT times and cannot even set that property but require the
user to pick the right DT.

If you are still against a sysfs knob, I see no way to provide a user
accessible way to prevent the PHY to be suspended. And the user is the
only reliable instance to decide not to suspend it.

Sebastian

[PATCH] net: phy: Add sysfs attribute to prevent PHY suspend

[David Miller]

From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Date: Mon, 10 Mar 2014 01:37:32 +0100

> The mechanism is manual, no automatic way to determine it.

We recognize BIOS and ACPI bugs and work around them, by looking at
version information and whatnot, so you really can't convince me that
something similar can't be done here perhaps in the platform code.

[PATCH] net: phy: Add sysfs attribute to prevent PHY suspend

[Sebastian Hesselbarth]

On 03/10/2014 01:41 AM, David Miller wrote:
> From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Date: Mon, 10 Mar 2014 01:37:32 +0100
>
>> The mechanism is manual, no automatic way to determine it.
>
> We recognize BIOS and ACPI bugs and work around them, by looking at
> version information and whatnot, so you really can't convince me that
> something similar can't be done here perhaps in the platform code.

Hmm, if the is a way to determine the version of that particual u-boot
I'd be happy to exploit that information. But I honestly doubt that.
Compared to u-boot bootloader and kernel interaction, BIOS and ACPI
are well-defined protocols.

I personally, would prefer everybody should update his broken
bootloaders, but that will just not happen.

Anyway, at least for the two boards in question, we know a bootloader
workaround. The version does support user commands to re-enable the PHY
by writing the corresponding registers.

Unfortunately, the is a bug in phy_ethtool_get_wol that up to now,
prevents most PHYs (without .wol callbacks) from being suspended.
I wanted to get in a way to disable suspend before sending a fix.

If you are that against a sysfs knob, I guess, we will just see how
many more bootloaders are broken and some will not have a way to write
PHY registers.

Sebastian

[PATCH] net: phy: Add sysfs attribute to prevent PHY suspend

[David Miller]

From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Date: Mon, 10 Mar 2014 01:53:33 +0100

> On 03/10/2014 01:41 AM, David Miller wrote:
>> From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> Date: Mon, 10 Mar 2014 01:37:32 +0100
>>
>>> The mechanism is manual, no automatic way to determine it.
>>
>> We recognize BIOS and ACPI bugs and work around them, by looking at
>> version information and whatnot, so you really can't convince me that
>> something similar can't be done here perhaps in the platform code.
> 
> Hmm, if the is a way to determine the version of that particual u-boot
> I'd be happy to exploit that information. But I honestly doubt that.
> Compared to u-boot bootloader and kernel interaction, BIOS and ACPI
> are well-defined protocols.
> 
> I personally, would prefer everybody should update his broken
> bootloaders, but that will just not happen.

What you can do is have a test that _perhaps_ covers a "broader than
reality" list of broken bootloader cases.

Then you have something the bootloader can provide which indicates
that it has been fixed.

[PATCH] net: phy: Add sysfs attribute to prevent PHY suspend

[Sebastian Hesselbarth]

On 03/10/2014 03:40 AM, David Miller wrote:
> From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Date: Mon, 10 Mar 2014 01:53:33 +0100
>
>> On 03/10/2014 01:41 AM, David Miller wrote:
>>> From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>>> Date: Mon, 10 Mar 2014 01:37:32 +0100
>>>
>>>> The mechanism is manual, no automatic way to determine it.
>>>
>>> We recognize BIOS and ACPI bugs and work around them, by looking at
>>> version information and whatnot, so you really can't convince me that
>>> something similar can't be done here perhaps in the platform code.
>>
>> Hmm, if the is a way to determine the version of that particual u-boot
>> I'd be happy to exploit that information. But I honestly doubt that.
>> Compared to u-boot bootloader and kernel interaction, BIOS and ACPI
>> are well-defined protocols.
>>
>> I personally, would prefer everybody should update his broken
>> bootloaders, but that will just not happen.
>
> What you can do is have a test that _perhaps_ covers a "broader than
> reality" list of broken bootloader cases.
>
> Then you have something the bootloader can provide which indicates
> that it has been fixed.

I think we can just pass the workaround responsibility back to the
bootloader. You rejected both easy-to-maintain workarounds for good
reasons, both bootloaders I know of can be fixed by just resuming the
PHY by bootloader commands. Users of this two boards can prepend their
tftpboot commands with an appropriate write to BMCR.

Let's just ignore this for now and wait for a really unfixable
bootloader.

Sebastian

[PATCH] net: phy: Add sysfs attribute to prevent PHY suspend

[One Thousand Gnomes]

On Mon, 10 Mar 2014 01:53:33 +0100
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:

> On 03/10/2014 01:41 AM, David Miller wrote:
> > From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> > Date: Mon, 10 Mar 2014 01:37:32 +0100
> >
> >> The mechanism is manual, no automatic way to determine it.
> >
> > We recognize BIOS and ACPI bugs and work around them, by looking at
> > version information and whatnot, so you really can't convince me that
> > something similar can't be done here perhaps in the platform code.
> 
> Hmm, if the is a way to determine the version of that particual u-boot
> I'd be happy to exploit that information.

If there isn't a way for a kernel to determine the U-boot version then
maybe that should get fixed instead. That also solves your problem
because if you can't find the uboot version you know its too old.

Alan

[PATCH] net: phy: Add sysfs attribute to prevent PHY suspend

[Florian Fainelli]

2014-03-10 7:25 GMT-07:00 One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>:
> On Mon, 10 Mar 2014 01:53:33 +0100
> Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:
>
>> On 03/10/2014 01:41 AM, David Miller wrote:
>> > From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> > Date: Mon, 10 Mar 2014 01:37:32 +0100
>> >
>> >> The mechanism is manual, no automatic way to determine it.
>> >
>> > We recognize BIOS and ACPI bugs and work around them, by looking at
>> > version information and whatnot, so you really can't convince me that
>> > something similar can't be done here perhaps in the platform code.
>>
>> Hmm, if the is a way to determine the version of that particual u-boot
>> I'd be happy to exploit that information.
>
> If there isn't a way for a kernel to determine the U-boot version then
> maybe that should get fixed instead. That also solves your problem
> because if you can't find the uboot version you know its too old.

Once you have fixed how to determine the u-boot version (which BTW, is
probably much simpler to determine from user-space by reading from the
MTD partition exposing it, and using strings on the binary blob), you
are left with the other bootloaders out there which also do not clear
the BMCR_PWRDOWN bit of your PHY and will fail booting off the network
as well.

While I agree that there should be something done to help any OS
determine which bootloader version/model it got booted from (ala. x86
with type_of_bootloader), we still need a way to control this policy
(auto-suspending the unused Ethernet PHYs) from Linux, regardless of
whether the boot program is broken or not.
-- 
Florian