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

Simon Horman
2026-04-10T20:12:25Z

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@gmail.com/

> +
> +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@gmail.com/