[PATCH iwl-net] idpf: check error for register_netdev() on init

[PATCH iwl-net] idpf: check error for register_netdev() on init

[Emil Tantilov]

Current init logic ignores the error code from register_netdev(),
which will cause WARN_ON() on attempt to unregister it, if there was one,
and there is no info for the user that the creation of the netdev failed.

WARNING: CPU: 89 PID: 6902 at net/core/dev.c:11512 unregister_netdevice_many_notify+0x211/0x1a10
...
[ 3707.563641]  unregister_netdev+0x1c/0x30
[ 3707.563656]  idpf_vport_dealloc+0x5cf/0xce0 [idpf]
[ 3707.563684]  idpf_deinit_task+0xef/0x160 [idpf]
[ 3707.563712]  idpf_vc_core_deinit+0x84/0x320 [idpf]
[ 3707.563739]  idpf_remove+0xbf/0x780 [idpf]
[ 3707.563769]  pci_device_remove+0xab/0x1e0
[ 3707.563786]  device_release_driver_internal+0x371/0x530
[ 3707.563803]  driver_detach+0xbf/0x180
[ 3707.563816]  bus_remove_driver+0x11b/0x2a0
[ 3707.563829]  pci_unregister_driver+0x2a/0x250

Introduce an error check and log the vport number and error code.
On removal make sure to check VPORT_REG_NETDEV flag prior to calling
unregister and free on the netdev.

Add local variables for idx, vport_config and netdev for readability.

Fixes: 0fe45467a104 ("idpf: add create vport and netdev configuration")
Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
Suggested-by: Tony Nguyen <anthony.l.nguyen@intel.com>
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf_lib.c | 27 ++++++++++++++--------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
index a3d6b8f198a8..a322a8ac771e 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
@@ -927,15 +927,19 @@ static int idpf_stop(struct net_device *netdev)
 static void idpf_decfg_netdev(struct idpf_vport *vport)
 {
 	struct idpf_adapter *adapter = vport->adapter;
+	u16 idx = vport->idx;
 
 	kfree(vport->rx_ptype_lkup);
 	vport->rx_ptype_lkup = NULL;
 
-	unregister_netdev(vport->netdev);
-	free_netdev(vport->netdev);
+	if (test_and_clear_bit(IDPF_VPORT_REG_NETDEV,
+			       adapter->vport_config[idx]->flags)) {
+		unregister_netdev(vport->netdev);
+		free_netdev(vport->netdev);
+	}
 	vport->netdev = NULL;
 
-	adapter->netdevs[vport->idx] = NULL;
+	adapter->netdevs[idx] = NULL;
 }
 
 /**
@@ -1536,12 +1540,17 @@ void idpf_init_task(struct work_struct *work)
 	}
 
 	for (index = 0; index < adapter->max_vports; index++) {
-		if (adapter->netdevs[index] &&
-		    !test_bit(IDPF_VPORT_REG_NETDEV,
-			      adapter->vport_config[index]->flags)) {
-			register_netdev(adapter->netdevs[index]);
-			set_bit(IDPF_VPORT_REG_NETDEV,
-				adapter->vport_config[index]->flags);
+		struct idpf_vport_config *vport_config = adapter->vport_config[index];
+		struct net_device *netdev = adapter->netdevs[index];
+
+		if (netdev && !test_bit(IDPF_VPORT_REG_NETDEV, vport_config->flags)) {
+			err = register_netdev(netdev);
+			if (err) {
+				dev_err(&pdev->dev, "failed to register netdev for vport %d: %pe\n",
+					index, ERR_PTR(err));
+				continue;
+			}
+			set_bit(IDPF_VPORT_REG_NETDEV, vport_config->flags);
 		}
 	}
 
-- 
2.17.2

Re: [PATCH iwl-net] idpf: check error for register_netdev() on init

[Simon Horman]

On Mon, Feb 10, 2025 at 06:38:51PM -0800, Emil Tantilov wrote:
> Current init logic ignores the error code from register_netdev(),
> which will cause WARN_ON() on attempt to unregister it, if there was one,
> and there is no info for the user that the creation of the netdev failed.
> 
> WARNING: CPU: 89 PID: 6902 at net/core/dev.c:11512 unregister_netdevice_many_notify+0x211/0x1a10
> ...
> [ 3707.563641]  unregister_netdev+0x1c/0x30
> [ 3707.563656]  idpf_vport_dealloc+0x5cf/0xce0 [idpf]
> [ 3707.563684]  idpf_deinit_task+0xef/0x160 [idpf]
> [ 3707.563712]  idpf_vc_core_deinit+0x84/0x320 [idpf]
> [ 3707.563739]  idpf_remove+0xbf/0x780 [idpf]
> [ 3707.563769]  pci_device_remove+0xab/0x1e0
> [ 3707.563786]  device_release_driver_internal+0x371/0x530
> [ 3707.563803]  driver_detach+0xbf/0x180
> [ 3707.563816]  bus_remove_driver+0x11b/0x2a0
> [ 3707.563829]  pci_unregister_driver+0x2a/0x250
> 
> Introduce an error check and log the vport number and error code.
> On removal make sure to check VPORT_REG_NETDEV flag prior to calling
> unregister and free on the netdev.
> 
> Add local variables for idx, vport_config and netdev for readability.
> 
> Fixes: 0fe45467a104 ("idpf: add create vport and netdev configuration")
> Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
> Suggested-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
> ---
>  drivers/net/ethernet/intel/idpf/idpf_lib.c | 27 ++++++++++++++--------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c

...

> @@ -1536,12 +1540,17 @@ void idpf_init_task(struct work_struct *work)
>  	}
>  
>  	for (index = 0; index < adapter->max_vports; index++) {
> -		if (adapter->netdevs[index] &&
> -		    !test_bit(IDPF_VPORT_REG_NETDEV,
> -			      adapter->vport_config[index]->flags)) {
> -			register_netdev(adapter->netdevs[index]);
> -			set_bit(IDPF_VPORT_REG_NETDEV,
> -				adapter->vport_config[index]->flags);
> +		struct idpf_vport_config *vport_config = adapter->vport_config[index];
> +		struct net_device *netdev = adapter->netdevs[index];
> +
> +		if (netdev && !test_bit(IDPF_VPORT_REG_NETDEV, vport_config->flags)) {
> +			err = register_netdev(netdev);
> +			if (err) {
> +				dev_err(&pdev->dev, "failed to register netdev for vport %d: %pe\n",
> +					index, ERR_PTR(err));
> +				continue;
> +			}
> +			set_bit(IDPF_VPORT_REG_NETDEV, vport_config->flags);
>  		}
>  	}

Hi Emil,

I'm wondering if we could reduce indentation and lines longer
than 80 characters in the above like this (completely untested!):


	for (index = 0; index < adapter->max_vports; index++) {
		struct idpf_vport_config *vport_config = adapter->vport_config[index];
		struct net_device *netdev = adapter->netdevs[index];

		if (!netdev ||
		    test_bit(IDPF_VPORT_REG_NETDEV, vport_config->flags))
		    continue;

		err = register_netdev(netdev);
		if (err) {
			dev_err(&pdev->dev, "failed to register netdev for vport %d: %pe\n",
				index, ERR_PTR(err));
			continue;
		}
		set_bit(IDPF_VPORT_REG_NETDEV, vport_config->flags);
	}

Re: [Intel-wired-lan] [PATCH iwl-net] idpf: check error for register_netdev() on init

[Tantilov, Emil S]

On 2/12/2025 10:21 AM, Simon Horman wrote:
> On Mon, Feb 10, 2025 at 06:38:51PM -0800, Emil Tantilov wrote:
>> Current init logic ignores the error code from register_netdev(),
>> which will cause WARN_ON() on attempt to unregister it, if there was one,
>> and there is no info for the user that the creation of the netdev failed.
>>
>> WARNING: CPU: 89 PID: 6902 at net/core/dev.c:11512 unregister_netdevice_many_notify+0x211/0x1a10
>> ...
>> [ 3707.563641]  unregister_netdev+0x1c/0x30
>> [ 3707.563656]  idpf_vport_dealloc+0x5cf/0xce0 [idpf]
>> [ 3707.563684]  idpf_deinit_task+0xef/0x160 [idpf]
>> [ 3707.563712]  idpf_vc_core_deinit+0x84/0x320 [idpf]
>> [ 3707.563739]  idpf_remove+0xbf/0x780 [idpf]
>> [ 3707.563769]  pci_device_remove+0xab/0x1e0
>> [ 3707.563786]  device_release_driver_internal+0x371/0x530
>> [ 3707.563803]  driver_detach+0xbf/0x180
>> [ 3707.563816]  bus_remove_driver+0x11b/0x2a0
>> [ 3707.563829]  pci_unregister_driver+0x2a/0x250
>>
>> Introduce an error check and log the vport number and error code.
>> On removal make sure to check VPORT_REG_NETDEV flag prior to calling
>> unregister and free on the netdev.
>>
>> Add local variables for idx, vport_config and netdev for readability.
>>
>> Fixes: 0fe45467a104 ("idpf: add create vport and netdev configuration")
>> Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
>> Suggested-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
>> ---
>>   drivers/net/ethernet/intel/idpf/idpf_lib.c | 27 ++++++++++++++--------
>>   1 file changed, 18 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> 
> ...
> 
>> @@ -1536,12 +1540,17 @@ void idpf_init_task(struct work_struct *work)
>>   	}
>>   
>>   	for (index = 0; index < adapter->max_vports; index++) {
>> -		if (adapter->netdevs[index] &&
>> -		    !test_bit(IDPF_VPORT_REG_NETDEV,
>> -			      adapter->vport_config[index]->flags)) {
>> -			register_netdev(adapter->netdevs[index]);
>> -			set_bit(IDPF_VPORT_REG_NETDEV,
>> -				adapter->vport_config[index]->flags);
>> +		struct idpf_vport_config *vport_config = adapter->vport_config[index];
>> +		struct net_device *netdev = adapter->netdevs[index];
>> +
>> +		if (netdev && !test_bit(IDPF_VPORT_REG_NETDEV, vport_config->flags)) {
>> +			err = register_netdev(netdev);
>> +			if (err) {
>> +				dev_err(&pdev->dev, "failed to register netdev for vport %d: %pe\n",
>> +					index, ERR_PTR(err));
>> +				continue;
>> +			}
>> +			set_bit(IDPF_VPORT_REG_NETDEV, vport_config->flags);
>>   		}
>>   	}
> 
> Hi Emil,
> 
> I'm wondering if we could reduce indentation and lines longer
> than 80 characters in the above like this (completely untested!):
I was mostly trying to focus on the fix itself, since this patch is -net 
bound. The >80 line came about from the introduction of the local netdev 
and it seemed cleaner to keep it in one line. I can just split the check 
as in the original code.

> 
> 
> 	for (index = 0; index < adapter->max_vports; index++) {
> 		struct idpf_vport_config *vport_config = adapter->vport_config[index];
> 		struct net_device *netdev = adapter->netdevs[index];
> 
> 		if (!netdev ||
> 		    test_bit(IDPF_VPORT_REG_NETDEV, vport_config->flags))
> 		    continue;
Again, because its mainly to add the error checking I am not sure if its 
OK to re-shuffle the logic.

> 
> 		err = register_netdev(netdev);
> 		if (err) {
> 			dev_err(&pdev->dev, "failed to register netdev for vport %d: %pe\n",
> 				index, ERR_PTR(err));
> 			continue;
> 		}
> 		set_bit(IDPF_VPORT_REG_NETDEV, vport_config->flags);
> 	}

Don't mind re-spinning (and testing) v2 with the proposed change, if 
it's not infringing on the guidelines for submission to -net.

Thanks,
Emil

Re: [Intel-wired-lan] [PATCH iwl-net] idpf: check error for register_netdev() on init

[Przemek Kitszel]

On 2/13/25 21:39, Tantilov, Emil S wrote:
> On 2/12/2025 10:21 AM, Simon Horman wrote:
>> On Mon, Feb 10, 2025 at 06:38:51PM -0800, Emil Tantilov wrote:
>>> Current init logic ignores the error code from register_netdev(),
>>> which will cause WARN_ON() on attempt to unregister it, if there was 
>>> one,
>>> and there is no info for the user that the creation of the netdev 
>>> failed.


>> Hi Emil,
>>
>> I'm wondering if we could reduce indentation and lines longer
>> than 80 characters in the above like this (completely untested!):
> I was mostly trying to focus on the fix itself, since this patch is -net 
> bound. The >80 line came about from the introduction of the local netdev 
> and it seemed cleaner to keep it in one line. I can just split the check 
> as in the original code.
> 
>>
>>
>>     for (index = 0; index < adapter->max_vports; index++) {
>>         struct idpf_vport_config *vport_config = adapter- 
>> >vport_config[index];
>>         struct net_device *netdev = adapter->netdevs[index];
>>
>>         if (!netdev ||
>>             test_bit(IDPF_VPORT_REG_NETDEV, vport_config->flags))
>>             continue;
> Again, because its mainly to add the error checking I am not sure if its 
> OK to re-shuffle the logic.
> 
>>
>>         err = register_netdev(netdev);
>>         if (err) {
>>             dev_err(&pdev->dev, "failed to register netdev for vport 
>> %d: %pe\n",
>>                 index, ERR_PTR(err));
>>             continue;
>>         }
>>         set_bit(IDPF_VPORT_REG_NETDEV, vport_config->flags);
>>     }
> 
> Don't mind re-spinning (and testing) v2 with the proposed change, if 
> it's not infringing on the guidelines for submission to -net.
> 
> Thanks,
> Emil

Emil, you are right that we generally don't want to do refactors on -net
submissions. In this particular case your code was just replacing the
body of the loop. Simon's snippet did the same, just with less
indentation.

The "early return" (or "continue") style is better, also in this case.

Re: [Intel-wired-lan] [PATCH iwl-net] idpf: check error for register_netdev() on init

[Simon Horman]

On Thu, Feb 13, 2025 at 12:39:03PM -0800, Tantilov, Emil S wrote:
> On 2/12/2025 10:21 AM, Simon Horman wrote:
> > On Mon, Feb 10, 2025 at 06:38:51PM -0800, Emil Tantilov wrote:
> > > Current init logic ignores the error code from register_netdev(),
> > > which will cause WARN_ON() on attempt to unregister it, if there was one,
> > > and there is no info for the user that the creation of the netdev failed.
> > > 
> > > WARNING: CPU: 89 PID: 6902 at net/core/dev.c:11512 unregister_netdevice_many_notify+0x211/0x1a10
> > > ...
> > > [ 3707.563641]  unregister_netdev+0x1c/0x30
> > > [ 3707.563656]  idpf_vport_dealloc+0x5cf/0xce0 [idpf]
> > > [ 3707.563684]  idpf_deinit_task+0xef/0x160 [idpf]
> > > [ 3707.563712]  idpf_vc_core_deinit+0x84/0x320 [idpf]
> > > [ 3707.563739]  idpf_remove+0xbf/0x780 [idpf]
> > > [ 3707.563769]  pci_device_remove+0xab/0x1e0
> > > [ 3707.563786]  device_release_driver_internal+0x371/0x530
> > > [ 3707.563803]  driver_detach+0xbf/0x180
> > > [ 3707.563816]  bus_remove_driver+0x11b/0x2a0
> > > [ 3707.563829]  pci_unregister_driver+0x2a/0x250
> > > 
> > > Introduce an error check and log the vport number and error code.
> > > On removal make sure to check VPORT_REG_NETDEV flag prior to calling
> > > unregister and free on the netdev.
> > > 
> > > Add local variables for idx, vport_config and netdev for readability.
> > > 
> > > Fixes: 0fe45467a104 ("idpf: add create vport and netdev configuration")
> > > Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
> > > Suggested-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > > Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
> > > ---
> > >   drivers/net/ethernet/intel/idpf/idpf_lib.c | 27 ++++++++++++++--------
> > >   1 file changed, 18 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > 
> > ...
> > 
> > > @@ -1536,12 +1540,17 @@ void idpf_init_task(struct work_struct *work)
> > >   	}
> > >   	for (index = 0; index < adapter->max_vports; index++) {
> > > -		if (adapter->netdevs[index] &&
> > > -		    !test_bit(IDPF_VPORT_REG_NETDEV,
> > > -			      adapter->vport_config[index]->flags)) {
> > > -			register_netdev(adapter->netdevs[index]);
> > > -			set_bit(IDPF_VPORT_REG_NETDEV,
> > > -				adapter->vport_config[index]->flags);
> > > +		struct idpf_vport_config *vport_config = adapter->vport_config[index];
> > > +		struct net_device *netdev = adapter->netdevs[index];
> > > +
> > > +		if (netdev && !test_bit(IDPF_VPORT_REG_NETDEV, vport_config->flags)) {
> > > +			err = register_netdev(netdev);
> > > +			if (err) {
> > > +				dev_err(&pdev->dev, "failed to register netdev for vport %d: %pe\n",
> > > +					index, ERR_PTR(err));
> > > +				continue;
> > > +			}
> > > +			set_bit(IDPF_VPORT_REG_NETDEV, vport_config->flags);
> > >   		}
> > >   	}
> > 
> > Hi Emil,
> > 
> > I'm wondering if we could reduce indentation and lines longer
> > than 80 characters in the above like this (completely untested!):
> I was mostly trying to focus on the fix itself, since this patch is -net
> bound. The >80 line came about from the introduction of the local netdev and
> it seemed cleaner to keep it in one line. I can just split the check as in
> the original code.
> 
> > 
> > 
> > 	for (index = 0; index < adapter->max_vports; index++) {
> > 		struct idpf_vport_config *vport_config = adapter->vport_config[index];
> > 		struct net_device *netdev = adapter->netdevs[index];
> > 
> > 		if (!netdev ||
> > 		    test_bit(IDPF_VPORT_REG_NETDEV, vport_config->flags))
> > 		    continue;
> Again, because its mainly to add the error checking I am not sure if its OK
> to re-shuffle the logic.
> 
> > 
> > 		err = register_netdev(netdev);
> > 		if (err) {
> > 			dev_err(&pdev->dev, "failed to register netdev for vport %d: %pe\n",
> > 				index, ERR_PTR(err));
> > 			continue;
> > 		}
> > 		set_bit(IDPF_VPORT_REG_NETDEV, vport_config->flags);
> > 	}
> 
> Don't mind re-spinning (and testing) v2 with the proposed change, if it's
> not infringing on the guidelines for submission to -net.

Thanks,

I see your point about not wanting to change logic for a -net patch.

My feeling is that the change is trivial enough to fit within -net
boundaries. But if you think there is any risk of it regressing
then feel free to go with your original version.