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

["Wilczynski, Michal" <michal.wilczynski@intel.com>]

Created a v3, need to clarify one of mine answers

On 6/29/2022 11:54 AM, Wilczynski, Michal wrote:
> Hi, Thanks for your review
>
> On 6/25/2022 9:10 AM, Paul Menzel wrote:
>> 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?
>
>
> Will try to find exact numbers.
>
>
>>
>>> 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.
>
>
> The reason why it's a bit cryptic is that the devlink mechanism is not 
> upstreamed yet, and epct way of doing
>
> that doesn't seem to work with upstream driver.
>
>
> Will post an example of devlink way of doing that, it will be 
> upstreamed very soon.
>
>
>
>>
>> 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.
>
>
> Yep, will fix that
>
>
>>
>>> 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?
>
>
> The code received a bit of refactor, logical flow is a bit different, 
> now we don't upload firmware to the device
>
> immediately after loading it from the disk. We change the topology 
> using AQ comand, then trigger CORER.
>
> And only after that we upload firmware to the device.
>
>
>>
>>>       }
>>>   -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?
>
>
> This is an opt-in feature, so we don't treat failure as an error.
>
>
>>
>>> +        else
>>> +            dev_info(dev, "Transmit balancing feature enabled\n");
>>
>> Add a blank line below?
>
> Sure
>
>
>>
>>> +        /* 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?
>
>
> CORER is a type of reset of our hardware. This concept seems to be 
> used extensively in our driver, not sure
>
> if this requires an explanation in the comment.
>
>
>>
>>> +         */
>>> +        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?


This works like that by design, cause we have a number of different DDP 
packages with different versions.

So hinting the version user would need upgrade to is not convenient here.


>>
>> Should this be a warning?
>
>
> I will try to find out exact versions required.
>
> This could maybe be a warning, it wasn't cause this is an opt-in feature.
>
>
>>
>>>       }
>>>   -    /* 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?
>
>
> Sure
>
>
>>
>>> +        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?
>
>
> Seems like a good idea
>
>
>>
>>>       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