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

From: Leon Romanovsky <leon@kernel.org>
To: Stefan Assmann <sassmann@kpanic.de>
Cc: netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
	patryk.piotrowski@intel.com
Subject: Re: [Intel-wired-lan] [PATCH net-next] iavf: check that state transitions happen under lock
Date: Sun, 6 Nov 2022 21:14:50 +0200	[thread overview]
Message-ID: <Y2gHqj18Tz66k4ZN@unreal> (raw)
In-Reply-To: <20221028134515.253022-1-sassmann@kpanic.de>

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
> 
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan