[Intel-wired-lan] [PATCH net v1 1/2] iavf: Add helper to check if iavf_remove() is in progress

[Intel-wired-lan] [PATCH net v1 1/2] iavf: Add helper to check if iavf_remove() is in progress

[Jedrzej Jagielski]

From: Brett Creeley <brett.creeley@intel.com>

Currently the driver checks if the __IAVF_IN_REMOVE_TASK bit is set in
the adapter's crit_section bitmap. This is fine, but if the
implementation were to ever change, i.e. a mutex was introduced all of
the callers of test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section)
would have to change. Fix this by introducing the
iavf_is_remove_in_progress() helper function.

Signed-off-by: Brett Creeley <brett.creeley@intel.com>
Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf.h      |  1 +
 drivers/net/ethernet/intel/iavf/iavf_main.c | 17 ++++++++++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
index 49aed3e506a6..c787c152f5b3 100644
--- a/drivers/net/ethernet/intel/iavf/iavf.h
+++ b/drivers/net/ethernet/intel/iavf/iavf.h
@@ -502,6 +502,7 @@ int iavf_parse_vf_resource_msg(struct iavf_adapter *adapter);
 void iavf_schedule_reset(struct iavf_adapter *adapter);
 void iavf_schedule_request_stats(struct iavf_adapter *adapter);
 void iavf_reset(struct iavf_adapter *adapter);
+bool iavf_is_remove_in_progress(struct iavf_adapter *adapter);
 void iavf_set_ethtool_ops(struct net_device *netdev);
 void iavf_update_stats(struct iavf_adapter *adapter);
 void iavf_reset_interrupt_capability(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 f3ecb3bca33d..eec74ee5fc19 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -268,6 +268,15 @@ int iavf_lock_timeout(struct mutex *lock, unsigned int msecs)
 	return -1;
 }
 
+/**
+ * iavf_is_remove_in_progress - Check if a iavf_remove() is in progress
+ * @adapter: board private structure
+ */
+bool iavf_is_remove_in_progress(struct iavf_adapter *adapter)
+{
+	return test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section);
+}
+
 /**
  * iavf_schedule_reset - Set the flags and schedule a reset event
  * @adapter: board private structure
@@ -2588,8 +2597,7 @@ static void iavf_watchdog_task(struct work_struct *work)
 				   msecs_to_jiffies(1));
 		return;
 	case __IAVF_INIT_FAILED:
-		if (test_bit(__IAVF_IN_REMOVE_TASK,
-			     &adapter->crit_section)) {
+		if (iavf_is_remove_in_progress(adapter)) {
 			/* Do not update the state and do not reschedule
 			 * watchdog task, iavf_remove should handle this state
 			 * as it can loop forever
@@ -2613,8 +2621,7 @@ static void iavf_watchdog_task(struct work_struct *work)
 		queue_delayed_work(iavf_wq, &adapter->watchdog_task, HZ);
 		return;
 	case __IAVF_COMM_FAILED:
-		if (test_bit(__IAVF_IN_REMOVE_TASK,
-			     &adapter->crit_section)) {
+		if (iavf_is_remove_in_progress(adapter)) {
 			/* Set state to __IAVF_INIT_FAILED and perform remove
 			 * steps. Remove IAVF_FLAG_PF_COMMS_FAILED so the task
 			 * doesn't bring the state back to __IAVF_COMM_FAILED.
@@ -3057,7 +3064,7 @@ static void iavf_adminq_task(struct work_struct *work)
 
 	if ((adapter->flags & IAVF_FLAG_SETUP_NETDEV_FEATURES)) {
 		if (adapter->netdev_registered ||
-		    !test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section)) {
+		    !(iavf_is_remove_in_progress(adapter))) {
 			struct net_device *netdev = adapter->netdev;
 
 			rtnl_lock();
-- 
2.27.0

[Intel-wired-lan] [PATCH net v1 2/2] iavf: Fix call trace during removing iavf driver

[Jedrzej Jagielski]

From: Grzegorz Szczurek <grzegorzx.szczurek@intel.com>

If VF has configured tc qdisc with number of queues less than original
set and then the driver went in removing process, tc resources are also
releasing. Attempt of modify real num tx/rx in this state is causing
call trace, because kernel is trying to releasing uninitialized queues.

Fix by not modifying the number of queues when removing a driver.

Fixes: d5b33d024496 ("i40evf: add ndo_setup_tc callback to i40evf")
Signed-off-by: Grzegorz Szczurek <grzegorzx.szczurek@intel.com>
Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index eec74ee5fc19..1b46a97f4ad6 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -3478,6 +3478,9 @@ static int __iavf_setup_tc(struct net_device *netdev, void *type_data)
 		}
 	}
 exit:
+	if (iavf_is_remove_in_progress(adapter))
+		return 0;
+
 	return ret;
 }
 
-- 
2.27.0

[Intel-wired-lan] [PATCH net v1 2/2] iavf: Fix call trace during removing iavf driver

[Paul Menzel]

Dear Jedrzej, dear Grzegorz,


Am 27.05.22 um 10:09 schrieb Jedrzej Jagielski:
> From: Grzegorz Szczurek <grzegorzx.szczurek@intel.com>

For the summary, maybe be more specific about the action:

 > Do not modify queue number when removing iavf driver

> If VF has configured tc qdisc with number of queues less than original
> set and then the driver went in removing process, tc resources are also

s/went/goes/

> releasing. Attempt of modify real num tx/rx in this state is causing

released?

> call trace, because kernel is trying to releasing uninitialized queues.

to release

Please add parts of the call trace.

> 
> Fix by not modifying the number of queues when removing a driver.

Fix *it* by ?


Kind regards,

Paul

> 
> Fixes: d5b33d024496 ("i40evf: add ndo_setup_tc callback to i40evf")
> Signed-off-by: Grzegorz Szczurek <grzegorzx.szczurek@intel.com>
> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> ---
>   drivers/net/ethernet/intel/iavf/iavf_main.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index eec74ee5fc19..1b46a97f4ad6 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -3478,6 +3478,9 @@ static int __iavf_setup_tc(struct net_device *netdev, void *type_data)
>   		}
>   	}
>   exit:
> +	if (iavf_is_remove_in_progress(adapter))
> +		return 0;
> +
>   	return ret;
>   }
>   

[Intel-wired-lan] [PATCH net v1 2/2] iavf: Fix call trace during removing iavf driver

[Tony Nguyen]

On 5/27/2022 1:09 AM, Jedrzej Jagielski wrote:
> From: Grzegorz Szczurek <grzegorzx.szczurek@intel.com>
> 
> If VF has configured tc qdisc with number of queues less than original
> set and then the driver went in removing process, tc resources are also
> releasing. Attempt of modify real num tx/rx in this state is causing
> call trace, because kernel is trying to releasing uninitialized queues.
> 
> Fix by not modifying the number of queues when removing a driver.
> 
> Fixes: d5b33d024496 ("i40evf: add ndo_setup_tc callback to i40evf")
> Signed-off-by: Grzegorz Szczurek <grzegorzx.szczurek@intel.com>
> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> ---
>   drivers/net/ethernet/intel/iavf/iavf_main.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index eec74ee5fc19..1b46a97f4ad6 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -3478,6 +3478,9 @@ static int __iavf_setup_tc(struct net_device *netdev, void *type_data)
>   		}
>   	}
>   exit:
> +	if (iavf_is_remove_in_progress(adapter))
> +		return 0;

Why do we check this condition at the end of the function after 
everything is done instead of the beginning?

Also, this function from patch 1, is a refactor for checking a bit. 
Refactors shouldn't be going to net unless necessary. This patch can 
directly check the bit and the other patch can be sent as a follow on 
after this patch has been applied.

Thanks,
Tony

>   	return ret;
>   }
>   

[Intel-wired-lan] [PATCH net v1 1/2] iavf: Add helper to check if iavf_remove() is in progress

[Tony Nguyen]

On 5/27/2022 1:09 AM, Jedrzej Jagielski wrote:
> From: Brett Creeley <brett.creeley@intel.com>
> 
> Currently the driver checks if the __IAVF_IN_REMOVE_TASK bit is set in
> the adapter's crit_section bitmap. This is fine, but if the
> implementation were to ever change, i.e. a mutex was introduced all of
> the callers of test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section)
> would have to change. Fix this by introducing the
> iavf_is_remove_in_progress() helper function.

As I mentioned in patch 2, this refactor should go to net-next, not net.

Thanks,
Tony

> Signed-off-by: Brett Creeley <brett.creeley@intel.com>
> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>

[Intel-wired-lan] [PATCH net v1 2/2] iavf: Fix call trace during removing iavf driver

[Jagielski, Jedrzej]

Hello Paul,

>Dear Jedrzej, dear Grzegorz,
>
>
>Am 27.05.22 um 10:09 schrieb Jedrzej Jagielski:
>> From: Grzegorz Szczurek <grzegorzx.szczurek@intel.com>
>
>For the summary, maybe be more specific about the action:
>
> > Do not modify queue number when removing iavf driver
>
>> If VF has configured tc qdisc with number of queues less than original
>> set and then the driver went in removing process, tc resources are also
>
>s/went/goes/
>
>> releasing. Attempt of modify real num tx/rx in this state is causing
>
>released?
>
>> call trace, because kernel is trying to releasing uninitialized queues.
>
>to release
>
>Please add parts of the call trace.
>
>> 
>> Fix by not modifying the number of queues when removing a driver.
>
>Fix *it* by ?

Thank you for your notices. Commit msg will be corrected.

>
>
>Kind regards,
>
>Paul
>
>> 
>> Fixes: d5b33d024496 ("i40evf: add ndo_setup_tc callback to i40evf")
>> Signed-off-by: Grzegorz Szczurek <grzegorzx.szczurek@intel.com>
>> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
>> ---
>>   drivers/net/ethernet/intel/iavf/iavf_main.c | 3 +++
>>   1 file changed, 3 insertions(+)
>> 
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> index eec74ee5fc19..1b46a97f4ad6 100644
>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> @@ -3478,6 +3478,9 @@ static int __iavf_setup_tc(struct net_device *netdev, void *type_data)
>>   		}
>>   	}
>>   exit:
>> +	if (iavf_is_remove_in_progress(adapter))
>> +		return 0;
>> +
>>   	return ret;
>>   }
>>   

Best regards,
Jedrzej


-----Original Message-----
From: Paul Menzel <pmenzel@molgen.mpg.de> 
Sent: pi?tek, 27 maja 2022 14:21
To: Jagielski, Jedrzej <jedrzej.jagielski@intel.com>; Szczurek, GrzegorzX <grzegorzx.szczurek@intel.com>
Cc: intel-wired-lan at lists.osuosl.org
Subject: Re: [Intel-wired-lan] [PATCH net v1 2/2] iavf: Fix call trace during removing iavf driver

Dear Jedrzej, dear Grzegorz,


Am 27.05.22 um 10:09 schrieb Jedrzej Jagielski:
> From: Grzegorz Szczurek <grzegorzx.szczurek@intel.com>

For the summary, maybe be more specific about the action:

 > Do not modify queue number when removing iavf driver

> If VF has configured tc qdisc with number of queues less than original
> set and then the driver went in removing process, tc resources are also

s/went/goes/

> releasing. Attempt of modify real num tx/rx in this state is causing

released?

> call trace, because kernel is trying to releasing uninitialized queues.

to release

Please add parts of the call trace.

> 
> Fix by not modifying the number of queues when removing a driver.

Fix *it* by ?


Kind regards,

Paul

> 
> Fixes: d5b33d024496 ("i40evf: add ndo_setup_tc callback to i40evf")
> Signed-off-by: Grzegorz Szczurek <grzegorzx.szczurek@intel.com>
> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> ---
>   drivers/net/ethernet/intel/iavf/iavf_main.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index eec74ee5fc19..1b46a97f4ad6 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -3478,6 +3478,9 @@ static int __iavf_setup_tc(struct net_device *netdev, void *type_data)
>   		}
>   	}
>   exit:
> +	if (iavf_is_remove_in_progress(adapter))
> +		return 0;
> +
>   	return ret;
>   }
>   

[Intel-wired-lan] [PATCH net v1 1/2] iavf: Add helper to check if iavf_remove() is in progress

[Jagielski, Jedrzej]

>On 5/27/2022 1:09 AM, Jedrzej Jagielski wrote:
>> From: Brett Creeley <brett.creeley@intel.com>
>> 
>> Currently the driver checks if the __IAVF_IN_REMOVE_TASK bit is set in 
>> the adapter's crit_section bitmap. This is fine, but if the 
>> implementation were to ever change, i.e. a mutex was introduced all of 
>> the callers of test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section) 
>> would have to change. Fix this by introducing the
>> iavf_is_remove_in_progress() helper function.
>
>As I mentioned in patch 2, this refactor should go to net-next, not net.

Sure, I will send it to net-next. I just thought that if this patch contains 
dependencies needed by another patch fixing call trace I can send it
as a patch serie directly to net.
I will use bit check in the other patch as you wrote.
>
>Thanks,
>Tony

Thanks,
Jedrzej 
>
>> Signed-off-by: Brett Creeley <brett.creeley@intel.com>
>> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>


-----Original Message-----
From: Nguyen, Anthony L <anthony.l.nguyen@intel.com> 
Sent: pi?tek, 27 maja 2022 18:58
To: Jagielski, Jedrzej <jedrzej.jagielski@intel.com>; intel-wired-lan at lists.osuosl.org
Cc: Brett Creeley <brett.creeley@intel.com>
Subject: Re: [Intel-wired-lan] [PATCH net v1 1/2] iavf: Add helper to check if iavf_remove() is in progress



On 5/27/2022 1:09 AM, Jedrzej Jagielski wrote:
> From: Brett Creeley <brett.creeley@intel.com>
> 
> Currently the driver checks if the __IAVF_IN_REMOVE_TASK bit is set in 
> the adapter's crit_section bitmap. This is fine, but if the 
> implementation were to ever change, i.e. a mutex was introduced all of 
> the callers of test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section) 
> would have to change. Fix this by introducing the
> iavf_is_remove_in_progress() helper function.

As I mentioned in patch 2, this refactor should go to net-next, not net.

Thanks,
Tony

> Signed-off-by: Brett Creeley <brett.creeley@intel.com>
> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>

Re: [Intel-wired-lan] [PATCH net v1 2/2] iavf: Fix call trace during removing iavf driver

[Jagielski, Jedrzej]

>On 5/27/2022 1:09 AM, Jedrzej Jagielski wrote:
>> From: Grzegorz Szczurek <grzegorzx.szczurek@intel.com>
>> 
>> If VF has configured tc qdisc with number of queues less than original
>> set and then the driver went in removing process, tc resources are also
>> releasing. Attempt of modify real num tx/rx in this state is causing
>> call trace, because kernel is trying to releasing uninitialized queues.
>> 
>> Fix by not modifying the number of queues when removing a driver.
>> 
>> Fixes: d5b33d024496 ("i40evf: add ndo_setup_tc callback to i40evf")
>> Signed-off-by: Grzegorz Szczurek <grzegorzx.szczurek@intel.com>
>> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
>> ---
>>   drivers/net/ethernet/intel/iavf/iavf_main.c | 3 +++
>>   1 file changed, 3 insertions(+)
>> 
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> index eec74ee5fc19..1b46a97f4ad6 100644
>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> @@ -3478,6 +3478,9 @@ static int __iavf_setup_tc(struct net_device *netdev, void *type_data)
>>   		}
>>   	}
>>   exit:
>> +	if (iavf_is_remove_in_progress(adapter))
>> +		return 0;
>
>Why do we check this condition at the end of the function after 
>everything is done instead of the beginning?

Because we want the tc resources to be cleared at first - that is
delete queue_channel section which restores the deafult txrx ques values.
Then there is goto jump and after that our check is added. 
Also that check blocks the final txrx update so place it there seems to be
sensible. 

>
>Also, this function from patch 1, is a refactor for checking a bit. 
>Refactors shouldn't be going to net unless necessary. This patch can 
>directly check the bit and the other patch can be sent as a follow on 
>after this patch has been applied.
>

Sure these two patches will be splitted and applied to different trees.


>Thanks,
>Tony
>
>>   	return ret;
>>   }

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