netdev - Re: [PATCH] stmmac: Don't exit mdio registration when mdio subnode is not found in the DTS

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 2 Jan 2016 16:11:18 +0100
From:	Romain Perier <romain.perier@...il.com>
To:	Florian Fainelli <f.fainelli@...il.com>
Cc:	peppe.cavallaro@...com, netdev <netdev@...r.kernel.org>,
	"open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>
Subject: Re: [PATCH] stmmac: Don't exit mdio registration when mdio subnode is
 not found in the DTS

Hi all,

2016-01-02 6:51 GMT+01:00 Florian Fainelli <f.fainelli@...il.com>:
> On December 29, 2015 6:05:35 AM PST, Romain Perier <romain.perier@...il.com> wrote:
>>Originally, most of the platforms using this driver did not define an
>>mdio subnode
>>in the devicetree. Commit e34d65 ("stmmac: create of compatible mdio
>>bus for stmmac driver")
>>introduced a backward compatibily issue by using of_mdiobus_register
>>explicitly
>>with an mdio subnode. This patch fixes the issue by calling the
>>function
>>mdiobus_register, when mdio subnode is not found. The driver is now
>>compatible
>>with both modes.
>
> Looks reasonable to me, though you will want to make sure the different DTSes get updated to include the proper compatible node for the MDIO bus.
>
>>
>>Signed-off-by: Romain Perier <romain.perier@...il.com>
>
> Please include a Fixes tag to help keep track of changes.

Sure, sorry for my ignorance, but I did not know that we're supposed
to add a Fixes tag in this situation.

>
>>---
>> drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>>diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>>b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>>index 16c85cc..0034de44 100644
>>--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>>+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>>@@ -218,8 +218,7 @@ int stmmac_mdio_register(struct net_device *ndev)
>>               if (mdio_node) {
>>                       netdev_dbg(ndev, "FOUND MDIO subnode\n");
>>               } else {
>>-                      netdev_err(ndev, "NO MDIO subnode\n");
>>-                      return 0;
>>+                      netdev_warn(ndev, "No MDIO subnode found\n");
>>               }
>>       }
>>
>>@@ -251,7 +250,10 @@ int stmmac_mdio_register(struct net_device *ndev)
>>       new_bus->phy_mask = mdio_bus_data->phy_mask;
>>       new_bus->parent = priv->device;
>>
>>-      err = of_mdiobus_register(new_bus, mdio_node);
>>+      if (IS_ENABLED(CONFIG_OF) && mdio_node)
>

> You should be able to drop the IS_ENABLED part since there is an inline provided in the non-OF case which does a fallback to mdiobus_register().

Good point

>
>>+              err = of_mdiobus_register(new_bus, mdio_node);
>>+      else
>>+              err = mdiobus_register(new_bus);
>>       if (err != 0) {
>
> This looks reasonable, does it work if we just assign mdio_node to the pdev->dev.of_node to be backwards compatible with DTSes which have not yet been updated?
>

I don't understand what you mean. Instead of defining mdio_node with
NULL, you propose to assign it to pdev->dev.of_node as its default
value... then it would be reassigned with "child_nod" if a subnode
with the compatible string "snps,dwmac-mdio" is found, or kept with
its default value otherwise... and in this case we could call
of_mdiobus_register(new_bus, mdio_node) all the time ? (it removes the
conditonnal branch)

Regards,
Romain
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html