[S39,09/15] ice: Don't reject odd values of usecs set by user

From: Brett Creeley <brett.creeley@intel.com>

Currently if a user sets an odd [tx|rx]-usecs value through ethtool,
the request is denied because the hardware is set to have an ITR
granularity of 2us. This caused poor customer experience. Fix this by
aligning to a register allowed value, which results in rounding down.
Also, print a once per ring container type message to be clear about
our intentions.

Also, change the ITR_TO_REG define to be the bitwise and of the ITR
setting and the ICE_ITR_MASK. This makes the purpose of ITR_TO_REG more
obvious.

Signed-off-by: Brett Creeley <brett.creeley@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ethtool.c | 49 +++++++++++++++-----
 drivers/net/ethernet/intel/ice/ice_txrx.h    |  2 +-
 2 files changed, 39 insertions(+), 12 deletions(-)

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On
> Behalf Of Tony Nguyen
> Sent: Monday, January 27, 2020 12:59 AM
> To: intel-wired-lan@lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH S39 09/15] ice: Don't reject odd values of
> usecs set by user
> 
> From: Brett Creeley <brett.creeley@intel.com>
> 
> Currently if a user sets an odd [tx|rx]-usecs value through ethtool, the
> request is denied because the hardware is set to have an ITR granularity of
> 2us. This caused poor customer experience. Fix this by aligning to a register
> allowed value, which results in rounding down.
> Also, print a once per ring container type message to be clear about our
> intentions.
> 
> Also, change the ITR_TO_REG define to be the bitwise and of the ITR setting
> and the ICE_ITR_MASK. This makes the purpose of ITR_TO_REG more
> obvious.
> 
> Signed-off-by: Brett Creeley <brett.creeley@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ethtool.c | 49 +++++++++++++++-----
>  drivers/net/ethernet/intel/ice/ice_txrx.h    |  2 +-
>  2 files changed, 39 insertions(+), 12 deletions(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Tony Nguyen
> Sent: Monday, January 27, 2020 12:59 AM
> To: intel-wired-lan@lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH S39 09/15] ice: Don't reject odd values of
> usecs set by user
> 
> From: Brett Creeley <brett.creeley@intel.com>
> 
> Currently if a user sets an odd [tx|rx]-usecs value through ethtool,
> the request is denied because the hardware is set to have an ITR
> granularity of 2us. This caused poor customer experience. Fix this by
> aligning to a register allowed value, which results in rounding down.
> Also, print a once per ring container type message to be clear about
> our intentions.
> 
> Also, change the ITR_TO_REG define to be the bitwise and of the ITR
> setting and the ICE_ITR_MASK. This makes the purpose of ITR_TO_REG more
> obvious.
> 
> Signed-off-by: Brett Creeley <brett.creeley@intel.com>

Cc: aaron.f.rowden@intel.com

> ---
>  drivers/net/ethernet/intel/ice/ice_ethtool.c | 49 +++++++++++++++-----
>  drivers/net/ethernet/intel/ice/ice_txrx.h    |  2 +-
>  2 files changed, 39 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> index db14ec2e0b46..ae0b63d5673d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> @@ -3488,21 +3488,13 @@ ice_set_rc_coalesce(enum ice_container_type
> c_type, struct ethtool_coalesce *ec,
>  		return -EINVAL;
>  	}
> 
> -	/* hardware only supports an ITR granularity of 2us */
> -	if (coalesce_usecs % 2 != 0) {
> -		netdev_info(vsi->netdev, "Invalid value, %s-usecs must be
> even\n",
> -			    c_type_str);
> -		return -EINVAL;
> -	}
> -
>  	if (use_adaptive_coalesce) {
>  		rc->itr_setting |= ICE_ITR_DYNAMIC;
>  	} else {
> -		/* store user facing value how it was set */
> +		/* save the user set usecs */
>  		rc->itr_setting = coalesce_usecs;
> -		/* set to static and convert to value HW understands */
> -		rc->target_itr =
> -			ITR_TO_REG(ITR_REG_ALIGN(rc->itr_setting));
> +		/* device ITR granularity is in 2 usec increments */
> +		rc->target_itr = ITR_REG_ALIGN(rc->itr_setting);
>  	}
> 
>  	return 0;
> @@ -3595,6 +3587,30 @@ ice_is_coalesce_param_invalid(struct net_device
> *netdev,
>  	return 0;
>  }
> 
> +/**
> + * ice_print_if_odd_usecs - print message if user tries to set odd [tx|rx]-usecs
> + * @netdev: netdev used for print
> + * @itr_setting: previous user setting
> + * @use_adaptive_coalesce: if adaptive coalesce is enabled or being enabled
> + * @coalesce_usecs: requested value of [tx|rx]-usecs
> + * @c_type_str: either "rx" or "tx" to match user set field of [tx|rx]-usecs
> + */
> +static void
> +ice_print_if_odd_usecs(struct net_device *netdev, u16 itr_setting,
> +		       u32 use_adaptive_coalesce, u32 coalesce_usecs,
> +		       const char *c_type_str)
> +{
> +	if (use_adaptive_coalesce)
> +		return;
> +
> +	itr_setting = ITR_TO_REG(itr_setting);
> +
> +	if (itr_setting != coalesce_usecs && (coalesce_usecs % 2))
> +		netdev_info(netdev, "User set %s-usecs to %d, device only
> supports even values. Rounding down and attempting to set %s-usecs to
> %d\n",
> +			    c_type_str, coalesce_usecs, c_type_str,
> +			    ITR_REG_ALIGN(coalesce_usecs));
> +}
> +
>  /**
>   * __ice_set_coalesce - set ITR/INTRL values for the device
>   * @netdev: pointer to the netdev associated with this query
> @@ -3615,8 +3631,19 @@ __ice_set_coalesce(struct net_device *netdev,
> struct ethtool_coalesce *ec,
>  		return -EINVAL;
> 
>  	if (q_num < 0) {
> +		struct ice_q_vector *q_vector = vsi->q_vectors[0];
>  		int v_idx;
> 
> +		if (q_vector) {
> +			ice_print_if_odd_usecs(netdev, q_vector-
> >rx.itr_setting,
> +					       ec->use_adaptive_rx_coalesce,
> +					       ec->rx_coalesce_usecs, "rx");
> +
> +			ice_print_if_odd_usecs(netdev, q_vector-
> >tx.itr_setting,
> +					       ec->use_adaptive_tx_coalesce,
> +					       ec->tx_coalesce_usecs, "tx");
> +		}
> +
>  		ice_for_each_q_vector(vsi, v_idx) {
>  			/* In some cases if DCB is configured the num_[rx|tx]q
>  			 * can be less than vsi->num_q_vectors. This check
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h
> b/drivers/net/ethernet/intel/ice/ice_txrx.h
> index a86270696df1..3e3cc2599824 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.h
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
> @@ -222,7 +222,7 @@ enum ice_rx_dtype {
>  #define ICE_ITR_GRAN_S		1	/* ITR granularity is always
> 2us */
>  #define ICE_ITR_GRAN_US		BIT(ICE_ITR_GRAN_S)
>  #define ICE_ITR_MASK		0x1FFE	/* ITR register value alignment mask
> */
> -#define ITR_REG_ALIGN(setting)	__ALIGN_MASK(setting,
> ~ICE_ITR_MASK)
> +#define ITR_REG_ALIGN(setting)	((setting) & ICE_ITR_MASK)
> 
>  #define ICE_ITR_ADAPTIVE_MIN_INC	0x0002
>  #define ICE_ITR_ADAPTIVE_MIN_USECS	0x0002
> --
> 2.20.1
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Dear Tony,


On 2020-01-27 09:59, Tony Nguyen wrote:
> From: Brett Creeley <brett.creeley@intel.com>
> 
> Currently if a user sets an odd [tx|rx]-usecs value through ethtool,
> the request is denied because the hardware is set to have an ITR
> granularity of 2us. This caused poor customer experience. Fix this by
> aligning to a register allowed value, which results in rounding down.
> Also, print a once per ring container type message to be clear about
> our intentions.
> 
> Also, change the ITR_TO_REG define to be the bitwise and of the ITR
> setting and the ICE_ITR_MASK. This makes the purpose of ITR_TO_REG more
> obvious.
> 
> Signed-off-by: Brett Creeley <brett.creeley@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ethtool.c | 49 +++++++++++++++-----
>  drivers/net/ethernet/intel/ice/ice_txrx.h    |  2 +-
>  2 files changed, 39 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> index db14ec2e0b46..ae0b63d5673d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> @@ -3488,21 +3488,13 @@ ice_set_rc_coalesce(enum ice_container_type c_type, struct ethtool_coalesce *ec,
>  		return -EINVAL;
>  	}
>  
> -	/* hardware only supports an ITR granularity of 2us */
> -	if (coalesce_usecs % 2 != 0) {
> -		netdev_info(vsi->netdev, "Invalid value, %s-usecs must be even\n",
> -			    c_type_str);
> -		return -EINVAL;
> -	}
> -
>  	if (use_adaptive_coalesce) {
>  		rc->itr_setting |= ICE_ITR_DYNAMIC;
>  	} else {
> -		/* store user facing value how it was set */
> +		/* save the user set usecs */
>  		rc->itr_setting = coalesce_usecs;
> -		/* set to static and convert to value HW understands */
> -		rc->target_itr =
> -			ITR_TO_REG(ITR_REG_ALIGN(rc->itr_setting));
> +		/* device ITR granularity is in 2 usec increments */
> +		rc->target_itr = ITR_REG_ALIGN(rc->itr_setting);
>  	}
>  
>  	return 0;
> @@ -3595,6 +3587,30 @@ ice_is_coalesce_param_invalid(struct net_device *netdev,
>  	return 0;
>  }
>  
> +/**
> + * ice_print_if_odd_usecs - print message if user tries to set odd [tx|rx]-usecs
> + * @netdev: netdev used for print
> + * @itr_setting: previous user setting
> + * @use_adaptive_coalesce: if adaptive coalesce is enabled or being enabled
> + * @coalesce_usecs: requested value of [tx|rx]-usecs
> + * @c_type_str: either "rx" or "tx" to match user set field of [tx|rx]-usecs

Why `c_type_str`? What does it mean?

> + */
> +static void
> +ice_print_if_odd_usecs(struct net_device *netdev, u16 itr_setting,
> +		       u32 use_adaptive_coalesce, u32 coalesce_usecs,
> +		       const char *c_type_str)
> +{
> +	if (use_adaptive_coalesce)
> +		return;

Why not check that before calling the function and not passing the it to this
one.

> +
> +	itr_setting = ITR_TO_REG(itr_setting);
> +
> +	if (itr_setting != coalesce_usecs && (coalesce_usecs % 2))
> +		netdev_info(netdev, "User set %s-usecs to %d, device only supports even values. Rounding down and attempting to set %s-usecs to %d\n",
> +			    c_type_str, coalesce_usecs, c_type_str,
> +			    ITR_REG_ALIGN(coalesce_usecs));
> +}
> +
>  /**
>   * __ice_set_coalesce - set ITR/INTRL values for the device
>   * @netdev: pointer to the netdev associated with this query
> @@ -3615,8 +3631,19 @@ __ice_set_coalesce(struct net_device *netdev, struct ethtool_coalesce *ec,
>  		return -EINVAL;
>  
>  	if (q_num < 0) {
> +		struct ice_q_vector *q_vector = vsi->q_vectors[0];
>  		int v_idx;
>  
> +		if (q_vector) {

Why not check for `(coalesce_usecs % 2)` here already?

> +			ice_print_if_odd_usecs(netdev, q_vector->rx.itr_setting,
> +					       ec->use_adaptive_rx_coalesce,
> +					       ec->rx_coalesce_usecs, "rx");
> +
> +			ice_print_if_odd_usecs(netdev, q_vector->tx.itr_setting,
> +					       ec->use_adaptive_tx_coalesce,
> +					       ec->tx_coalesce_usecs, "tx");
> +		}
> +

I do not know of such a construct in the rest of the Linux kernel. Is there
a better way to achieve your goal?

>  		ice_for_each_q_vector(vsi, v_idx) {
>  			/* In some cases if DCB is configured the num_[rx|tx]q
>  			 * can be less than vsi->num_q_vectors. This check
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h
> index a86270696df1..3e3cc2599824 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.h
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
> @@ -222,7 +222,7 @@ enum ice_rx_dtype {
>  #define ICE_ITR_GRAN_S		1	/* ITR granularity is always 2us */
>  #define ICE_ITR_GRAN_US		BIT(ICE_ITR_GRAN_S)
>  #define ICE_ITR_MASK		0x1FFE	/* ITR register value alignment mask */
> -#define ITR_REG_ALIGN(setting)	__ALIGN_MASK(setting, ~ICE_ITR_MASK)
> +#define ITR_REG_ALIGN(setting)	((setting) & ICE_ITR_MASK)
>  
>  #define ICE_ITR_ADAPTIVE_MIN_INC	0x0002
>  #define ICE_ITR_ADAPTIVE_MIN_USECS	0x0002
> 


Kind regards,

Paul

On Tue, 2020-02-11 at 18:21 +0100, Paul Menzel wrote:
> Dear Tony,
> 
> 
> On 2020-01-27 09:59, Tony Nguyen wrote:
> > From: Brett Creeley <brett.creeley@intel.com>
> > 
> > Currently if a user sets an odd [tx|rx]-usecs value through
> > ethtool,
> > the request is denied because the hardware is set to have an ITR
> > granularity of 2us. This caused poor customer experience. Fix this
> > by
> > aligning to a register allowed value, which results in rounding
> > down.
> > Also, print a once per ring container type message to be clear
> > about
> > our intentions.
> > 
> > Also, change the ITR_TO_REG define to be the bitwise and of the ITR
> > setting and the ICE_ITR_MASK. This makes the purpose of ITR_TO_REG
> > more
> > obvious.
> > 
> > Signed-off-by: Brett Creeley <brett.creeley@intel.com>
> > ---
> >  drivers/net/ethernet/intel/ice/ice_ethtool.c | 49 +++++++++++++++-
> > ----
> >  drivers/net/ethernet/intel/ice/ice_txrx.h    |  2 +-
> >  2 files changed, 39 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> > b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> > index db14ec2e0b46..ae0b63d5673d 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> > @@ -3488,21 +3488,13 @@ ice_set_rc_coalesce(enum ice_container_type
> > c_type, struct ethtool_coalesce *ec,
> >  		return -EINVAL;
> >  	}
> >  
> > -	/* hardware only supports an ITR granularity of 2us */
> > -	if (coalesce_usecs % 2 != 0) {
> > -		netdev_info(vsi->netdev, "Invalid value, %s-usecs must
> > be even\n",
> > -			    c_type_str);
> > -		return -EINVAL;
> > -	}
> > -
> >  	if (use_adaptive_coalesce) {
> >  		rc->itr_setting |= ICE_ITR_DYNAMIC;
> >  	} else {
> > -		/* store user facing value how it was set */
> > +		/* save the user set usecs */
> >  		rc->itr_setting = coalesce_usecs;
> > -		/* set to static and convert to value HW understands */
> > -		rc->target_itr =
> > -			ITR_TO_REG(ITR_REG_ALIGN(rc->itr_setting));
> > +		/* device ITR granularity is in 2 usec increments */
> > +		rc->target_itr = ITR_REG_ALIGN(rc->itr_setting);
> >  	}
> >  
> >  	return 0;
> > @@ -3595,6 +3587,30 @@ ice_is_coalesce_param_invalid(struct
> > net_device *netdev,
> >  	return 0;
> >  }
> >  
> > +/**
> > + * ice_print_if_odd_usecs - print message if user tries to set odd
> > [tx|rx]-usecs
> > + * @netdev: netdev used for print
> > + * @itr_setting: previous user setting
> > + * @use_adaptive_coalesce: if adaptive coalesce is enabled or
> > being enabled
> > + * @coalesce_usecs: requested value of [tx|rx]-usecs
> > + * @c_type_str: either "rx" or "tx" to match user set field of
> > [tx|rx]-usecs
> 
> Why `c_type_str`? What does it mean?

It stands for container type string; so whether the container type is
"rx" or "tx".  It is has the same usage in ice_set_rx_coalesce() so we
continued with the convention here.

> > + */
> > +static void
> > +ice_print_if_odd_usecs(struct net_device *netdev, u16 itr_setting,
> > +		       u32 use_adaptive_coalesce, u32 coalesce_usecs,
> > +		       const char *c_type_str)
> > +{
> > +	if (use_adaptive_coalesce)
> > +		return;
> 
> Why not check that before calling the function and not passing the it
> to this
> one.

This is put in the function instead of before calling it to reduce code
duplication.  Adaptive coalesce can be set for Tx and Rx; usecs can
also be set for Tx and RX so putting it in here allows us to use the
same code to do the checks.  Otherwise, we will need to check each
condition for both Tx and Rx before calling this function.

> > +
> > +	itr_setting = ITR_TO_REG(itr_setting);
> > +
> > +	if (itr_setting != coalesce_usecs && (coalesce_usecs % 2))
> > +		netdev_info(netdev, "User set %s-usecs to %d, device
> > only supports even values. Rounding down and attempting to set %s-
> > usecs to %d\n",
> > +			    c_type_str, coalesce_usecs, c_type_str,
> > +			    ITR_REG_ALIGN(coalesce_usecs));
> > +}
> > +
> >  /**
> >   * __ice_set_coalesce - set ITR/INTRL values for the device
> >   * @netdev: pointer to the netdev associated with this query
> > @@ -3615,8 +3631,19 @@ __ice_set_coalesce(struct net_device
> > *netdev, struct ethtool_coalesce *ec,
> >  		return -EINVAL;
> >  
> >  	if (q_num < 0) {
> > +		struct ice_q_vector *q_vector = vsi->q_vectors[0];
> >  		int v_idx;
> >  
> > +		if (q_vector) {
> 
> Why not check for `(coalesce_usecs % 2)` here already?

Explained in previous response.

> > +			ice_print_if_odd_usecs(netdev, q_vector-
> > >rx.itr_setting,
> > +					       ec-
> > >use_adaptive_rx_coalesce,
> > +					       ec->rx_coalesce_usecs,
> > "rx");
> > +
> > +			ice_print_if_odd_usecs(netdev, q_vector-
> > >tx.itr_setting,
> > +					       ec-
> > >use_adaptive_tx_coalesce,
> > +					       ec->tx_coalesce_usecs,
> > "tx");
> > +		}
> > +
> 
> I do not know of such a construct in the rest of the Linux kernel. Is
> there
> a better way to achieve your goal?

I'm not aware of anything else to use, but I'm open to other thoughts
or suggestions if you have any.

Thanks,
Tony