Re: [Intel-wired-lan] [PATCH net-next v2 2/2] ice: Enable switching default tx scheduler topology

[Paul Menzel <pmenzel@molgen.mpg.de>]

Dear Michal,


Thank you for your patch.

Am 24.06.22 um 12:21 schrieb Michal Wilczynski:
> Introduce support for tx scheduler topology change, based on
> user selection, from default 9-layer to 5-layer.
> In order for switch to be successful there is a new NVM
> and DDP package required.

What versions are needed exactly?

> This commit enables 5-layer topology in init path of
> the driver, so before ice driver load, the user selection
> should be changed in NVM using some external tools.

Please add one example, how to change it.

Could you please reflow for 75 characters per line, and do not break 
lines just because a sentence ends, or add a blank line between paragraphs?

> Title: Enable switching default tx scheduler topology
> Change-type: ImplementationChange

These two tags are not used upstream I thinks.

> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_common.c   |   2 +
>   .../net/ethernet/intel/ice/ice_flex_pipe.c    |   3 +-
>   drivers/net/ethernet/intel/ice/ice_main.c     | 113 +++++++++++++++---
>   drivers/net/ethernet/intel/ice/ice_sched.c    |  35 +++---
>   4 files changed, 116 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
> index 8b65e2bfb160..167f9d5c345a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -1535,6 +1535,8 @@ ice_aq_send_cmd(struct ice_hw *hw, struct ice_aq_desc *desc, void *buf,
>   	case ice_aqc_opc_set_port_params:
>   	case ice_aqc_opc_get_vlan_mode_parameters:
>   	case ice_aqc_opc_set_vlan_mode_parameters:
> +	case ice_aqc_opc_set_tx_topo:
> +	case ice_aqc_opc_get_tx_topo:
>   	case ice_aqc_opc_add_recipe:
>   	case ice_aqc_opc_recipe_to_profile:
>   	case ice_aqc_opc_get_recipe:
> diff --git a/drivers/net/ethernet/intel/ice/ice_flex_pipe.c b/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
> index b3bd345ea0f3..c2359366b48e 100644
> --- a/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
> +++ b/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
> @@ -1953,7 +1953,8 @@ int ice_cfg_tx_topo(struct ice_hw *hw, u8 *buf, u32 len)
>   	/* acquire global lock to make sure that set topology issued
>   	 * by one PF
>   	 */
> -	status = ice_acquire_global_cfg_lock(hw, ICE_RES_WRITE);
> +	status = ice_acquire_res(hw, ICE_GLOBAL_CFG_LOCK_RES_ID, ICE_RES_WRITE,
> +				 ICE_GLOBAL_CFG_LOCK_TIMEOUT);
>   	if (status) {
>   		ice_debug(hw, ICE_DBG_INIT, "Failed to acquire global lock\n");
>   		return status;
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index c1ac2f746714..5f827bea05d9 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -4453,11 +4453,11 @@ static char *ice_get_opt_fw_name(struct ice_pf *pf)
>   /**
>    * ice_request_fw - Device initialization routine
>    * @pf: pointer to the PF instance
> + * @firmware: double pointer to firmware struct
>    */
> -static void ice_request_fw(struct ice_pf *pf)
> +static int ice_request_fw(struct ice_pf *pf, const struct firmware **firmware)
>   {
>   	char *opt_fw_filename = ice_get_opt_fw_name(pf);
> -	const struct firmware *firmware = NULL;
>   	struct device *dev = ice_pf_to_dev(pf);
>   	int err = 0;
>   
> @@ -4466,29 +4466,98 @@ static void ice_request_fw(struct ice_pf *pf)
>   	 * and warning messages for other errors.
>   	 */
>   	if (opt_fw_filename) {
> -		err = firmware_request_nowarn(&firmware, opt_fw_filename, dev);
> -		if (err) {
> -			kfree(opt_fw_filename);
> -			goto dflt_pkg_load;
> -		}
> -
> -		/* request for firmware was successful. Download to device */
> -		ice_load_pkg(firmware, pf);
> +		err = firmware_request_nowarn(firmware, opt_fw_filename, dev);
>   		kfree(opt_fw_filename);
> -		release_firmware(firmware);
> -		return;
> +		if (!err)
> +			return err;

Why is the code above removed?

>   	}
>   
> -dflt_pkg_load:
> -	err = request_firmware(&firmware, ICE_DDP_PKG_FILE, dev);
> -	if (err) {
> +	err = request_firmware(firmware, ICE_DDP_PKG_FILE, dev);
> +	if (err)
>   		dev_err(dev, "The DDP package file was not found or could not be read. Entering Safe Mode\n");
> -		return;
> +
> +	return err;
> +}
> +
> +/**
> + * ice_init_tx_topology - performs Tx topology initialization
> + * @hw: pointer to the hardware structure
> + * @firmware: pointer to firmware structure
> + */
> +static int ice_init_tx_topology(struct ice_hw *hw,
> +				const struct firmware *firmware)
> +{
> +	u8 num_tx_sched_layers = hw->num_tx_sched_layers;
> +	struct ice_pf *pf = hw->back;
> +	struct device *dev;
> +	u8 *buf_copy;
> +	int err = 0;
> +
> +	dev = ice_pf_to_dev(pf);
> +	/* ice_cfg_tx_topo buf argument is not a constant,
> +	 * so we have to make a copy
> +	 */
> +	buf_copy = devm_kmemdup(ice_hw_to_dev(hw), firmware->data,
> +				firmware->size, GFP_KERNEL);
> +
> +	err = ice_cfg_tx_topo(hw, buf_copy, firmware->size);
> +	if (!err) {
> +		if (hw->num_tx_sched_layers > num_tx_sched_layers)
> +			dev_info(dev, "Transmit balancing feature disabled\n");

Should this be an error?

> +		else
> +			dev_info(dev, "Transmit balancing feature enabled\n");

Add a blank line below?

> +		/* if there was a change in topology ice_cfg_tx_topo triggered
> +		 * a CORER and we need to re-init hw

What is CORER? Add a dot/period at the end?

> +		 */
> +		ice_deinit_hw(hw);
> +		err = ice_init_hw(hw);
> +
> +		/* in this case we're not allowing safe mode */
> +		devm_kfree(ice_hw_to_dev(hw), buf_copy);
> +
> +		return err;
> +
> +	} else if (err == -EIO) {
> +		dev_info(dev, "DDP package does not support transmit balancing feature - please update to the latest DDP package and try again\n");

Please mention a version, where it’s supposed to work? Can the DDP 
version be printed out too?

Should this be a warning?

>   	}
>   
> -	/* request for firmware was successful. Download to device */
> +	devm_kfree(ice_hw_to_dev(hw), buf_copy);
> +
> +	return 0;
> +}
> +
> +/**
> + * ice_init_ddp_config - DDP related configuration
> + * @hw: pointer to the hardware structure
> + * @pf: pointer to pf structure
> + *
> + * This function loads DDP file from the disk, then initializes tx
> + * topology. At the end DDP package is loaded on the card.
> + */
> +static int ice_init_ddp_config(struct ice_hw *hw, struct ice_pf *pf)
> +{
> +	struct device *dev = ice_pf_to_dev(pf);
> +	const struct firmware *firmware = NULL;
> +	int err = 0;
> +
> +	err = ice_request_fw(pf, &firmware);
> +	if (err)
> +	/* we can still operate in safe mode if DDP package load fails */

Indent the comment?

> +		return 0;
> +
> +	err = ice_init_tx_topology(hw, firmware);
> +	if (err) {
> +		dev_err(dev, "ice_init_hw during change of tx topology failed: %d\n",
> +			err);
> +		release_firmware(firmware);
> +		return err;
> +	}
> +
> +	/* Download firmware to device */
>   	ice_load_pkg(firmware, pf);
>   	release_firmware(firmware);
> +
> +	return 0;
>   }
>   
>   /**
> @@ -4641,9 +4710,15 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
>   
>   	ice_init_feature_support(pf);
>   
> -	ice_request_fw(pf);
> +	err = ice_init_ddp_config(hw, pf);
> +
> +	/* during topology change ice_init_hw may fail */
> +	if (err) {
> +		err = -EIO;
> +		goto err_exit_unroll;
> +	}
>   
> -	/* if ice_request_fw fails, ICE_FLAG_ADV_FEATURES bit won't be
> +	/* if ice_init_ddp_config fails, ICE_FLAG_ADV_FEATURES bit won't be
>   	 * set in pf->state, which will cause ice_is_safe_mode to return
>   	 * true
>   	 */
> diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c b/drivers/net/ethernet/intel/ice/ice_sched.c
> index 7947223536e3..f18a7121ca55 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sched.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sched.c
> @@ -1102,12 +1102,11 @@ static u8 ice_sched_get_vsi_layer(struct ice_hw *hw)
>   	 *     5 or less       sw_entry_point_layer
>   	 */
>   	/* calculate the VSI layer based on number of layers. */
> -	if (hw->num_tx_sched_layers > ICE_VSI_LAYER_OFFSET + 1) {
> -		u8 layer = hw->num_tx_sched_layers - ICE_VSI_LAYER_OFFSET;
> -
> -		if (layer > hw->sw_entry_point_layer)
> -			return layer;
> -	}
> +	if (hw->num_tx_sched_layers == ICE_SCHED_9_LAYERS)
> +		return hw->num_tx_sched_layers - ICE_VSI_LAYER_OFFSET;
> +	else if (hw->num_tx_sched_layers == ICE_SCHED_5_LAYERS)
> +		/* qgroup and VSI layers are same */
> +		return hw->num_tx_sched_layers - ICE_QGRP_LAYER_OFFSET;
>   	return hw->sw_entry_point_layer;
>   }
>   
> @@ -1124,12 +1123,8 @@ static u8 ice_sched_get_agg_layer(struct ice_hw *hw)
>   	 *     7 or less       sw_entry_point_layer
>   	 */
>   	/* calculate the aggregator layer based on number of layers. */
> -	if (hw->num_tx_sched_layers > ICE_AGG_LAYER_OFFSET + 1) {
> -		u8 layer = hw->num_tx_sched_layers - ICE_AGG_LAYER_OFFSET;
> -
> -		if (layer > hw->sw_entry_point_layer)
> -			return layer;
> -	}
> +	if (hw->num_tx_sched_layers == ICE_SCHED_9_LAYERS)
> +		return hw->num_tx_sched_layers - ICE_AGG_LAYER_OFFSET;
>   	return hw->sw_entry_point_layer;
>   }
>   
> @@ -1485,10 +1480,11 @@ ice_sched_get_free_qparent(struct ice_port_info *pi, u16 vsi_handle, u8 tc,
>   {
>   	struct ice_sched_node *vsi_node, *qgrp_node;
>   	struct ice_vsi_ctx *vsi_ctx;
> +	u8 qgrp_layer, vsi_layer;
>   	u16 max_children;
> -	u8 qgrp_layer;
>   
>   	qgrp_layer = ice_sched_get_qgrp_layer(pi->hw);
> +	vsi_layer = ice_sched_get_vsi_layer(pi->hw);
>   	max_children = pi->hw->max_children[qgrp_layer];
>   
>   	vsi_ctx = ice_get_vsi_ctx(pi->hw, vsi_handle);
> @@ -1499,6 +1495,12 @@ ice_sched_get_free_qparent(struct ice_port_info *pi, u16 vsi_handle, u8 tc,
>   	if (!vsi_node)
>   		return NULL;
>   
> +	/* If the queue group and vsi layer are same then queues
> +	 * are all attached directly to VSI
> +	 */
> +	if (qgrp_layer == vsi_layer)
> +		return vsi_node;
> +
>   	/* get the first queue group node from VSI sub-tree */
>   	qgrp_node = ice_sched_get_first_node(pi, vsi_node, qgrp_layer);
>   	while (qgrp_node) {
> @@ -3178,8 +3180,9 @@ ice_sched_add_rl_profile(struct ice_port_info *pi,
>   	u8 profile_type;
>   	int status;
>   
> -	if (layer_num >= ICE_AQC_TOPO_MAX_LEVEL_NUM)
> +	if (!pi || layer_num >= pi->hw->num_tx_sched_layers)
>   		return NULL;
> +
>   	switch (rl_type) {
>   	case ICE_MIN_BW:
>   		profile_type = ICE_AQC_RL_PROFILE_TYPE_CIR;
> @@ -3194,8 +3197,6 @@ ice_sched_add_rl_profile(struct ice_port_info *pi,
>   		return NULL;
>   	}
>   
> -	if (!pi)
> -		return NULL;

This hunk (and above) could be a separate commit?

>   	hw = pi->hw;
>   	list_for_each_entry(rl_prof_elem, &pi->rl_prof_list[layer_num],
>   			    list_entry)
> @@ -3425,7 +3426,7 @@ ice_sched_rm_rl_profile(struct ice_port_info *pi, u8 layer_num, u8 profile_type,
>   	struct ice_aqc_rl_profile_info *rl_prof_elem;
>   	int status = 0;
>   
> -	if (layer_num >= ICE_AQC_TOPO_MAX_LEVEL_NUM)
> +	if (layer_num >= pi->hw->num_tx_sched_layers)
>   		return -EINVAL;
>   	/* Check the existing list for RL profile */
>   	list_for_each_entry(rl_prof_elem, &pi->rl_prof_list[layer_num],


Kind regards,

Paul
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan