On 05/26/2017 06:00 PM, woojung....@microchip.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
