regression?: ti: cpsw: warning from phy_connect()->sysfs_create_link()-sysfs_warn_dup()

regression?: ti: cpsw: warning from phy_connect()->sysfs_create_link()-sysfs_warn_dup()

[Grygorii Strashko]

Hi Florian,

The TI CPSW driver produces warning as below when booted in switch mode:
[    8.882295] sysfs: cannot create duplicate filename '/devices/platform/44000000.ocp/48484000.ethernet/net/eth0/phydev'
[    8.999859] CPU: 1 PID: 356 Comm: systemd-network Not tainted 4.16.0-rc3-00010-g6cc3ff6-dirty #225
...
[    9.012352] Hardware name: Generic DRA74X (Flattened Device Tree)
[    9.018901] Backtrace: 
[    9.021376] [<c010c918>] (dump_backtrace) from [<c010cbe8>] (show_stack+0x18/0x1c)
[    9.028986]  r7:ed036240 r6:60070013 r5:00000000 r4:c0d598a0
[    9.034684] [<c010cbd0>] (show_stack) from [<c088e1f0>] (dump_stack+0x8c/0xa0)
[    9.041954] [<c088e164>] (dump_stack) from [<c02ba59c>] (sysfs_warn_dup+0x60/0x6c)
[    9.049564]  r7:ed036240 r6:ed036240 r5:c0b579bc r4:ed10c000
[    9.055264] [<c02ba53c>] (sysfs_warn_dup) from [<c02ba930>] (sysfs_do_create_link_sd+0xbc/0xc4)
[    9.064006]  r7:ed036240 r6:ffffffef r5:00000000 r4:ed034660
[    9.069701] [<c02ba874>] (sysfs_do_create_link_sd) from [<c02ba968>] (sysfs_create_link+0x30/0x3c)
[    9.078706]  r9:00000008 r8:00000000 r7:ed02f008 r6:ee015ae8 r5:ee015800 r4:ed02f000
[    9.086497] [<c02ba938>] (sysfs_create_link) from [<c060f070>] (phy_attach_direct+0x180/0x1f4)
[    9.095163] [<c060eef0>] (phy_attach_direct) from [<c060f1cc>] (phy_connect_direct+0x1c/0x54)
[    9.103735]  r10:00000001 r9:ee015d0c r8:00000008 r7:c062e84c r6:c062e84c r5:ed02f000
[    9.111609]  r4:ed02f000 r3:00000008
[    9.115217] [<c060f1b0>] (phy_connect_direct) from [<c060f250>] (phy_connect+0x4c/0x80)
[    9.123270]  r7:c062e84c r6:ee015800 r5:ed02f000 r4:ee693664
[    9.128969] [<c060f204>] (phy_connect) from [<c062abc8>] (cpsw_slave_open+0x21c/0x274)
[    9.136926]  r9:ee015d0c r8:ee015d00 r7:ed018a10 r6:ee015800 r5:ee015d00 r4:ed032630
[    9.144715] [<c062a9ac>] (cpsw_slave_open) from [<c062b28c>] (cpsw_ndo_open+0x158/0x55c)
[    9.152860]  r10:00000001 r9:00000000 r8:ee015d00 r7:c0d04c48 r6:ee015800 r5:ed018a10
[    9.160730]  r4:ed032630
[    9.163291] [<c062b134>] (cpsw_ndo_open) from [<c0765350>] (__dev_open+0xd4/0x158)
[    9.170900]  r10:00000000 r9:ec885db4 r8:ee01582c r7:c09a9c00 r6:00000000 r5:c0d04c48
[    9.178768]  r4:ee015800
[    9.181326] [<c076527c>] (__dev_open) from [<c0765748>] (__dev_change_flags+0x174/0x1c0)

The reason of the warning is that in switch mode CPSW drivers is connecting two Net PHYs to 
a single network device (it has been done this way when driver was initially introduced), so
now it produces warning during boot because of commits:

5568363f0cb3 ("net: phy: Create sysfs reciprocal links for attached_dev/phydev"
a3995460491d ("net: phy: Relax error checking on sysfs_create_link()"
^ both went in v4.13

Honestly, I'm not sure how to fix it the best way (the simplest fix is below), taking into account
that we are not ready to do big reworks in CPSW driver.

Sry, for the late report - in LKML most of dual port TI platforms configured to work in
dual mac mode, therefore two network devices are created and warning not displayed.

-- 
regards,
-grygorii
------
>From ef2e612652cb7afa844993b23156315f0df7d24f Mon Sep 17 00:00:00 2001
From: Grygorii Strashko <grygorii.strashko@ti.com>
Date: Mon, 26 Feb 2018 13:53:28 -0600
Subject: [PATCH] [RFC] net: phy: suppress warning when >1 phys connected to one netdev

Some ethernet drivers (like TI CPSW) may connect and manage >1 Net PHYs per
one netdevice, as result such drivers will produce warning during system
boot: 'sysfs: cannot create duplicate filename
'/devices/platform/44000000.ocp/48484000.ethernet/net/eth0/phydev''.

The code introduced waring was added by commit 5568363f0cb3 ("net: phy:
Create sysfs reciprocal links for attached_dev/phydev").

Fix above, by using sysfs_create_link_nowarn() when "phydev" link from
netdev->phydev is created in phy_attach_direct()

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/net/phy/phy_device.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index d39ae77..7010a92 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1014,8 +1014,9 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	err = sysfs_create_link(&phydev->mdio.dev.kobj, &dev->dev.kobj,
 				"attached_dev");
 	if (!err) {
-		err = sysfs_create_link(&dev->dev.kobj, &phydev->mdio.dev.kobj,
-					"phydev");
+		err = sysfs_create_link_nowarn(&dev->dev.kobj,
+					       &phydev->mdio.dev.kobj,
+					       "phydev");
 		if (err)
 			goto error;
 
-- 
2.10.5

Re: regression?: ti: cpsw: warning from phy_connect()->sysfs_create_link()-sysfs_warn_dup()

[Florian Fainelli]

On 02/26/2018 12:08 PM, Grygorii Strashko wrote:
> Hi Florian,
> 
> The TI CPSW driver produces warning as below when booted in switch mode:
> [    8.882295] sysfs: cannot create duplicate filename '/devices/platform/44000000.ocp/48484000.ethernet/net/eth0/phydev'
> [    8.999859] CPU: 1 PID: 356 Comm: systemd-network Not tainted 4.16.0-rc3-00010-g6cc3ff6-dirty #225
> ...
> [    9.012352] Hardware name: Generic DRA74X (Flattened Device Tree)
> [    9.018901] Backtrace: 
> [    9.021376] [<c010c918>] (dump_backtrace) from [<c010cbe8>] (show_stack+0x18/0x1c)
> [    9.028986]  r7:ed036240 r6:60070013 r5:00000000 r4:c0d598a0
> [    9.034684] [<c010cbd0>] (show_stack) from [<c088e1f0>] (dump_stack+0x8c/0xa0)
> [    9.041954] [<c088e164>] (dump_stack) from [<c02ba59c>] (sysfs_warn_dup+0x60/0x6c)
> [    9.049564]  r7:ed036240 r6:ed036240 r5:c0b579bc r4:ed10c000
> [    9.055264] [<c02ba53c>] (sysfs_warn_dup) from [<c02ba930>] (sysfs_do_create_link_sd+0xbc/0xc4)
> [    9.064006]  r7:ed036240 r6:ffffffef r5:00000000 r4:ed034660
> [    9.069701] [<c02ba874>] (sysfs_do_create_link_sd) from [<c02ba968>] (sysfs_create_link+0x30/0x3c)
> [    9.078706]  r9:00000008 r8:00000000 r7:ed02f008 r6:ee015ae8 r5:ee015800 r4:ed02f000
> [    9.086497] [<c02ba938>] (sysfs_create_link) from [<c060f070>] (phy_attach_direct+0x180/0x1f4)
> [    9.095163] [<c060eef0>] (phy_attach_direct) from [<c060f1cc>] (phy_connect_direct+0x1c/0x54)
> [    9.103735]  r10:00000001 r9:ee015d0c r8:00000008 r7:c062e84c r6:c062e84c r5:ed02f000
> [    9.111609]  r4:ed02f000 r3:00000008
> [    9.115217] [<c060f1b0>] (phy_connect_direct) from [<c060f250>] (phy_connect+0x4c/0x80)
> [    9.123270]  r7:c062e84c r6:ee015800 r5:ed02f000 r4:ee693664
> [    9.128969] [<c060f204>] (phy_connect) from [<c062abc8>] (cpsw_slave_open+0x21c/0x274)
> [    9.136926]  r9:ee015d0c r8:ee015d00 r7:ed018a10 r6:ee015800 r5:ee015d00 r4:ed032630
> [    9.144715] [<c062a9ac>] (cpsw_slave_open) from [<c062b28c>] (cpsw_ndo_open+0x158/0x55c)
> [    9.152860]  r10:00000001 r9:00000000 r8:ee015d00 r7:c0d04c48 r6:ee015800 r5:ed018a10
> [    9.160730]  r4:ed032630
> [    9.163291] [<c062b134>] (cpsw_ndo_open) from [<c0765350>] (__dev_open+0xd4/0x158)
> [    9.170900]  r10:00000000 r9:ec885db4 r8:ee01582c r7:c09a9c00 r6:00000000 r5:c0d04c48
> [    9.178768]  r4:ee015800
> [    9.181326] [<c076527c>] (__dev_open) from [<c0765748>] (__dev_change_flags+0x174/0x1c0)
> 
> The reason of the warning is that in switch mode CPSW drivers is connecting two Net PHYs to 
> a single network device (it has been done this way when driver was initially introduced), so
> now it produces warning during boot because of commits:
> 
> 5568363f0cb3 ("net: phy: Create sysfs reciprocal links for attached_dev/phydev"
> a3995460491d ("net: phy: Relax error checking on sysfs_create_link()"
> ^ both went in v4.13
> 
> Honestly, I'm not sure how to fix it the best way (the simplest fix is below), taking into account
> that we are not ready to do big reworks in CPSW driver.

The CPSW driver is duplicating a fair amount of what the DSA subsystem
does properly without breaking any assumptions about the 1:1 mapping
that can exist between a network device and PHY device. Having a PHY
device without a network device is fine in premise, although discouraged.

Migrating to DSA is certainly a big task, but I would strongly encourage
you to consider doing it, since that would solve this problem, and
probably many more.

> 
> Sry, for the late report - in LKML most of dual port TI platforms configured to work in
> dual mac mode, therefore two network devices are created and warning not displayed.
> 


-- 
Florian

Re: regression?: ti: cpsw: warning from phy_connect()->sysfs_create_link()-sysfs_warn_dup()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

Hi Grygorii,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on v4.16-rc3 next-20180227]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Grygorii-Strashko/regression-ti-cpsw-warning-from-phy_connect-sysfs_create_link-sysfs_warn_dup/20180228-000634
config: i386-randconfig-i0-02270651 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> ERROR: "sysfs_create_link_nowarn" [drivers/net/phy/libphy.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31732 bytes --]

Re: regression: ti: cpsw: warning from phy_connect()->sysfs_create_link()-sysfs_warn_dup()

[Grygorii Strashko]

On 02/26/2018 02:16 PM, Florian Fainelli wrote:
> On 02/26/2018 12:08 PM, Grygorii Strashko wrote:
>> Hi Florian,
>>
>> The TI CPSW driver produces warning as below when booted in switch mode:
>> [    8.882295] sysfs: cannot create duplicate filename '/devices/platform/44000000.ocp/48484000.ethernet/net/eth0/phydev'
>> [    8.999859] CPU: 1 PID: 356 Comm: systemd-network Not tainted 4.16.0-rc3-00010-g6cc3ff6-dirty #225
>> ...
>> [    9.012352] Hardware name: Generic DRA74X (Flattened Device Tree)
>> [    9.018901] Backtrace:
>> [    9.021376] [<c010c918>] (dump_backtrace) from [<c010cbe8>] (show_stack+0x18/0x1c)
>> [    9.028986]  r7:ed036240 r6:60070013 r5:00000000 r4:c0d598a0
>> [    9.034684] [<c010cbd0>] (show_stack) from [<c088e1f0>] (dump_stack+0x8c/0xa0)
>> [    9.041954] [<c088e164>] (dump_stack) from [<c02ba59c>] (sysfs_warn_dup+0x60/0x6c)
>> [    9.049564]  r7:ed036240 r6:ed036240 r5:c0b579bc r4:ed10c000
>> [    9.055264] [<c02ba53c>] (sysfs_warn_dup) from [<c02ba930>] (sysfs_do_create_link_sd+0xbc/0xc4)
>> [    9.064006]  r7:ed036240 r6:ffffffef r5:00000000 r4:ed034660
>> [    9.069701] [<c02ba874>] (sysfs_do_create_link_sd) from [<c02ba968>] (sysfs_create_link+0x30/0x3c)
>> [    9.078706]  r9:00000008 r8:00000000 r7:ed02f008 r6:ee015ae8 r5:ee015800 r4:ed02f000
>> [    9.086497] [<c02ba938>] (sysfs_create_link) from [<c060f070>] (phy_attach_direct+0x180/0x1f4)
>> [    9.095163] [<c060eef0>] (phy_attach_direct) from [<c060f1cc>] (phy_connect_direct+0x1c/0x54)
>> [    9.103735]  r10:00000001 r9:ee015d0c r8:00000008 r7:c062e84c r6:c062e84c r5:ed02f000
>> [    9.111609]  r4:ed02f000 r3:00000008
>> [    9.115217] [<c060f1b0>] (phy_connect_direct) from [<c060f250>] (phy_connect+0x4c/0x80)
>> [    9.123270]  r7:c062e84c r6:ee015800 r5:ed02f000 r4:ee693664
>> [    9.128969] [<c060f204>] (phy_connect) from [<c062abc8>] (cpsw_slave_open+0x21c/0x274)
>> [    9.136926]  r9:ee015d0c r8:ee015d00 r7:ed018a10 r6:ee015800 r5:ee015d00 r4:ed032630
>> [    9.144715] [<c062a9ac>] (cpsw_slave_open) from [<c062b28c>] (cpsw_ndo_open+0x158/0x55c)
>> [    9.152860]  r10:00000001 r9:00000000 r8:ee015d00 r7:c0d04c48 r6:ee015800 r5:ed018a10
>> [    9.160730]  r4:ed032630
>> [    9.163291] [<c062b134>] (cpsw_ndo_open) from [<c0765350>] (__dev_open+0xd4/0x158)
>> [    9.170900]  r10:00000000 r9:ec885db4 r8:ee01582c r7:c09a9c00 r6:00000000 r5:c0d04c48
>> [    9.178768]  r4:ee015800
>> [    9.181326] [<c076527c>] (__dev_open) from [<c0765748>] (__dev_change_flags+0x174/0x1c0)
>>
>> The reason of the warning is that in switch mode CPSW drivers is connecting two Net PHYs to
>> a single network device (it has been done this way when driver was initially introduced), so
>> now it produces warning during boot because of commits:
>>
>> 5568363f0cb3 ("net: phy: Create sysfs reciprocal links for attached_dev/phydev"
>> a3995460491d ("net: phy: Relax error checking on sysfs_create_link()"
>> ^ both went in v4.13
>>
>> Honestly, I'm not sure how to fix it the best way (the simplest fix is below), taking into account
>> that we are not ready to do big reworks in CPSW driver.

I've got additional testing data and this actually a *regression*, because 
second CPSW Port became broken after above commits due to Net PHY 
connection failure.

> 
> The CPSW driver is duplicating a fair amount of what the DSA subsystem
> does properly without breaking any assumptions about the 1:1 mapping
> that can exist between a network device and PHY device. Having a PHY
> device without a network device is fine in premise, although discouraged.
> 
> Migrating to DSA is certainly a big task, but I would strongly encourage
> you to consider doing it, since that would solve this problem, and
> probably many more.

We are actively investigating possibility to use DSA for this driver,
but unfortunately this is looks very problematic, because this is old driver with
stable ABI and stable device tree binding which are also ABI. 
So, this driver can't be simply migrating to use DSA and as possible solution
new driver can be introduced in long term perspective which will
follow DSA binding requirements and reuse as max as possible code
of the current CPSW driver. Even in this case, old driver
will need to be supported during some transition period and *be functional*.

For now I'd be very appreciated if functionality of current TI CPSW driver will be
restored and propose to consider below fix, which will make it work again:

=============
>From 5f67320d985a533da785e3643e5e63ba7395b4ae Mon Sep 17 00:00:00 2001
From: Grygorii Strashko <grygorii.strashko@ti.com>
Date: Wed, 14 Mar 2018 14:01:12 -0500
Subject: [PATCH] net: phy: skip error checking when creating sysfs link
 netdev->phydev

Some ethernet drivers (like TI CPSW) may connect and manage >1 Net PHYs per
one netdevice, as result such drivers will produce warning during system
boot and fail to connect second phy to netdevice when PHY device
framework will try to create sysfs link netdev->phydev for second PHY,
because sysfs link with the same name has been created already for the first PHY.
As result, second CPSW external port will became unusable.

Fix it by skipping error checking when PHY device
framework creating sysfs link netdev->phydev. The sysfs_create_link()
will still produce warning, but we can live with it as
system functionality will not be broken. 

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/net/phy/phy_device.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 67f25ac..e2c34c3 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1000,9 +1000,10 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 				"attached_dev");
 	if (!err) {
 		err = sysfs_create_link(&dev->dev.kobj, &phydev->mdio.dev.kobj,
-					"phydev");
-		if (err)
-			goto error;
+				  "phydev");
+		/* don't check return value here as some net drivers can
+		 * use one netdevice with more then one phy
+		 */
 
 		phydev->sysfs_links = true;
 	}
-- 
2.10.5




-- 
regards,
-grygorii

Re: regression: ti: cpsw: warning from phy_connect()->sysfs_create_link()-sysfs_warn_dup()

[Florian Fainelli]

On 03/14/2018 12:40 PM, Grygorii Strashko wrote:
> 
> 
> On 02/26/2018 02:16 PM, Florian Fainelli wrote:
>> On 02/26/2018 12:08 PM, Grygorii Strashko wrote:
>>> Hi Florian,
>>>
>>> The TI CPSW driver produces warning as below when booted in switch mode:
>>> [    8.882295] sysfs: cannot create duplicate filename '/devices/platform/44000000.ocp/48484000.ethernet/net/eth0/phydev'
>>> [    8.999859] CPU: 1 PID: 356 Comm: systemd-network Not tainted 4.16.0-rc3-00010-g6cc3ff6-dirty #225
>>> ...
>>> [    9.012352] Hardware name: Generic DRA74X (Flattened Device Tree)
>>> [    9.018901] Backtrace:
>>> [    9.021376] [<c010c918>] (dump_backtrace) from [<c010cbe8>] (show_stack+0x18/0x1c)
>>> [    9.028986]  r7:ed036240 r6:60070013 r5:00000000 r4:c0d598a0
>>> [    9.034684] [<c010cbd0>] (show_stack) from [<c088e1f0>] (dump_stack+0x8c/0xa0)
>>> [    9.041954] [<c088e164>] (dump_stack) from [<c02ba59c>] (sysfs_warn_dup+0x60/0x6c)
>>> [    9.049564]  r7:ed036240 r6:ed036240 r5:c0b579bc r4:ed10c000
>>> [    9.055264] [<c02ba53c>] (sysfs_warn_dup) from [<c02ba930>] (sysfs_do_create_link_sd+0xbc/0xc4)
>>> [    9.064006]  r7:ed036240 r6:ffffffef r5:00000000 r4:ed034660
>>> [    9.069701] [<c02ba874>] (sysfs_do_create_link_sd) from [<c02ba968>] (sysfs_create_link+0x30/0x3c)
>>> [    9.078706]  r9:00000008 r8:00000000 r7:ed02f008 r6:ee015ae8 r5:ee015800 r4:ed02f000
>>> [    9.086497] [<c02ba938>] (sysfs_create_link) from [<c060f070>] (phy_attach_direct+0x180/0x1f4)
>>> [    9.095163] [<c060eef0>] (phy_attach_direct) from [<c060f1cc>] (phy_connect_direct+0x1c/0x54)
>>> [    9.103735]  r10:00000001 r9:ee015d0c r8:00000008 r7:c062e84c r6:c062e84c r5:ed02f000
>>> [    9.111609]  r4:ed02f000 r3:00000008
>>> [    9.115217] [<c060f1b0>] (phy_connect_direct) from [<c060f250>] (phy_connect+0x4c/0x80)
>>> [    9.123270]  r7:c062e84c r6:ee015800 r5:ed02f000 r4:ee693664
>>> [    9.128969] [<c060f204>] (phy_connect) from [<c062abc8>] (cpsw_slave_open+0x21c/0x274)
>>> [    9.136926]  r9:ee015d0c r8:ee015d00 r7:ed018a10 r6:ee015800 r5:ee015d00 r4:ed032630
>>> [    9.144715] [<c062a9ac>] (cpsw_slave_open) from [<c062b28c>] (cpsw_ndo_open+0x158/0x55c)
>>> [    9.152860]  r10:00000001 r9:00000000 r8:ee015d00 r7:c0d04c48 r6:ee015800 r5:ed018a10
>>> [    9.160730]  r4:ed032630
>>> [    9.163291] [<c062b134>] (cpsw_ndo_open) from [<c0765350>] (__dev_open+0xd4/0x158)
>>> [    9.170900]  r10:00000000 r9:ec885db4 r8:ee01582c r7:c09a9c00 r6:00000000 r5:c0d04c48
>>> [    9.178768]  r4:ee015800
>>> [    9.181326] [<c076527c>] (__dev_open) from [<c0765748>] (__dev_change_flags+0x174/0x1c0)
>>>
>>> The reason of the warning is that in switch mode CPSW drivers is connecting two Net PHYs to
>>> a single network device (it has been done this way when driver was initially introduced), so
>>> now it produces warning during boot because of commits:
>>>
>>> 5568363f0cb3 ("net: phy: Create sysfs reciprocal links for attached_dev/phydev"
>>> a3995460491d ("net: phy: Relax error checking on sysfs_create_link()"
>>> ^ both went in v4.13
>>>
>>> Honestly, I'm not sure how to fix it the best way (the simplest fix is below), taking into account
>>> that we are not ready to do big reworks in CPSW driver.
> 
> I've got additional testing data and this actually a *regression*, because 
> second CPSW Port became broken after above commits due to Net PHY 
> connection failure.
> 
>>
>> The CPSW driver is duplicating a fair amount of what the DSA subsystem
>> does properly without breaking any assumptions about the 1:1 mapping
>> that can exist between a network device and PHY device. Having a PHY
>> device without a network device is fine in premise, although discouraged.
>>
>> Migrating to DSA is certainly a big task, but I would strongly encourage
>> you to consider doing it, since that would solve this problem, and
>> probably many more.
> 
> We are actively investigating possibility to use DSA for this driver,
> but unfortunately this is looks very problematic, because this is old driver with
> stable ABI and stable device tree binding which are also ABI. 
> So, this driver can't be simply migrating to use DSA and as possible solution
> new driver can be introduced in long term perspective which will
> follow DSA binding requirements and reuse as max as possible code
> of the current CPSW driver. Even in this case, old driver
> will need to be supported during some transition period and *be functional*.

That is fair enough, maybe we could explore breaking things more nicely
within DSA such that the Device Tree parsing is largely independent from
the internal representation, and as a result,you could reuse the
existing binding you have, but leverage the DSA infrastructure where it
makes sense, food for thought.

> 
> For now I'd be very appreciated if functionality of current TI CPSW driver will be
> restored and propose to consider below fix, which will make it work again:
> 
> =============
> From 5f67320d985a533da785e3643e5e63ba7395b4ae Mon Sep 17 00:00:00 2001
> From: Grygorii Strashko <grygorii.strashko@ti.com>
> Date: Wed, 14 Mar 2018 14:01:12 -0500
> Subject: [PATCH] net: phy: skip error checking when creating sysfs link
>  netdev->phydev
> 
> Some ethernet drivers (like TI CPSW) may connect and manage >1 Net PHYs per
> one netdevice, as result such drivers will produce warning during system
> boot and fail to connect second phy to netdevice when PHY device
> framework will try to create sysfs link netdev->phydev for second PHY,
> because sysfs link with the same name has been created already for the first PHY.
> As result, second CPSW external port will became unusable.
> 
> Fix it by skipping error checking when PHY device
> framework creating sysfs link netdev->phydev. The sysfs_create_link()
> will still produce warning, but we can live with it as
> system functionality will not be broken. 
> 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
>  drivers/net/phy/phy_device.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 67f25ac..e2c34c3 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1000,9 +1000,10 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  				"attached_dev");
>  	if (!err) {
>  		err = sysfs_create_link(&dev->dev.kobj, &phydev->mdio.dev.kobj,
> -					"phydev");
> -		if (err)
> -			goto error;
> +				  "phydev");
> +		/* don't check return value here as some net drivers can
> +		 * use one netdevice with more then one phy
> +		 */
>  
>  		phydev->sysfs_links = true;

Can you make sure that the single boolean tracking the state of both
sysfs links is not going to cause any problem in your case? Try
unbinding the PHY driver from your PHY device for instance to check
that. If that still works and does not produce a warning then:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

and maybe add a Fixes: tag when you submit this officially?
-- 
Florian

Re: regression: ti: cpsw: warning from phy_connect()->sysfs_create_link()-sysfs_warn_dup()

[Grygorii Strashko]

On 03/14/2018 02:50 PM, Florian Fainelli wrote:
> On 03/14/2018 12:40 PM, Grygorii Strashko wrote:
>>
>>
>> On 02/26/2018 02:16 PM, Florian Fainelli wrote:
>>> On 02/26/2018 12:08 PM, Grygorii Strashko wrote:
>>>> Hi Florian,
>>>>
>>>> The TI CPSW driver produces warning as below when booted in switch mode:
>>>> [    8.882295] sysfs: cannot create duplicate filename '/devices/platform/44000000.ocp/48484000.ethernet/net/eth0/phydev'
>>>> [    8.999859] CPU: 1 PID: 356 Comm: systemd-network Not tainted 4.16.0-rc3-00010-g6cc3ff6-dirty #225
>>>> ...
>>>> [    9.012352] Hardware name: Generic DRA74X (Flattened Device Tree)
>>>> [    9.018901] Backtrace:
>>>> [    9.021376] [<c010c918>] (dump_backtrace) from [<c010cbe8>] (show_stack+0x18/0x1c)
>>>> [    9.028986]  r7:ed036240 r6:60070013 r5:00000000 r4:c0d598a0
>>>> [    9.034684] [<c010cbd0>] (show_stack) from [<c088e1f0>] (dump_stack+0x8c/0xa0)
>>>> [    9.041954] [<c088e164>] (dump_stack) from [<c02ba59c>] (sysfs_warn_dup+0x60/0x6c)
>>>> [    9.049564]  r7:ed036240 r6:ed036240 r5:c0b579bc r4:ed10c000
>>>> [    9.055264] [<c02ba53c>] (sysfs_warn_dup) from [<c02ba930>] (sysfs_do_create_link_sd+0xbc/0xc4)
>>>> [    9.064006]  r7:ed036240 r6:ffffffef r5:00000000 r4:ed034660
>>>> [    9.069701] [<c02ba874>] (sysfs_do_create_link_sd) from [<c02ba968>] (sysfs_create_link+0x30/0x3c)
>>>> [    9.078706]  r9:00000008 r8:00000000 r7:ed02f008 r6:ee015ae8 r5:ee015800 r4:ed02f000
>>>> [    9.086497] [<c02ba938>] (sysfs_create_link) from [<c060f070>] (phy_attach_direct+0x180/0x1f4)
>>>> [    9.095163] [<c060eef0>] (phy_attach_direct) from [<c060f1cc>] (phy_connect_direct+0x1c/0x54)
>>>> [    9.103735]  r10:00000001 r9:ee015d0c r8:00000008 r7:c062e84c r6:c062e84c r5:ed02f000
>>>> [    9.111609]  r4:ed02f000 r3:00000008
>>>> [    9.115217] [<c060f1b0>] (phy_connect_direct) from [<c060f250>] (phy_connect+0x4c/0x80)
>>>> [    9.123270]  r7:c062e84c r6:ee015800 r5:ed02f000 r4:ee693664
>>>> [    9.128969] [<c060f204>] (phy_connect) from [<c062abc8>] (cpsw_slave_open+0x21c/0x274)
>>>> [    9.136926]  r9:ee015d0c r8:ee015d00 r7:ed018a10 r6:ee015800 r5:ee015d00 r4:ed032630
>>>> [    9.144715] [<c062a9ac>] (cpsw_slave_open) from [<c062b28c>] (cpsw_ndo_open+0x158/0x55c)
>>>> [    9.152860]  r10:00000001 r9:00000000 r8:ee015d00 r7:c0d04c48 r6:ee015800 r5:ed018a10
>>>> [    9.160730]  r4:ed032630
>>>> [    9.163291] [<c062b134>] (cpsw_ndo_open) from [<c0765350>] (__dev_open+0xd4/0x158)
>>>> [    9.170900]  r10:00000000 r9:ec885db4 r8:ee01582c r7:c09a9c00 r6:00000000 r5:c0d04c48
>>>> [    9.178768]  r4:ee015800
>>>> [    9.181326] [<c076527c>] (__dev_open) from [<c0765748>] (__dev_change_flags+0x174/0x1c0)
>>>>
>>>> The reason of the warning is that in switch mode CPSW drivers is connecting two Net PHYs to
>>>> a single network device (it has been done this way when driver was initially introduced), so
>>>> now it produces warning during boot because of commits:
>>>>
>>>> 5568363f0cb3 ("net: phy: Create sysfs reciprocal links for attached_dev/phydev"
>>>> a3995460491d ("net: phy: Relax error checking on sysfs_create_link()"
>>>> ^ both went in v4.13
>>>>
>>>> Honestly, I'm not sure how to fix it the best way (the simplest fix is below), taking into account
>>>> that we are not ready to do big reworks in CPSW driver.
>>
>> I've got additional testing data and this actually a *regression*, because
>> second CPSW Port became broken after above commits due to Net PHY
>> connection failure.
>>
>>>
>>> The CPSW driver is duplicating a fair amount of what the DSA subsystem
>>> does properly without breaking any assumptions about the 1:1 mapping
>>> that can exist between a network device and PHY device. Having a PHY
>>> device without a network device is fine in premise, although discouraged.
>>>
>>> Migrating to DSA is certainly a big task, but I would strongly encourage
>>> you to consider doing it, since that would solve this problem, and
>>> probably many more.
>>
>> We are actively investigating possibility to use DSA for this driver,
>> but unfortunately this is looks very problematic, because this is old driver with
>> stable ABI and stable device tree binding which are also ABI.
>> So, this driver can't be simply migrating to use DSA and as possible solution
>> new driver can be introduced in long term perspective which will
>> follow DSA binding requirements and reuse as max as possible code
>> of the current CPSW driver. Even in this case, old driver
>> will need to be supported during some transition period and *be functional*.
> 
> That is fair enough, maybe we could explore breaking things more nicely
> within DSA such that the Device Tree parsing is largely independent from
> the internal representation, and as a result,you could reuse the
> existing binding you have, but leverage the DSA infrastructure where it
> makes sense, food for thought.
> 
>>
>> For now I'd be very appreciated if functionality of current TI CPSW driver will be
>> restored and propose to consider below fix, which will make it work again:
>>
>> =============
>>  From 5f67320d985a533da785e3643e5e63ba7395b4ae Mon Sep 17 00:00:00 2001
>> From: Grygorii Strashko <grygorii.strashko@ti.com>
>> Date: Wed, 14 Mar 2018 14:01:12 -0500
>> Subject: [PATCH] net: phy: skip error checking when creating sysfs link
>>   netdev->phydev
>>
>> Some ethernet drivers (like TI CPSW) may connect and manage >1 Net PHYs per
>> one netdevice, as result such drivers will produce warning during system
>> boot and fail to connect second phy to netdevice when PHY device
>> framework will try to create sysfs link netdev->phydev for second PHY,
>> because sysfs link with the same name has been created already for the first PHY.
>> As result, second CPSW external port will became unusable.
>>
>> Fix it by skipping error checking when PHY device
>> framework creating sysfs link netdev->phydev. The sysfs_create_link()
>> will still produce warning, but we can live with it as
>> system functionality will not be broken.
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>>   drivers/net/phy/phy_device.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 67f25ac..e2c34c3 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -1000,9 +1000,10 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>>   				"attached_dev");
>>   	if (!err) {
>>   		err = sysfs_create_link(&dev->dev.kobj, &phydev->mdio.dev.kobj,
>> -					"phydev");
>> -		if (err)
>> -			goto error;
>> +				  "phydev");
>> +		/* don't check return value here as some net drivers can
>> +		 * use one netdevice with more then one phy
>> +		 */
>>   
>>   		phydev->sysfs_links = true;
> 
> Can you make sure that the single boolean tracking the state of both
> sysfs links is not going to cause any problem in your case?

I've tested it with ifconfig up/down as CPSW driver does
PHY connect in ndo_open() and PHY disconnect in ndo_close()
and saw no issues (sysfs_remove_link()->kernfs_remove_by_name()
handles -ENOENT internally with no warning or other issues for second phy).


 Try
> unbinding the PHY driver from your PHY device for instance to check
> that. If that still works and does not produce a warning then:

It will still produce warning, a can also use sysfs_create_link_nowarn()
and resend if you agree.

> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> and maybe add a Fixes: tag when you submit this officially?
> 

sure.

-- 
regards,
-grygorii

Re: regression: ti: cpsw: warning from phy_connect()->sysfs_create_link()-sysfs_warn_dup()

[Andrew Lunn]

> I've got additional testing data and this actually a *regression*, because 
> second CPSW Port became broken after above commits due to Net PHY 
> connection failure.

Hi Grygorii

I'm not sure this works by design, more by chance. And i think this is
the only MAC driver which does this. Can i suggest you test net-next
frequently, and review phylib patches, because i expect it will break
again unless you keep a close eye on changes. Developers simply don't
expect two phys connected to one MAC.

       Thanks
	Andrew