* [PATCH net-next 0/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev @ 2017-05-24 19:33 Florian Fainelli 2017-05-24 19:33 ` [PATCH net-next 1/3] " Florian Fainelli ` (2 more replies) 0 siblings, 3 replies; 19+ messages in thread From: Florian Fainelli @ 2017-05-24 19:33 UTC (permalink / raw) To: netdev; +Cc: davem, andrew, Florian Fainelli Hi David, Andrew, This patch series addresses a device topology shortcoming where a program scanning /sys would not be able to establish a mapping between the network device and the PHY device. In the process it turned out that no PHY device documentation existed for sysfs attributes. Thanks! Florian Fainelli (3): net: phy: Create sysfs reciprocal links for attached_dev/phydev net: sysfs: Document "phydev" symbolic link net: sysfs: Document PHY device sysfs attributes Documentation/ABI/testing/sysfs-class-net | 8 ++++++ Documentation/ABI/testing/sysfs-class-net-phydev | 32 ++++++++++++++++++++++++ drivers/net/phy/phy_device.c | 11 ++++++++ 3 files changed, 51 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-net-phydev -- 2.9.3 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev 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 ` Florian Fainelli 2017-05-26 23:34 ` Woojung.Huh 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 2 siblings, 1 reply; 19+ messages in thread From: Florian Fainelli @ 2017-05-24 19:33 UTC (permalink / raw) To: netdev; +Cc: davem, andrew, Florian Fainelli There is currently no way for a program scanning /sys to know whether a network device is attached to a particular PHY device, just like the PHY device is not pointed back to its attached network device. Create a symbolic link in the network device's namespace named "phydev" which points to the PHY device and create a symbolic link in the PHY device's namespace named "attached_dev" that points back to the network device. These links are set up during phy_attach_direct() and removed during phy_detach() for symetry. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/net/phy/phy_device.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 1219eeab69d1..8151477c7027 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -960,6 +960,15 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev, phydev->attached_dev = dev; dev->phydev = phydev; + 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; phydev->dev_flags = flags; @@ -1050,6 +1059,8 @@ void phy_detach(struct phy_device *phydev) struct mii_bus *bus; int i; + sysfs_remove_link(&dev->dev.kobj, "phydev"); + sysfs_remove_link(&phydev->mdio.dev.kobj, "attached_dev"); phydev->attached_dev->phydev = NULL; phydev->attached_dev = NULL; phy_suspend(phydev); -- 2.9.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* RE: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev 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 0 siblings, 1 reply; 19+ messages in thread From: Woojung.Huh @ 2017-05-26 23:34 UTC (permalink / raw) To: f.fainelli, netdev; +Cc: davem, andrew > @@ -960,6 +960,15 @@ int phy_attach_direct(struct net_device *dev, struct > phy_device *phydev, > > phydev->attached_dev = dev; > dev->phydev = phydev; > + 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; > Florian, I knew that it is applied to net-next. However, sysfs_create_link() fails when fixed phy (drivers/net/phy/fixed_phy.c) Do you have a chance to test with it? - Woojung ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev 2017-05-26 23:34 ` Woojung.Huh @ 2017-05-26 23:44 ` Florian Fainelli 2017-05-26 23:52 ` Woojung.Huh 0 siblings, 1 reply; 19+ messages in thread From: Florian Fainelli @ 2017-05-26 23:44 UTC (permalink / raw) To: Woojung.Huh, netdev; +Cc: davem, andrew Hi Woojung, On 05/26/2017 04:34 PM, Woojung.Huh@microchip.com wrote: >> @@ -960,6 +960,15 @@ int phy_attach_direct(struct net_device *dev, struct >> phy_device *phydev, >> >> phydev->attached_dev = dev; >> dev->phydev = phydev; >> + 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; >> > Florian, > > I knew that it is applied to net-next. > However, sysfs_create_link() fails when fixed phy (drivers/net/phy/fixed_phy.c) > Do you have a chance to test with it? I did, my main test system (BCM7445 w/ bcm_sf2) has one normal PHY driver and 3 fixed PHYs (including one for the CPU port/master netdev), see below. What kind of error do you get here? # ls -l /sys/class/net/gphy/phydev lrwxrwxrwx 1 root root 0 Jan 1 00:00 /sys/class/net/gphy/phydev -> ../../../f0b403c0.mdio/mdio_bus/f0b403c0.mdio/f0b403c0.mdio:05 # ls -l /sys/class/net/*/phydev lrwxrwxrwx 1 root root 0 Jan 1 00:01 /sys/class/net/eth0/phydev -> ../../../../Fixed MDIO bus.0/mdio_bus/fixed-0/fixed-0:00 lrwxrwxrwx 1 root root 0 Jan 1 00:00 /sys/class/net/gphy/phydev -> ../../../f0b403c0.mdio/mdio_bus/f0b403c0.mdio/f0b403c0.mdio:05 lrwxrwxrwx 1 root root 0 Jan 1 00:01 /sys/class/net/moca/phydev -> ../../../../../Fixed MDIO bus.0/mdio_bus/fixed-0/fixed-0:02 lrwxrwxrwx 1 root root 0 Jan 1 00:01 /sys/class/net/rgmii_1/phydev -> ../../../mdio_bus/sf2-1/sf2-1:00 lrwxrwxrwx 1 root root 0 Jan 1 00:01 /sys/class/net/rgmii_2/phydev -> ../../../../../Fixed MDIO bus.0/mdio_bus/fixed-0/fixed-0:01 # ls -l /sys/class/mdio_bus/fixed-0/*/attached_dev lrwxrwxrwx 1 root root 0 Jan 1 00:01 /sys/class/mdio_bus/fixed-0/fixed-0:00/attached_dev -> ../../../../rdb/f04a0000.ethernet/net/eth0 lrwxrwxrwx 1 root root 0 Jan 1 00:02 /sys/class/mdio_bus/fixed-0/fixed-0:01/attached_dev -> ../../../../rdb/rdb:switch_top@f0b00000/f0b00000.ethernet_switch/net/rgmii_2 lrwxrwxrwx 1 root root 0 Jan 1 00:02 /sys/class/mdio_bus/fixed-0/fixed-0:02/attached_dev -> ../../../../rdb/rdb:switch_top@f0b00000/f0b00000.ethernet_switch/net/moca # ls -l /sys/class/mdio_bus/f0b403c0.mdio/f0b403c0.mdio\:05/attached_dev lrwxrwxrwx 1 root root 0 Jan 1 00:02 /sys/class/mdio_bus/f0b403c0.mdio/f0b403c0.mdio:05/attached_dev -> ../../../../f0b00000.ethernet_switch/net/gphy > > - Woojung > -- Florian ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev 2017-05-26 23:44 ` Florian Fainelli @ 2017-05-26 23:52 ` Woojung.Huh 2017-05-27 0:02 ` Florian Fainelli 0 siblings, 1 reply; 19+ messages in thread From: Woojung.Huh @ 2017-05-26 23:52 UTC (permalink / raw) To: f.fainelli, netdev; +Cc: davem, andrew Hi Florian, > > I knew that it is applied to net-next. > > However, sysfs_create_link() fails when fixed phy > (drivers/net/phy/fixed_phy.c) > > Do you have a chance to test with it? > > I did, my main test system (BCM7445 w/ bcm_sf2) has one normal PHY > driver and 3 fixed PHYs (including one for the CPU port/master netdev), > see below. > > What kind of error do you get here? sysfs_create_link() returns -2 (-ENOENT). > > # ls -l /sys/class/net/gphy/phydev > lrwxrwxrwx 1 root root 0 Jan 1 00:00 > /sys/class/net/gphy/phydev -> > ../../../f0b403c0.mdio/mdio_bus/f0b403c0.mdio/f0b403c0.mdio:05 > # ls -l /sys/class/net/*/phydev > lrwxrwxrwx 1 root root 0 Jan 1 00:01 > /sys/class/net/eth0/phydev -> ../../../../Fixed MDIO > bus.0/mdio_bus/fixed-0/fixed-0:00 > lrwxrwxrwx 1 root root 0 Jan 1 00:00 > /sys/class/net/gphy/phydev -> > ../../../f0b403c0.mdio/mdio_bus/f0b403c0.mdio/f0b403c0.mdio:05 > lrwxrwxrwx 1 root root 0 Jan 1 00:01 > /sys/class/net/moca/phydev -> ../../../../../Fixed MDIO > bus.0/mdio_bus/fixed-0/fixed-0:02 > lrwxrwxrwx 1 root root 0 Jan 1 00:01 > /sys/class/net/rgmii_1/phydev -> ../../../mdio_bus/sf2-1/sf2-1:00 > lrwxrwxrwx 1 root root 0 Jan 1 00:01 > /sys/class/net/rgmii_2/phydev -> ../../../../../Fixed MDIO > bus.0/mdio_bus/fixed-0/fixed-0:01 > > # ls -l /sys/class/mdio_bus/fixed-0/*/attached_dev > lrwxrwxrwx 1 root root 0 Jan 1 00:01 > /sys/class/mdio_bus/fixed-0/fixed-0:00/attached_dev -> > ../../../../rdb/f04a0000.ethernet/net/eth0 > lrwxrwxrwx 1 root root 0 Jan 1 00:02 > /sys/class/mdio_bus/fixed-0/fixed-0:01/attached_dev -> > ../../../../rdb/rdb:switch_top@f0b00000/f0b00000.ethernet_switch/net/rgm > ii_2 > lrwxrwxrwx 1 root root 0 Jan 1 00:02 > /sys/class/mdio_bus/fixed-0/fixed-0:02/attached_dev -> > ../../../../rdb/rdb:switch_top@f0b00000/f0b00000.ethernet_switch/net/moc > a > > # ls -l /sys/class/mdio_bus/f0b403c0.mdio/f0b403c0.mdio\:05/attached_dev > lrwxrwxrwx 1 root root 0 Jan 1 00:02 > /sys/class/mdio_bus/f0b403c0.mdio/f0b403c0.mdio:05/attached_dev -> > ../../../../f0b00000.ethernet_switch/net/gphy /sys/class/mdio_bus/devices/fixed-0:00/* exists. But not /sys/class/net/eth0.. because Ethernet driver initialization failed. - Woojung ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev 2017-05-26 23:52 ` Woojung.Huh @ 2017-05-27 0:02 ` Florian Fainelli 2017-05-27 0:20 ` Woojung.Huh 0 siblings, 1 reply; 19+ messages in thread From: Florian Fainelli @ 2017-05-27 0:02 UTC (permalink / raw) To: Woojung.Huh, netdev; +Cc: davem, andrew On 05/26/2017 04:52 PM, Woojung.Huh@microchip.com wrote: > Hi Florian, > >>> I knew that it is applied to net-next. >>> However, sysfs_create_link() fails when fixed phy >> (drivers/net/phy/fixed_phy.c) >>> Do you have a chance to test with it? >> >> I did, my main test system (BCM7445 w/ bcm_sf2) has one normal PHY >> driver and 3 fixed PHYs (including one for the CPU port/master netdev), >> see below. >> >> What kind of error do you get here? > sysfs_create_link() returns -2 (-ENOENT). > >> >> # ls -l /sys/class/net/gphy/phydev >> lrwxrwxrwx 1 root root 0 Jan 1 00:00 >> /sys/class/net/gphy/phydev -> >> ../../../f0b403c0.mdio/mdio_bus/f0b403c0.mdio/f0b403c0.mdio:05 >> # ls -l /sys/class/net/*/phydev >> lrwxrwxrwx 1 root root 0 Jan 1 00:01 >> /sys/class/net/eth0/phydev -> ../../../../Fixed MDIO >> bus.0/mdio_bus/fixed-0/fixed-0:00 >> lrwxrwxrwx 1 root root 0 Jan 1 00:00 >> /sys/class/net/gphy/phydev -> >> ../../../f0b403c0.mdio/mdio_bus/f0b403c0.mdio/f0b403c0.mdio:05 >> lrwxrwxrwx 1 root root 0 Jan 1 00:01 >> /sys/class/net/moca/phydev -> ../../../../../Fixed MDIO >> bus.0/mdio_bus/fixed-0/fixed-0:02 >> lrwxrwxrwx 1 root root 0 Jan 1 00:01 >> /sys/class/net/rgmii_1/phydev -> ../../../mdio_bus/sf2-1/sf2-1:00 >> lrwxrwxrwx 1 root root 0 Jan 1 00:01 >> /sys/class/net/rgmii_2/phydev -> ../../../../../Fixed MDIO >> bus.0/mdio_bus/fixed-0/fixed-0:01 >> >> # ls -l /sys/class/mdio_bus/fixed-0/*/attached_dev >> lrwxrwxrwx 1 root root 0 Jan 1 00:01 >> /sys/class/mdio_bus/fixed-0/fixed-0:00/attached_dev -> >> ../../../../rdb/f04a0000.ethernet/net/eth0 >> lrwxrwxrwx 1 root root 0 Jan 1 00:02 >> /sys/class/mdio_bus/fixed-0/fixed-0:01/attached_dev -> >> ../../../../rdb/rdb:switch_top@f0b00000/f0b00000.ethernet_switch/net/rgm >> ii_2 >> lrwxrwxrwx 1 root root 0 Jan 1 00:02 >> /sys/class/mdio_bus/fixed-0/fixed-0:02/attached_dev -> >> ../../../../rdb/rdb:switch_top@f0b00000/f0b00000.ethernet_switch/net/moc >> a >> >> # ls -l /sys/class/mdio_bus/f0b403c0.mdio/f0b403c0.mdio\:05/attached_dev >> lrwxrwxrwx 1 root root 0 Jan 1 00:02 >> /sys/class/mdio_bus/f0b403c0.mdio/f0b403c0.mdio:05/attached_dev -> >> ../../../../f0b00000.ethernet_switch/net/gphy > > /sys/class/mdio_bus/devices/fixed-0:00/* exists. > But not /sys/class/net/eth0.. because Ethernet driver initialization failed. OK, I am confused now. You are describing what is going on with your platform right? Can you describe a bit further here what is happening and with which type of interface? Is this with the CPU interface or something? -- Florian ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev 2017-05-27 0:02 ` Florian Fainelli @ 2017-05-27 0:20 ` Woojung.Huh 2017-05-27 0:33 ` Florian Fainelli 0 siblings, 1 reply; 19+ messages in thread From: Woojung.Huh @ 2017-05-27 0:20 UTC (permalink / raw) To: f.fainelli, netdev; +Cc: davem, andrew > OK, I am confused now. You are describing what is going on with your > platform right? Can you describe a bit further here what is happening > and with which type of interface? Is this with the CPU interface or > something? Yes. It's on our platform. Like your platform, fixed phy is used to connect switch CPU port/master netdev. GMAC of SoC is cadence/macb.c with fixed phy modification. static int macb_mii_probe(struct net_device *dev) { ... phydev = phy_find_first(bp->mii_bus); if (!phydev) { phydev = fixed_phy_register(PHY_POLL, &fphy_status, -1, NULL); if (IS_ERR(phydev)) { netdev_err(dev, "no PHY found\n"); return -ENXIO; } } ... When failed to find phydev from phy_find_first(), it forces to fixed phy. ... /* attach the mac to the phy */ ret = phy_connect_direct(dev, phydev, &macb_handle_link_change, bp->phy_interface); sysfs_create_lin() inside of phy_connect_direct() fails. What is driver you are testing? I can check the file. Thanks. - Woojung ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev 2017-05-27 0:20 ` Woojung.Huh @ 2017-05-27 0:33 ` Florian Fainelli 2017-05-27 0:43 ` Woojung.Huh 0 siblings, 1 reply; 19+ messages in thread From: Florian Fainelli @ 2017-05-27 0:33 UTC (permalink / raw) To: Woojung.Huh, netdev; +Cc: davem, andrew On 05/26/2017 05:20 PM, Woojung.Huh@microchip.com wrote: >> OK, I am confused now. You are describing what is going on with your >> platform right? Can you describe a bit further here what is happening >> and with which type of interface? Is this with the CPU interface or >> something? > > Yes. It's on our platform. > Like your platform, fixed phy is used to connect switch CPU port/master netdev. > GMAC of SoC is cadence/macb.c with fixed phy modification. > > static int macb_mii_probe(struct net_device *dev) > { > ... > phydev = phy_find_first(bp->mii_bus); > if (!phydev) { > phydev = fixed_phy_register(PHY_POLL, &fphy_status, -1, NULL); > if (IS_ERR(phydev)) { > netdev_err(dev, "no PHY found\n"); > return -ENXIO; > }> } > ... > When failed to find phydev from phy_find_first(), it forces to fixed phy. > ... > /* attach the mac to the phy */ > ret = phy_connect_direct(dev, phydev, &macb_handle_link_change, > bp->phy_interface); > > sysfs_create_lin() inside of phy_connect_direct() fails. 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.... 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().... > > What is driver you are testing? I can check the file. Drivers involved are the following: drivers/net/ethernet/broadcom/bcmsysport.c, drivers/net/dsa/bcm_sf2.c drivers/net/ethernet/broadcom/genet/ drivers/net/phy/bcm7xxx.c -- Florian ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev 2017-05-27 0:33 ` Florian Fainelli @ 2017-05-27 0:43 ` Woojung.Huh 2017-05-27 0:46 ` Florian Fainelli 0 siblings, 1 reply; 19+ messages in thread From: Woojung.Huh @ 2017-05-27 0:43 UTC (permalink / raw) To: f.fainelli, netdev; +Cc: davem, andrew 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. > > > > What is driver you are testing? I can check the file. > > Drivers involved are the following: > > drivers/net/ethernet/broadcom/bcmsysport.c, > drivers/net/dsa/bcm_sf2.c > drivers/net/ethernet/broadcom/genet/ > drivers/net/phy/bcm7xxx.c Thanks for list of files. - Woojung ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev 2017-05-27 0:43 ` Woojung.Huh @ 2017-05-27 0:46 ` Florian Fainelli 2017-05-27 1:00 ` Woojung.Huh 0 siblings, 1 reply; 19+ messages in thread From: Florian Fainelli @ 2017-05-27 0:46 UTC (permalink / raw) To: Woojung.Huh, netdev; +Cc: davem, andrew Hi Woojung, On 05/26/2017 05:43 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; err = sysfs_create_link(&dev->dev.kobj, &phydev->mdio.dev.kobj, "phydev"); - if (err) + if (err && err != -ENOENT) goto error; phydev->dev_flags = flags; -- Florian ^ permalink raw reply related [flat|nested] 19+ messages in thread
* RE: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev 2017-05-27 0:46 ` Florian Fainelli @ 2017-05-27 1:00 ` Woojung.Huh 2017-05-27 1:26 ` Florian Fainelli 0 siblings, 1 reply; 19+ messages in thread From: Woojung.Huh @ 2017-05-27 1:00 UTC (permalink / raw) To: f.fainelli, netdev; +Cc: davem, andrew 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev 2017-05-27 1:00 ` Woojung.Huh @ 2017-05-27 1:26 ` Florian Fainelli 2017-05-27 1:31 ` Woojung.Huh 0 siblings, 1 reply; 19+ messages in thread From: Florian Fainelli @ 2017-05-27 1:26 UTC (permalink / raw) To: Woojung.Huh, netdev; +Cc: davem, andrew 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* RE: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev 2017-05-27 1:26 ` Florian Fainelli @ 2017-05-27 1:31 ` Woojung.Huh 2017-05-27 3:22 ` Florian Fainelli 0 siblings, 1 reply; 19+ messages in thread From: Woojung.Huh @ 2017-05-27 1:31 UTC (permalink / raw) To: f.fainelli, netdev; +Cc: davem, andrew > > 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; > Looks better and clean. How about sysfs_remove_link() in phy_detach()? - Woojung ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev 2017-05-27 1:31 ` Woojung.Huh @ 2017-05-27 3:22 ` Florian Fainelli 0 siblings, 0 replies; 19+ messages in thread From: Florian Fainelli @ 2017-05-27 3:22 UTC (permalink / raw) To: Woojung.Huh, netdev; +Cc: davem, andrew On 05/26/2017 06:31 PM, Woojung.Huh@microchip.com wrote: >> >> 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; >> > Looks better and clean. > > How about sysfs_remove_link() in phy_detach()? Good catch, I was able to reproduce the situation in which macb calls phy_connect_direct(), will submit a patch shortly. Thanks! -- Florian ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH net-next 2/3] net: sysfs: Document "phydev" symbolic link 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-24 19:33 ` Florian Fainelli 2017-05-24 19:33 ` [PATCH net-next 3/3] net: sysfs: Document PHY device sysfs attributes Florian Fainelli 2 siblings, 0 replies; 19+ messages in thread From: Florian Fainelli @ 2017-05-24 19:33 UTC (permalink / raw) To: netdev; +Cc: davem, andrew, Florian Fainelli Now that we link the network device to its PHY device, document this sysfs symbolic link. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- Documentation/ABI/testing/sysfs-class-net | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Documentation/ABI/testing/sysfs-class-net b/Documentation/ABI/testing/sysfs-class-net index 668604fc8e06..6856da99b6f7 100644 --- a/Documentation/ABI/testing/sysfs-class-net +++ b/Documentation/ABI/testing/sysfs-class-net @@ -251,3 +251,11 @@ Contact: netdev@vger.kernel.org Description: Indicates the unique physical switch identifier of a switch this port belongs to, as a string. + +What: /sys/class/net/<iface>/phydev +Date: May 2017 +KernelVersion: 4.13 +Contact: netdev@vger.kernel.org +Description: + Symbolic link to the PHY device this network device is attached + to. -- 2.9.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH net-next 3/3] net: sysfs: Document PHY device sysfs attributes 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-24 19:33 ` [PATCH net-next 2/3] net: sysfs: Document "phydev" symbolic link Florian Fainelli @ 2017-05-24 19:33 ` Florian Fainelli 2017-05-24 20:10 ` Andrew Lunn 2 siblings, 1 reply; 19+ messages in thread From: Florian Fainelli @ 2017-05-24 19:33 UTC (permalink / raw) To: netdev; +Cc: davem, andrew, Florian Fainelli Document the different sysfs attributes that exist for PHY devices: attached_dev, phy_has_fixups, phy_id and phy_interface. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- Documentation/ABI/testing/sysfs-class-net-phydev | 32 ++++++++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-net-phydev diff --git a/Documentation/ABI/testing/sysfs-class-net-phydev b/Documentation/ABI/testing/sysfs-class-net-phydev new file mode 100644 index 000000000000..a05831667c3d --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-net-phydev @@ -0,0 +1,32 @@ +What: /sys/class/mdio_bus/<bus>/<device>/attached_dev +Date: May 2017 +KernelVersion: 4.13 +Contact: netdev@vger.kernel.org +Description: + Symbolic link to the network device this PHY device is + attached to. + +What: /sys/class/mdio_bus/<bus>/<device>/phy_has_fixups +Date: February 2014 +KernelVersion: 3.15 +Contact: netdev@vger.kernel.org +Description: + Boolean value indicating whether the PHY device has + any fixups registered against it (phy_register_fixup) + +What: /sys/class/mdio_bus/<bus>/<device>/phy_id +Date: November 2012 +KernelVersion: 3.8 +Contact: netdev@vger.kernel.org +Description: + 32-bit hexadecimal value corresponding to the PHY device's OUI, + model and revision number. + +What: /sys/class/mdio_bus/<bus>/<device>/phy_interface +Date: February 2014 +KernelVersion: 3.15 +Contact: netdev@vger.kernel.org +Description: + String value indicating the PHY interface, possible + values are in include/linux/phy.h function phy_modes. + -- 2.9.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 3/3] net: sysfs: Document PHY device sysfs attributes 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 0 siblings, 1 reply; 19+ messages in thread From: Andrew Lunn @ 2017-05-24 20:10 UTC (permalink / raw) To: Florian Fainelli; +Cc: netdev, davem > +What: /sys/class/mdio_bus/<bus>/<device>/phy_interface > +Date: February 2014 > +KernelVersion: 3.15 > +Contact: netdev@vger.kernel.org > +Description: > + String value indicating the PHY interface, possible > + values are in include/linux/phy.h function phy_modes. Hi Florian Does this suggest that these strings should be moved to include/uapi/linux/phy.h? Andrew ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 3/3] net: sysfs: Document PHY device sysfs attributes 2017-05-24 20:10 ` Andrew Lunn @ 2017-05-24 23:07 ` Florian Fainelli 2017-05-25 0:03 ` Florian Fainelli 0 siblings, 1 reply; 19+ messages in thread From: Florian Fainelli @ 2017-05-24 23:07 UTC (permalink / raw) To: Andrew Lunn; +Cc: netdev, davem Hi Andrew, On 05/24/2017 01:10 PM, Andrew Lunn wrote: >> +What: /sys/class/mdio_bus/<bus>/<device>/phy_interface >> +Date: February 2014 >> +KernelVersion: 3.15 >> +Contact: netdev@vger.kernel.org >> +Description: >> + String value indicating the PHY interface, possible >> + values are in include/linux/phy.h function phy_modes. > > Hi Florian > > Does this suggest that these strings should be moved to > include/uapi/linux/phy.h? Humm, I suppose we could do that, although I am not sure ally sure what we would be putting in there (at least for now). -- Florian ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 3/3] net: sysfs: Document PHY device sysfs attributes 2017-05-24 23:07 ` Florian Fainelli @ 2017-05-25 0:03 ` Florian Fainelli 0 siblings, 0 replies; 19+ messages in thread From: Florian Fainelli @ 2017-05-25 0:03 UTC (permalink / raw) To: Andrew Lunn; +Cc: netdev, davem On 05/24/2017 04:07 PM, Florian Fainelli wrote: > Hi Andrew, > > On 05/24/2017 01:10 PM, Andrew Lunn wrote: >>> +What: /sys/class/mdio_bus/<bus>/<device>/phy_interface >>> +Date: February 2014 >>> +KernelVersion: 3.15 >>> +Contact: netdev@vger.kernel.org >>> +Description: >>> + String value indicating the PHY interface, possible >>> + values are in include/linux/phy.h function phy_modes. >> >> Hi Florian >> >> Does this suggest that these strings should be moved to >> include/uapi/linux/phy.h? > > Humm, I suppose we could do that, although I am not sure ally sure what > we would be putting in there (at least for now). I actually prefer to put the possible values in the sysfs documentation files instead of in a UAPI header. Will submit a v2. -- Florian ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2017-05-27 3:22 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.