LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] net: phy: allow scanning busses with missing phys
@ 2018-04-24 16:09 Alexandre Belloni
  2018-04-24 16:37 ` Florian Fainelli
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Alexandre Belloni @ 2018-04-24 16:09 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Florian Fainelli, David S . Miller, Allan Nielsen,
	Thomas Petazzoni, netdev, linux-kernel, Alexandre Belloni

Some MDIO busses will error out when trying to read a phy address with no
phy present at that address. In that case, probing the bus will fail
because __mdiobus_register() is scanning the bus for all possible phys
addresses.

In case MII_PHYSID1 returns -EIO or -ENODEV, consider there is no phy at
this address and set the phy ID to 0xffffffff which is then properly
handled in get_phy_device().

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/net/phy/phy_device.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index ac23322a32e1..9e4ba8e80a18 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -535,8 +535,17 @@ static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id,
 
 	/* Grab the bits from PHYIR1, and put them in the upper half */
 	phy_reg = mdiobus_read(bus, addr, MII_PHYSID1);
-	if (phy_reg < 0)
+	if (phy_reg < 0) {
+		/* if there is no device, return without an error so scanning
+		 * the bus works properly
+		 */
+		if (phy_reg == -EIO || phy_reg == -ENODEV) {
+			*phy_id = 0xffffffff;
+			return 0;
+		}
+
 		return -EIO;
+	}
 
 	*phy_id = (phy_reg & 0xffff) << 16;
 
-- 
2.17.0

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: phy: allow scanning busses with missing phys
  2018-04-24 16:09 [PATCH] net: phy: allow scanning busses with missing phys Alexandre Belloni
@ 2018-04-24 16:37 ` Florian Fainelli
  2018-04-24 18:01   ` Andrew Lunn
  2018-04-24 18:09   ` Alexandre Belloni
  2018-04-25  1:09 ` Florian Fainelli
  2018-04-25 17:01 ` David Miller
  2 siblings, 2 replies; 7+ messages in thread
From: Florian Fainelli @ 2018-04-24 16:37 UTC (permalink / raw)
  To: Alexandre Belloni, Andrew Lunn
  Cc: David S . Miller, Allan Nielsen, Thomas Petazzoni, netdev, linux-kernel



On 04/24/2018 09:09 AM, Alexandre Belloni wrote:
> Some MDIO busses will error out when trying to read a phy address with no
> phy present at that address. In that case, probing the bus will fail
> because __mdiobus_register() is scanning the bus for all possible phys
> addresses.
> 
> In case MII_PHYSID1 returns -EIO or -ENODEV, consider there is no phy at
> this address and set the phy ID to 0xffffffff which is then properly
> handled in get_phy_device().

Humm, why not have your MDIO bus implementation do the scanning itself
in a reset() callback, which happens before probing the bus, and based
on the results, set phy_mask accordingly such that only PHYs present are
populated?

My only concern with your change is that we are having a special
treatment for EIO and ENODEV, so we must make sure MDIO bus drivers are
all conforming to that.

> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>  drivers/net/phy/phy_device.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index ac23322a32e1..9e4ba8e80a18 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -535,8 +535,17 @@ static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id,
>  
>  	/* Grab the bits from PHYIR1, and put them in the upper half */
>  	phy_reg = mdiobus_read(bus, addr, MII_PHYSID1);
> -	if (phy_reg < 0)
> +	if (phy_reg < 0) {
> +		/* if there is no device, return without an error so scanning
> +		 * the bus works properly
> +		 */
> +		if (phy_reg == -EIO || phy_reg == -ENODEV) {
> +			*phy_id = 0xffffffff;
> +			return 0;
> +		}
> +
>  		return -EIO;
> +	}
>  
>  	*phy_id = (phy_reg & 0xffff) << 16;
>  
> 

-- 
Florian

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: phy: allow scanning busses with missing phys
  2018-04-24 16:37 ` Florian Fainelli
@ 2018-04-24 18:01   ` Andrew Lunn
  2018-04-25  1:08     ` Florian Fainelli
  2018-04-24 18:09   ` Alexandre Belloni
  1 sibling, 1 reply; 7+ messages in thread
From: Andrew Lunn @ 2018-04-24 18:01 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Alexandre Belloni, David S . Miller, Allan Nielsen,
	Thomas Petazzoni, netdev, linux-kernel

On Tue, Apr 24, 2018 at 09:37:09AM -0700, Florian Fainelli wrote:
> 
> 
> On 04/24/2018 09:09 AM, Alexandre Belloni wrote:
> > Some MDIO busses will error out when trying to read a phy address with no
> > phy present at that address. In that case, probing the bus will fail
> > because __mdiobus_register() is scanning the bus for all possible phys
> > addresses.
> > 
> > In case MII_PHYSID1 returns -EIO or -ENODEV, consider there is no phy at
> > this address and set the phy ID to 0xffffffff which is then properly
> > handled in get_phy_device().
> 
> Humm, why not have your MDIO bus implementation do the scanning itself
> in a reset() callback, which happens before probing the bus, and based
> on the results, set phy_mask accordingly such that only PHYs present are
> populated?

Hi Florian

Seems a bit odd have the driver do this, when the core could.

> My only concern with your change is that we are having a special
> treatment for EIO and ENODEV, so we must make sure MDIO bus drivers are
> all conforming to that.
 
I don't see how it could be a problem. It used to be any error was a
real error, and would stop the bus from loading. It now means there is
nothing there. The only possible side effect is an mdio bus driver
might remain loaded without any devices if all reads return
ENODEV/EIO, were as before it would probably never load. 

	    Andrew

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: phy: allow scanning busses with missing phys
  2018-04-24 16:37 ` Florian Fainelli
  2018-04-24 18:01   ` Andrew Lunn
@ 2018-04-24 18:09   ` Alexandre Belloni
  1 sibling, 0 replies; 7+ messages in thread
From: Alexandre Belloni @ 2018-04-24 18:09 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Andrew Lunn, David S . Miller, Allan Nielsen, Thomas Petazzoni,
	netdev, linux-kernel

On 24/04/2018 09:37:09-0700, Florian Fainelli wrote:
> 
> 
> On 04/24/2018 09:09 AM, Alexandre Belloni wrote:
> > Some MDIO busses will error out when trying to read a phy address with no
> > phy present at that address. In that case, probing the bus will fail
> > because __mdiobus_register() is scanning the bus for all possible phys
> > addresses.
> > 
> > In case MII_PHYSID1 returns -EIO or -ENODEV, consider there is no phy at
> > this address and set the phy ID to 0xffffffff which is then properly
> > handled in get_phy_device().
> 
> Humm, why not have your MDIO bus implementation do the scanning itself
> in a reset() callback, which happens before probing the bus, and based
> on the results, set phy_mask accordingly such that only PHYs present are
> populated?
> 
> My only concern with your change is that we are having a special
> treatment for EIO and ENODEV, so we must make sure MDIO bus drivers are
> all conforming to that.
> 

That was what I was doing in [1] but it seems that Andrew preferred this way.

The third solution I was seeing was to return phy_reg instead of -EIO so
the MDIO driver can return -ENODEV and that would be passed to
get_phy_device(). __mdiobus_register() seems to handle -ENODEV properly.

My coccinelle-fu is not great but the following drivers can return
-ENODEV from their read callback:

drivers/net/ethernet/marvell/mvmdio.c
drivers/net/ethernet/hisilicon/hix5hd2_gmac.c (seeing the error message,
this has probably been copy pasted)

[1] https://marc.info/?l=linux-netdev&m=152183609927933&w=2

> > 
> > Suggested-by: Andrew Lunn <andrew@lunn.ch>
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > ---
> >  drivers/net/phy/phy_device.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index ac23322a32e1..9e4ba8e80a18 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -535,8 +535,17 @@ static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id,
> >  
> >  	/* Grab the bits from PHYIR1, and put them in the upper half */
> >  	phy_reg = mdiobus_read(bus, addr, MII_PHYSID1);
> > -	if (phy_reg < 0)
> > +	if (phy_reg < 0) {
> > +		/* if there is no device, return without an error so scanning
> > +		 * the bus works properly
> > +		 */
> > +		if (phy_reg == -EIO || phy_reg == -ENODEV) {
> > +			*phy_id = 0xffffffff;
> > +			return 0;
> > +		}
> > +
> >  		return -EIO;
> > +	}
> >  
> >  	*phy_id = (phy_reg & 0xffff) << 16;
> >  
> > 
> 
> -- 
> Florian

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: phy: allow scanning busses with missing phys
  2018-04-24 18:01   ` Andrew Lunn
@ 2018-04-25  1:08     ` Florian Fainelli
  0 siblings, 0 replies; 7+ messages in thread
From: Florian Fainelli @ 2018-04-25  1:08 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Alexandre Belloni, David S . Miller, Allan Nielsen,
	Thomas Petazzoni, netdev, linux-kernel

On 04/24/2018 11:01 AM, Andrew Lunn wrote:
> On Tue, Apr 24, 2018 at 09:37:09AM -0700, Florian Fainelli wrote:
>>
>>
>> On 04/24/2018 09:09 AM, Alexandre Belloni wrote:
>>> Some MDIO busses will error out when trying to read a phy address with no
>>> phy present at that address. In that case, probing the bus will fail
>>> because __mdiobus_register() is scanning the bus for all possible phys
>>> addresses.
>>>
>>> In case MII_PHYSID1 returns -EIO or -ENODEV, consider there is no phy at
>>> this address and set the phy ID to 0xffffffff which is then properly
>>> handled in get_phy_device().
>>
>> Humm, why not have your MDIO bus implementation do the scanning itself
>> in a reset() callback, which happens before probing the bus, and based
>> on the results, set phy_mask accordingly such that only PHYs present are
>> populated?
> 
> Hi Florian
> 
> Seems a bit odd have the driver do this, when the core could.

I could see arguments for doing it in either location, if this is done
in the driver, the driver is responsible for knowing what addresses will
respond, which does not seem to big of a stretch. If done by core, we
lift the driver from taking care of that. Either way is fine, really.

> 
>> My only concern with your change is that we are having a special
>> treatment for EIO and ENODEV, so we must make sure MDIO bus drivers are
>> all conforming to that.
>  
> I don't see how it could be a problem. It used to be any error was a
> real error, and would stop the bus from loading. It now means there is
> nothing there. The only possible side effect is an mdio bus driver
> might remain loaded without any devices if all reads return
> ENODEV/EIO, were as before it would probably never load. 

Right, though that does not seem to be a big problem.

> 
> 	    Andrew
> 


-- 
Florian

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: phy: allow scanning busses with missing phys
  2018-04-24 16:09 [PATCH] net: phy: allow scanning busses with missing phys Alexandre Belloni
  2018-04-24 16:37 ` Florian Fainelli
@ 2018-04-25  1:09 ` Florian Fainelli
  2018-04-25 17:01 ` David Miller
  2 siblings, 0 replies; 7+ messages in thread
From: Florian Fainelli @ 2018-04-25  1:09 UTC (permalink / raw)
  To: Alexandre Belloni, Andrew Lunn
  Cc: David S . Miller, Allan Nielsen, Thomas Petazzoni, netdev, linux-kernel

On 04/24/2018 09:09 AM, Alexandre Belloni wrote:
> Some MDIO busses will error out when trying to read a phy address with no
> phy present at that address. In that case, probing the bus will fail
> because __mdiobus_register() is scanning the bus for all possible phys
> addresses.
> 
> In case MII_PHYSID1 returns -EIO or -ENODEV, consider there is no phy at
> this address and set the phy ID to 0xffffffff which is then properly
> handled in get_phy_device().
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: phy: allow scanning busses with missing phys
  2018-04-24 16:09 [PATCH] net: phy: allow scanning busses with missing phys Alexandre Belloni
  2018-04-24 16:37 ` Florian Fainelli
  2018-04-25  1:09 ` Florian Fainelli
@ 2018-04-25 17:01 ` David Miller
  2 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2018-04-25 17:01 UTC (permalink / raw)
  To: alexandre.belloni
  Cc: andrew, f.fainelli, Allan.Nielsen, thomas.petazzoni, netdev,
	linux-kernel

From: Alexandre Belloni <alexandre.belloni@bootlin.com>
Date: Tue, 24 Apr 2018 18:09:04 +0200

> Some MDIO busses will error out when trying to read a phy address with no
> phy present at that address. In that case, probing the bus will fail
> because __mdiobus_register() is scanning the bus for all possible phys
> addresses.
> 
> In case MII_PHYSID1 returns -EIO or -ENODEV, consider there is no phy at
> this address and set the phy ID to 0xffffffff which is then properly
> handled in get_phy_device().
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

Applied, thank you.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-04-25 17:01 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-24 16:09 [PATCH] net: phy: allow scanning busses with missing phys Alexandre Belloni
2018-04-24 16:37 ` Florian Fainelli
2018-04-24 18:01   ` Andrew Lunn
2018-04-25  1:08     ` Florian Fainelli
2018-04-24 18:09   ` Alexandre Belloni
2018-04-25  1:09 ` Florian Fainelli
2018-04-25 17:01 ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).