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

["Tantilov, Emil S" <emil.s.tantilov@intel.com>]

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