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

[Simon Horman <horms@kernel.org>]

On Wed, Mar 05, 2025 at 02:30:30PM +0100, Szapar-Mudlaw, Martyna wrote:
> 
> 
> 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()."

Yes, thanks :)