diff mbox series

[iwl-next,v5,4/5] ice: Add tx_scheduling_layers devlink param

Message ID 20240228142054.474626-5-mateusz.polchlopek@intel.com
State Changes Requested
Headers show
Series ice: Support 5 layer Tx scheduler topology | expand

Commit Message

Mateusz Polchlopek Feb. 28, 2024, 2:20 p.m. UTC
From: Lukasz Czapnik <lukasz.czapnik@intel.com>

It was observed that Tx performance was inconsistent across all queues
and/or VSIs and that it was directly connected to existing 9-layer
topology of the Tx scheduler.

Introduce new private devlink param - tx_scheduling_layers. This parameter
gives user flexibility to choose the 5-layer transmit scheduler topology
which helps to smooth out the transmit performance.

Allowed parameter values are 5 and 9.

Example usage:

Show:
devlink dev param show pci/0000:4b:00.0 name tx_scheduling_layers
pci/0000:4b:00.0:
  name tx_scheduling_layers type driver-specific
    values:
      cmode permanent value 9

Set:
devlink dev param set pci/0000:4b:00.0 name tx_scheduling_layers value 5
cmode permanent

devlink dev param set pci/0000:4b:00.0 name tx_scheduling_layers value 9
cmode permanent

Signed-off-by: Lukasz Czapnik <lukasz.czapnik@intel.com>
Co-developed-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
---
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   8 +
 drivers/net/ethernet/intel/ice/ice_devlink.c  | 169 ++++++++++++++++++
 .../net/ethernet/intel/ice/ice_fw_update.c    |   7 +-
 .../net/ethernet/intel/ice/ice_fw_update.h    |   3 +
 drivers/net/ethernet/intel/ice/ice_nvm.c      |   7 +-
 drivers/net/ethernet/intel/ice/ice_nvm.h      |   3 +
 6 files changed, 189 insertions(+), 8 deletions(-)

Comments

Jiri Pirko Feb. 28, 2024, 3:57 p.m. UTC | #1
Wed, Feb 28, 2024 at 03:20:53PM CET, mateusz.polchlopek@intel.com wrote:
>From: Lukasz Czapnik <lukasz.czapnik@intel.com>
>
>It was observed that Tx performance was inconsistent across all queues
>and/or VSIs and that it was directly connected to existing 9-layer
>topology of the Tx scheduler.
>
>Introduce new private devlink param - tx_scheduling_layers. This parameter
>gives user flexibility to choose the 5-layer transmit scheduler topology
>which helps to smooth out the transmit performance.
>
>Allowed parameter values are 5 and 9.
>
>Example usage:
>
>Show:
>devlink dev param show pci/0000:4b:00.0 name tx_scheduling_layers
>pci/0000:4b:00.0:
>  name tx_scheduling_layers type driver-specific
>    values:
>      cmode permanent value 9
>
>Set:
>devlink dev param set pci/0000:4b:00.0 name tx_scheduling_layers value 5
>cmode permanent
>
>devlink dev param set pci/0000:4b:00.0 name tx_scheduling_layers value 9
>cmode permanent
>
>Signed-off-by: Lukasz Czapnik <lukasz.czapnik@intel.com>
>Co-developed-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>---
> .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   8 +
> drivers/net/ethernet/intel/ice/ice_devlink.c  | 169 ++++++++++++++++++
> .../net/ethernet/intel/ice/ice_fw_update.c    |   7 +-
> .../net/ethernet/intel/ice/ice_fw_update.h    |   3 +
> drivers/net/ethernet/intel/ice/ice_nvm.c      |   7 +-
> drivers/net/ethernet/intel/ice/ice_nvm.h      |   3 +
> 6 files changed, 189 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>index 02102e937b30..4143b50bd15d 100644
>--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>@@ -1692,6 +1692,14 @@ struct ice_aqc_nvm {
> };
> 
> #define ICE_AQC_NVM_START_POINT			0
>+#define ICE_AQC_NVM_TX_TOPO_MOD_ID             0x14B
>+
>+struct ice_aqc_nvm_tx_topo_user_sel {
>+	__le16 length;
>+	u8 data;
>+#define ICE_AQC_NVM_TX_TOPO_USER_SEL	BIT(4)
>+	u8 reserved;
>+};
> 
> /* NVM Checksum Command (direct, 0x0706) */
> struct ice_aqc_nvm_checksum {
>diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
>index cc717175178b..db4872990e51 100644
>--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
>+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
>@@ -770,6 +770,167 @@ ice_devlink_port_unsplit(struct devlink *devlink, struct devlink_port *port,
> 	return ice_devlink_port_split(devlink, port, 1, extack);
> }
> 
>+/**
>+ * 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.
>+ *
>+ * Returns 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
>+ * @layers: value to be saved in flash
>+ *
>+ * Variable "layers" defines user's preference about number of layers in Tx
>+ * Scheduler Topology Tree. This choice should be stored in PFA TLV field
>+ * and be picked up by driver, next time during init.
>+ *
>+ * Returns zero when save was successful, negative values otherwise.
>+ */
>+static int ice_update_tx_topo_user_sel(struct ice_pf *pf, int 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_WRITE);
>+	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 (layers == ICE_SCHED_5_LAYERS)
>+		usr_sel.data |= ICE_AQC_NVM_TX_TOPO_USER_SEL;
>+	else
>+		usr_sel.data &= ~ICE_AQC_NVM_TX_TOPO_USER_SEL;
>+
>+	err = ice_write_one_nvm_block(pf, ICE_AQC_NVM_TX_TOPO_MOD_ID, 2,
>+				      sizeof(usr_sel.data), &usr_sel.data,
>+				      true, NULL, NULL);
>+	if (err)
>+		err = -EIO;
>+
>+exit_release_res:
>+	ice_release_nvm(hw);
>+
>+	return err;
>+}
>+
>+/**
>+ * ice_devlink_tx_sched_layers_get - Get tx_scheduling_layers parameter
>+ * @devlink: pointer to the devlink instance
>+ * @id: the parameter ID to set
>+ * @ctx: context to store the parameter value
>+ *
>+ * Returns 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_pf *pf = devlink_priv(devlink);
>+	struct device *dev = ice_pf_to_dev(pf);
>+	int err;
>+
>+	err = ice_get_tx_topo_user_sel(pf, &ctx->val.vu8);
>+	if (err) {
>+		dev_warn(dev, "Failed to read Tx Scheduler Tree - User Selection data from flash\n");

I wonder why we don't propagate extack to these callbacks. Care to add
it and use it instead of dmesg please?


>+		return -EIO;
>+	}
>+
>+	return 0;
>+}
>+
>+/**
>+ * ice_devlink_tx_sched_layers_set - Set tx_scheduling_layers parameter
>+ * @devlink: pointer to the devlink instance
>+ * @id: the parameter ID to set
>+ * @ctx: context to get the parameter value
>+ *
>+ * Returns zero on success and negative value on failure.
>+ */
>+static int ice_devlink_tx_sched_layers_set(struct devlink *devlink, u32 id,
>+					   struct devlink_param_gset_ctx *ctx)
>+{
>+	struct ice_pf *pf = devlink_priv(devlink);
>+	struct device *dev = ice_pf_to_dev(pf);
>+	int err;
>+
>+	err = ice_update_tx_topo_user_sel(pf, ctx->val.vu8);
>+	if (err)
>+		return -EIO;
>+
>+	dev_warn(dev, "Tx scheduling layers have been changed on this device. You must reboot the system for the change to take effect.");

Reboot the system? Why not re-plug the whole building while you are at
it? :)


>+
>+	return 0;
>+}
>+
>+/**
>+ * ice_devlink_tx_sched_layers_validate - Validate passed tx_scheduling_layers
>+ *                                       parameter value
>+ * @devlink: unused pointer to devlink instance
>+ * @id: the parameter ID to validate
>+ * @val: value to validate
>+ * @extack: netlink extended ACK structure
>+ *
>+ * Supported values are:
>+ * - 5 - five layers Tx Scheduler Topology Tree
>+ * - 9 - nine layers Tx Scheduler Topology Tree
>+ *
>+ * Returns zero when passed parameter value is supported. Negative value on
>+ * error.
>+ */
>+static int ice_devlink_tx_sched_layers_validate(struct devlink *devlink, u32 id,
>+					        union devlink_param_value val,
>+					        struct netlink_ext_ack *extack)
>+{
>+	struct ice_pf *pf = devlink_priv(devlink);
>+	struct ice_hw *hw = &pf->hw;
>+
>+	if (!hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) {
>+		NL_SET_ERR_MSG_MOD(extack, "Error: Requested feature is not supported by the FW on this device. Update the FW and run this command again.");

Drop the "Error: " prefix. Does not make sense to have it. Also, "Update
FW" remark looks quite odd.

>+		return -EOPNOTSUPP;
>+	}
>+
>+	if (val.vu8 != ICE_SCHED_5_LAYERS && val.vu8 != ICE_SCHED_9_LAYERS) {
>+		NL_SET_ERR_MSG_MOD(extack, "Error: Wrong number of tx scheduler layers provided.");

Drop the "Error: " prefix. Does not make sense to have it.


>+		return -EINVAL;
>+	}
>+
>+	return 0;
>+}
>+
> /**
>  * ice_tear_down_devlink_rate_tree - removes devlink-rate exported tree
>  * @pf: pf struct
>@@ -1601,6 +1762,7 @@ static int ice_devlink_loopback_validate(struct devlink *devlink, u32 id,
> enum ice_param_id {
> 	ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
> 	ICE_DEVLINK_PARAM_ID_LOOPBACK,
>+	ICE_DEVLINK_PARAM_ID_TX_BALANCE,
> };
> 
> static const struct devlink_param ice_devlink_params[] = {
>@@ -1618,6 +1780,13 @@ static const struct devlink_param ice_devlink_params[] = {
> 			     ice_devlink_loopback_get,
> 			     ice_devlink_loopback_set,
> 			     ice_devlink_loopback_validate),
>+	DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_TX_BALANCE,
>+			     "tx_scheduling_layers",
>+			     DEVLINK_PARAM_TYPE_U8,
>+			     BIT(DEVLINK_PARAM_CMODE_PERMANENT),
>+			     ice_devlink_tx_sched_layers_get,
>+			     ice_devlink_tx_sched_layers_set,
>+			     ice_devlink_tx_sched_layers_validate),
> };
> 
> static void ice_devlink_free(void *devlink_ptr)
>diff --git a/drivers/net/ethernet/intel/ice/ice_fw_update.c b/drivers/net/ethernet/intel/ice/ice_fw_update.c
>index 319a2d6fe26c..f81db6c107c8 100644
>--- a/drivers/net/ethernet/intel/ice/ice_fw_update.c
>+++ b/drivers/net/ethernet/intel/ice/ice_fw_update.c
>@@ -286,10 +286,9 @@ ice_send_component_table(struct pldmfw *context, struct pldmfw_component *compon
>  *
>  * Returns: zero on success, or a negative error code on failure.
>  */
>-static int
>-ice_write_one_nvm_block(struct ice_pf *pf, u16 module, u32 offset,
>-			u16 block_size, u8 *block, bool last_cmd,
>-			u8 *reset_level, struct netlink_ext_ack *extack)
>+int ice_write_one_nvm_block(struct ice_pf *pf, u16 module, u32 offset,
>+			    u16 block_size, u8 *block, bool last_cmd,
>+			    u8 *reset_level, struct netlink_ext_ack *extack)
> {
> 	u16 completion_module, completion_retval;
> 	struct device *dev = ice_pf_to_dev(pf);
>diff --git a/drivers/net/ethernet/intel/ice/ice_fw_update.h b/drivers/net/ethernet/intel/ice/ice_fw_update.h
>index 750574885716..04b200462757 100644
>--- a/drivers/net/ethernet/intel/ice/ice_fw_update.h
>+++ b/drivers/net/ethernet/intel/ice/ice_fw_update.h
>@@ -9,5 +9,8 @@ int ice_devlink_flash_update(struct devlink *devlink,
> 			     struct netlink_ext_ack *extack);
> int ice_get_pending_updates(struct ice_pf *pf, u8 *pending,
> 			    struct netlink_ext_ack *extack);
>+int ice_write_one_nvm_block(struct ice_pf *pf, u16 module, u32 offset,
>+			    u16 block_size, u8 *block, bool last_cmd,
>+			    u8 *reset_level, struct netlink_ext_ack *extack);
> 
> #endif
>diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
>index d4e05d2cb30c..84eab92dc03c 100644
>--- a/drivers/net/ethernet/intel/ice/ice_nvm.c
>+++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
>@@ -18,10 +18,9 @@
>  *
>  * Read the NVM using the admin queue commands (0x0701)
>  */
>-static int
>-ice_aq_read_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset, u16 length,
>-		void *data, bool last_command, bool read_shadow_ram,
>-		struct ice_sq_cd *cd)
>+int ice_aq_read_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset,
>+		    u16 length, void *data, bool last_command,
>+		    bool read_shadow_ram, struct ice_sq_cd *cd)
> {
> 	struct ice_aq_desc desc;
> 	struct ice_aqc_nvm *cmd;
>diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.h b/drivers/net/ethernet/intel/ice/ice_nvm.h
>index 774c2317967d..63cdc6bdac58 100644
>--- a/drivers/net/ethernet/intel/ice/ice_nvm.h
>+++ b/drivers/net/ethernet/intel/ice/ice_nvm.h
>@@ -14,6 +14,9 @@ struct ice_orom_civd_info {
> 
> int ice_acquire_nvm(struct ice_hw *hw, enum ice_aq_res_access_type access);
> void ice_release_nvm(struct ice_hw *hw);
>+int ice_aq_read_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset,
>+		    u16 length, void *data, bool last_command,
>+		    bool read_shadow_ram, struct ice_sq_cd *cd);
> int
> ice_read_flat_nvm(struct ice_hw *hw, u32 offset, u32 *length, u8 *data,
> 		  bool read_shadow_ram);
>-- 
>2.38.1
>
Mateusz Polchlopek March 1, 2024, 1:41 p.m. UTC | #2
On 2/28/2024 4:57 PM, Jiri Pirko wrote:
> Wed, Feb 28, 2024 at 03:20:53PM CET, mateusz.polchlopek@intel.com wrote:
>> From: Lukasz Czapnik <lukasz.czapnik@intel.com>
>>
>> It was observed that Tx performance was inconsistent across all queues
>> and/or VSIs and that it was directly connected to existing 9-layer
>> topology of the Tx scheduler.
>>
>> Introduce new private devlink param - tx_scheduling_layers. This parameter
>> gives user flexibility to choose the 5-layer transmit scheduler topology
>> which helps to smooth out the transmit performance.
>>
>> Allowed parameter values are 5 and 9.
>>
>> Example usage:
>>
>> Show:
>> devlink dev param show pci/0000:4b:00.0 name tx_scheduling_layers
>> pci/0000:4b:00.0:
>>   name tx_scheduling_layers type driver-specific
>>     values:
>>       cmode permanent value 9
>>
>> Set:
>> devlink dev param set pci/0000:4b:00.0 name tx_scheduling_layers value 5
>> cmode permanent
>>
>> devlink dev param set pci/0000:4b:00.0 name tx_scheduling_layers value 9
>> cmode permanent
>>
>> Signed-off-by: Lukasz Czapnik <lukasz.czapnik@intel.com>
>> Co-developed-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>> ---
>> .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   8 +
>> drivers/net/ethernet/intel/ice/ice_devlink.c  | 169 ++++++++++++++++++
>> .../net/ethernet/intel/ice/ice_fw_update.c    |   7 +-
>> .../net/ethernet/intel/ice/ice_fw_update.h    |   3 +
>> drivers/net/ethernet/intel/ice/ice_nvm.c      |   7 +-
>> drivers/net/ethernet/intel/ice/ice_nvm.h      |   3 +
>> 6 files changed, 189 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>> index 02102e937b30..4143b50bd15d 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>> @@ -1692,6 +1692,14 @@ struct ice_aqc_nvm {
>> };
>>
>> #define ICE_AQC_NVM_START_POINT			0
>> +#define ICE_AQC_NVM_TX_TOPO_MOD_ID             0x14B
>> +
>> +struct ice_aqc_nvm_tx_topo_user_sel {
>> +	__le16 length;
>> +	u8 data;
>> +#define ICE_AQC_NVM_TX_TOPO_USER_SEL	BIT(4)
>> +	u8 reserved;
>> +};
>>
>> /* NVM Checksum Command (direct, 0x0706) */
>> struct ice_aqc_nvm_checksum {
>> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
>> index cc717175178b..db4872990e51 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
>> @@ -770,6 +770,167 @@ ice_devlink_port_unsplit(struct devlink *devlink, struct devlink_port *port,
>> 	return ice_devlink_port_split(devlink, port, 1, extack);
>> }
>>
>> +/**
>> + * 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.
>> + *
>> + * Returns 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
>> + * @layers: value to be saved in flash
>> + *
>> + * Variable "layers" defines user's preference about number of layers in Tx
>> + * Scheduler Topology Tree. This choice should be stored in PFA TLV field
>> + * and be picked up by driver, next time during init.
>> + *
>> + * Returns zero when save was successful, negative values otherwise.
>> + */
>> +static int ice_update_tx_topo_user_sel(struct ice_pf *pf, int 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_WRITE);
>> +	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 (layers == ICE_SCHED_5_LAYERS)
>> +		usr_sel.data |= ICE_AQC_NVM_TX_TOPO_USER_SEL;
>> +	else
>> +		usr_sel.data &= ~ICE_AQC_NVM_TX_TOPO_USER_SEL;
>> +
>> +	err = ice_write_one_nvm_block(pf, ICE_AQC_NVM_TX_TOPO_MOD_ID, 2,
>> +				      sizeof(usr_sel.data), &usr_sel.data,
>> +				      true, NULL, NULL);
>> +	if (err)
>> +		err = -EIO;
>> +
>> +exit_release_res:
>> +	ice_release_nvm(hw);
>> +
>> +	return err;
>> +}
>> +
>> +/**
>> + * ice_devlink_tx_sched_layers_get - Get tx_scheduling_layers parameter
>> + * @devlink: pointer to the devlink instance
>> + * @id: the parameter ID to set
>> + * @ctx: context to store the parameter value
>> + *
>> + * Returns 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_pf *pf = devlink_priv(devlink);
>> +	struct device *dev = ice_pf_to_dev(pf);
>> +	int err;
>> +
>> +	err = ice_get_tx_topo_user_sel(pf, &ctx->val.vu8);
>> +	if (err) {
>> +		dev_warn(dev, "Failed to read Tx Scheduler Tree - User Selection data from flash\n");
> 
> I wonder why we don't propagate extack to these callbacks. Care to add
> it and use it instead of dmesg please?
> 
> 

Good point Jiri, but it is 'get' (in 'set' the same situation) function 
from DEVLINK_PARAM_DRIVER which defines that in 'set'/'get' there is no 
extack parameter. From 'get' function I can probably remove this message 
as it is not so important and just return error code...

>> +		return -EIO;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * ice_devlink_tx_sched_layers_set - Set tx_scheduling_layers parameter
>> + * @devlink: pointer to the devlink instance
>> + * @id: the parameter ID to set
>> + * @ctx: context to get the parameter value
>> + *
>> + * Returns zero on success and negative value on failure.
>> + */
>> +static int ice_devlink_tx_sched_layers_set(struct devlink *devlink, u32 id,
>> +					   struct devlink_param_gset_ctx *ctx)
>> +{
>> +	struct ice_pf *pf = devlink_priv(devlink);
>> +	struct device *dev = ice_pf_to_dev(pf);
>> +	int err;
>> +
>> +	err = ice_update_tx_topo_user_sel(pf, ctx->val.vu8);
>> +	if (err)
>> +		return -EIO;
>> +
>> +	dev_warn(dev, "Tx scheduling layers have been changed on this device. You must reboot the system for the change to take effect.");
> 
> Reboot the system? Why not re-plug the whole building while you are at
> it? :)
> 
> 

... but what about 'set' function? The information about reboot has to 
stay and still - there is no extack in 'set' function.

>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * ice_devlink_tx_sched_layers_validate - Validate passed tx_scheduling_layers
>> + *                                       parameter value
>> + * @devlink: unused pointer to devlink instance
>> + * @id: the parameter ID to validate
>> + * @val: value to validate
>> + * @extack: netlink extended ACK structure
>> + *
>> + * Supported values are:
>> + * - 5 - five layers Tx Scheduler Topology Tree
>> + * - 9 - nine layers Tx Scheduler Topology Tree
>> + *
>> + * Returns zero when passed parameter value is supported. Negative value on
>> + * error.
>> + */
>> +static int ice_devlink_tx_sched_layers_validate(struct devlink *devlink, u32 id,
>> +					        union devlink_param_value val,
>> +					        struct netlink_ext_ack *extack)
>> +{
>> +	struct ice_pf *pf = devlink_priv(devlink);
>> +	struct ice_hw *hw = &pf->hw;
>> +
>> +	if (!hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) {
>> +		NL_SET_ERR_MSG_MOD(extack, "Error: Requested feature is not supported by the FW on this device. Update the FW and run this command again.");
> 
> Drop the "Error: " prefix. Does not make sense to have it. Also, "Update
> FW" remark looks quite odd.
> 
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	if (val.vu8 != ICE_SCHED_5_LAYERS && val.vu8 != ICE_SCHED_9_LAYERS) {
>> +		NL_SET_ERR_MSG_MOD(extack, "Error: Wrong number of tx scheduler layers provided.");
> 
> Drop the "Error: " prefix. Does not make sense to have it.
> 
> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> /**
>>   * ice_tear_down_devlink_rate_tree - removes devlink-rate exported tree
>>   * @pf: pf struct
>> @@ -1601,6 +1762,7 @@ static int ice_devlink_loopback_validate(struct devlink *devlink, u32 id,
>> enum ice_param_id {
>> 	ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
>> 	ICE_DEVLINK_PARAM_ID_LOOPBACK,
>> +	ICE_DEVLINK_PARAM_ID_TX_BALANCE,
>> };
>>
>> static const struct devlink_param ice_devlink_params[] = {
>> @@ -1618,6 +1780,13 @@ static const struct devlink_param ice_devlink_params[] = {
>> 			     ice_devlink_loopback_get,
>> 			     ice_devlink_loopback_set,
>> 			     ice_devlink_loopback_validate),
>> +	DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_TX_BALANCE,
>> +			     "tx_scheduling_layers",
>> +			     DEVLINK_PARAM_TYPE_U8,
>> +			     BIT(DEVLINK_PARAM_CMODE_PERMANENT),
>> +			     ice_devlink_tx_sched_layers_get,
>> +			     ice_devlink_tx_sched_layers_set,
>> +			     ice_devlink_tx_sched_layers_validate),
>> };
>>
>> static void ice_devlink_free(void *devlink_ptr)
>> diff --git a/drivers/net/ethernet/intel/ice/ice_fw_update.c b/drivers/net/ethernet/intel/ice/ice_fw_update.c
>> index 319a2d6fe26c..f81db6c107c8 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_fw_update.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_fw_update.c
>> @@ -286,10 +286,9 @@ ice_send_component_table(struct pldmfw *context, struct pldmfw_component *compon
>>   *
>>   * Returns: zero on success, or a negative error code on failure.
>>   */
>> -static int
>> -ice_write_one_nvm_block(struct ice_pf *pf, u16 module, u32 offset,
>> -			u16 block_size, u8 *block, bool last_cmd,
>> -			u8 *reset_level, struct netlink_ext_ack *extack)
>> +int ice_write_one_nvm_block(struct ice_pf *pf, u16 module, u32 offset,
>> +			    u16 block_size, u8 *block, bool last_cmd,
>> +			    u8 *reset_level, struct netlink_ext_ack *extack)
>> {
>> 	u16 completion_module, completion_retval;
>> 	struct device *dev = ice_pf_to_dev(pf);
>> diff --git a/drivers/net/ethernet/intel/ice/ice_fw_update.h b/drivers/net/ethernet/intel/ice/ice_fw_update.h
>> index 750574885716..04b200462757 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_fw_update.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_fw_update.h
>> @@ -9,5 +9,8 @@ int ice_devlink_flash_update(struct devlink *devlink,
>> 			     struct netlink_ext_ack *extack);
>> int ice_get_pending_updates(struct ice_pf *pf, u8 *pending,
>> 			    struct netlink_ext_ack *extack);
>> +int ice_write_one_nvm_block(struct ice_pf *pf, u16 module, u32 offset,
>> +			    u16 block_size, u8 *block, bool last_cmd,
>> +			    u8 *reset_level, struct netlink_ext_ack *extack);
>>
>> #endif
>> diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
>> index d4e05d2cb30c..84eab92dc03c 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_nvm.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
>> @@ -18,10 +18,9 @@
>>   *
>>   * Read the NVM using the admin queue commands (0x0701)
>>   */
>> -static int
>> -ice_aq_read_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset, u16 length,
>> -		void *data, bool last_command, bool read_shadow_ram,
>> -		struct ice_sq_cd *cd)
>> +int ice_aq_read_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset,
>> +		    u16 length, void *data, bool last_command,
>> +		    bool read_shadow_ram, struct ice_sq_cd *cd)
>> {
>> 	struct ice_aq_desc desc;
>> 	struct ice_aqc_nvm *cmd;
>> diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.h b/drivers/net/ethernet/intel/ice/ice_nvm.h
>> index 774c2317967d..63cdc6bdac58 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_nvm.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_nvm.h
>> @@ -14,6 +14,9 @@ struct ice_orom_civd_info {
>>
>> int ice_acquire_nvm(struct ice_hw *hw, enum ice_aq_res_access_type access);
>> void ice_release_nvm(struct ice_hw *hw);
>> +int ice_aq_read_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset,
>> +		    u16 length, void *data, bool last_command,
>> +		    bool read_shadow_ram, struct ice_sq_cd *cd);
>> int
>> ice_read_flat_nvm(struct ice_hw *hw, u32 offset, u32 *length, u8 *data,
>> 		  bool read_shadow_ram);
>> -- 
>> 2.38.1
>>

Other comments will be resolved in the next version.
Thanks
Jiri Pirko March 1, 2024, 3:46 p.m. UTC | #3
Fri, Mar 01, 2024 at 02:41:56PM CET, mateusz.polchlopek@intel.com wrote:
>
>
>On 2/28/2024 4:57 PM, Jiri Pirko wrote:
>> Wed, Feb 28, 2024 at 03:20:53PM CET, mateusz.polchlopek@intel.com wrote:
>> > From: Lukasz Czapnik <lukasz.czapnik@intel.com>
>> > 
>> > It was observed that Tx performance was inconsistent across all queues
>> > and/or VSIs and that it was directly connected to existing 9-layer
>> > topology of the Tx scheduler.
>> > 
>> > Introduce new private devlink param - tx_scheduling_layers. This parameter
>> > gives user flexibility to choose the 5-layer transmit scheduler topology
>> > which helps to smooth out the transmit performance.
>> > 
>> > Allowed parameter values are 5 and 9.
>> > 
>> > Example usage:
>> > 
>> > Show:
>> > devlink dev param show pci/0000:4b:00.0 name tx_scheduling_layers
>> > pci/0000:4b:00.0:
>> >   name tx_scheduling_layers type driver-specific
>> >     values:
>> >       cmode permanent value 9
>> > 
>> > Set:
>> > devlink dev param set pci/0000:4b:00.0 name tx_scheduling_layers value 5
>> > cmode permanent
>> > 
>> > devlink dev param set pci/0000:4b:00.0 name tx_scheduling_layers value 9
>> > cmode permanent
>> > 
>> > Signed-off-by: Lukasz Czapnik <lukasz.czapnik@intel.com>
>> > Co-developed-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>> > Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>> > ---
>> > .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   8 +
>> > drivers/net/ethernet/intel/ice/ice_devlink.c  | 169 ++++++++++++++++++
>> > .../net/ethernet/intel/ice/ice_fw_update.c    |   7 +-
>> > .../net/ethernet/intel/ice/ice_fw_update.h    |   3 +
>> > drivers/net/ethernet/intel/ice/ice_nvm.c      |   7 +-
>> > drivers/net/ethernet/intel/ice/ice_nvm.h      |   3 +
>> > 6 files changed, 189 insertions(+), 8 deletions(-)
>> > 
>> > diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>> > index 02102e937b30..4143b50bd15d 100644
>> > --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>> > +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>> > @@ -1692,6 +1692,14 @@ struct ice_aqc_nvm {
>> > };
>> > 
>> > #define ICE_AQC_NVM_START_POINT			0
>> > +#define ICE_AQC_NVM_TX_TOPO_MOD_ID             0x14B
>> > +
>> > +struct ice_aqc_nvm_tx_topo_user_sel {
>> > +	__le16 length;
>> > +	u8 data;
>> > +#define ICE_AQC_NVM_TX_TOPO_USER_SEL	BIT(4)
>> > +	u8 reserved;
>> > +};
>> > 
>> > /* NVM Checksum Command (direct, 0x0706) */
>> > struct ice_aqc_nvm_checksum {
>> > diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
>> > index cc717175178b..db4872990e51 100644
>> > --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
>> > +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
>> > @@ -770,6 +770,167 @@ ice_devlink_port_unsplit(struct devlink *devlink, struct devlink_port *port,
>> > 	return ice_devlink_port_split(devlink, port, 1, extack);
>> > }
>> > 
>> > +/**
>> > + * 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.
>> > + *
>> > + * Returns 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
>> > + * @layers: value to be saved in flash
>> > + *
>> > + * Variable "layers" defines user's preference about number of layers in Tx
>> > + * Scheduler Topology Tree. This choice should be stored in PFA TLV field
>> > + * and be picked up by driver, next time during init.
>> > + *
>> > + * Returns zero when save was successful, negative values otherwise.
>> > + */
>> > +static int ice_update_tx_topo_user_sel(struct ice_pf *pf, int 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_WRITE);
>> > +	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 (layers == ICE_SCHED_5_LAYERS)
>> > +		usr_sel.data |= ICE_AQC_NVM_TX_TOPO_USER_SEL;
>> > +	else
>> > +		usr_sel.data &= ~ICE_AQC_NVM_TX_TOPO_USER_SEL;
>> > +
>> > +	err = ice_write_one_nvm_block(pf, ICE_AQC_NVM_TX_TOPO_MOD_ID, 2,
>> > +				      sizeof(usr_sel.data), &usr_sel.data,
>> > +				      true, NULL, NULL);
>> > +	if (err)
>> > +		err = -EIO;
>> > +
>> > +exit_release_res:
>> > +	ice_release_nvm(hw);
>> > +
>> > +	return err;
>> > +}
>> > +
>> > +/**
>> > + * ice_devlink_tx_sched_layers_get - Get tx_scheduling_layers parameter
>> > + * @devlink: pointer to the devlink instance
>> > + * @id: the parameter ID to set
>> > + * @ctx: context to store the parameter value
>> > + *
>> > + * Returns 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_pf *pf = devlink_priv(devlink);
>> > +	struct device *dev = ice_pf_to_dev(pf);
>> > +	int err;
>> > +
>> > +	err = ice_get_tx_topo_user_sel(pf, &ctx->val.vu8);
>> > +	if (err) {
>> > +		dev_warn(dev, "Failed to read Tx Scheduler Tree - User Selection data from flash\n");
>> 
>> I wonder why we don't propagate extack to these callbacks. Care to add
>> it and use it instead of dmesg please?
>> 
>> 
>
>Good point Jiri, but it is 'get' (in 'set' the same situation) function from
>DEVLINK_PARAM_DRIVER which defines that in 'set'/'get' there is no extack

So add it.


>parameter. From 'get' function I can probably remove this message as it is
>not so important and just return error code...
>
>> > +		return -EIO;
>> > +	}
>> > +
>> > +	return 0;
>> > +}
>> > +
>> > +/**
>> > + * ice_devlink_tx_sched_layers_set - Set tx_scheduling_layers parameter
>> > + * @devlink: pointer to the devlink instance
>> > + * @id: the parameter ID to set
>> > + * @ctx: context to get the parameter value
>> > + *
>> > + * Returns zero on success and negative value on failure.
>> > + */
>> > +static int ice_devlink_tx_sched_layers_set(struct devlink *devlink, u32 id,
>> > +					   struct devlink_param_gset_ctx *ctx)
>> > +{
>> > +	struct ice_pf *pf = devlink_priv(devlink);
>> > +	struct device *dev = ice_pf_to_dev(pf);
>> > +	int err;
>> > +
>> > +	err = ice_update_tx_topo_user_sel(pf, ctx->val.vu8);
>> > +	if (err)
>> > +		return -EIO;
>> > +
>> > +	dev_warn(dev, "Tx scheduling layers have been changed on this device. You must reboot the system for the change to take effect.");
>> 
>> Reboot the system? Why not re-plug the whole building while you are at
>> it? :)
>> 
>> 
>
>... but what about 'set' function? The information about reboot has to stay
>and still - there is no extack in 'set' function.

My point is, do you really need to "reboot"? Message like this sounds
very odd coming from a nic driver. Woundn't pci slot powercycle do the
trick for example?


>
>> > +
>> > +	return 0;
>> > +}
>> > +
>> > +/**
>> > + * ice_devlink_tx_sched_layers_validate - Validate passed tx_scheduling_layers
>> > + *                                       parameter value
>> > + * @devlink: unused pointer to devlink instance
>> > + * @id: the parameter ID to validate
>> > + * @val: value to validate
>> > + * @extack: netlink extended ACK structure
>> > + *
>> > + * Supported values are:
>> > + * - 5 - five layers Tx Scheduler Topology Tree
>> > + * - 9 - nine layers Tx Scheduler Topology Tree
>> > + *
>> > + * Returns zero when passed parameter value is supported. Negative value on
>> > + * error.
>> > + */
>> > +static int ice_devlink_tx_sched_layers_validate(struct devlink *devlink, u32 id,
>> > +					        union devlink_param_value val,
>> > +					        struct netlink_ext_ack *extack)
>> > +{
>> > +	struct ice_pf *pf = devlink_priv(devlink);
>> > +	struct ice_hw *hw = &pf->hw;
>> > +
>> > +	if (!hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) {
>> > +		NL_SET_ERR_MSG_MOD(extack, "Error: Requested feature is not supported by the FW on this device. Update the FW and run this command again.");
>> 
>> Drop the "Error: " prefix. Does not make sense to have it. Also, "Update
>> FW" remark looks quite odd.
>> 
>> > +		return -EOPNOTSUPP;
>> > +	}
>> > +
>> > +	if (val.vu8 != ICE_SCHED_5_LAYERS && val.vu8 != ICE_SCHED_9_LAYERS) {
>> > +		NL_SET_ERR_MSG_MOD(extack, "Error: Wrong number of tx scheduler layers provided.");
>> 
>> Drop the "Error: " prefix. Does not make sense to have it.
>> 
>> 
>> > +		return -EINVAL;
>> > +	}
>> > +
>> > +	return 0;
>> > +}
>> > +
>> > /**
>> >   * ice_tear_down_devlink_rate_tree - removes devlink-rate exported tree
>> >   * @pf: pf struct
>> > @@ -1601,6 +1762,7 @@ static int ice_devlink_loopback_validate(struct devlink *devlink, u32 id,
>> > enum ice_param_id {
>> > 	ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
>> > 	ICE_DEVLINK_PARAM_ID_LOOPBACK,
>> > +	ICE_DEVLINK_PARAM_ID_TX_BALANCE,
>> > };
>> > 
>> > static const struct devlink_param ice_devlink_params[] = {
>> > @@ -1618,6 +1780,13 @@ static const struct devlink_param ice_devlink_params[] = {
>> > 			     ice_devlink_loopback_get,
>> > 			     ice_devlink_loopback_set,
>> > 			     ice_devlink_loopback_validate),
>> > +	DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_TX_BALANCE,
>> > +			     "tx_scheduling_layers",
>> > +			     DEVLINK_PARAM_TYPE_U8,
>> > +			     BIT(DEVLINK_PARAM_CMODE_PERMANENT),
>> > +			     ice_devlink_tx_sched_layers_get,
>> > +			     ice_devlink_tx_sched_layers_set,
>> > +			     ice_devlink_tx_sched_layers_validate),
>> > };
>> > 
>> > static void ice_devlink_free(void *devlink_ptr)
>> > diff --git a/drivers/net/ethernet/intel/ice/ice_fw_update.c b/drivers/net/ethernet/intel/ice/ice_fw_update.c
>> > index 319a2d6fe26c..f81db6c107c8 100644
>> > --- a/drivers/net/ethernet/intel/ice/ice_fw_update.c
>> > +++ b/drivers/net/ethernet/intel/ice/ice_fw_update.c
>> > @@ -286,10 +286,9 @@ ice_send_component_table(struct pldmfw *context, struct pldmfw_component *compon
>> >   *
>> >   * Returns: zero on success, or a negative error code on failure.
>> >   */
>> > -static int
>> > -ice_write_one_nvm_block(struct ice_pf *pf, u16 module, u32 offset,
>> > -			u16 block_size, u8 *block, bool last_cmd,
>> > -			u8 *reset_level, struct netlink_ext_ack *extack)
>> > +int ice_write_one_nvm_block(struct ice_pf *pf, u16 module, u32 offset,
>> > +			    u16 block_size, u8 *block, bool last_cmd,
>> > +			    u8 *reset_level, struct netlink_ext_ack *extack)
>> > {
>> > 	u16 completion_module, completion_retval;
>> > 	struct device *dev = ice_pf_to_dev(pf);
>> > diff --git a/drivers/net/ethernet/intel/ice/ice_fw_update.h b/drivers/net/ethernet/intel/ice/ice_fw_update.h
>> > index 750574885716..04b200462757 100644
>> > --- a/drivers/net/ethernet/intel/ice/ice_fw_update.h
>> > +++ b/drivers/net/ethernet/intel/ice/ice_fw_update.h
>> > @@ -9,5 +9,8 @@ int ice_devlink_flash_update(struct devlink *devlink,
>> > 			     struct netlink_ext_ack *extack);
>> > int ice_get_pending_updates(struct ice_pf *pf, u8 *pending,
>> > 			    struct netlink_ext_ack *extack);
>> > +int ice_write_one_nvm_block(struct ice_pf *pf, u16 module, u32 offset,
>> > +			    u16 block_size, u8 *block, bool last_cmd,
>> > +			    u8 *reset_level, struct netlink_ext_ack *extack);
>> > 
>> > #endif
>> > diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
>> > index d4e05d2cb30c..84eab92dc03c 100644
>> > --- a/drivers/net/ethernet/intel/ice/ice_nvm.c
>> > +++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
>> > @@ -18,10 +18,9 @@
>> >   *
>> >   * Read the NVM using the admin queue commands (0x0701)
>> >   */
>> > -static int
>> > -ice_aq_read_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset, u16 length,
>> > -		void *data, bool last_command, bool read_shadow_ram,
>> > -		struct ice_sq_cd *cd)
>> > +int ice_aq_read_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset,
>> > +		    u16 length, void *data, bool last_command,
>> > +		    bool read_shadow_ram, struct ice_sq_cd *cd)
>> > {
>> > 	struct ice_aq_desc desc;
>> > 	struct ice_aqc_nvm *cmd;
>> > diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.h b/drivers/net/ethernet/intel/ice/ice_nvm.h
>> > index 774c2317967d..63cdc6bdac58 100644
>> > --- a/drivers/net/ethernet/intel/ice/ice_nvm.h
>> > +++ b/drivers/net/ethernet/intel/ice/ice_nvm.h
>> > @@ -14,6 +14,9 @@ struct ice_orom_civd_info {
>> > 
>> > int ice_acquire_nvm(struct ice_hw *hw, enum ice_aq_res_access_type access);
>> > void ice_release_nvm(struct ice_hw *hw);
>> > +int ice_aq_read_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset,
>> > +		    u16 length, void *data, bool last_command,
>> > +		    bool read_shadow_ram, struct ice_sq_cd *cd);
>> > int
>> > ice_read_flat_nvm(struct ice_hw *hw, u32 offset, u32 *length, u8 *data,
>> > 		  bool read_shadow_ram);
>> > -- 
>> > 2.38.1
>> > 
>
>Other comments will be resolved in the next version.
>Thanks
Mateusz Polchlopek March 5, 2024, 12:31 p.m. UTC | #4
On 3/1/2024 4:46 PM, Jiri Pirko wrote:
> Fri, Mar 01, 2024 at 02:41:56PM CET, mateusz.polchlopek@intel.com wrote:
>>
>>
>> On 2/28/2024 4:57 PM, Jiri Pirko wrote:
>>> Wed, Feb 28, 2024 at 03:20:53PM CET, mateusz.polchlopek@intel.com wrote:
>>>> From: Lukasz Czapnik <lukasz.czapnik@intel.com>
>>>>
>>>> It was observed that Tx performance was inconsistent across all queues
>>>> and/or VSIs and that it was directly connected to existing 9-layer
>>>> topology of the Tx scheduler.
>>>>
>>>> Introduce new private devlink param - tx_scheduling_layers. This parameter
>>>> gives user flexibility to choose the 5-layer transmit scheduler topology
>>>> which helps to smooth out the transmit performance.
>>>>
>>>> Allowed parameter values are 5 and 9.
>>>>
>>>> Example usage:
>>>>
>>>> Show:
>>>> devlink dev param show pci/0000:4b:00.0 name tx_scheduling_layers
>>>> pci/0000:4b:00.0:
>>>>    name tx_scheduling_layers type driver-specific
>>>>      values:
>>>>        cmode permanent value 9
>>>>
>>>> Set:
>>>> devlink dev param set pci/0000:4b:00.0 name tx_scheduling_layers value 5
>>>> cmode permanent
>>>>
>>>> devlink dev param set pci/0000:4b:00.0 name tx_scheduling_layers value 9
>>>> cmode permanent
>>>>
>>>> Signed-off-by: Lukasz Czapnik <lukasz.czapnik@intel.com>
>>>> Co-developed-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>>>> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>>>> ---
>>>> .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   8 +
>>>> drivers/net/ethernet/intel/ice/ice_devlink.c  | 169 ++++++++++++++++++
>>>> .../net/ethernet/intel/ice/ice_fw_update.c    |   7 +-
>>>> .../net/ethernet/intel/ice/ice_fw_update.h    |   3 +
>>>> drivers/net/ethernet/intel/ice/ice_nvm.c      |   7 +-
>>>> drivers/net/ethernet/intel/ice/ice_nvm.h      |   3 +
>>>> 6 files changed, 189 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>>>> index 02102e937b30..4143b50bd15d 100644
>>>> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>>>> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>>>> @@ -1692,6 +1692,14 @@ struct ice_aqc_nvm {
>>>> };
>>>>
>>>> #define ICE_AQC_NVM_START_POINT			0
>>>> +#define ICE_AQC_NVM_TX_TOPO_MOD_ID             0x14B
>>>> +
>>>> +struct ice_aqc_nvm_tx_topo_user_sel {
>>>> +	__le16 length;
>>>> +	u8 data;
>>>> +#define ICE_AQC_NVM_TX_TOPO_USER_SEL	BIT(4)
>>>> +	u8 reserved;
>>>> +};
>>>>
>>>> /* NVM Checksum Command (direct, 0x0706) */
>>>> struct ice_aqc_nvm_checksum {
>>>> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
>>>> index cc717175178b..db4872990e51 100644
>>>> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
>>>> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
>>>> @@ -770,6 +770,167 @@ ice_devlink_port_unsplit(struct devlink *devlink, struct devlink_port *port,
>>>> 	return ice_devlink_port_split(devlink, port, 1, extack);
>>>> }
>>>>
>>>> +/**
>>>> + * 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.
>>>> + *
>>>> + * Returns 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
>>>> + * @layers: value to be saved in flash
>>>> + *
>>>> + * Variable "layers" defines user's preference about number of layers in Tx
>>>> + * Scheduler Topology Tree. This choice should be stored in PFA TLV field
>>>> + * and be picked up by driver, next time during init.
>>>> + *
>>>> + * Returns zero when save was successful, negative values otherwise.
>>>> + */
>>>> +static int ice_update_tx_topo_user_sel(struct ice_pf *pf, int 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_WRITE);
>>>> +	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 (layers == ICE_SCHED_5_LAYERS)
>>>> +		usr_sel.data |= ICE_AQC_NVM_TX_TOPO_USER_SEL;
>>>> +	else
>>>> +		usr_sel.data &= ~ICE_AQC_NVM_TX_TOPO_USER_SEL;
>>>> +
>>>> +	err = ice_write_one_nvm_block(pf, ICE_AQC_NVM_TX_TOPO_MOD_ID, 2,
>>>> +				      sizeof(usr_sel.data), &usr_sel.data,
>>>> +				      true, NULL, NULL);
>>>> +	if (err)
>>>> +		err = -EIO;
>>>> +
>>>> +exit_release_res:
>>>> +	ice_release_nvm(hw);
>>>> +
>>>> +	return err;
>>>> +}
>>>> +
>>>> +/**
>>>> + * ice_devlink_tx_sched_layers_get - Get tx_scheduling_layers parameter
>>>> + * @devlink: pointer to the devlink instance
>>>> + * @id: the parameter ID to set
>>>> + * @ctx: context to store the parameter value
>>>> + *
>>>> + * Returns 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_pf *pf = devlink_priv(devlink);
>>>> +	struct device *dev = ice_pf_to_dev(pf);
>>>> +	int err;
>>>> +
>>>> +	err = ice_get_tx_topo_user_sel(pf, &ctx->val.vu8);
>>>> +	if (err) {
>>>> +		dev_warn(dev, "Failed to read Tx Scheduler Tree - User Selection data from flash\n");
>>>
>>> I wonder why we don't propagate extack to these callbacks. Care to add
>>> it and use it instead of dmesg please?
>>>
>>>
>>
>> Good point Jiri, but it is 'get' (in 'set' the same situation) function from
>> DEVLINK_PARAM_DRIVER which defines that in 'set'/'get' there is no extack
> 
> So add it.
> 
> 
>> parameter. From 'get' function I can probably remove this message as it is
>> not so important and just return error code...
>>
>>>> +		return -EIO;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * ice_devlink_tx_sched_layers_set - Set tx_scheduling_layers parameter
>>>> + * @devlink: pointer to the devlink instance
>>>> + * @id: the parameter ID to set
>>>> + * @ctx: context to get the parameter value
>>>> + *
>>>> + * Returns zero on success and negative value on failure.
>>>> + */
>>>> +static int ice_devlink_tx_sched_layers_set(struct devlink *devlink, u32 id,
>>>> +					   struct devlink_param_gset_ctx *ctx)
>>>> +{
>>>> +	struct ice_pf *pf = devlink_priv(devlink);
>>>> +	struct device *dev = ice_pf_to_dev(pf);
>>>> +	int err;
>>>> +
>>>> +	err = ice_update_tx_topo_user_sel(pf, ctx->val.vu8);
>>>> +	if (err)
>>>> +		return -EIO;
>>>> +
>>>> +	dev_warn(dev, "Tx scheduling layers have been changed on this device. You must reboot the system for the change to take effect.");
>>>
>>> Reboot the system? Why not re-plug the whole building while you are at
>>> it? :)
>>>
>>>
>>
>> ... but what about 'set' function? The information about reboot has to stay
>> and still - there is no extack in 'set' function.
> 
> My point is, do you really need to "reboot"? Message like this sounds
> very odd coming from a nic driver. Woundn't pci slot powercycle do the
> trick for example?
> 
> 
>>
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * ice_devlink_tx_sched_layers_validate - Validate passed tx_scheduling_layers
>>>> + *                                       parameter value
>>>> + * @devlink: unused pointer to devlink instance
>>>> + * @id: the parameter ID to validate
>>>> + * @val: value to validate
>>>> + * @extack: netlink extended ACK structure
>>>> + *
>>>> + * Supported values are:
>>>> + * - 5 - five layers Tx Scheduler Topology Tree
>>>> + * - 9 - nine layers Tx Scheduler Topology Tree
>>>> + *
>>>> + * Returns zero when passed parameter value is supported. Negative value on
>>>> + * error.
>>>> + */
>>>> +static int ice_devlink_tx_sched_layers_validate(struct devlink *devlink, u32 id,
>>>> +					        union devlink_param_value val,
>>>> +					        struct netlink_ext_ack *extack)
>>>> +{
>>>> +	struct ice_pf *pf = devlink_priv(devlink);
>>>> +	struct ice_hw *hw = &pf->hw;
>>>> +
>>>> +	if (!hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) {
>>>> +		NL_SET_ERR_MSG_MOD(extack, "Error: Requested feature is not supported by the FW on this device. Update the FW and run this command again.");
>>>
>>> Drop the "Error: " prefix. Does not make sense to have it. Also, "Update
>>> FW" remark looks quite odd.
>>>
>>>> +		return -EOPNOTSUPP;
>>>> +	}
>>>> +
>>>> +	if (val.vu8 != ICE_SCHED_5_LAYERS && val.vu8 != ICE_SCHED_9_LAYERS) {
>>>> +		NL_SET_ERR_MSG_MOD(extack, "Error: Wrong number of tx scheduler layers provided.");
>>>
>>> Drop the "Error: " prefix. Does not make sense to have it.
>>>
>>>
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> /**
>>>>    * ice_tear_down_devlink_rate_tree - removes devlink-rate exported tree
>>>>    * @pf: pf struct
>>>> @@ -1601,6 +1762,7 @@ static int ice_devlink_loopback_validate(struct devlink *devlink, u32 id,
>>>> enum ice_param_id {
>>>> 	ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
>>>> 	ICE_DEVLINK_PARAM_ID_LOOPBACK,
>>>> +	ICE_DEVLINK_PARAM_ID_TX_BALANCE,
>>>> };
>>>>
>>>> static const struct devlink_param ice_devlink_params[] = {
>>>> @@ -1618,6 +1780,13 @@ static const struct devlink_param ice_devlink_params[] = {
>>>> 			     ice_devlink_loopback_get,
>>>> 			     ice_devlink_loopback_set,
>>>> 			     ice_devlink_loopback_validate),
>>>> +	DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_TX_BALANCE,
>>>> +			     "tx_scheduling_layers",
>>>> +			     DEVLINK_PARAM_TYPE_U8,
>>>> +			     BIT(DEVLINK_PARAM_CMODE_PERMANENT),
>>>> +			     ice_devlink_tx_sched_layers_get,
>>>> +			     ice_devlink_tx_sched_layers_set,
>>>> +			     ice_devlink_tx_sched_layers_validate),
>>>> };
>>>>
>>>> static void ice_devlink_free(void *devlink_ptr)
>>>> diff --git a/drivers/net/ethernet/intel/ice/ice_fw_update.c b/drivers/net/ethernet/intel/ice/ice_fw_update.c
>>>> index 319a2d6fe26c..f81db6c107c8 100644
>>>> --- a/drivers/net/ethernet/intel/ice/ice_fw_update.c
>>>> +++ b/drivers/net/ethernet/intel/ice/ice_fw_update.c
>>>> @@ -286,10 +286,9 @@ ice_send_component_table(struct pldmfw *context, struct pldmfw_component *compon
>>>>    *
>>>>    * Returns: zero on success, or a negative error code on failure.
>>>>    */
>>>> -static int
>>>> -ice_write_one_nvm_block(struct ice_pf *pf, u16 module, u32 offset,
>>>> -			u16 block_size, u8 *block, bool last_cmd,
>>>> -			u8 *reset_level, struct netlink_ext_ack *extack)
>>>> +int ice_write_one_nvm_block(struct ice_pf *pf, u16 module, u32 offset,
>>>> +			    u16 block_size, u8 *block, bool last_cmd,
>>>> +			    u8 *reset_level, struct netlink_ext_ack *extack)
>>>> {
>>>> 	u16 completion_module, completion_retval;
>>>> 	struct device *dev = ice_pf_to_dev(pf);
>>>> diff --git a/drivers/net/ethernet/intel/ice/ice_fw_update.h b/drivers/net/ethernet/intel/ice/ice_fw_update.h
>>>> index 750574885716..04b200462757 100644
>>>> --- a/drivers/net/ethernet/intel/ice/ice_fw_update.h
>>>> +++ b/drivers/net/ethernet/intel/ice/ice_fw_update.h
>>>> @@ -9,5 +9,8 @@ int ice_devlink_flash_update(struct devlink *devlink,
>>>> 			     struct netlink_ext_ack *extack);
>>>> int ice_get_pending_updates(struct ice_pf *pf, u8 *pending,
>>>> 			    struct netlink_ext_ack *extack);
>>>> +int ice_write_one_nvm_block(struct ice_pf *pf, u16 module, u32 offset,
>>>> +			    u16 block_size, u8 *block, bool last_cmd,
>>>> +			    u8 *reset_level, struct netlink_ext_ack *extack);
>>>>
>>>> #endif
>>>> diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
>>>> index d4e05d2cb30c..84eab92dc03c 100644
>>>> --- a/drivers/net/ethernet/intel/ice/ice_nvm.c
>>>> +++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
>>>> @@ -18,10 +18,9 @@
>>>>    *
>>>>    * Read the NVM using the admin queue commands (0x0701)
>>>>    */
>>>> -static int
>>>> -ice_aq_read_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset, u16 length,
>>>> -		void *data, bool last_command, bool read_shadow_ram,
>>>> -		struct ice_sq_cd *cd)
>>>> +int ice_aq_read_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset,
>>>> +		    u16 length, void *data, bool last_command,
>>>> +		    bool read_shadow_ram, struct ice_sq_cd *cd)
>>>> {
>>>> 	struct ice_aq_desc desc;
>>>> 	struct ice_aqc_nvm *cmd;
>>>> diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.h b/drivers/net/ethernet/intel/ice/ice_nvm.h
>>>> index 774c2317967d..63cdc6bdac58 100644
>>>> --- a/drivers/net/ethernet/intel/ice/ice_nvm.h
>>>> +++ b/drivers/net/ethernet/intel/ice/ice_nvm.h
>>>> @@ -14,6 +14,9 @@ struct ice_orom_civd_info {
>>>>
>>>> int ice_acquire_nvm(struct ice_hw *hw, enum ice_aq_res_access_type access);
>>>> void ice_release_nvm(struct ice_hw *hw);
>>>> +int ice_aq_read_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset,
>>>> +		    u16 length, void *data, bool last_command,
>>>> +		    bool read_shadow_ram, struct ice_sq_cd *cd);
>>>> int
>>>> ice_read_flat_nvm(struct ice_hw *hw, u32 offset, u32 *length, u8 *data,
>>>> 		  bool read_shadow_ram);
>>>> -- 
>>>> 2.38.1
>>>>
>>
>> Other comments will be resolved in the next version.
>> Thanks

Okay, thanks Jiri. I will solve problems in next version.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 02102e937b30..4143b50bd15d 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1692,6 +1692,14 @@  struct ice_aqc_nvm {
 };
 
 #define ICE_AQC_NVM_START_POINT			0
+#define ICE_AQC_NVM_TX_TOPO_MOD_ID             0x14B
+
+struct ice_aqc_nvm_tx_topo_user_sel {
+	__le16 length;
+	u8 data;
+#define ICE_AQC_NVM_TX_TOPO_USER_SEL	BIT(4)
+	u8 reserved;
+};
 
 /* NVM Checksum Command (direct, 0x0706) */
 struct ice_aqc_nvm_checksum {
diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index cc717175178b..db4872990e51 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -770,6 +770,167 @@  ice_devlink_port_unsplit(struct devlink *devlink, struct devlink_port *port,
 	return ice_devlink_port_split(devlink, port, 1, extack);
 }
 
+/**
+ * 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.
+ *
+ * Returns 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
+ * @layers: value to be saved in flash
+ *
+ * Variable "layers" defines user's preference about number of layers in Tx
+ * Scheduler Topology Tree. This choice should be stored in PFA TLV field
+ * and be picked up by driver, next time during init.
+ *
+ * Returns zero when save was successful, negative values otherwise.
+ */
+static int ice_update_tx_topo_user_sel(struct ice_pf *pf, int 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_WRITE);
+	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 (layers == ICE_SCHED_5_LAYERS)
+		usr_sel.data |= ICE_AQC_NVM_TX_TOPO_USER_SEL;
+	else
+		usr_sel.data &= ~ICE_AQC_NVM_TX_TOPO_USER_SEL;
+
+	err = ice_write_one_nvm_block(pf, ICE_AQC_NVM_TX_TOPO_MOD_ID, 2,
+				      sizeof(usr_sel.data), &usr_sel.data,
+				      true, NULL, NULL);
+	if (err)
+		err = -EIO;
+
+exit_release_res:
+	ice_release_nvm(hw);
+
+	return err;
+}
+
+/**
+ * ice_devlink_tx_sched_layers_get - Get tx_scheduling_layers parameter
+ * @devlink: pointer to the devlink instance
+ * @id: the parameter ID to set
+ * @ctx: context to store the parameter value
+ *
+ * Returns 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_pf *pf = devlink_priv(devlink);
+	struct device *dev = ice_pf_to_dev(pf);
+	int err;
+
+	err = ice_get_tx_topo_user_sel(pf, &ctx->val.vu8);
+	if (err) {
+		dev_warn(dev, "Failed to read Tx Scheduler Tree - User Selection data from flash\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+/**
+ * ice_devlink_tx_sched_layers_set - Set tx_scheduling_layers parameter
+ * @devlink: pointer to the devlink instance
+ * @id: the parameter ID to set
+ * @ctx: context to get the parameter value
+ *
+ * Returns zero on success and negative value on failure.
+ */
+static int ice_devlink_tx_sched_layers_set(struct devlink *devlink, u32 id,
+					   struct devlink_param_gset_ctx *ctx)
+{
+	struct ice_pf *pf = devlink_priv(devlink);
+	struct device *dev = ice_pf_to_dev(pf);
+	int err;
+
+	err = ice_update_tx_topo_user_sel(pf, ctx->val.vu8);
+	if (err)
+		return -EIO;
+
+	dev_warn(dev, "Tx scheduling layers have been changed on this device. You must reboot the system for the change to take effect.");
+
+	return 0;
+}
+
+/**
+ * ice_devlink_tx_sched_layers_validate - Validate passed tx_scheduling_layers
+ *                                       parameter value
+ * @devlink: unused pointer to devlink instance
+ * @id: the parameter ID to validate
+ * @val: value to validate
+ * @extack: netlink extended ACK structure
+ *
+ * Supported values are:
+ * - 5 - five layers Tx Scheduler Topology Tree
+ * - 9 - nine layers Tx Scheduler Topology Tree
+ *
+ * Returns zero when passed parameter value is supported. Negative value on
+ * error.
+ */
+static int ice_devlink_tx_sched_layers_validate(struct devlink *devlink, u32 id,
+					        union devlink_param_value val,
+					        struct netlink_ext_ack *extack)
+{
+	struct ice_pf *pf = devlink_priv(devlink);
+	struct ice_hw *hw = &pf->hw;
+
+	if (!hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) {
+		NL_SET_ERR_MSG_MOD(extack, "Error: Requested feature is not supported by the FW on this device. Update the FW and run this command again.");
+		return -EOPNOTSUPP;
+	}
+
+	if (val.vu8 != ICE_SCHED_5_LAYERS && val.vu8 != ICE_SCHED_9_LAYERS) {
+		NL_SET_ERR_MSG_MOD(extack, "Error: Wrong number of tx scheduler layers provided.");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /**
  * ice_tear_down_devlink_rate_tree - removes devlink-rate exported tree
  * @pf: pf struct
@@ -1601,6 +1762,7 @@  static int ice_devlink_loopback_validate(struct devlink *devlink, u32 id,
 enum ice_param_id {
 	ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
 	ICE_DEVLINK_PARAM_ID_LOOPBACK,
+	ICE_DEVLINK_PARAM_ID_TX_BALANCE,
 };
 
 static const struct devlink_param ice_devlink_params[] = {
@@ -1618,6 +1780,13 @@  static const struct devlink_param ice_devlink_params[] = {
 			     ice_devlink_loopback_get,
 			     ice_devlink_loopback_set,
 			     ice_devlink_loopback_validate),
+	DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_TX_BALANCE,
+			     "tx_scheduling_layers",
+			     DEVLINK_PARAM_TYPE_U8,
+			     BIT(DEVLINK_PARAM_CMODE_PERMANENT),
+			     ice_devlink_tx_sched_layers_get,
+			     ice_devlink_tx_sched_layers_set,
+			     ice_devlink_tx_sched_layers_validate),
 };
 
 static void ice_devlink_free(void *devlink_ptr)
diff --git a/drivers/net/ethernet/intel/ice/ice_fw_update.c b/drivers/net/ethernet/intel/ice/ice_fw_update.c
index 319a2d6fe26c..f81db6c107c8 100644
--- a/drivers/net/ethernet/intel/ice/ice_fw_update.c
+++ b/drivers/net/ethernet/intel/ice/ice_fw_update.c
@@ -286,10 +286,9 @@  ice_send_component_table(struct pldmfw *context, struct pldmfw_component *compon
  *
  * Returns: zero on success, or a negative error code on failure.
  */
-static int
-ice_write_one_nvm_block(struct ice_pf *pf, u16 module, u32 offset,
-			u16 block_size, u8 *block, bool last_cmd,
-			u8 *reset_level, struct netlink_ext_ack *extack)
+int ice_write_one_nvm_block(struct ice_pf *pf, u16 module, u32 offset,
+			    u16 block_size, u8 *block, bool last_cmd,
+			    u8 *reset_level, struct netlink_ext_ack *extack)
 {
 	u16 completion_module, completion_retval;
 	struct device *dev = ice_pf_to_dev(pf);
diff --git a/drivers/net/ethernet/intel/ice/ice_fw_update.h b/drivers/net/ethernet/intel/ice/ice_fw_update.h
index 750574885716..04b200462757 100644
--- a/drivers/net/ethernet/intel/ice/ice_fw_update.h
+++ b/drivers/net/ethernet/intel/ice/ice_fw_update.h
@@ -9,5 +9,8 @@  int ice_devlink_flash_update(struct devlink *devlink,
 			     struct netlink_ext_ack *extack);
 int ice_get_pending_updates(struct ice_pf *pf, u8 *pending,
 			    struct netlink_ext_ack *extack);
+int ice_write_one_nvm_block(struct ice_pf *pf, u16 module, u32 offset,
+			    u16 block_size, u8 *block, bool last_cmd,
+			    u8 *reset_level, struct netlink_ext_ack *extack);
 
 #endif
diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
index d4e05d2cb30c..84eab92dc03c 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.c
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
@@ -18,10 +18,9 @@ 
  *
  * Read the NVM using the admin queue commands (0x0701)
  */
-static int
-ice_aq_read_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset, u16 length,
-		void *data, bool last_command, bool read_shadow_ram,
-		struct ice_sq_cd *cd)
+int ice_aq_read_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset,
+		    u16 length, void *data, bool last_command,
+		    bool read_shadow_ram, struct ice_sq_cd *cd)
 {
 	struct ice_aq_desc desc;
 	struct ice_aqc_nvm *cmd;
diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.h b/drivers/net/ethernet/intel/ice/ice_nvm.h
index 774c2317967d..63cdc6bdac58 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.h
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.h
@@ -14,6 +14,9 @@  struct ice_orom_civd_info {
 
 int ice_acquire_nvm(struct ice_hw *hw, enum ice_aq_res_access_type access);
 void ice_release_nvm(struct ice_hw *hw);
+int ice_aq_read_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset,
+		    u16 length, void *data, bool last_command,
+		    bool read_shadow_ram, struct ice_sq_cd *cd);
 int
 ice_read_flat_nvm(struct ice_hw *hw, u32 offset, u32 *length, u8 *data,
 		  bool read_shadow_ram);