| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <FD695891-F3B8-4388-8887-A9DC997828B4@gmail.com>
Date: Sat, 21 Oct 2017 19:01:45 -0700
From: Florian Fainelli <f.fainelli@...il.com>
To: Geert Uytterhoeven <geert+renesas@...der.be>,
Andrew Lunn <andrew@...n.ch>, Rob Herring <robh+dt@...nel.org>,
Frank Rowand <frowand.list@...il.com>
CC: netdev@...r.kernel.org, devicetree@...r.kernel.org,
linux-renesas-soc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] of_mdio: Fix broken PHY IRQ in case of probe deferral
On October 18, 2017 4:54:03 AM PDT, Geert Uytterhoeven <geert+renesas@...der.be> wrote:
>If an Ethernet PHY is initialized before the interrupt controller it is
>connected to, a message like the following is printed:
>
> irq: no irq domain found for /interrupt-controller@...c0000 !
>
>However, the actual error is ignored, leading to a non-functional
>(POLL)
>PHY interrupt later:
>
>Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY driver
>[Micrel KSZ8041RNLI] (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01,
>irq=POLL)
>
>Depending on whether the PHY driver will fall back to polling, Ethernet
>may or may not work.
>
>To fix this:
> 1. Switch of_mdiobus_register_phy() from irq_of_parse_and_map() to
> of_irq_get().
> Unlike the former, the latter returns -EPROBE_DEFER if the
> interrupt controller is not yet available, so this condition can be
> detected.
> Other errors are handled the same as before, i.e. use the passed
> mdio->irq[addr] as interrupt.
> 2. Propagate and handle errors from of_mdiobus_register_phy() and
> of_mdiobus_register_device().
>
>Signed-off-by: Geert Uytterhoeven <geert+renesas@...der.be>
Reviewed-by : Florian Fainelli <f.fainelli@...il.com>
I still can't make sure this is not a problem for multiple PHYs hanging off the same bus, but like anything else, we'll deal with problems later if they arise.
>---
>Seen on e.g. r8a7791/koelsch when using the new CPG/MSSR clock driver,
>which will hit upstream in v4.15. I assume it always happened on RZ/G1
>in mainline.
>
>The actual patch is unchanged since v1, sent on May 18. Obviously I
>still cannot test it on a system with multiple PHYs, just like v1.
>
>How can we proceed?
>
>Note that if you are worried about the MDIO subsystem not handling
>(partial) teardown and reprobe correctly in the presence of multiple
>PHYs, that can already be triggered since commit a5597008dbc23087
>("phy:
>fixed_phy: Add gpio to determine link up/down."), which handles
>EPROBE_DEFER for GPIOs.
>
>Thanks!
>
>v2:
> - Update for non-functional interrupts being printed as "POLL" instead
> of "-1" since commit 5e369aefdce4818c ("net: stmmac: Delete dead
> code for MDIO registration").
>---
> drivers/of/of_mdio.c | 39 +++++++++++++++++++++++++++------------
> 1 file changed, 27 insertions(+), 12 deletions(-)
>
>diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
>index d94dd8b77abd5140..98258583abb0b405 100644
>--- a/drivers/of/of_mdio.c
>+++ b/drivers/of/of_mdio.c
>@@ -44,7 +44,7 @@ static int of_get_phy_id(struct device_node *device,
>u32 *phy_id)
> return -EINVAL;
> }
>
>-static void of_mdiobus_register_phy(struct mii_bus *mdio,
>+static int of_mdiobus_register_phy(struct mii_bus *mdio,
> struct device_node *child, u32 addr)
> {
> struct phy_device *phy;
>@@ -60,9 +60,13 @@ static void of_mdiobus_register_phy(struct mii_bus
>*mdio,
> else
> phy = get_phy_device(mdio, addr, is_c45);
> if (IS_ERR(phy))
>- return;
>+ return PTR_ERR(phy);
>
>- rc = irq_of_parse_and_map(child, 0);
>+ rc = of_irq_get(child, 0);
>+ if (rc == -EPROBE_DEFER) {
>+ phy_device_free(phy);
>+ return rc;
>+ }
> if (rc > 0) {
> phy->irq = rc;
> mdio->irq[addr] = rc;
>@@ -84,22 +88,23 @@ static void of_mdiobus_register_phy(struct mii_bus
>*mdio,
> if (rc) {
> phy_device_free(phy);
> of_node_put(child);
>- return;
>+ return rc;
> }
>
> dev_dbg(&mdio->dev, "registered phy %s at address %i\n",
> child->name, addr);
>+ return 0;
> }
>
>-static void of_mdiobus_register_device(struct mii_bus *mdio,
>- struct device_node *child, u32 addr)
>+static int of_mdiobus_register_device(struct mii_bus *mdio,
>+ struct device_node *child, u32 addr)
> {
> struct mdio_device *mdiodev;
> int rc;
>
> mdiodev = mdio_device_create(mdio, addr);
> if (IS_ERR(mdiodev))
>- return;
>+ return PTR_ERR(mdiodev);
>
> /* Associate the OF node with the device structure so it
> * can be looked up later.
>@@ -112,11 +117,12 @@ static void of_mdiobus_register_device(struct
>mii_bus *mdio,
> if (rc) {
> mdio_device_free(mdiodev);
> of_node_put(child);
>- return;
>+ return rc;
> }
>
> dev_dbg(&mdio->dev, "registered mdio device %s at address %i\n",
> child->name, addr);
>+ return 0;
> }
>
> /* The following is a list of PHY compatible strings which appear in
>@@ -219,9 +225,11 @@ int of_mdiobus_register(struct mii_bus *mdio,
>struct device_node *np)
> }
>
> if (of_mdiobus_child_is_phy(child))
>- of_mdiobus_register_phy(mdio, child, addr);
>+ rc = of_mdiobus_register_phy(mdio, child, addr);
> else
>- of_mdiobus_register_device(mdio, child, addr);
>+ rc = of_mdiobus_register_device(mdio, child, addr);
>+ if (rc)
>+ goto unregister;
> }
>
> if (!scanphys)
>@@ -242,12 +250,19 @@ int of_mdiobus_register(struct mii_bus *mdio,
>struct device_node *np)
> dev_info(&mdio->dev, "scan phy %s at address %i\n",
> child->name, addr);
>
>- if (of_mdiobus_child_is_phy(child))
>- of_mdiobus_register_phy(mdio, child, addr);
>+ if (of_mdiobus_child_is_phy(child)) {
>+ rc = of_mdiobus_register_phy(mdio, child, addr);
>+ if (rc)
>+ goto unregister;
>+ }
> }
> }
>
> return 0;
>+
>+unregister:
>+ mdiobus_unregister(mdio);
>+ return rc;
> }
> EXPORT_SYMBOL(of_mdiobus_register);
>
--
Florian
Powered by blists - more mailing lists