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

On Wed, Aug 04, 2021 at 11:33:10AM +0300, Ivan T. Ivanov wrote:

I have sent new patch[1] which I think is proper fix for this.

[1] https://lore.kernel.org/netdev/20210804081339.19909-1-iivanov@suse.de/T/#u

Thanks.

I haven't reviewed the driver, but the patch itself LGTM from the
point of view that phy_read_status() should definitely only be
called with phydev->lock held.

I think we also need the "Doing it all yourself" section in
Documentation/networking/phy.rst fixed to specify that if you
call this function, you must be holding phydev->lock.

Lastly, I'm wondering how many other places call phy_read_status()
without holding phydev->lock - sounds like something that needs a
kernel-wide review, and then possibly we should introduce a lockdep
check for this in phy_read_status() to catch any new introductions.

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

On Mon, Aug 09, 2021 at 03:16:33PM +0100, Russell King (Oracle) wrote:

On Wed, Aug 04, 2021 at 11:33:10AM +0300, Ivan T. Ivanov wrote:

quoted

I have sent new patch[1] which I think is proper fix for this.

[1] https://lore.kernel.org/netdev/20210804081339.19909-1-iivanov@suse.de/T/#u

Thanks.

I haven't reviewed the driver, but the patch itself LGTM from the
point of view that phy_read_status() should definitely only be
called with phydev->lock held.

I'm cooking up a patchset which makes phy_read_status() take the
lock. I don't see any external callers taking the lock, so all the
changes are internal to phylib.

The change is however made a bit more complex by phy_read_status()
being in a header file, not phy.c. I wounder if there is some build
dependencies, modules vs built in. So my first patch simply moves it
into phy.c no other change. I will push it to github and let 0-day
chew on it for a while and see if it finds any build failures.

     Andrew