From: Florian Fainelli <f.fainelli@gmail•com>
To: Woojung.Huh@microchip•com, netdev@vger•kernel.org
Cc: davem@davemloft•net, andrew@lunn•ch
Subject: Re: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev
Date: Fri, 26 May 2017 18:26:02 -0700 [thread overview]
Message-ID: <dc4a3f50-3ff3-e690-9893-0575ddea42f6@gmail.com> (raw)
In-Reply-To: <9235D6609DB808459E95D78E17F2E43D40A9DB32@CHN-SV-EXMX02.mchp-main.com>
On 05/26/2017 06:00 PM, Woojung.Huh@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
next prev parent reply other threads:[~2017-05-27 1:26 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-24 19:33 [PATCH net-next 0/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev Florian Fainelli
2017-05-24 19:33 ` [PATCH net-next 1/3] " Florian Fainelli
2017-05-26 23:34 ` Woojung.Huh
2017-05-26 23:44 ` Florian Fainelli
2017-05-26 23:52 ` Woojung.Huh
2017-05-27 0:02 ` Florian Fainelli
2017-05-27 0:20 ` Woojung.Huh
2017-05-27 0:33 ` Florian Fainelli
2017-05-27 0:43 ` Woojung.Huh
2017-05-27 0:46 ` Florian Fainelli
2017-05-27 1:00 ` Woojung.Huh
2017-05-27 1:26 ` Florian Fainelli [this message]
2017-05-27 1:31 ` Woojung.Huh
2017-05-27 3:22 ` Florian Fainelli
2017-05-24 19:33 ` [PATCH net-next 2/3] net: sysfs: Document "phydev" symbolic link Florian Fainelli
2017-05-24 19:33 ` [PATCH net-next 3/3] net: sysfs: Document PHY device sysfs attributes Florian Fainelli
2017-05-24 20:10 ` Andrew Lunn
2017-05-24 23:07 ` Florian Fainelli
2017-05-25 0:03 ` Florian Fainelli
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=dc4a3f50-3ff3-e690-9893-0575ddea42f6@gmail.com \
--to=f.fainelli@gmail$(echo .)com \
--cc=Woojung.Huh@microchip$(echo .)com \
--cc=andrew@lunn$(echo .)ch \
--cc=davem@davemloft$(echo .)net \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox