Re: [PATCH net-next] iavf: check that state transitions happen under lock

[Leon Romanovsky <leon@kernel.org>]

On Mon, Nov 07, 2022 at 12:57:42PM +0100, Stefan Assmann wrote:
> On 06.11.22 20:14, Leon Romanovsky wrote:
> 
> > On Fri, Oct 28, 2022 at 03:45:15PM +0200, Stefan Assmann wrote:
> 
> >> Add a check to make sure crit_lock is being held during every state
> 
> >> transition and print a warning if that's not the case. For convenience
> 
> >> a wrapper is added that helps pointing out where the locking is missing.
> 
> >>
> 
> >> Make an exception for iavf_probe() as that is too early in the init
> 
> >> process and generates a false positive report.
> 
> >>
> 
> >> Signed-off-by: Stefan Assmann <sassmann@kpanic.de>
> 
> >> ---
> 
> >>  drivers/net/ethernet/intel/iavf/iavf.h      | 23 +++++++++++++++------
> 
> >>  drivers/net/ethernet/intel/iavf/iavf_main.c |  2 +-
> 
> >>  2 files changed, 18 insertions(+), 7 deletions(-)
> 
> >>
> 
> >> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
> 
> >> index 3f6187c16424..28f41bbc9c86 100644
> 
> >> --- a/drivers/net/ethernet/intel/iavf/iavf.h
> 
> >> +++ b/drivers/net/ethernet/intel/iavf/iavf.h
> 
> >> @@ -498,19 +498,30 @@ static inline const char *iavf_state_str(enum iavf_state_t state)
> 
> >>  	}
> 
> >>  }
> 
> >>  
> 
> >> -static inline void iavf_change_state(struct iavf_adapter *adapter,
> 
> >> -				     enum iavf_state_t state)
> 
> >> +static inline void __iavf_change_state(struct iavf_adapter *adapter,
> 
> >> +				       enum iavf_state_t state,
> 
> >> +				       const char *func,
> 
> >> +				       int line)
> 
> >>  {
> 
> >>  	if (adapter->state != state) {
> 
> >>  		adapter->last_state = adapter->state;
> 
> >>  		adapter->state = state;
> 
> >>  	}
> 
> >> -	dev_dbg(&adapter->pdev->dev,
> 
> >> -		"state transition from:%s to:%s\n",
> 
> >> -		iavf_state_str(adapter->last_state),
> 
> >> -		iavf_state_str(adapter->state));
> 
> >> +	if (mutex_is_locked(&adapter->crit_lock))
> 
> > 
> 
> > Please use lockdep for that, and not reinvent it.
> 
> > In you case lockdep_assert_held(&adapter->crit_lock).
> 
> 
> 
> Lockdep is mostly enabled in debug kernel but I was hoping to get
> 
> warnings in production environments as well. As those transitions don't
> 
> happen too often it shouldn't hurt performance.
> 
> 
> 
> > In addition, mutex_is_locked() doesn't check that this specific function
> 
> > is locked. It checks that this lock is used now.
> 
> 
> 
> You are correct, this check only triggers if crit_lock is not locked at
> 
> all. It would be better to check the lock owner, but I couldn't find an
> 
> easy way to do that. Better than no check IMO but we can drop it if you
> 
> don't see a benefit in it.

Yes, please drop it. AFAIK, lockdep doesn't add much overhead while enabled.

Thanks

> 
> 
> 
> Thanks for the comments!
> 
> 
> 
>   Stefan
>