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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20250306175422.GD3666230@kernel.org>
Date: Thu, 6 Mar 2025 17:54:22 +0000
From: Simon Horman <horms@...nel.org>
To: "Szapar-Mudlaw, Martyna" <martyna.szapar-mudlaw@...ux.intel.com>
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 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 :)