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

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
hinic3: Add ethtool coalesce ops

This patch implements ethtool .get_coalesce and .set_coalesce callback
functions to allow users to configure and monitor RX coalesce settings.

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c b/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
> index be26698fc658..a4b2d5ba81f8 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c

[ ... ]

> @@ -985,9 +990,230 @@ static void hinic3_get_pause_stats(struct net_device *netdev,
>  	kfree(ps);
>  }
>
> +static int hinic3_set_queue_coalesce(struct net_device *netdev, u16 q_id,
> +				     struct hinic3_intr_coal_info *coal)
> +{
> +	struct hinic3_nic_dev *nic_dev = netdev_priv(netdev);
> +	struct hinic3_intr_coal_info *intr_coal;
> +	struct hinic3_interrupt_info info = {};
> +	int err;
> +
> +	intr_coal = &nic_dev->intr_coalesce[q_id];
> +
> +	intr_coal->coalesce_timer_cfg = coal->coalesce_timer_cfg;
> +	intr_coal->pending_limit = coal->pending_limit;
> +	intr_coal->rx_pending_limit_low = coal->rx_pending_limit_low;
> +	intr_coal->rx_pending_limit_high = coal->rx_pending_limit_high;
> +
> +	if (!test_bit(HINIC3_INTF_UP, &nic_dev->flags) ||
> +	    q_id >= nic_dev->q_params.num_qps || nic_dev->adaptive_rx_coal)
> +		return 0;
> +
> +	info.msix_index = nic_dev->q_params.irq_cfg[q_id].msix_entry_idx;
> +	info.interrupt_coalesc_set = 1;
> +	info.coalesc_timer_cfg = intr_coal->coalesce_timer_cfg;
> +	info.pending_limit = intr_coal->pending_limit;
> +	info.resend_timer_cfg = intr_coal->resend_timer_cfg;
> +	err = hinic3_set_interrupt_cfg(nic_dev->hwdev, info);
> +	if (err) {
> +		netdev_warn(netdev, "Failed to set queue%u coalesce\n", q_id);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +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 -ERANGE;
> +		}
> +	}
> +	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) {
> +		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 -EINVAL;
> +	}
> +
> +	return 0;
> +}

Does this validation allow non-zero low with zero high? For example, if
rx_max_coalesced_frames_low=10 and rx_max_coalesced_frames_high=0, the
condition 'low > high' would be true and return -EINVAL, which is correct.

However, this validation still accepts the case where low equals high, including
when both are zero. In v03, a reviewer asked: "So this would allow non-zero low
and zero high. For example, low = 10, high = 0. Is this expected?" The original
v03 logic was 'low >= high && high > 0' which rejected the case where low
equals high. In v04, this was changed to 'low > high' only, but the reviewer's
concern about whether allowing low=high (including low=0, high=0) is expected
behavior was not directly addressed.

Reference: https://lore.kernel.org/netdev/18d5a9b7-9c8d-4fc4-9332-fab27449dc58@xxxxxxxxx/

> +
> +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);
> +}

[ ... ]

> @@ -1003,6 +1229,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)

[ ... ]

> @@ -985,9 +990,230 @@ static void hinic3_get_pause_stats(struct net_device *netdev,
>
>  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,

Was the removal of ETHTOOL_COALESCE_TX_USECS intentional? In v02, the code
included 'ETHTOOL_COALESCE_USECS | ETHTOOL_COALESCE_PKT_RATE_RX_USECS', but
v03 and v04 changed this to only RX-specific parameters. A reviewer asked if
this was intentional, but there was no response from the author and the v04
changelog does not mention this concern.

Reference: https://lore.kernel.org/netdev/18d5a9b7-9c8d-4fc4-9332-fab27449dc58@xxxxxxxxx/