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

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

[Stefan Assmann]

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))
+		dev_dbg(&adapter->pdev->dev, "%s:%d state transition %s to %s\n",
+			func, line,
+			iavf_state_str(adapter->last_state),
+			iavf_state_str(adapter->state));
+	else
+		dev_warn(&adapter->pdev->dev, "%s:%d state transition %s to %s without locking!\n",
+			 func, line,
+			 iavf_state_str(adapter->last_state),
+			 iavf_state_str(adapter->state));
 }
 
+#define iavf_change_state(adapter, state) \
+	__iavf_change_state(adapter, state, __func__, __LINE__)
+
 int iavf_up(struct iavf_adapter *adapter);
 void iavf_down(struct iavf_adapter *adapter);
 int iavf_process_config(struct iavf_adapter *adapter);
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 3fc572341781..bbc0c9f347a7 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -4892,7 +4892,7 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	hw->back = adapter;
 
 	adapter->msg_enable = BIT(DEFAULT_DEBUG_LEVEL_SHIFT) - 1;
-	iavf_change_state(adapter, __IAVF_STARTUP);
+	adapter->state = __IAVF_STARTUP;
 
 	/* Call save state here because it relies on the adapter struct. */
 	pci_save_state(pdev);
-- 
2.37.3

RE: [Intel-wired-lan] [PATCH net-next] iavf: check that state transitions happen under lock

[Jankowski, Konrad0]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Stefan
> Assmann
> Sent: Friday, October 28, 2022 3:45 PM
> To: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; Piotrowski, Patryk <patryk.piotrowski@intel.com>;
> sassmann@kpanic.de
> Subject: [Intel-wired-lan] [PATCH net-next] iavf: check that state transitions
> happen under lock
> 
> 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

Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>

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

[Leon Romanovsky]

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).

In addition, mutex_is_locked() doesn't check that this specific function
is locked. It checks that this lock is used now.

> +		dev_dbg(&adapter->pdev->dev, "%s:%d state transition %s to %s\n",
> +			func, line,
> +			iavf_state_str(adapter->last_state),
> +			iavf_state_str(adapter->state));
> +	else
> +		dev_warn(&adapter->pdev->dev, "%s:%d state transition %s to %s without locking!\n",
> +			 func, line,
> +			 iavf_state_str(adapter->last_state),
> +			 iavf_state_str(adapter->state));
>  }
>  
> +#define iavf_change_state(adapter, state) \
> +	__iavf_change_state(adapter, state, __func__, __LINE__)
> +
>  int iavf_up(struct iavf_adapter *adapter);
>  void iavf_down(struct iavf_adapter *adapter);
>  int iavf_process_config(struct iavf_adapter *adapter);
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 3fc572341781..bbc0c9f347a7 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -4892,7 +4892,7 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	hw->back = adapter;
>  
>  	adapter->msg_enable = BIT(DEFAULT_DEBUG_LEVEL_SHIFT) - 1;
> -	iavf_change_state(adapter, __IAVF_STARTUP);
> +	adapter->state = __IAVF_STARTUP;
>  
>  	/* Call save state here because it relies on the adapter struct. */
>  	pci_save_state(pdev);
> -- 
> 2.37.3
> 

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

[Stefan Assmann]

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.



Thanks for the comments!



  Stefan

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

[Leon Romanovsky]

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
>