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

On 03/16/2018 02:11 PM, Florian Fainelli wrote:

On March 16, 2018 11:42:21 AM PDT, Grygorii Strashko [off-list ref] wrote:

quoted

On 03/16/2018 12:34 PM, Florian Fainelli wrote:

quoted

On 03/16/2018 10:22 AM, Andrew Lunn wrote:

quoted

On Wed, Mar 14, 2018 at 05:26:22PM -0500, Grygorii Strashko wrote:

quoted

Some ethernet drivers (like TI CPSW) may connect and manage >1 Net

PHYs per

quoted

quoted

quoted

one netdevice, as result such drivers will produce warning during

system

quoted

quoted

quoted

boot and fail to connect second phy to netdevice when PHYLIB

framework

quoted

quoted

quoted

will try to create sysfs link netdev->phydev for second PHY
in phy_attach_direct(), because sysfs link with the same name has

been

quoted

quoted

quoted

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"

quoted

quoted

quoted

a3995460491d ("net: phy: Relax error checking on

sysfs_create_link()"

quoted

quoted

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.

quoted

quoted

quoted

For 99% of MAC drivers, having two PHYs is an error, so we want to

aid

quoted

quoted

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.

That's true, I'm fixing use case with >1 and I'll try to re-implement using flag as requested.
But note, this patch in its current form fixes 1:1 (phydev:netdev) use case also (at least as i understand it),
because current code will just kill net connection if create sysfs link fails, so in case of net boot -
failure logs will not be accessible without direct access to the device.

Actually, how can i pass this flag "<name of the flag>" from CPSW to of_phy_connect()->phy_attach_direct()?
The parameter "flags" == phy_device->dev_flags is used to pass PHY driver's specific options, so can't be used.

The phydrv->mdiodrv.flags can be accessible only after call to of_phy_connect()/phy_connect(), 
but sysfs links are created inside these functions.

Thanks.
-- 
regards,
-grygorii