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

["Szapar-Mudlaw, Martyna" <martyna.szapar-mudlaw@linux.intel.com>]

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@linux.intel.com>
>>> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
>>> Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
>>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>>> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
>>> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>>
>> ...
>>
> 
>