| Message ID | ccc40b9d-8ee0-43a1-5009-2cc95ca79c85@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: replace mutex_is_locked with lockdep_assert_held in phylib | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 32 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Wed, Jan 06, 2021 at 02:03:40PM +0100, Heiner Kallweit wrote: > Switch to lockdep_assert_held(_once), similar to what is being done > in other subsystems. One advantage is that there's zero runtime > overhead if lockdep support isn't enabled. Hi Heiner I'm not sure we are bothered about performance here. MDIO operations are slow, a mutex check is fast relative to that. I wonder how many do development work with lockdep enabled? I think i prefer catching hard to find locking bugs earlier, verses a tiny performance overhead. Andrew
On 06.01.2021 23:39, Andrew Lunn wrote: > On Wed, Jan 06, 2021 at 02:03:40PM +0100, Heiner Kallweit wrote: >> Switch to lockdep_assert_held(_once), similar to what is being done >> in other subsystems. One advantage is that there's zero runtime >> overhead if lockdep support isn't enabled. > > Hi Heiner > Hi Andrew, > I'm not sure we are bothered about performance here. MDIO operations > are slow, a mutex check is fast relative to that. > Right, the performance gain is neglectible here. What I see is that more and more similar checks (e.g. in_softirq, in_irq) are migrated to the lockdep framework. And as stated in the commit message I've seen a number of equivalent patches in other subsystems. > I wonder how many do development work with lockdep enabled? I think i > prefer catching hard to find locking bugs earlier, verses a tiny > performance overhead. > Well, I always develop with lockdep enabled and like the fact that it provides a multitude of checks with minimal overhead. Would be interesting to know the ratio of kernel developers counting on lockdep. > Andrew > Heiner
On Wed, Jan 06, 2021 at 02:03:40PM +0100, Heiner Kallweit wrote: > Switch to lockdep_assert_held(_once), similar to what is being done > in other subsystems. One advantage is that there's zero runtime > overhead if lockdep support isn't enabled. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Thu, 7 Jan 2021 22:15:06 +0100 Andrew Lunn wrote: > On Wed, Jan 06, 2021 at 02:03:40PM +0100, Heiner Kallweit wrote: > > Switch to lockdep_assert_held(_once), similar to what is being done > > in other subsystems. One advantage is that there's zero runtime > > overhead if lockdep support isn't enabled. > > > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> Applied, thanks! If I was willing to let my pedantic flag fly I'd ask for the lockdep header to be included explicitly, but I guess in practice the chances of it not being pulled into sources which use locking is 0.
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c index 2b42e4606..040509b81 100644 --- a/drivers/net/phy/mdio_bus.c +++ b/drivers/net/phy/mdio_bus.c @@ -740,7 +740,7 @@ int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum) { int retval; - WARN_ON_ONCE(!mutex_is_locked(&bus->mdio_lock)); + lockdep_assert_held_once(&bus->mdio_lock); retval = bus->read(bus, addr, regnum); @@ -766,7 +766,7 @@ int __mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val) { int err; - WARN_ON_ONCE(!mutex_is_locked(&bus->mdio_lock)); + lockdep_assert_held_once(&bus->mdio_lock); err = bus->write(bus, addr, regnum, val); diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 45f75533c..9cb7e4dbf 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -724,7 +724,7 @@ static int phy_check_link_status(struct phy_device *phydev) { int err; - WARN_ON(!mutex_is_locked(&phydev->lock)); + lockdep_assert_held(&phydev->lock); /* Keep previous state if loopback is enabled because some PHYs * report that Link is Down when loopback is enabled. diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 80c2e646c..8447e56ba 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1740,7 +1740,7 @@ int __phy_resume(struct phy_device *phydev) struct phy_driver *phydrv = phydev->drv; int ret; - WARN_ON(!mutex_is_locked(&phydev->lock)); + lockdep_assert_held(&phydev->lock); if (!phydrv || !phydrv->resume) return 0;
Switch to lockdep_assert_held(_once), similar to what is being done in other subsystems. One advantage is that there's zero runtime overhead if lockdep support isn't enabled. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/phy/mdio_bus.c | 4 ++-- drivers/net/phy/phy.c | 2 +- drivers/net/phy/phy_device.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-)