linux-kernel - Re: [PATCH 0/2] net: phy: relax error checking when creating sysfs link netdev->phydev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 16 Mar 2018 12:11:15 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Grygorii Strashko <grygorii.strashko@...com>,
        Andrew Lunn <andrew@...n.ch>
CC:     "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Sekhar Nori <nsekhar@...com>, linux-kernel@...r.kernel.org,
        linux-omap@...r.kernel.org
Subject: Re: [PATCH 0/2] net: phy: relax error checking when creating sysfs link netdev->phydev

On March 16, 2018 11:42:21 AM PDT, Grygorii Strashko <grygorii.strashko@...com> wrote:
>
>
>On 03/16/2018 12:34 PM, Florian Fainelli wrote:
>> 
>> 
>> On 03/16/2018 10:22 AM, Andrew Lunn wrote:
>>> On Wed, Mar 14, 2018 at 05:26:22PM -0500, Grygorii Strashko wrote:
>>>> 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 PHYLIB
>framework
>>>> will try to create sysfs link netdev->phydev for second PHY
>>>> in phy_attach_direct(), because sysfs link with the same name has
>been
>>>> created already for the first PHY.
>>>> As result, second CPSW external port will became unusable.
>>>> This issue was introduced by commits:
>>>> 5568363f0cb3 ("net: phy: Create sysfs reciprocal links for
>attached_dev/phydev"
>>>> a3995460491d ("net: phy: Relax error checking on
>sysfs_create_link()"
>>>
>>> I wonder if it would be better to add a flag to the phydev that
>>> indicates it is the second PHY connected to a MAC? Add a bit to
>>> phydrv->mdiodrv.flags. If that bit is set, don't create the sysfs
>>> file.
>> 
>> We could indeed do that, I am fine with Grygorii's approach though in
>> making the creation more silent and non fatal.
>
>The link phydev->netdev still can be created. And failure to create
>links
>is non fatal error in my opinion. 

They should not be fatal I agree, but it's nice to know when you are doing something wrong anyway.

>
>> 
>>>
>>> For 99% of MAC drivers, having two PHYs is an error, so we want to
>aid
>>> debug by reporting the sysfs error.
>> That is true, either way is fine with me, really.
>> 
>
>Error still will be reported, just not warning and it will be
>non-fatal.
>So, with this patch set it will be possible now to continue boot (NFS
>for example),
>connect to the system and gather logs.

The point Andrew is trying to make is that you address one particular failure in the PHY creation path when using > 1 PHY devices with a network device. Using a flag would easily allow us to be more future proof with other parts of PHYLIB  for your particular use case if that becomes necessary. This gives you less incentive to fix this use case though.

-- 
Florian