[Intel-wired-lan] [PATCH net-next 0/5] iavf: cleanup, improvements in init and

[Intel-wired-lan] [PATCH net-next 0/5] iavf: cleanup, improvements in init and

[Michal Schmidt]

Here are a couple of iavf cleanups and then improvements for the
initialization flow (waiting for the VF reset) and driver removal.

Michal Schmidt (5):
  iavf: fix comments about old bit locks
  iavf: simplify mutex_trylock+sleep loops
  iavf: in iavf_down, don't queue watchdog_task if comms failed
  iavf: in iavf_down, disable queues when removing the driver
  iavf: fix the waiting time for initial reset

 drivers/net/ethernet/intel/iavf/iavf_main.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

-- 
2.41.0

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH net-next 1/5] iavf: fix comments about old bit locks

[Michal Schmidt]

Bit lock __IAVF_IN_CRITICAL_TASK does not exist anymore since commit
5ac49f3c2702 ("iavf: use mutexes for locking of critical sections").
Adjust the comments accordingly.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 43c47c633162..98ecf5d5a2f2 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -1276,7 +1276,7 @@ static void iavf_configure(struct iavf_adapter *adapter)
  * iavf_up_complete - Finish the last steps of bringing up a connection
  * @adapter: board private structure
  *
- * Expects to be called while holding the __IAVF_IN_CRITICAL_TASK bit lock.
+ * Expects to be called while holding crit_lock.
  **/
 static void iavf_up_complete(struct iavf_adapter *adapter)
 {
@@ -1400,7 +1400,7 @@ static void iavf_clear_adv_rss_conf(struct iavf_adapter *adapter)
  * iavf_down - Shutdown the connection processing
  * @adapter: board private structure
  *
- * Expects to be called while holding the __IAVF_IN_CRITICAL_TASK bit lock.
+ * Expects to be called while holding crit_lock.
  **/
 void iavf_down(struct iavf_adapter *adapter)
 {
-- 
2.41.0

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next 1/5] iavf: fix comments about old bit locks

[Drewek, Wojciech]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Michal Schmidt
> Sent: Tuesday, October 10, 2023 2:25 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Radoslaw Tyl
> <radoslawx.tyl@intel.com>; Brandeburg, Jesse <jesse.brandeburg@intel.com>
> Subject: [Intel-wired-lan] [PATCH net-next 1/5] iavf: fix comments about old
> bit locks
> 
> Bit lock __IAVF_IN_CRITICAL_TASK does not exist anymore since commit
> 5ac49f3c2702 ("iavf: use mutexes for locking of critical sections").
> Adjust the comments accordingly.
> 
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>

Thanks for your contribution, Michal!
Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>

> ---
>  drivers/net/ethernet/intel/iavf/iavf_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 43c47c633162..98ecf5d5a2f2 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -1276,7 +1276,7 @@ static void iavf_configure(struct iavf_adapter
> *adapter)
>   * iavf_up_complete - Finish the last steps of bringing up a connection
>   * @adapter: board private structure
>   *
> - * Expects to be called while holding the __IAVF_IN_CRITICAL_TASK bit lock.
> + * Expects to be called while holding crit_lock.
>   **/
>  static void iavf_up_complete(struct iavf_adapter *adapter)
>  {
> @@ -1400,7 +1400,7 @@ static void iavf_clear_adv_rss_conf(struct
> iavf_adapter *adapter)
>   * iavf_down - Shutdown the connection processing
>   * @adapter: board private structure
>   *
> - * Expects to be called while holding the __IAVF_IN_CRITICAL_TASK bit lock.
> + * Expects to be called while holding crit_lock.
>   **/
>  void iavf_down(struct iavf_adapter *adapter)
>  {
> --
> 2.41.0
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next 1/5] iavf: fix comments about old bit locks

[Romanowski, Rafal]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Drewek, Wojciech
> Sent: Tuesday, October 10, 2023 1:24 PM
> To: mschmidt <mschmidt@redhat.com>; intel-wired-lan@lists.osuosl.org
> Cc: Radoslaw Tyl <radoslawx.tyl@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; Brandeburg, Jesse
> <jesse.brandeburg@intel.com>
> Subject: Re: [Intel-wired-lan] [PATCH net-next 1/5] iavf: fix comments about
> old bit locks
> 
> > -----Original Message-----
> > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> > Of Michal Schmidt
> > Sent: Tuesday, October 10, 2023 2:25 AM
> > To: intel-wired-lan@lists.osuosl.org
> > Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Radoslaw Tyl
> > <radoslawx.tyl@intel.com>; Brandeburg, Jesse
> > <jesse.brandeburg@intel.com>
> > Subject: [Intel-wired-lan] [PATCH net-next 1/5] iavf: fix comments
> > about old bit locks
> >
> > Bit lock __IAVF_IN_CRITICAL_TASK does not exist anymore since commit
> > 5ac49f3c2702 ("iavf: use mutexes for locking of critical sections").
> > Adjust the comments accordingly.
> >
> > Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> 
> Thanks for your contribution, Michal!
> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
> 
> > ---
> >  drivers/net/ethernet/intel/iavf/iavf_main.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > index 43c47c633162..98ecf5d5a2f2 100644
> > --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > @@ -1276,7 +1276,7 @@ static void iavf_configure(struct iavf_adapter
> > *adapter)
> >   * iavf_up_complete - Finish the last steps of bringing up a connection
> >   * @adapter: board private structure
> >   *
> > - * Expects to be called while holding the __IAVF_IN_CRITICAL_TASK bit lock.
> > + * Expects to be called while holding crit_lock.
> >   **/
> >  static void iavf_up_complete(struct iavf_adapter *adapter)  { @@
> > -1400,7 +1400,7 @@ static void iavf_clear_adv_rss_conf(struct
> > iavf_adapter *adapter)
> >   * iavf_down - Shutdown the connection processing
> >   * @adapter: board private structure
> >   *
> > - * Expects to be called while holding the __IAVF_IN_CRITICAL_TASK bit lock.
> > + * Expects to be called while holding crit_lock.
> >   **/
> >  void iavf_down(struct iavf_adapter *adapter)  {
> > --
> > 2.41.0
> >
> > _______________________________________________
> > Intel-wired-lan mailing list
> > Intel-wired-lan@osuosl.org
> > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan


Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>


_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH net-next 2/5] iavf: simplify mutex_trylock+sleep loops

[Michal Schmidt]

This pattern appears in two places in the iavf source code:
  while (!mutex_trylock(...))
      usleep_range(...);

That's just mutex_lock with extra steps.
The pattern is a leftover from when iavf used bit flags instead of
mutexes for locking. Commit 5ac49f3c2702 ("iavf: use mutexes for locking
of critical sections") replaced test_and_set_bit with !mutex_trylock,
preserving the pattern.

Simplify it to mutex_lock.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 98ecf5d5a2f2..03156ca523fe 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -3016,8 +3016,7 @@ static void iavf_reset_task(struct work_struct *work)
 		return;
 	}
 
-	while (!mutex_trylock(&adapter->client_lock))
-		usleep_range(500, 1000);
+	mutex_lock(&adapter->client_lock);
 	if (CLIENT_ENABLED(adapter)) {
 		adapter->flags &= ~(IAVF_FLAG_CLIENT_NEEDS_OPEN |
 				    IAVF_FLAG_CLIENT_NEEDS_CLOSE |
@@ -5069,8 +5068,7 @@ static int __maybe_unused iavf_suspend(struct device *dev_d)
 
 	netif_device_detach(netdev);
 
-	while (!mutex_trylock(&adapter->crit_lock))
-		usleep_range(500, 1000);
+	mutex_lock(&adapter->crit_lock);
 
 	if (netif_running(netdev)) {
 		rtnl_lock();
-- 
2.41.0

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next 2/5] iavf: simplify mutex_trylock+sleep loops

[Drewek, Wojciech]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Michal Schmidt
> Sent: Tuesday, October 10, 2023 2:25 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Radoslaw Tyl
> <radoslawx.tyl@intel.com>; Brandeburg, Jesse <jesse.brandeburg@intel.com>
> Subject: [Intel-wired-lan] [PATCH net-next 2/5] iavf: simplify
> mutex_trylock+sleep loops
> 
> This pattern appears in two places in the iavf source code:
>   while (!mutex_trylock(...))
>       usleep_range(...);

I found a few other places with similar pattern (iavf_configure_clsflower e.g).
Do you think we can fix them as well?

> 
> That's just mutex_lock with extra steps.
> The pattern is a leftover from when iavf used bit flags instead of
> mutexes for locking. Commit 5ac49f3c2702 ("iavf: use mutexes for locking
> of critical sections") replaced test_and_set_bit with !mutex_trylock,
> preserving the pattern.
> 
> Simplify it to mutex_lock.
> 
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
>  drivers/net/ethernet/intel/iavf/iavf_main.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 98ecf5d5a2f2..03156ca523fe 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -3016,8 +3016,7 @@ static void iavf_reset_task(struct work_struct
> *work)
>  		return;
>  	}
> 
> -	while (!mutex_trylock(&adapter->client_lock))
> -		usleep_range(500, 1000);
> +	mutex_lock(&adapter->client_lock);
>  	if (CLIENT_ENABLED(adapter)) {
>  		adapter->flags &= ~(IAVF_FLAG_CLIENT_NEEDS_OPEN |
>  				    IAVF_FLAG_CLIENT_NEEDS_CLOSE |
> @@ -5069,8 +5068,7 @@ static int __maybe_unused iavf_suspend(struct
> device *dev_d)
> 
>  	netif_device_detach(netdev);
> 
> -	while (!mutex_trylock(&adapter->crit_lock))
> -		usleep_range(500, 1000);
> +	mutex_lock(&adapter->crit_lock);
> 
>  	if (netif_running(netdev)) {
>  		rtnl_lock();
> --
> 2.41.0
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next 2/5] iavf: simplify mutex_trylock+sleep loops

[Michal Schmidt]

Dne 10. 10. 23 v 13:30 Drewek, Wojciech napsal:
>> -----Original Message-----
>> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
>> Michal Schmidt
>> Sent: Tuesday, October 10, 2023 2:25 AM
>> To: intel-wired-lan@lists.osuosl.org
>> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Radoslaw Tyl
>> <radoslawx.tyl@intel.com>; Brandeburg, Jesse <jesse.brandeburg@intel.com>
>> Subject: [Intel-wired-lan] [PATCH net-next 2/5] iavf: simplify
>> mutex_trylock+sleep loops
>>
>> This pattern appears in two places in the iavf source code:
>>    while (!mutex_trylock(...))
>>        usleep_range(...);
> 
> I found a few other places with similar pattern (iavf_configure_clsflower e.g).
> Do you think we can fix them as well?

I think so. But those are with some sort of a timeout, so replacing them 
would be slightly less obviously correct than these here. Later.
Michal

>> That's just mutex_lock with extra steps.
>> The pattern is a leftover from when iavf used bit flags instead of
>> mutexes for locking. Commit 5ac49f3c2702 ("iavf: use mutexes for locking
>> of critical sections") replaced test_and_set_bit with !mutex_trylock,
>> preserving the pattern.
>>
>> Simplify it to mutex_lock.
>>
>> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
>> ---
>>   drivers/net/ethernet/intel/iavf/iavf_main.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
>> b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> index 98ecf5d5a2f2..03156ca523fe 100644
>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> @@ -3016,8 +3016,7 @@ static void iavf_reset_task(struct work_struct
>> *work)
>>   		return;
>>   	}
>>
>> -	while (!mutex_trylock(&adapter->client_lock))
>> -		usleep_range(500, 1000);
>> +	mutex_lock(&adapter->client_lock);
>>   	if (CLIENT_ENABLED(adapter)) {
>>   		adapter->flags &= ~(IAVF_FLAG_CLIENT_NEEDS_OPEN |
>>   				    IAVF_FLAG_CLIENT_NEEDS_CLOSE |
>> @@ -5069,8 +5068,7 @@ static int __maybe_unused iavf_suspend(struct
>> device *dev_d)
>>
>>   	netif_device_detach(netdev);
>>
>> -	while (!mutex_trylock(&adapter->crit_lock))
>> -		usleep_range(500, 1000);
>> +	mutex_lock(&adapter->crit_lock);
>>
>>   	if (netif_running(netdev)) {
>>   		rtnl_lock();
>> --
>> 2.41.0
>>
>> _______________________________________________
>> Intel-wired-lan mailing list
>> Intel-wired-lan@osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
> 

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next 2/5] iavf: simplify mutex_trylock+sleep loops

[Drewek, Wojciech]

> -----Original Message-----
> From: Michal Schmidt <mschmidt@redhat.com>
> Sent: Tuesday, October 10, 2023 1:57 PM
> To: Drewek, Wojciech <wojciech.drewek@intel.com>; intel-wired-
> lan@lists.osuosl.org
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Radoslaw Tyl
> <radoslawx.tyl@intel.com>; Brandeburg, Jesse <jesse.brandeburg@intel.com>
> Subject: Re: [Intel-wired-lan] [PATCH net-next 2/5] iavf: simplify
> mutex_trylock+sleep loops
> 
> Dne 10. 10. 23 v 13:30 Drewek, Wojciech napsal:
> >> -----Original Message-----
> >> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> >> Michal Schmidt
> >> Sent: Tuesday, October 10, 2023 2:25 AM
> >> To: intel-wired-lan@lists.osuosl.org
> >> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Radoslaw Tyl
> >> <radoslawx.tyl@intel.com>; Brandeburg, Jesse
> <jesse.brandeburg@intel.com>
> >> Subject: [Intel-wired-lan] [PATCH net-next 2/5] iavf: simplify
> >> mutex_trylock+sleep loops
> >>
> >> This pattern appears in two places in the iavf source code:
> >>    while (!mutex_trylock(...))
> >>        usleep_range(...);
> >
> > I found a few other places with similar pattern (iavf_configure_clsflower e.g).
> > Do you think we can fix them as well?
> 
> I think so. But those are with some sort of a timeout, so replacing them
> would be slightly less obviously correct than these here. Later.
> Michal

Makes sense.
Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>

> 
> >> That's just mutex_lock with extra steps.
> >> The pattern is a leftover from when iavf used bit flags instead of
> >> mutexes for locking. Commit 5ac49f3c2702 ("iavf: use mutexes for locking
> >> of critical sections") replaced test_and_set_bit with !mutex_trylock,
> >> preserving the pattern.
> >>
> >> Simplify it to mutex_lock.
> >>
> >> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> >> ---
> >>   drivers/net/ethernet/intel/iavf/iavf_main.c | 6 ++----
> >>   1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> >> b/drivers/net/ethernet/intel/iavf/iavf_main.c
> >> index 98ecf5d5a2f2..03156ca523fe 100644
> >> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> >> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> >> @@ -3016,8 +3016,7 @@ static void iavf_reset_task(struct work_struct
> >> *work)
> >>   		return;
> >>   	}
> >>
> >> -	while (!mutex_trylock(&adapter->client_lock))
> >> -		usleep_range(500, 1000);
> >> +	mutex_lock(&adapter->client_lock);
> >>   	if (CLIENT_ENABLED(adapter)) {
> >>   		adapter->flags &= ~(IAVF_FLAG_CLIENT_NEEDS_OPEN |
> >>   				    IAVF_FLAG_CLIENT_NEEDS_CLOSE |
> >> @@ -5069,8 +5068,7 @@ static int __maybe_unused iavf_suspend(struct
> >> device *dev_d)
> >>
> >>   	netif_device_detach(netdev);
> >>
> >> -	while (!mutex_trylock(&adapter->crit_lock))
> >> -		usleep_range(500, 1000);
> >> +	mutex_lock(&adapter->crit_lock);
> >>
> >>   	if (netif_running(netdev)) {
> >>   		rtnl_lock();
> >> --
> >> 2.41.0
> >>
> >> _______________________________________________
> >> Intel-wired-lan mailing list
> >> Intel-wired-lan@osuosl.org
> >> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
> >

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next 2/5] iavf: simplify mutex_trylock+sleep loops

[Romanowski, Rafal]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Drewek, Wojciech
> Sent: Tuesday, October 10, 2023 2:08 PM
> To: mschmidt <mschmidt@redhat.com>; intel-wired-lan@lists.osuosl.org
> Cc: Radoslaw Tyl <radoslawx.tyl@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; Brandeburg, Jesse
> <jesse.brandeburg@intel.com>
> Subject: Re: [Intel-wired-lan] [PATCH net-next 2/5] iavf: simplify
> mutex_trylock+sleep loops
> 
> 
> 
> > -----Original Message-----
> > From: Michal Schmidt <mschmidt@redhat.com>
> > Sent: Tuesday, October 10, 2023 1:57 PM
> > To: Drewek, Wojciech <wojciech.drewek@intel.com>; intel-wired-
> > lan@lists.osuosl.org
> > Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Radoslaw Tyl
> > <radoslawx.tyl@intel.com>; Brandeburg, Jesse
> > <jesse.brandeburg@intel.com>
> > Subject: Re: [Intel-wired-lan] [PATCH net-next 2/5] iavf: simplify
> > mutex_trylock+sleep loops
> >
> > Dne 10. 10. 23 v 13:30 Drewek, Wojciech napsal:
> > >> -----Original Message-----
> > >> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On
> > >> Behalf Of Michal Schmidt
> > >> Sent: Tuesday, October 10, 2023 2:25 AM
> > >> To: intel-wired-lan@lists.osuosl.org
> > >> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Radoslaw Tyl
> > >> <radoslawx.tyl@intel.com>; Brandeburg, Jesse
> > <jesse.brandeburg@intel.com>
> > >> Subject: [Intel-wired-lan] [PATCH net-next 2/5] iavf: simplify
> > >> mutex_trylock+sleep loops
> > >>
> > >> This pattern appears in two places in the iavf source code:
> > >>    while (!mutex_trylock(...))
> > >>        usleep_range(...);
> > >
> > > I found a few other places with similar pattern (iavf_configure_clsflower
> e.g).
> > > Do you think we can fix them as well?
> >
> > I think so. But those are with some sort of a timeout, so replacing
> > them would be slightly less obviously correct than these here. Later.
> > Michal
> 
> Makes sense.
> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
> 
> >
> > >> That's just mutex_lock with extra steps.
> > >> The pattern is a leftover from when iavf used bit flags instead of
> > >> mutexes for locking. Commit 5ac49f3c2702 ("iavf: use mutexes for
> > >> locking of critical sections") replaced test_and_set_bit with
> > >> !mutex_trylock, preserving the pattern.
> > >>
> > >> Simplify it to mutex_lock.
> > >>
> > >> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> > >> ---
> > >>   drivers/net/ethernet/intel/iavf/iavf_main.c | 6 ++----
> > >>   1 file changed, 2 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > >> b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > >> index 98ecf5d5a2f2..03156ca523fe 100644
> > >> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > >> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > >> @@ -3016,8 +3016,7 @@ static void iavf_reset_task(struct
> > >> work_struct
> > >> *work)
> > >>   		return;
> > >>   	}
> > >>
> > >> -	while (!mutex_trylock(&adapter->client_lock))
> > >> -		usleep_range(500, 1000);
> > >> +	mutex_lock(&adapter->client_lock);
> > >>   	if (CLIENT_ENABLED(adapter)) {
> > >>   		adapter->flags &= ~(IAVF_FLAG_CLIENT_NEEDS_OPEN |
> > >>   				    IAVF_FLAG_CLIENT_NEEDS_CLOSE | @@ -
> 5069,8 +5068,7 @@
> > >> static int __maybe_unused iavf_suspend(struct device *dev_d)
> > >>
> > >>   	netif_device_detach(netdev);
> > >>
> > >> -	while (!mutex_trylock(&adapter->crit_lock))
> > >> -		usleep_range(500, 1000);
> > >> +	mutex_lock(&adapter->crit_lock);
> > >>
> > >>   	if (netif_running(netdev)) {
> > >>   		rtnl_lock();
> > >> --
> > >> 2.41.0
> > >>
> > >> _______________________________________________
> > >> Intel-wired-lan mailing list
> > >> Intel-wired-lan@osuosl.org
> > >> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
> > >
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan


Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>


_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH net-next 3/5] iavf: in iavf_down, don't queue watchdog_task if comms failed

[Michal Schmidt]

The reason for queueing watchdog_task is to have it process the
aq_required flags that are being set here. If comms failed, there's
nothing to do, so return early.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 03156ca523fe..0b808fa34801 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -1420,8 +1420,10 @@ void iavf_down(struct iavf_adapter *adapter)
 	iavf_clear_fdir_filters(adapter);
 	iavf_clear_adv_rss_conf(adapter);
 
-	if (!(adapter->flags & IAVF_FLAG_PF_COMMS_FAILED) &&
-	    !(test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section))) {
+	if (adapter->flags & IAVF_FLAG_PF_COMMS_FAILED)
+		return;
+
+	if (!test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section)) {
 		/* cancel any current operation */
 		adapter->current_op = VIRTCHNL_OP_UNKNOWN;
 		/* Schedule operations to close down the HW. Don't wait
-- 
2.41.0

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next 3/5] iavf: in iavf_down, don't queue watchdog_task if comms failed

[Drewek, Wojciech]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Michal Schmidt
> Sent: Tuesday, October 10, 2023 2:25 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Radoslaw Tyl
> <radoslawx.tyl@intel.com>; Brandeburg, Jesse <jesse.brandeburg@intel.com>
> Subject: [Intel-wired-lan] [PATCH net-next 3/5] iavf: in iavf_down, don't
> queue watchdog_task if comms failed
> 
> The reason for queueing watchdog_task is to have it process the
> aq_required flags that are being set here. If comms failed, there's
> nothing to do, so return early.
> 
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>

Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>

> ---
>  drivers/net/ethernet/intel/iavf/iavf_main.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 03156ca523fe..0b808fa34801 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -1420,8 +1420,10 @@ void iavf_down(struct iavf_adapter *adapter)
>  	iavf_clear_fdir_filters(adapter);
>  	iavf_clear_adv_rss_conf(adapter);
> 
> -	if (!(adapter->flags & IAVF_FLAG_PF_COMMS_FAILED) &&
> -	    !(test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section))) {
> +	if (adapter->flags & IAVF_FLAG_PF_COMMS_FAILED)
> +		return;
> +
> +	if (!test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section)) {
>  		/* cancel any current operation */
>  		adapter->current_op = VIRTCHNL_OP_UNKNOWN;
>  		/* Schedule operations to close down the HW. Don't wait
> --
> 2.41.0
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next 3/5] iavf: in iavf_down, don't queue watchdog_task if comms failed

[Romanowski, Rafal]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Drewek, Wojciech
> Sent: Tuesday, October 10, 2023 1:34 PM
> To: mschmidt <mschmidt@redhat.com>; intel-wired-lan@lists.osuosl.org
> Cc: Radoslaw Tyl <radoslawx.tyl@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; Brandeburg, Jesse
> <jesse.brandeburg@intel.com>
> Subject: Re: [Intel-wired-lan] [PATCH net-next 3/5] iavf: in iavf_down, don't
> queue watchdog_task if comms failed
> 
> 
> 
> > -----Original Message-----
> > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> > Of Michal Schmidt
> > Sent: Tuesday, October 10, 2023 2:25 AM
> > To: intel-wired-lan@lists.osuosl.org
> > Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Radoslaw Tyl
> > <radoslawx.tyl@intel.com>; Brandeburg, Jesse
> > <jesse.brandeburg@intel.com>
> > Subject: [Intel-wired-lan] [PATCH net-next 3/5] iavf: in iavf_down,
> > don't queue watchdog_task if comms failed
> >
> > The reason for queueing watchdog_task is to have it process the
> > aq_required flags that are being set here. If comms failed, there's
> > nothing to do, so return early.
> >
> > Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> 
> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
> 
> > ---
> >  drivers/net/ethernet/intel/iavf/iavf_main.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > index 03156ca523fe..0b808fa34801 100644
> > --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > @@ -1420,8 +1420,10 @@ void iavf_down(struct iavf_adapter *adapter)
> >  	iavf_clear_fdir_filters(adapter);
> >  	iavf_clear_adv_rss_conf(adapter);
> >
> > -	if (!(adapter->flags & IAVF_FLAG_PF_COMMS_FAILED) &&
> > -	    !(test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section))) {
> > +	if (adapter->flags & IAVF_FLAG_PF_COMMS_FAILED)
> > +		return;
> > +
> > +	if (!test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section)) {
> >  		/* cancel any current operation */
> >  		adapter->current_op = VIRTCHNL_OP_UNKNOWN;
> >  		/* Schedule operations to close down the HW. Don't wait
> > --
> > 2.41.0
> >
> > _______________________________________________
> > Intel-wired-lan mailing list
> > Intel-wired-lan@osuosl.org
> > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan


Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>



_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH net-next 4/5] iavf: in iavf_down, disable queues when removing the driver

[Michal Schmidt]

In iavf_down, we're skipping the scheduling of certain operations if
the driver is being removed. However, the IAVF_FLAG_AQ_DISABLE_QUEUES
request must not be skipped in this case, because iavf_close waits
for the transition to the __IAVF_DOWN state, which happens in
iavf_virtchnl_completion after the queues are released.

Without this fix, "rmmod iavf" takes half a second per interface that's
up and prints the "Device resources not yet released" warning.

Fixes: c8de44b577eb ("iavf: do not process adminq tasks when __IAVF_IN_REMOVE_TASK is set")
Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 0b808fa34801..2ab08b015b85 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -1440,9 +1440,9 @@ void iavf_down(struct iavf_adapter *adapter)
 			adapter->aq_required |= IAVF_FLAG_AQ_DEL_FDIR_FILTER;
 		if (!list_empty(&adapter->adv_rss_list_head))
 			adapter->aq_required |= IAVF_FLAG_AQ_DEL_ADV_RSS_CFG;
-		adapter->aq_required |= IAVF_FLAG_AQ_DISABLE_QUEUES;
 	}
 
+	adapter->aq_required |= IAVF_FLAG_AQ_DISABLE_QUEUES;
 	mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
 }
 
-- 
2.41.0

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next 4/5] iavf: in iavf_down, disable queues when removing the driver

[Drewek, Wojciech]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Michal Schmidt
> Sent: Tuesday, October 10, 2023 2:25 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Radoslaw Tyl
> <radoslawx.tyl@intel.com>; Brandeburg, Jesse <jesse.brandeburg@intel.com>
> Subject: [Intel-wired-lan] [PATCH net-next 4/5] iavf: in iavf_down, disable
> queues when removing the driver
> 
> In iavf_down, we're skipping the scheduling of certain operations if
> the driver is being removed. However, the IAVF_FLAG_AQ_DISABLE_QUEUES
> request must not be skipped in this case, because iavf_close waits
> for the transition to the __IAVF_DOWN state, which happens in
> iavf_virtchnl_completion after the queues are released.
> 
> Without this fix, "rmmod iavf" takes half a second per interface that's
> up and prints the "Device resources not yet released" warning.
> 
> Fixes: c8de44b577eb ("iavf: do not process adminq tasks when
> __IAVF_IN_REMOVE_TASK is set")
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>

Looks like a fix, could be a separate patch with net as a target.
Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>

> ---
>  drivers/net/ethernet/intel/iavf/iavf_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 0b808fa34801..2ab08b015b85 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -1440,9 +1440,9 @@ void iavf_down(struct iavf_adapter *adapter)
>  			adapter->aq_required |=
> IAVF_FLAG_AQ_DEL_FDIR_FILTER;
>  		if (!list_empty(&adapter->adv_rss_list_head))
>  			adapter->aq_required |=
> IAVF_FLAG_AQ_DEL_ADV_RSS_CFG;
> -		adapter->aq_required |= IAVF_FLAG_AQ_DISABLE_QUEUES;
>  	}
> 
> +	adapter->aq_required |= IAVF_FLAG_AQ_DISABLE_QUEUES;
>  	mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
>  }
> 
> --
> 2.41.0
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next 4/5] iavf: in iavf_down, disable queues when removing the driver

[Michal Schmidt]

Dne 10. 10. 23 v 13:39 Drewek, Wojciech napsal:
> 
> 
>> -----Original Message-----
>> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
>> Michal Schmidt
>> Sent: Tuesday, October 10, 2023 2:25 AM
>> To: intel-wired-lan@lists.osuosl.org
>> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Radoslaw Tyl
>> <radoslawx.tyl@intel.com>; Brandeburg, Jesse <jesse.brandeburg@intel.com>
>> Subject: [Intel-wired-lan] [PATCH net-next 4/5] iavf: in iavf_down, disable
>> queues when removing the driver
>>
>> In iavf_down, we're skipping the scheduling of certain operations if
>> the driver is being removed. However, the IAVF_FLAG_AQ_DISABLE_QUEUES
>> request must not be skipped in this case, because iavf_close waits
>> for the transition to the __IAVF_DOWN state, which happens in
>> iavf_virtchnl_completion after the queues are released.
>>
>> Without this fix, "rmmod iavf" takes half a second per interface that's
>> up and prints the "Device resources not yet released" warning.
>>
>> Fixes: c8de44b577eb ("iavf: do not process adminq tasks when
>> __IAVF_IN_REMOVE_TASK is set")
>> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> 
> Looks like a fix, could be a separate patch with net as a target.

I did not want to separate it from patch 3/5, because it changes the 
logic when IAVF_FLAG_PF_COMMS_FAILED is set. But on second thought, it 
should still work fine in that case too. aq_required would just get 
reset to zero in iavf_watchdog_task in the __IAVF_COMM_FAILED state.

Michal

> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
> 
>> ---
>>   drivers/net/ethernet/intel/iavf/iavf_main.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
>> b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> index 0b808fa34801..2ab08b015b85 100644
>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> @@ -1440,9 +1440,9 @@ void iavf_down(struct iavf_adapter *adapter)
>>   			adapter->aq_required |=
>> IAVF_FLAG_AQ_DEL_FDIR_FILTER;
>>   		if (!list_empty(&adapter->adv_rss_list_head))
>>   			adapter->aq_required |=
>> IAVF_FLAG_AQ_DEL_ADV_RSS_CFG;
>> -		adapter->aq_required |= IAVF_FLAG_AQ_DISABLE_QUEUES;
>>   	}
>>
>> +	adapter->aq_required |= IAVF_FLAG_AQ_DISABLE_QUEUES;
>>   	mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
>>   }
>>
>> --
>> 2.41.0
>>
>> _______________________________________________
>> Intel-wired-lan mailing list
>> Intel-wired-lan@osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
> 

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next 4/5] iavf: in iavf_down, disable queues when removing the driver

[Drewek, Wojciech]

> -----Original Message-----
> From: Michal Schmidt <mschmidt@redhat.com>
> Sent: Tuesday, October 10, 2023 2:22 PM
> To: Drewek, Wojciech <wojciech.drewek@intel.com>; intel-wired-
> lan@lists.osuosl.org
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Radoslaw Tyl
> <radoslawx.tyl@intel.com>; Brandeburg, Jesse <jesse.brandeburg@intel.com>
> Subject: Re: [Intel-wired-lan] [PATCH net-next 4/5] iavf: in iavf_down, disable
> queues when removing the driver
> 
> Dne 10. 10. 23 v 13:39 Drewek, Wojciech napsal:
> >
> >
> >> -----Original Message-----
> >> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> >> Michal Schmidt
> >> Sent: Tuesday, October 10, 2023 2:25 AM
> >> To: intel-wired-lan@lists.osuosl.org
> >> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Radoslaw Tyl
> >> <radoslawx.tyl@intel.com>; Brandeburg, Jesse
> <jesse.brandeburg@intel.com>
> >> Subject: [Intel-wired-lan] [PATCH net-next 4/5] iavf: in iavf_down, disable
> >> queues when removing the driver
> >>
> >> In iavf_down, we're skipping the scheduling of certain operations if
> >> the driver is being removed. However, the
> IAVF_FLAG_AQ_DISABLE_QUEUES
> >> request must not be skipped in this case, because iavf_close waits
> >> for the transition to the __IAVF_DOWN state, which happens in
> >> iavf_virtchnl_completion after the queues are released.
> >>
> >> Without this fix, "rmmod iavf" takes half a second per interface that's
> >> up and prints the "Device resources not yet released" warning.
> >>
> >> Fixes: c8de44b577eb ("iavf: do not process adminq tasks when
> >> __IAVF_IN_REMOVE_TASK is set")
> >> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> >
> > Looks like a fix, could be a separate patch with net as a target.
> 
> I did not want to separate it from patch 3/5, because it changes the
> logic when IAVF_FLAG_PF_COMMS_FAILED is set. But on second thought, it
> should still work fine in that case too. aq_required would just get
> reset to zero in iavf_watchdog_task in the __IAVF_COMM_FAILED state.

I see, so it's up to you :)

> 
> Michal
> 
> > Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
> >
> >> ---
> >>   drivers/net/ethernet/intel/iavf/iavf_main.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> >> b/drivers/net/ethernet/intel/iavf/iavf_main.c
> >> index 0b808fa34801..2ab08b015b85 100644
> >> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> >> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> >> @@ -1440,9 +1440,9 @@ void iavf_down(struct iavf_adapter *adapter)
> >>   			adapter->aq_required |=
> >> IAVF_FLAG_AQ_DEL_FDIR_FILTER;
> >>   		if (!list_empty(&adapter->adv_rss_list_head))
> >>   			adapter->aq_required |=
> >> IAVF_FLAG_AQ_DEL_ADV_RSS_CFG;
> >> -		adapter->aq_required |= IAVF_FLAG_AQ_DISABLE_QUEUES;
> >>   	}
> >>
> >> +	adapter->aq_required |= IAVF_FLAG_AQ_DISABLE_QUEUES;
> >>   	mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
> >>   }
> >>
> >> --
> >> 2.41.0
> >>
> >> _______________________________________________
> >> Intel-wired-lan mailing list
> >> Intel-wired-lan@osuosl.org
> >> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
> >

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next 4/5] iavf: in iavf_down, disable queues when removing the driver

[Romanowski, Rafal]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Drewek, Wojciech
> Sent: Tuesday, October 10, 2023 3:56 PM
> To: mschmidt <mschmidt@redhat.com>; intel-wired-lan@lists.osuosl.org
> Cc: Radoslaw Tyl <radoslawx.tyl@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; Brandeburg, Jesse
> <jesse.brandeburg@intel.com>
> Subject: Re: [Intel-wired-lan] [PATCH net-next 4/5] iavf: in iavf_down, disable
> queues when removing the driver
> 
> 
> 
> > -----Original Message-----
> > From: Michal Schmidt <mschmidt@redhat.com>
> > Sent: Tuesday, October 10, 2023 2:22 PM
> > To: Drewek, Wojciech <wojciech.drewek@intel.com>; intel-wired-
> > lan@lists.osuosl.org
> > Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Radoslaw Tyl
> > <radoslawx.tyl@intel.com>; Brandeburg, Jesse
> > <jesse.brandeburg@intel.com>
> > Subject: Re: [Intel-wired-lan] [PATCH net-next 4/5] iavf: in
> > iavf_down, disable queues when removing the driver
> >
> > Dne 10. 10. 23 v 13:39 Drewek, Wojciech napsal:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On
> > >> Behalf Of Michal Schmidt
> > >> Sent: Tuesday, October 10, 2023 2:25 AM
> > >> To: intel-wired-lan@lists.osuosl.org
> > >> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Radoslaw Tyl
> > >> <radoslawx.tyl@intel.com>; Brandeburg, Jesse
> > <jesse.brandeburg@intel.com>
> > >> Subject: [Intel-wired-lan] [PATCH net-next 4/5] iavf: in iavf_down,
> > >> disable queues when removing the driver
> > >>
> > >> In iavf_down, we're skipping the scheduling of certain operations
> > >> if the driver is being removed. However, the
> > IAVF_FLAG_AQ_DISABLE_QUEUES
> > >> request must not be skipped in this case, because iavf_close waits
> > >> for the transition to the __IAVF_DOWN state, which happens in
> > >> iavf_virtchnl_completion after the queues are released.
> > >>
> > >> Without this fix, "rmmod iavf" takes half a second per interface
> > >> that's up and prints the "Device resources not yet released" warning.
> > >>
> > >> Fixes: c8de44b577eb ("iavf: do not process adminq tasks when
> > >> __IAVF_IN_REMOVE_TASK is set")
> > >> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> > >
> > > Looks like a fix, could be a separate patch with net as a target.
> >
> > I did not want to separate it from patch 3/5, because it changes the
> > logic when IAVF_FLAG_PF_COMMS_FAILED is set. But on second thought, it
> > should still work fine in that case too. aq_required would just get
> > reset to zero in iavf_watchdog_task in the __IAVF_COMM_FAILED state.
> 
> I see, so it's up to you :)
> 
> >
> > Michal
> >
> > > Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
> > >
> > >> ---
> > >>   drivers/net/ethernet/intel/iavf/iavf_main.c | 2 +-
> > >>   1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > >> b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > >> index 0b808fa34801..2ab08b015b85 100644
> > >> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > >> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > >> @@ -1440,9 +1440,9 @@ void iavf_down(struct iavf_adapter *adapter)
> > >>   			adapter->aq_required |=
> > >> IAVF_FLAG_AQ_DEL_FDIR_FILTER;
> > >>   		if (!list_empty(&adapter->adv_rss_list_head))
> > >>   			adapter->aq_required |=
> > >> IAVF_FLAG_AQ_DEL_ADV_RSS_CFG;
> > >> -		adapter->aq_required |= IAVF_FLAG_AQ_DISABLE_QUEUES;
> > >>   	}
> > >>
> > >> +	adapter->aq_required |= IAVF_FLAG_AQ_DISABLE_QUEUES;
> > >>   	mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
> > >>   }
> > >>
> > >> --
> > >> 2.41.0
> > >>
> > >> _______________________________________________
> > >> Intel-wired-lan mailing list
> > >> Intel-wired-lan@osuosl.org
> > >> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
> > >
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan


Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>



_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH net-next 5/5] iavf: fix the waiting time for initial reset

[Michal Schmidt]

Every time I create VFs on ice, I receive at least one "Device is still
in reset (-16), retrying" message per VF. It recovers fine, but typical
usecases should not trigger scary-looking messages.

The waiting for reset is too short. It makes no sense to check every 10
microseconds. Typical reset waiting times are at least tens of
milliseconds and can be several seconds. I suspect the polling interval
was meant to be 10 milliseconds all along.

IAVF_RESET_WAIT_COMPLETE_COUNT is defined as 2000, so the total waiting
time could be over 20 seconds. I have seen resets take 5 seconds (with
128 VFs on ice).

The added benefit of not triggering the "Device is still in reset" path
is that we avoid going through the __IAVF_INIT_FAILED state, which would
take a full second before retrying.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 2ab08b015b85..f35d74566faa 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -4791,7 +4791,7 @@ static int iavf_check_reset_complete(struct iavf_hw *hw)
 		if ((rstat == VIRTCHNL_VFR_VFACTIVE) ||
 		    (rstat == VIRTCHNL_VFR_COMPLETED))
 			return 0;
-		usleep_range(10, 20);
+		msleep(IAVF_RESET_WAIT_MS);
 	}
 	return -EBUSY;
 }
-- 
2.41.0

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next 5/5] iavf: fix the waiting time for initial reset

[Drewek, Wojciech]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Michal Schmidt
> Sent: Tuesday, October 10, 2023 2:25 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Radoslaw Tyl
> <radoslawx.tyl@intel.com>; Brandeburg, Jesse <jesse.brandeburg@intel.com>
> Subject: [Intel-wired-lan] [PATCH net-next 5/5] iavf: fix the waiting time for
> initial reset
> 
> Every time I create VFs on ice, I receive at least one "Device is still
> in reset (-16), retrying" message per VF. It recovers fine, but typical
> usecases should not trigger scary-looking messages.
> 
> The waiting for reset is too short. It makes no sense to check every 10
> microseconds. Typical reset waiting times are at least tens of
> milliseconds and can be several seconds. I suspect the polling interval
> was meant to be 10 milliseconds all along.
> 
> IAVF_RESET_WAIT_COMPLETE_COUNT is defined as 2000, so the total waiting
> time could be over 20 seconds. I have seen resets take 5 seconds (with
> 128 VFs on ice).
> 
> The added benefit of not triggering the "Device is still in reset" path
> is that we avoid going through the __IAVF_INIT_FAILED state, which would
> take a full second before retrying.
> 
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>

Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>

> ---
>  drivers/net/ethernet/intel/iavf/iavf_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 2ab08b015b85..f35d74566faa 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -4791,7 +4791,7 @@ static int iavf_check_reset_complete(struct
> iavf_hw *hw)
>  		if ((rstat == VIRTCHNL_VFR_VFACTIVE) ||
>  		    (rstat == VIRTCHNL_VFR_COMPLETED))
>  			return 0;
> -		usleep_range(10, 20);
> +		msleep(IAVF_RESET_WAIT_MS);
>  	}
>  	return -EBUSY;
>  }
> --
> 2.41.0
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next 5/5] iavf: fix the waiting time for initial reset

[Romanowski, Rafal]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Drewek, Wojciech
> Sent: Tuesday, October 10, 2023 1:43 PM
> To: mschmidt <mschmidt@redhat.com>; intel-wired-lan@lists.osuosl.org
> Cc: Radoslaw Tyl <radoslawx.tyl@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; Brandeburg, Jesse
> <jesse.brandeburg@intel.com>
> Subject: Re: [Intel-wired-lan] [PATCH net-next 5/5] iavf: fix the waiting time
> for initial reset
> 
> 
> 
> > -----Original Message-----
> > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> > Of Michal Schmidt
> > Sent: Tuesday, October 10, 2023 2:25 AM
> > To: intel-wired-lan@lists.osuosl.org
> > Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Radoslaw Tyl
> > <radoslawx.tyl@intel.com>; Brandeburg, Jesse
> > <jesse.brandeburg@intel.com>
> > Subject: [Intel-wired-lan] [PATCH net-next 5/5] iavf: fix the waiting
> > time for initial reset
> >
> > Every time I create VFs on ice, I receive at least one "Device is
> > still in reset (-16), retrying" message per VF. It recovers fine, but
> > typical usecases should not trigger scary-looking messages.
> >
> > The waiting for reset is too short. It makes no sense to check every
> > 10 microseconds. Typical reset waiting times are at least tens of
> > milliseconds and can be several seconds. I suspect the polling
> > interval was meant to be 10 milliseconds all along.
> >
> > IAVF_RESET_WAIT_COMPLETE_COUNT is defined as 2000, so the total
> > waiting time could be over 20 seconds. I have seen resets take 5
> > seconds (with
> > 128 VFs on ice).
> >
> > The added benefit of not triggering the "Device is still in reset"
> > path is that we avoid going through the __IAVF_INIT_FAILED state,
> > which would take a full second before retrying.
> >
> > Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> 
> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
> 
> > ---
> >  drivers/net/ethernet/intel/iavf/iavf_main.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > index 2ab08b015b85..f35d74566faa 100644
> > --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > @@ -4791,7 +4791,7 @@ static int iavf_check_reset_complete(struct
> > iavf_hw *hw)
> >  		if ((rstat == VIRTCHNL_VFR_VFACTIVE) ||
> >  		    (rstat == VIRTCHNL_VFR_COMPLETED))
> >  			return 0;
> > -		usleep_range(10, 20);
> > +		msleep(IAVF_RESET_WAIT_MS);
> >  	}
> >  	return -EBUSY;
> >  }
> > --
> > 2.41.0
> >
> > _______________________________________________
> > Intel-wired-lan mailing list
> > Intel-wired-lan@osuosl.org
> > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan


Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>



_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next 0/5] iavf: cleanup, improvements in init and

[Przemek Kitszel]

On 10/10/23 02:24, Michal Schmidt wrote:
> Here are a couple of iavf cleanups and then improvements for the
> initialization flow (waiting for the VF reset) and driver removal.
> 
> Michal Schmidt (5):
>    iavf: fix comments about old bit locks
>    iavf: simplify mutex_trylock+sleep loops
>    iavf: in iavf_down, don't queue watchdog_task if comms failed
>    iavf: in iavf_down, disable queues when removing the driver
>    iavf: fix the waiting time for initial reset
> 
>   drivers/net/ethernet/intel/iavf/iavf_main.c | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
> 

Very nice series, thanks a lot!
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan