netdev - Re: [Intel-wired-lan] [PATCH iwl-net] idpf: check error for register

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <02b91659-83d2-482d-9b28-f814829b1005@intel.com>
Date: Fri, 14 Feb 2025 14:32:20 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: "Tantilov, Emil S" <emil.s.tantilov@...el.com>, Simon Horman
	<horms@...nel.org>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
	<decot@...gle.com>, <willemb@...gle.com>, <anthony.l.nguyen@...el.com>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <madhu.chittim@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH iwl-net] idpf: check error for
 register_netdev() on init

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.