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.stras...@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.stras...@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.faine...@gmail.com>
>
> and maybe add a Fixes: tag when you submit this officially?
>
sure.
--
regards,
-grygorii
