Re: [PATCH] net: phy: leds: Trigger leds only if PHY speed is known

On Wed, Aug 11, 2021 at 12:51:04PM +0300, Ivan T. Ivanov wrote:

There are a few callers of this, but then we have a few callers of
genphy_read_status() too, which are outside just implementing 
phy_driver->read_status() and don't use locking.

I think we need to strongly discourage people using the genphy*
functions directly from anything except phylib driver code. Any
such use is a layering violation.

I think it does make sense to have a set of lower-level API
functions that do the locking necessary, rather than having the
phylib locking spread out across multiple network drivers. It's
better to have it in one place.

Then there a few users of phy_init_eee() which uses phy_read_status(),
again without locking.

That is kind-of a special case - phy_init_eee() can be called by
network drivers from within the phylib adjust_link() callback, and
this callback is made while holding the phydev's lock. So those
cases are safe.

If phy_init_eee() is called outside of that, and the lock isn't
taken, then yes, it's buggy.

This all said, I can't say that I have particularly liked the
phy_init_eee() API. It seems to mix interface-up like configuration
(do we wish clocks to stop in EEE) with working out whether EEE
should be enabled for the speed/duplex that we've just read from
the PHY. However, most users of this function are being called as a
result of a link-up event when we already know the link parameters,
so we shouldn't be re-interrogating the PHY at this point. It seems
to me to be entirely unnecessary.

I think will be easier if we protect public phylib API internally with
lock, otherwise it is easy to make mistakes, obviously. But still this
will not protect users which directly dereference phy_device members.

As Andrew says... but there are some members that network drivers
have to access due to the design of phylib, such as speed, duplex,
*pause, link, etc. Indeed these can change at any time when
phydev->lock is not held due to the action of phylib polling or
link interrupts from the PHY. So, accessing them outside of the
adjust_link() callback without holding the lock is racy.

Note that phylink's usage is similarly safe to the adjust_link()
callback; its access to these members is similarly protected by
phydev->lock taken in the phylib state machine - we use the
slightly lower-level phy_link_change() hook which avoids some of
the help that phylib provides to network drivers (which phylink
really doesn't want phylib managing.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!