diff mbox series

[1/4] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

Message ID 20180820121238.7779-1-a.fatoum@pengutronix.de
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series [1/4] net: macb: Fix regression breaking non-MDIO fixed-link PHYs | expand

Commit Message

Ahmad Fatoum Aug. 20, 2018, 12:12 p.m. UTC 
The referenced commit broke initializing macb on the EVB-KSZ9477 eval board.
There, of_mdiobus_register was called even for the fixed-link representing
the SPI-connected switch PHY, with the result that the driver attempts to
enumerate PHYs on a non-existent MDIO bus:

	libphy: MACB_mii_bus: probed
	mdio_bus f0028000.ethernet-ffffffff: fixed-link has invalid PHY address
	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 0
        [snip]
	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 31
	macb f0028000.ethernet: broken fixed-link specification

Cc: <stable@vger.kernel.org>
Fixes: 739de9a1563a ("net: macb: Reorganize macb_mii bringup")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/net/ethernet/cadence/macb_main.c | 27 +++++++++++++++---------
 1 file changed, 17 insertions(+), 10 deletions(-)

Fixes since v1:
	Added one more error path for failing to register fixed-link
	Fixed a whitespace issue

Comments

Andrew Lunn Aug. 20, 2018, 1:55 p.m. UTC | #1 
On Mon, Aug 20, 2018 at 02:12:35PM +0200, Ahmad Fatoum wrote:
> The referenced commit broke initializing macb on the EVB-KSZ9477 eval board.
> There, of_mdiobus_register was called even for the fixed-link representing
> the SPI-connected switch PHY, with the result that the driver attempts to
> enumerate PHYs on a non-existent MDIO bus:
> 
> 	libphy: MACB_mii_bus: probed

So there are two different things here:

> 	mdio_bus f0028000.ethernet-ffffffff: fixed-link has invalid PHY address
> 	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 0
>         [snip]
> 	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 31

These are the result of the fixed-link being considered a PHY in
of_mdiobus_register(). Patch 2 fixes that, turns it into a single
warning.

> 	macb f0028000.ethernet: broken fixed-link specification

Why is of_phy_register_fixed_link(np) failing?

> 
> Cc: <stable@vger.kernel.org>
> Fixes: 739de9a1563a ("net: macb: Reorganize macb_mii bringup")
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 27 +++++++++++++++---------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> Fixes since v1:
> 	Added one more error path for failing to register fixed-link
> 	Fixed a whitespace issue
> 
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index dc09f9a8a49b..ef6ce8691443 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -482,11 +482,6 @@ static int macb_mii_probe(struct net_device *dev)
>  
>  	if (np) {
>  		if (of_phy_is_fixed_link(np)) {
> -			if (of_phy_register_fixed_link(np) < 0) {
> -				dev_err(&bp->pdev->dev,
> -					"broken fixed-link specification\n");
> -				return -ENODEV;
> -			}

As a separate patch, please can you use the error code which
of_phy_register_fixed_link() returns, not -ENODEV. It is possible it
is returning EPROBE_DEFER.

   Andrew
Ahmad Fatoum Aug. 20, 2018, 3:56 p.m. UTC | #2 
On 08/20/2018 03:55 PM, Andrew Lunn wrote:
> Why is of_phy_register_fixed_link(np) failing?

Apparently, the fixed-link's gpio's FLAG_REQUESTED bit remained set
causing gpiod_request_commit to return -EBUSY in (v4.18.0):

[<c0667750>] (gpiod_request_commit) from [<c066890c>] (gpiod_request+0x64/0x88)             
[<c066890c>] (gpiod_request) from [<c066a338>] (gpiod_get_from_of_node+0x48/0x13c)          
[<c066a338>] (gpiod_get_from_of_node) from [<c094b92c>] (mdiobus_register_device+0x70/0x124)
[<c094b92c>] (mdiobus_register_device) from [<c0949d2c>] (phy_device_register+0xc/0xa0)     
[<c0949d2c>] (phy_device_register) from [<c094e824>] (fixed_phy_register+0xe8/0x1f8)        
[<c094e824>] (fixed_phy_register) from [<c0b84260>] (of_phy_register_fixed_link+0x150/0x1e4)
[<c0b84260>] (of_phy_register_fixed_link) from [<c0964908>] (macb_probe+0x548/0xa7c)        
[<c0964908>] (macb_probe) from [<c086ebec>] (platform_drv_probe+0x48/0x98)                  

But that's a separate issue, I'll remove this line from the commit message...
Andrew Lunn Aug. 20, 2018, 7:06 p.m. UTC | #3 
On Mon, Aug 20, 2018 at 05:56:53PM +0200, Ahmad Fatoum wrote:
> On 08/20/2018 03:55 PM, Andrew Lunn wrote:
> > Why is of_phy_register_fixed_link(np) failing?
> 
> Apparently, the fixed-link's gpio's FLAG_REQUESTED bit remained set
> causing gpiod_request_commit to return -EBUSY in (v4.18.0):
> 
> [<c0667750>] (gpiod_request_commit) from [<c066890c>] (gpiod_request+0x64/0x88)             
> [<c066890c>] (gpiod_request) from [<c066a338>] (gpiod_get_from_of_node+0x48/0x13c)          
> [<c066a338>] (gpiod_get_from_of_node) from [<c094b92c>] (mdiobus_register_device+0x70/0x124)
> [<c094b92c>] (mdiobus_register_device) from [<c0949d2c>] (phy_device_register+0xc/0xa0)     
> [<c0949d2c>] (phy_device_register) from [<c094e824>] (fixed_phy_register+0xe8/0x1f8)        
> [<c094e824>] (fixed_phy_register) from [<c0b84260>] (of_phy_register_fixed_link+0x150/0x1e4)
> [<c0b84260>] (of_phy_register_fixed_link) from [<c0964908>] (macb_probe+0x548/0xa7c)        
> [<c0964908>] (macb_probe) from [<c086ebec>] (platform_drv_probe+0x48/0x98)                  
> 
> But that's a separate issue, I'll remove this line from the commit message...

I would actually say, this is your real issue here. The warnings are
annoying, but i don't think they are fatal. This -EBUSY is what is
stopping the driver from loading, causing the real regression.

I'm guessing, but i think you will find the driver is loading once,
but hits a EPROBE_DEFFER condition, after getting the gpio. It does
not release the gpio correctly. Sometime later, it gets loaded again,
but the gpio is now in use, so you get the -EBUSY.

So check the error paths, and make sure cleanup is being done correct.
It could also be a phylib core bug...

   Andrew
Ahmad Fatoum Aug. 21, 2018, 8:26 a.m. UTC | #4 
On 08/20/2018 09:06 PM, Andrew Lunn wrote:
> I would actually say, this is your real issue here. The warnings are
> annoying, but i don't think they are fatal. This -EBUSY is what is
> stopping the driver from loading, causing the real regression.

My real issue is that a specific commit broke the driver and I would like
to partially revert that offending commit.

> I'm guessing, but i think you will find the driver is loading once,
> but hits a EPROBE_DEFFER condition, after getting the gpio. It does
> not release the gpio correctly. Sometime later, it gets loaded again,
> but the gpio is now in use, so you get the -EBUSY.
> 
> So check the error paths, and make sure cleanup is being done correct.
> It could also be a phylib core bug...

I've traced it some more: While mdiobus_register fails to find a PHY,
creation of the "MDIO" bus is still successful and it returns successfully,
having claimed the reset GPIO (These functions should really be called
miibus_register...).

of_phy_fixed_link_register tries to claim the same GPIO and fails.

A fix for that would be something along the lines of da47b45 
("phy: add support for a reset-gpio specification"), which caused a regression
and was unfortunately later reverted.

But regardless, there shouldn't have been an of_mdiobus_register and a MDIO bus probe
before registering the fixed-link in the first place and my patch remedies that.

Reintroducing da47b45 would be out-of-scope for this patch series.


Cheers
Ahmad
Andrew Lunn Aug. 21, 2018, 1:34 p.m. UTC | #5 
> I've traced it some more: While mdiobus_register fails to find a PHY,
> creation of the "MDIO" bus is still successful and it returns successfully,
> having claimed the reset GPIO.
> 
> of_phy_fixed_link_register tries to claim the same GPIO and fails.
 
I don't see where this is happening. It is looking for a gpio called
'link-gpios'.

> But regardless, there shouldn't have been an of_mdiobus_register and a MDIO bus probe
> before registering the fixed-link in the first place and my patch remedies that.

There are cases where you need both, e.g a switch controller over
MDIO. So we cannot make it one or the other.

However, there are currently no such boards. So far "net", lets go
with your partial revert patch. But we also need patches for
"net-next" which put it back again, allows both fixed-link and an
mdiobus inside a subnode, and not break backwards compatibility.

	Andrew
Ahmad Fatoum Aug. 21, 2018, 2:59 p.m. UTC | #6 
On 08/21/2018 03:34 PM, Andrew Lunn wrote:
> I don't see where this is happening. It is looking for a gpio called
> 'link-gpios'.

First while registering the MDIO bus in __mdiobus_register:

	gpiod = devm_gpiod_get_optional(&bus->dev, "reset", GPIOD_OUT_LOW);

and then again when registering the fixed-link in mdiobus_register_gpiod:

	gpiod = fwnode_get_named_gpiod(&mdiodev->dev.of_node->fwnode,
				       "reset-gpios", 0, GPIOD_OUT_LOW,
				       "PHY reset");
 
>> But regardless, there shouldn't have been an of_mdiobus_register and a MDIO bus probe
>> before registering the fixed-link in the first place and my patch remedies that.
> 
> There are cases where you need both, e.g a switch controller over
> MDIO. So we cannot make it one or the other.

I see.

> However, there are currently no such boards. So far "net", lets go
> with your partial revert patch.

I will resend the first patch.

> But we also need patches for
> "net-next" which put it back again, allows both fixed-link and an
> mdiobus inside a subnode, and not break backwards compatibility.

I'll resend the remainder when net-next opens again.


Cheers
Ahmad
diff mbox series

Patch

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index dc09f9a8a49b..ef6ce8691443 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -482,11 +482,6 @@  static int macb_mii_probe(struct net_device *dev)
 
 	if (np) {
 		if (of_phy_is_fixed_link(np)) {
-			if (of_phy_register_fixed_link(np) < 0) {
-				dev_err(&bp->pdev->dev,
-					"broken fixed-link specification\n");
-				return -ENODEV;
-			}
 			bp->phy_node = of_node_get(np);
 		} else {
 			bp->phy_node = of_parse_phandle(np, "phy-handle", 0);
@@ -569,7 +564,7 @@  static int macb_mii_init(struct macb *bp)
 {
 	struct macb_platform_data *pdata;
 	struct device_node *np;
-	int err;
+	int err = -ENXIO;
 
 	/* Enable management port */
 	macb_writel(bp, NCR, MACB_BIT(MPE));
@@ -592,12 +587,23 @@  static int macb_mii_init(struct macb *bp)
 	dev_set_drvdata(&bp->dev->dev, bp->mii_bus);
 
 	np = bp->pdev->dev.of_node;
-	if (pdata)
-		bp->mii_bus->phy_mask = pdata->phy_mask;
+	if (np && of_phy_is_fixed_link(np)) {
+		if (of_phy_register_fixed_link(np) < 0) {
+			dev_err(&bp->pdev->dev,
+				"broken fixed-link specification\n");
+			goto err_out_free_mdiobus;
+		}
+
+		err = mdiobus_register(bp->mii_bus);
+	} else {
+		if (pdata)
+			bp->mii_bus->phy_mask = pdata->phy_mask;
+
+		err = of_mdiobus_register(bp->mii_bus, np);
+	}
 
-	err = of_mdiobus_register(bp->mii_bus, np);
 	if (err)
-		goto err_out_free_mdiobus;
+		goto err_out_free_fixed_link;
 
 	err = macb_mii_probe(bp->dev);
 	if (err)
@@ -607,6 +613,7 @@  static int macb_mii_init(struct macb *bp)
 
 err_out_unregister_bus:
 	mdiobus_unregister(bp->mii_bus);
+err_out_free_fixed_link:
 	if (np && of_phy_is_fixed_link(np))
 		of_phy_deregister_fixed_link(np);
 err_out_free_mdiobus: