Date:   Wed, 14 Mar 2018 15:20:00 -0500
From:   Grygorii Strashko <grygorii.strashko@...com>
To:     Florian Fainelli <f.fainelli@...il.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 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@...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?

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@...il.com>
> 
> and maybe add a Fixes: tag when you submit this officially?
> 

sure.

-- 
regards,
-grygorii

Powered by blists - more mailing lists