| Message ID | 1394192092-27461-1-git-send-email-sebastian.hesselbarth@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@vger.kernel.org > Cc: linux-doc@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@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@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 >
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.
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
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.
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
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.
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
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.
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
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
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.
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@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;
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@vger.kernel.org Cc: linux-doc@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@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(+)