Date:   Wed, 14 Mar 2018 12:50:04 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Grygorii Strashko <grygorii.strashko@...com>,
        netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>
Cc:     David Miller <davem@...emloft.net>, Sekhar Nori <nsekhar@...com>
Subject: Re: regression: ti: cpsw: warning from
 phy_connect()->sysfs_create_link()-sysfs_warn_dup()

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@...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@...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@...il.com>

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

Powered by blists - more mailing lists