From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Fainelli Subject: Re: [PATCH net-next] net: phy: switch to lockdep_assert_held in phylib Date: Mon, 12 Nov 2018 10:58:48 -0800 Message-ID: References: <20181112174416.GF5075@lunn.ch> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: David Miller , "netdev@vger.kernel.org" To: Andrew Lunn , Heiner Kallweit Return-path: Received: from mail-pg1-f195.google.com ([209.85.215.195]:36972 "EHLO mail-pg1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727530AbeKMExW (ORCPT ); Mon, 12 Nov 2018 23:53:22 -0500 Received: by mail-pg1-f195.google.com with SMTP id 80so4470611pge.4 for ; Mon, 12 Nov 2018 10:58:51 -0800 (PST) In-Reply-To: <20181112174416.GF5075@lunn.ch> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 11/12/18 9:44 AM, Andrew Lunn wrote: > On Sun, Nov 11, 2018 at 10:33:08PM +0100, Heiner Kallweit wrote: >> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c >> index 2e59a8419..5cb06f021 100644 >> --- a/drivers/net/phy/mdio_bus.c >> +++ b/drivers/net/phy/mdio_bus.c >> @@ -541,7 +541,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); > > Hi Heiner > > I don't think there is a clear right/wrong here. This is not hot path > code. The cost for checking the lock is held is very small compared to > the actual MDIO transaction. So i don't think we really need to > optimise this. I do sometimes build with lockdep on, but not > always. So it is good to know when locking is broken on normal builds. > > Florian, what do you think? lockdep_assert_held_once() also looks at debug_locks (global variable) so it sounds like in that regard, it would be superior in that it allows an user-configurable, general debugging facility to behave consistently as opposed to always having opt-in debugging within the mdio_bus.c file, but that also has a lot of value. I have to admit debugging MDIO bus locking issues is not particularly fun, so I would probably stick with the current code in that regard. -- Florian