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 for Android: free password hash cracker in your pocket

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

Date:   Fri, 26 May 2017 18:26:02 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Woojung.Huh@...rochip.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



On 05/26/2017 06:00 PM, Woojung.Huh@...rochip.com wrote:
> 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;
> +	}

Yes, that's a very valid point, how about we even do this:

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f84414b8f2ee..dc666ec13651 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -960,15 +960,21 @@ 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() -> netdev_register_kobject() and
+        * does the dev->dev.kobj initialization. Here we only check for
+        * success which indicates that the network device kobject is
+        * ready.
+        */
        err = sysfs_create_link(&phydev->mdio.dev.kobj, &dev->dev.kobj,
                                "attached_dev");
-       if (err)
-               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;

-- 
Florian