[PATCH net-next] of: of_mdio: Check if MDIO bus controller is available

[PATCH net-next] of: of_mdio: Check if MDIO bus controller is available

[Florian Fainelli]

Add a check whether the 'struct device_node' pointer passed to
of_mdiobus_register() is an available (aka enabled) node in the Device
Tree.

Rationale for doing this are cases where an Ethernet MAC provides a MDIO
bus controller and node, and an additional Ethernet MAC might be
connecting its PHY/switches to that first MDIO bus controller, while
still embedding one internally which is therefore marked as "disabled".

Instead of sprinkling checks like these in callers of
of_mdiobus_register(), do this in a central location.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/of/of_mdio.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index b622b33dbf93..2f497790be1b 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -209,6 +209,10 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 	bool scanphys = false;
 	int addr, rc;
 
+	/* Do not continue if the node is disabled */
+	if (!of_device_is_available(np))
+		return -EINVAL;
+
 	/* Mask out all PHYs from auto probing.  Instead the PHYs listed in
 	 * the device tree are populated after the bus has been registered */
 	mdio->phy_mask = ~0;
-- 
2.1.0

Re: [PATCH net-next] of: of_mdio: Check if MDIO bus controller is available

[Andrew Lunn]

On Thu, Apr 28, 2016 at 02:55:10PM -0700, Florian Fainelli wrote:
> Add a check whether the 'struct device_node' pointer passed to
> of_mdiobus_register() is an available (aka enabled) node in the Device
> Tree.
> 
> Rationale for doing this are cases where an Ethernet MAC provides a MDIO
> bus controller and node, and an additional Ethernet MAC might be
> connecting its PHY/switches to that first MDIO bus controller, while
> still embedding one internally which is therefore marked as "disabled".
> 
> Instead of sprinkling checks like these in callers of
> of_mdiobus_register(), do this in a central location.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>
> ---
>  drivers/of/of_mdio.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index b622b33dbf93..2f497790be1b 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -209,6 +209,10 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>  	bool scanphys = false;
>  	int addr, rc;
>  
> +	/* Do not continue if the node is disabled */
> +	if (!of_device_is_available(np))
> +		return -EINVAL;

Could be bike shedding, but would ENODEV be better?

Some callers are going to have to look at the return value and decide
if it is a fatal error, and fail the whole probe, or a non-fatal error
and they should keep going. ENODEV seems less fatal...

Other than that,

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next] of: of_mdio: Check if MDIO bus controller is available

[Andrew Lunn]

On Thu, Apr 28, 2016 at 02:55:10PM -0700, Florian Fainelli wrote:
> Add a check whether the 'struct device_node' pointer passed to
> of_mdiobus_register() is an available (aka enabled) node in the Device
> Tree.
> 
> Rationale for doing this are cases where an Ethernet MAC provides a MDIO
> bus controller and node, and an additional Ethernet MAC might be
> connecting its PHY/switches to that first MDIO bus controller, while
> still embedding one internally which is therefore marked as "disabled".
> 
> Instead of sprinkling checks like these in callers of
> of_mdiobus_register(), do this in a central location.

I think this discussion has shown there is no documented best
practices for MDIO bus drivers and how PHYs nodes are placed within
device tree. Maybe you could document the generic MDIO binding, both
as integrated into a MAC device node, and as a separate device?

	   Andrew

Re: [PATCH net-next] of: of_mdio: Check if MDIO bus controller is available

[Florian Fainelli]

On 28/04/16 15:12, Andrew Lunn wrote:
> On Thu, Apr 28, 2016 at 02:55:10PM -0700, Florian Fainelli wrote:
>> Add a check whether the 'struct device_node' pointer passed to
>> of_mdiobus_register() is an available (aka enabled) node in the Device
>> Tree.
>>
>> Rationale for doing this are cases where an Ethernet MAC provides a MDIO
>> bus controller and node, and an additional Ethernet MAC might be
>> connecting its PHY/switches to that first MDIO bus controller, while
>> still embedding one internally which is therefore marked as "disabled".
>>
>> Instead of sprinkling checks like these in callers of
>> of_mdiobus_register(), do this in a central location.
> 
> I think this discussion has shown there is no documented best
> practices for MDIO bus drivers and how PHYs nodes are placed within
> device tree. Maybe you could document the generic MDIO binding, both
> as integrated into a MAC device node, and as a separate device?

Fair enough, I will submit something after re-spining this patch to use
-ENODEV, which I agree is a better return code. Did you want me to
remove that blurb from the commit message?
-- 
Florian

Re: [PATCH net-next] of: of_mdio: Check if MDIO bus controller is available

[Andrew Lunn]

> Fair enough, I will submit something after re-spining this patch to use
> -ENODEV, which I agree is a better return code. Did you want me to
> remove that blurb from the commit message?

Blurb looks good. More blurb is better than less...

      Andrew

Re: [PATCH net-next] of: of_mdio: Check if MDIO bus controller is available

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Thu, 28 Apr 2016 14:55:10 -0700

> Add a check whether the 'struct device_node' pointer passed to
> of_mdiobus_register() is an available (aka enabled) node in the Device
> Tree.
> 
> Rationale for doing this are cases where an Ethernet MAC provides a MDIO
> bus controller and node, and an additional Ethernet MAC might be
> connecting its PHY/switches to that first MDIO bus controller, while
> still embedding one internally which is therefore marked as "disabled".
> 
> Instead of sprinkling checks like these in callers of
> of_mdiobus_register(), do this in a central location.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Applied, thanks Florian.