Re: [PATCH net-next v03 3/6] hinic3: Add ethtool coalesce ops

[Mohsin Bashir <mohsin.bashr@gmail.com>]

> +static int is_coalesce_exceed_limit(struct net_device *netdev,
> +				    const struct ethtool_coalesce *coal)
> +{
> +	const struct {
> +		const char *name;
> +		u32 value;
> +		u32 limit;
> +	} coalesce_limits[] = {
> +		{"rx_coalesce_usecs",
> +		 coal->rx_coalesce_usecs,
> +		 COALESCE_MAX_TIMER_CFG},
> +		{"rx_max_coalesced_frames",
> +		 coal->rx_max_coalesced_frames,
> +		 COALESCE_MAX_PENDING_LIMIT},
> +		{"rx_max_coalesced_frames_low",
> +		 coal->rx_max_coalesced_frames_low,
> +		 COALESCE_MAX_PENDING_LIMIT},
> +		{"rx_max_coalesced_frames_high",
> +		 coal->rx_max_coalesced_frames_high,
> +		 COALESCE_MAX_PENDING_LIMIT},
> +	};
> +
> +	for (int i = 0; i < ARRAY_SIZE(coalesce_limits); i++) {
> +		if (coalesce_limits[i].value > coalesce_limits[i].limit) {
> +			netdev_err(netdev, "%s out of range %d-%d\n",
> +				   coalesce_limits[i].name, 0,
> +				   coalesce_limits[i].limit);
> +			return -EOPNOTSUPP;

Since we are failing a range check, maybe -ERANGE or -EINVAL would be 
more appropriate here.

> +		}
> +	}
> +	return 0;
> +}
> +
> +static int is_coalesce_legal(struct net_device *netdev,
> +			     const struct ethtool_coalesce *coal)
> +{
> +	int err;
> +
> +	err = is_coalesce_exceed_limit(netdev, coal);
> +	if (err)
> +		return err;
> +
> +	if (coal->rx_max_coalesced_frames_low >=
> +	    coal->rx_max_coalesced_frames_high &&
> +	    coal->rx_max_coalesced_frames_high > 0) {

So this would allow non-zero low and zero high. For example, low = 10, 
high = 0. Is this expected?

> +		netdev_err(netdev, "invalid coalesce frame high %u, low %u, unit %d\n",
> +			   coal->rx_max_coalesced_frames_high,
> +			   coal->rx_max_coalesced_frames_low,
> +			   COALESCE_PENDING_LIMIT_UNIT);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static void check_coalesce_align(struct net_device *netdev,
> +				 u32 item, u32 unit, const char *str)
> +{
> +	if (item % unit)
> +		netdev_warn(netdev, "%s in %d units, change to %u\n",
> +			    str, unit, item - item % unit);
> +}
> +
> +#define CHECK_COALESCE_ALIGN(member, unit) \
> +	check_coalesce_align(netdev, member, unit, #member)
> +
> +static void check_coalesce_changed(struct net_device *netdev,
> +				   u32 item, u32 unit, u32 ori_val,
> +				   const char *obj_str, const char *str)
> +{
> +	if ((item / unit) != ori_val)
> +		netdev_dbg(netdev, "Change %s from %d to %u %s\n",
> +			   str, ori_val * unit, item - item % unit, obj_str);
> +}
> +
> +#define CHECK_COALESCE_CHANGED(member, unit, ori_val, obj_str) \
> +	check_coalesce_changed(netdev, member, unit, ori_val, obj_str, #member)
> +
> +static int hinic3_set_hw_coal_param(struct net_device *netdev,
> +				    struct hinic3_intr_coal_info *intr_coal)
> +{
> +	struct hinic3_nic_dev *nic_dev = netdev_priv(netdev);
> +	int err;
> +	u16 i;
> +
> +	for (i = 0; i < nic_dev->max_qps; i++) {
> +		err = hinic3_set_queue_coalesce(netdev, i, intr_coal);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static int hinic3_get_coalesce(struct net_device *netdev,
> +			       struct ethtool_coalesce *coal,
> +			       struct kernel_ethtool_coalesce *kernel_coal,
> +			       struct netlink_ext_ack *extack)
> +{
> +	struct hinic3_nic_dev *nic_dev = netdev_priv(netdev);
> +	struct hinic3_intr_coal_info *interrupt_info;
> +
> +	interrupt_info = &nic_dev->intr_coalesce[0];
> +
> +	/* TX/RX uses the same interrupt.
> +	 * So we only declare RX ethtool_coalesce parameters.
> +	 */
> +	coal->rx_coalesce_usecs = interrupt_info->coalesce_timer_cfg *
> +				  COALESCE_TIMER_CFG_UNIT;
> +	coal->rx_max_coalesced_frames = interrupt_info->pending_limit *
> +					COALESCE_PENDING_LIMIT_UNIT;
> +
> +	coal->use_adaptive_rx_coalesce = nic_dev->adaptive_rx_coal;
> +
> +	coal->rx_max_coalesced_frames_high =
> +		interrupt_info->rx_pending_limit_high *
> +		COALESCE_PENDING_LIMIT_UNIT;
> +
> +	coal->rx_max_coalesced_frames_low =
> +		interrupt_info->rx_pending_limit_low *
> +		COALESCE_PENDING_LIMIT_UNIT;
> +
> +	return 0;
> +}
> +
> +static int hinic3_set_coalesce(struct net_device *netdev,
> +			       struct ethtool_coalesce *coal,
> +			       struct kernel_ethtool_coalesce *kernel_coal,
> +			       struct netlink_ext_ack *extack)
> +{
> +	struct hinic3_nic_dev *nic_dev = netdev_priv(netdev);
> +	struct hinic3_intr_coal_info *ori_intr_coal;
> +	struct hinic3_intr_coal_info intr_coal = {};
> +	char obj_str[32];
> +	int err;
> +
> +	err = is_coalesce_legal(netdev, coal);
> +	if (err)
> +		return err;
> +
> +	CHECK_COALESCE_ALIGN(coal->rx_coalesce_usecs, COALESCE_TIMER_CFG_UNIT);
> +	CHECK_COALESCE_ALIGN(coal->rx_max_coalesced_frames,
> +			     COALESCE_PENDING_LIMIT_UNIT);
> +	CHECK_COALESCE_ALIGN(coal->rx_max_coalesced_frames_high,
> +			     COALESCE_PENDING_LIMIT_UNIT);
> +	CHECK_COALESCE_ALIGN(coal->rx_max_coalesced_frames_low,
> +			     COALESCE_PENDING_LIMIT_UNIT);
> +
> +	ori_intr_coal = &nic_dev->intr_coalesce[0];
> +	snprintf(obj_str, sizeof(obj_str), "for netdev");
> +
> +	CHECK_COALESCE_CHANGED(coal->rx_coalesce_usecs, COALESCE_TIMER_CFG_UNIT,
> +			       ori_intr_coal->coalesce_timer_cfg, obj_str);
> +	CHECK_COALESCE_CHANGED(coal->rx_max_coalesced_frames,
> +			       COALESCE_PENDING_LIMIT_UNIT,
> +			       ori_intr_coal->pending_limit, obj_str);
> +	CHECK_COALESCE_CHANGED(coal->rx_max_coalesced_frames_high,
> +			       COALESCE_PENDING_LIMIT_UNIT,
> +			       ori_intr_coal->rx_pending_limit_high, obj_str);
> +	CHECK_COALESCE_CHANGED(coal->rx_max_coalesced_frames_low,
> +			       COALESCE_PENDING_LIMIT_UNIT,
> +			       ori_intr_coal->rx_pending_limit_low, obj_str);
> +
> +	intr_coal.coalesce_timer_cfg =
> +		(u8)(coal->rx_coalesce_usecs / COALESCE_TIMER_CFG_UNIT);
> +	intr_coal.pending_limit = (u8)(coal->rx_max_coalesced_frames /
> +				      COALESCE_PENDING_LIMIT_UNIT);
> +
> +	nic_dev->adaptive_rx_coal = coal->use_adaptive_rx_coalesce;
> +
> +	intr_coal.rx_pending_limit_high =
> +		(u8)(coal->rx_max_coalesced_frames_high /
> +		     COALESCE_PENDING_LIMIT_UNIT);
> +
> +	intr_coal.rx_pending_limit_low =
> +		(u8)(coal->rx_max_coalesced_frames_low /
> +		     COALESCE_PENDING_LIMIT_UNIT);
> +
> +	/* coalesce timer or pending set to zero will disable coalesce */
> +	if (!nic_dev->adaptive_rx_coal &&
> +	    (!intr_coal.coalesce_timer_cfg || !intr_coal.pending_limit))
> +		netdev_warn(netdev, "Coalesce will be disabled\n");
> +
> +	return hinic3_set_hw_coal_param(netdev, &intr_coal);
> +}
> +
>   static const struct ethtool_ops hinic3_ethtool_ops = {
> -	.supported_coalesce_params      = ETHTOOL_COALESCE_USECS |
> -					  ETHTOOL_COALESCE_PKT_RATE_RX_USECS,
> +	.supported_coalesce_params      = ETHTOOL_COALESCE_RX_USECS |
> +					  ETHTOOL_COALESCE_RX_MAX_FRAMES |
> +					  ETHTOOL_COALESCE_USE_ADAPTIVE_RX |
> +					  ETHTOOL_COALESCE_RX_MAX_FRAMES_LOW |
> +					  ETHTOOL_COALESCE_RX_MAX_FRAMES_HIGH,

Looks like ETHTOOL_COALESCE_TX_USECS support got dropped. is it intentional?

>   	.get_link_ksettings             = hinic3_get_link_ksettings,
>   	.get_drvinfo                    = hinic3_get_drvinfo,
>   	.get_msglevel                   = hinic3_get_msglevel,
> @@ -1004,6 +1231,8 @@ static const struct ethtool_ops hinic3_ethtool_ops = {
>   	.get_eth_ctrl_stats             = hinic3_get_eth_ctrl_stats,
>   	.get_rmon_stats                 = hinic3_get_rmon_stats,
>   	.get_pause_stats                = hinic3_get_pause_stats,
> +	.get_coalesce                   = hinic3_get_coalesce,
> +	.set_coalesce                   = hinic3_set_coalesce,
>   };
>   
>   void hinic3_set_ethtool_ops(struct net_device *netdev)