netdev - Re: [Intel-wired-lan] [PATCH iwl-next v1] ice: refactor the Tx scheduler feature

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ad9799ed-5313-4787-b982-c8fc82a281a2@linux.intel.com>
Date: Wed, 5 Mar 2025 14:30:30 +0100
From: "Szapar-Mudlaw, Martyna" <martyna.szapar-mudlaw@...ux.intel.com>
To: Simon Horman <horms@...nel.org>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
 Mateusz Polchlopek <mateusz.polchlopek@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v1] ice: refactor the Tx
 scheduler feature



On 3/4/2025 2:43 PM, Szapar-Mudlaw, Martyna wrote:
> 
> 
> On 3/3/2025 10:54 AM, Simon Horman wrote:
>> On Wed, Feb 26, 2025 at 12:33:56PM +0100, Mateusz Polchlopek wrote:
>>> Embed ice_get_tx_topo_user_sel() inside the only caller:
>>> ice_devlink_tx_sched_layers_get().
>>> Instead of jump from the wrapper to the function that does "get" 
>>> operation
>>> it does "get" itself.
>>>
>>> Remove unnecessary comment and make usage of str_enabled_disabled()
>>> in ice_init_tx_topology().
>>
>> Hi Mateusz,
>>
>> These changes seem reasonable to me.
>> But I wonder if they could be motivated in the commit message.
>>
>> What I mean is, the commit message explains what has been done.
>> But I think it should explain why it has been done.
> 
> Hi Simon,
> 
> I'm replying on behalf of Mateusz since he's on leave, and we didn't 
> want to keep this issue waiting too long.
> Would such extended commit message make sense and address your concerns?
> 
> "Simplify the code by eliminating an unnecessary wrapper function. 
> Previously, ice_devlink_tx_sched_layers_get() acted as a thin wrapper 
> around ice_get_tx_topo_user_sel(), adding no real value but increasing 
> code complexity. Since both functions were only used once, the wrapper 
> was redundant and contributed approximately 20 lines of unnecessary 
> code. By directly calling ice_get_tx_topo_user_sel(), improve 
> readability and reduce function jumps, without altering functionality.
> Also remove unnecessary comment and make usage of str_enabled_disabled() 
> in ice_init_tx_topology()."
> 
> Thank you,
> Martyna

Sorry, I caused some confusion in the previous version of proposed 
commit message. Here’s the corrected one:

"Simplify the code by eliminating an unnecessary wrapper function. 
Previously, ice_devlink_tx_sched_layers_get() acted as a thin wrapper 
around ice_get_tx_topo_user_sel(), adding no real value but increasing 
code complexity. Since both functions were only used once, the wrapper 
was redundant and contributed approximately 20 lines of unnecessary 
code. Remove ice_get_tx_topo_user_sel() and moves its instructions 
directly into ice_devlink_tx_sched_layers_get(), improving readability 
and reducing function jumps, without altering functionality.
Also remove unnecessary comment and make usage of str_enabled_disabled() 
in ice_init_tx_topology()."

> 
>>
>>> Suggested-by: Marcin Szycik <marcin.szycik@...ux.intel.com>
>>> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
>>> Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@...el.com>
>>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
>>> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
>>> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
>>
>> ...
>>
> 
>