netdev - RE: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached

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]

Message-ID: <9235D6609DB808459E95D78E17F2E43D40A9DB32@CHN-SV-EXMX02.mchp-main.com>
Date:   Sat, 27 May 2017 01:00:57 +0000
From:   <Woojung.Huh@...rochip.com>
To:     <f.fainelli@...il.com>, <netdev@...r.kernel.org>
CC:     <davem@...emloft.net>, <andrew@...n.ch>
Subject: RE: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links
 for attached_dev/phydev

Hi Florian,
> >> OK, so here is what is happening: macb_mii_init() calls macb_mii_probe()
> >> and so by the time we call phy_connect_direct(), we have not called
> >> register_netdevice() yet, netdev_register_kobject() has not been called
> >> either, and so sysfs_create_link() fails....
> > I just found same thing.
> > Yes, register_netdev() was not called at phy_connect_direct() time.
> >
> >> Let me think about a way to solve that, even though I am leaning towards
> >> ignoring the errors from sysfs_create_link() rather than fixing each and
> >> every Ethernet driver to make it probe its MII bus *after* calling
> >> register_netdevice()....
> > Agree.
> 
> Thanks, would the following work for you? I don't want to do something
> more complex than that, although, if we really wanted to see this
> information, we could imagine having netdev_register_kobject() check
> whether phydev->dev.kobj is valid, and set the symbolic link at that
> point... The problem with that approach is that we are no longer
> symetrical within the core PHYLIB code (phy_attach_direct and phy_detach).
>
> Let me know if this works so I can make a formal submission:
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index f84414b8f2ee..daad816ee1d1 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -960,14 +960,20 @@ int phy_attach_direct(struct net_device *dev,
> struct phy_device *phydev,
> 
>         phydev->attached_dev = dev;
>         dev->phydev = phydev;
> +
> +       /* Some Ethernet drivers try to connect to a PHY device before
> +        * calling register_netdevice(). register_netdevice() does
> ultimately
> +        * lead to netdev_register_kobject() which would do the
> dev->dev.kobj
> +        * initialization. Here we explicitly ignore those particular errors
> +        */
>         err = sysfs_create_link(&phydev->mdio.dev.kobj, &dev->dev.kobj,
>                                 "attached_dev");
> -       if (err)
> +       if (err && err != -ENOENT)
>                 goto error;
This one fine. However, next one returns -14 (-EFAULT)

>         err = sysfs_create_link(&dev->dev.kobj, &phydev->mdio.dev.kobj,
>                                 "phydev");
> -       if (err)
> +       if (err && err != -ENOENT)
>                 goto error;
No need to call 2nd sysfs_create_link(), how about following?

	err = sysfs_create_link(&phydev->mdio.dev.kobj, &dev->dev.kobj,
 				"attached_dev");
-	if (err)
+	if (err && err != -ENOENT)
 		goto error;
 
-	err = sysfs_create_link(&dev->dev.kobj, &phydev->mdio.dev.kobj,
-				"phydev");
-	if (err)
-		goto error;
+	if (!err) {
+		err = sysfs_create_link(&dev->dev.kobj, &phydev->mdio.dev.kobj,
+					"phydev");
+		if (err)
+			goto error;
+	}
 
 	phydev->dev_flags = flags;

- Woojung