| Message ID | 20250226113409.446325-1-mateusz.polchlopek@intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [Intel-wired-lan,iwl-next,v1] ice: refactor the Tx scheduler feature | expand |
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. > 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> ...
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 > >> 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> > > ... >
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> >> >> ... >> > >
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 :)
diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c index fcb199efbea5..2355e21d115c 100644 --- a/drivers/net/ethernet/intel/ice/devlink/devlink.c +++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c @@ -529,41 +529,6 @@ ice_devlink_reload_empr_finish(struct ice_pf *pf, return 0; } -/** - * ice_get_tx_topo_user_sel - Read user's choice from flash - * @pf: pointer to pf structure - * @layers: value read from flash will be saved here - * - * Reads user's preference for Tx Scheduler Topology Tree from PFA TLV. - * - * Return: zero when read was successful, negative values otherwise. - */ -static int ice_get_tx_topo_user_sel(struct ice_pf *pf, uint8_t *layers) -{ - struct ice_aqc_nvm_tx_topo_user_sel usr_sel = {}; - struct ice_hw *hw = &pf->hw; - int err; - - err = ice_acquire_nvm(hw, ICE_RES_READ); - if (err) - return err; - - err = ice_aq_read_nvm(hw, ICE_AQC_NVM_TX_TOPO_MOD_ID, 0, - sizeof(usr_sel), &usr_sel, true, true, NULL); - if (err) - goto exit_release_res; - - if (usr_sel.data & ICE_AQC_NVM_TX_TOPO_USER_SEL) - *layers = ICE_SCHED_5_LAYERS; - else - *layers = ICE_SCHED_9_LAYERS; - -exit_release_res: - ice_release_nvm(hw); - - return err; -} - /** * ice_update_tx_topo_user_sel - Save user's preference in flash * @pf: pointer to pf structure @@ -610,19 +575,36 @@ static int ice_update_tx_topo_user_sel(struct ice_pf *pf, int layers) * @id: the parameter ID to set * @ctx: context to store the parameter value * + * Reads user's preference for Tx Scheduler Topology Tree from PFA TLV. + * * Return: zero on success and negative value on failure. */ static int ice_devlink_tx_sched_layers_get(struct devlink *devlink, u32 id, struct devlink_param_gset_ctx *ctx) { + struct ice_aqc_nvm_tx_topo_user_sel usr_sel = {}; struct ice_pf *pf = devlink_priv(devlink); + struct ice_hw *hw = &pf->hw; int err; - err = ice_get_tx_topo_user_sel(pf, &ctx->val.vu8); + err = ice_acquire_nvm(hw, ICE_RES_READ); if (err) return err; - return 0; + err = ice_aq_read_nvm(hw, ICE_AQC_NVM_TX_TOPO_MOD_ID, 0, + sizeof(usr_sel), &usr_sel, true, true, NULL); + if (err) + goto exit_release_res; + + if (usr_sel.data & ICE_AQC_NVM_TX_TOPO_USER_SEL) + ctx->val.vu8 = ICE_SCHED_5_LAYERS; + else + ctx->val.vu8 = ICE_SCHED_9_LAYERS; + +exit_release_res: + ice_release_nvm(hw); + + return err; } /** diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c index 69d5b1a28491..a2f738eaf02e 100644 --- a/drivers/net/ethernet/intel/ice/ice_ddp.c +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c @@ -2324,8 +2324,6 @@ enum ice_ddp_state ice_copy_and_init_pkg(struct ice_hw *hw, const u8 *buf, * @flags: pointer to descriptor flags * @set: 0-get, 1-set topology * - * The function will get or set Tx topology - * * Return: zero when set was successful, negative values otherwise. */ static int diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index b084839eb811..9d9cad81b336 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -4544,10 +4544,10 @@ ice_init_tx_topology(struct ice_hw *hw, const struct firmware *firmware) dev = ice_pf_to_dev(pf); err = ice_cfg_tx_topo(hw, firmware->data, firmware->size); if (!err) { - if (hw->num_tx_sched_layers > num_tx_sched_layers) - dev_info(dev, "Tx scheduling layers switching feature disabled\n"); - else - dev_info(dev, "Tx scheduling layers switching feature enabled\n"); + dev_info(dev, "Tx scheduling layers switching feature %s\n", + str_enabled_disabled(hw->num_tx_sched_layers <= + num_tx_sched_layers)); + /* if there was a change in topology ice_cfg_tx_topo triggered * a CORER and we need to re-init hw */