[PATCH net-next 0/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev

[PATCH net-next 0/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev

[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

[PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev

[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

[PATCH net-next 2/3] net: sysfs: Document "phydev" symbolic link

[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

[PATCH net-next 3/3] net: sysfs: Document PHY device sysfs attributes

[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

Re: [PATCH net-next 3/3] net: sysfs: Document PHY device sysfs attributes

[Andrew Lunn]

> +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

Re: [PATCH net-next 3/3] net: sysfs: Document PHY device sysfs attributes

[Florian Fainelli]

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

Re: [PATCH net-next 3/3] net: sysfs: Document PHY device sysfs attributes

[Florian Fainelli]

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

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

[Woojung.Huh]

> @@ -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

Re: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev

[Florian Fainelli]

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

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

[Woojung.Huh]

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

Re: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev

[Florian Fainelli]

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

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

[Woojung.Huh]

> 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

Re: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev

[Florian Fainelli]

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

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

[Woojung.Huh]

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

Re: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev

[Florian Fainelli]

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

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

[Woojung.Huh]

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

Re: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev

[Florian Fainelli]

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

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

[Woojung.Huh]

> 
> 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

Re: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev

[Florian Fainelli]

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