[Intel-wired-lan] [PATCH S39 01/15] ice: Validate config for SW DCB map

[Paul Menzel]

Dear Tony,


On 2020-01-27 09:59, Tony Nguyen wrote:
> From: Avinash Dayanand <avinash.dayanand at intel.com>
> 
> Validate the inputs for SW DCB config received either via lldptool or pcap
> file. And don't apply DCB for bad bandwidth inputs or non-contiguous TCs.
> Without this patch, any config having bad inputs will cause the loss of
> link making PF unusable even after driver reload. Recoverable only via
> system reboot.
> 
> Signed-off-by: Avinash Dayanand <avinash.dayanand at intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen at intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_dcb_lib.c | 55 ++++++++++++++++++++
>  drivers/net/ethernet/intel/ice/ice_dcb_lib.h |  1 +
>  drivers/net/ethernet/intel/ice/ice_dcb_nl.c  |  7 +++
>  3 files changed, 63 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> index 0f4ca813a7ab..bd361212921c 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> @@ -169,6 +169,56 @@ ice_peer_prep_tc_change(struct ice_peer_dev_int *peer_dev_int,
>  	return 0;
>  }
>  
> +/**
> + * ice_dcb_bwchk - check if ETS bandwidth input parameters are correct
> + * @dcbcfg: pointer to DCB config structure
> + */
> +int ice_dcb_bwchk(struct ice_dcbx_cfg *dcbcfg)
> +{
> +	struct ice_dcb_ets_cfg *etscfg = &dcbcfg->etscfg;
> +	u8 num_tc, total_bw = 0;
> +	int i;
> +
> +	/* returns number of contigous TCs and 1 TC for non-contigous TCs,
> +	 * since at least 1 TC has to be configured
> +	 */
> +	num_tc = ice_dcb_get_num_tc(dcbcfg);
> +
> +	/* no bandwidth checks required if there's only one TC and assign
> +	 * all bandwidth to it i.e. to TC0 and return

…, so assign all bandwidth to TC0 and return

> +	 */
> +	if (num_tc == 1) {
> +		etscfg->tcbwtable[0] = ICE_TC_MAX_BW;
> +		return 0;
> +	}
> +	/* There are few rules with which TC bandwidth can be applied for any TC
> +	 * with a UP mapped to it.
> +	 *	1. All TCs have zero BW - Valid
> +	 *	   ex:  tcbw=0,0,0
> +	 *	2. First few non-zero and rest zero BW - Valid
> +	 *	   ex:  tcbw=100,0,0
> +	 *	3. Zero BW in between 2 non-zero BW TCs - Invalid
> +	 *	   ex:  tcbw=25,0,75
> +	 */
> +	for (i = 0; i < num_tc; i++) {
> +		/* don't allow zero BW for TCs other than TC0 */
> +		if (i && !etscfg->tcbwtable[i])
> +			goto err;

As the error handling is just `return -EINVAL`, please do that directly
here.

> +
> +		if (etscfg->tsatable[i] == ICE_IEEE_TSA_ETS)
> +			total_bw += etscfg->tcbwtable[i];
> +	}
> +
> +	 /* total bandwidth should be equal to 100 */
> +	if (total_bw != ICE_TC_MAX_BW)
> +		goto err;

Ditto.

Also, why not print an error for this case?

> +
> +	return 0;
> +
> +err:
> +	return -EINVAL;
> +}
> +
>  /**
>   * ice_pf_dcb_cfg - Apply new DCB configuration
>   * @pf: pointer to the PF struct
> @@ -206,6 +256,11 @@ int ice_pf_dcb_cfg(struct ice_pf *pf, struct ice_dcbx_cfg *new_cfg, bool locked)
>  	/* Notify capable peers about impending change to TCs */
>  	ice_for_each_peer(pf, NULL, ice_peer_prep_tc_change);
>  
> +	if (ice_dcb_bwchk(new_cfg)) {
> +		dev_err(dev, "Invalid config, not applying DCB\n");

It’d be useful to know what is incorrect. So, maybe move the error message
into the function.

> +		return -EINVAL;
> +	}
> +
>  	/* Store old config in case FW config fails */
>  	old_cfg = kmemdup(curr_cfg, sizeof(*old_cfg), GFP_KERNEL);
>  	if (!old_cfg)
> diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.h b/drivers/net/ethernet/intel/ice/ice_dcb_lib.h
> index bb53edf462ba..2b900da27f57 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.h
> +++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.h
> @@ -20,6 +20,7 @@ u8 ice_dcb_get_num_tc(struct ice_dcbx_cfg *dcbcfg);
>  u8 ice_dcb_get_tc(struct ice_vsi *vsi, int queue_index);
>  int
>  ice_pf_dcb_cfg(struct ice_pf *pf, struct ice_dcbx_cfg *new_cfg, bool locked);
> +int ice_dcb_bwchk(struct ice_dcbx_cfg *dcbcfg);
>  void ice_pf_dcb_recfg(struct ice_pf *pf);
>  void ice_vsi_cfg_dcb_rings(struct ice_vsi *vsi);
>  int ice_init_pf_dcb(struct ice_pf *pf, bool locked);
> diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_nl.c b/drivers/net/ethernet/intel/ice/ice_dcb_nl.c
> index b61aba428adb..a45e8abef8f3 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dcb_nl.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dcb_nl.c
> @@ -95,6 +95,12 @@ static int ice_dcbnl_setets(struct net_device *netdev, struct ieee_ets *ets)
>  		new_cfg->etsrec.prio_table[i] = ets->reco_prio_tc[i];
>  	}
>  
> +	if (ice_dcb_bwchk(new_cfg)) {
> +		netdev_err(netdev, "Invalid config, not applying DCB\n");
> +		err = -EINVAL;
> +		goto ets_out;

Is that good style to use goto in this case? Why can’t it be put after
`ice_pf_dcb_cfg()`?

> +	}
> +
>  	/* max_tc is a 1-8 value count of number of TC's, not a 0-7 value
>  	 * for the TC's index number.  Add one to value if not zero, and
>  	 * for zero set it to the FW's default value
> @@ -119,6 +125,7 @@ static int ice_dcbnl_setets(struct net_device *netdev, struct ieee_ets *ets)
>  	if (err == ICE_DCB_NO_HW_CHG)
>  		err = ICE_DCB_HW_CHG_RST;
>  
> +ets_out:
>  	mutex_unlock(&pf->tc_mutex);
>  	return err;
>  }


Kind regards,

Paul

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5174 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20200211/34f2edfc/attachment.p7s>