* [PATCH net-next] iavf: check that state transitions happen under lock
@ 2022-10-28 13:45 Stefan Assmann
2022-11-03 11:46 ` [Intel-wired-lan] " Jankowski, Konrad0
2022-11-06 19:14 ` Leon Romanovsky
0 siblings, 2 replies; 5+ messages in thread
From: Stefan Assmann @ 2022-10-28 13:45 UTC (permalink / raw)
To: intel-wired-lan; +Cc: netdev, anthony.l.nguyen, patryk.piotrowski, sassmann
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: [Intel-wired-lan] [PATCH net-next] iavf: check that state transitions happen under lock
2022-10-28 13:45 [PATCH net-next] iavf: check that state transitions happen under lock Stefan Assmann
@ 2022-11-03 11:46 ` Jankowski, Konrad0
2022-11-06 19:14 ` Leon Romanovsky
1 sibling, 0 replies; 5+ messages in thread
From: Jankowski, Konrad0 @ 2022-11-03 11:46 UTC (permalink / raw)
To: Stefan Assmann, intel-wired-lan; +Cc: netdev, Piotrowski, Patryk
> -----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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] iavf: check that state transitions happen under lock
2022-10-28 13:45 [PATCH net-next] iavf: check that state transitions happen under lock Stefan Assmann
2022-11-03 11:46 ` [Intel-wired-lan] " Jankowski, Konrad0
@ 2022-11-06 19:14 ` Leon Romanovsky
2022-11-07 11:57 ` Stefan Assmann
1 sibling, 1 reply; 5+ messages in thread
From: Leon Romanovsky @ 2022-11-06 19:14 UTC (permalink / raw)
To: Stefan Assmann
Cc: intel-wired-lan, netdev, anthony.l.nguyen, patryk.piotrowski
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] iavf: check that state transitions happen under lock
2022-11-06 19:14 ` Leon Romanovsky
@ 2022-11-07 11:57 ` Stefan Assmann
2022-11-09 13:24 ` Leon Romanovsky
0 siblings, 1 reply; 5+ messages in thread
From: Stefan Assmann @ 2022-11-07 11:57 UTC (permalink / raw)
To: Leon Romanovsky
Cc: intel-wired-lan, netdev, anthony.l.nguyen, patryk.piotrowski
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] iavf: check that state transitions happen under lock
2022-11-07 11:57 ` Stefan Assmann
@ 2022-11-09 13:24 ` Leon Romanovsky
0 siblings, 0 replies; 5+ messages in thread
From: Leon Romanovsky @ 2022-11-09 13:24 UTC (permalink / raw)
To: Stefan Assmann
Cc: intel-wired-lan, netdev, anthony.l.nguyen, patryk.piotrowski
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-11-09 13:24 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-28 13:45 [PATCH net-next] iavf: check that state transitions happen under lock Stefan Assmann
2022-11-03 11:46 ` [Intel-wired-lan] " Jankowski, Konrad0
2022-11-06 19:14 ` Leon Romanovsky
2022-11-07 11:57 ` Stefan Assmann
2022-11-09 13:24 ` Leon Romanovsky
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).