| Message ID | 1393174719-20806-1-git-send-email-sebastian.hesselbarth@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@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 >
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 <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@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
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
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.
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
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 <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@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.
> > 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 <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....
> 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
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);
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@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org --- drivers/net/phy/phy_device.c | 10 ++++++++++ 1 file changed, 10 insertions(+)