Re: [PATCH net-next v09 3/5] hinic3: Add ethtool coalesce ops

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
hinic3: Add ethtool coalesce ops

This patch implements the .get_coalesce and .set_coalesce ethtool callbacks
for the hinic3 driver. It allows users to configure and monitor RX coalesce
parameters.

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c b/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
[ ... ]
> @@ -975,9 +980,247 @@ static void hinic3_get_pause_stats(struct net_device *netdev,
> +static int hinic3_set_queue_coalesce(struct net_device *netdev, u16 q_id,
> +				     struct hinic3_intr_coal_info *coal,
> +				     struct netlink_ext_ack *extack)
> +{
> +	struct hinic3_nic_dev *nic_dev = netdev_priv(netdev);
> +	struct hinic3_intr_coal_info *intr_coal;
> +	struct hinic3_interrupt_info info = {};
> +	int err;
> +
> +	if (nic_dev->adaptive_rx_coal) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Static coalesce not allowed in adaptive RX mode");
> +		return -EINVAL;
> +	}

[Severity: High]
When adaptive RX coalesce is enabled via ethtool, hinic3_set_coalesce()
sets nic_dev->adaptive_rx_coal to 1 before calling this function.

Will this check then unconditionally return -EINVAL, failing the operation
while leaving the software state permanently incorrectly recording that
adaptive mode is on?

> +
> +	if (!test_bit(HINIC3_INTF_UP, &nic_dev->flags) ||
> +	    q_id >= nic_dev->q_params.num_qps)
> +		return 0;
> +
> +	spin_lock(&nic_dev->coal_lock);
> +
> +	intr_coal = &nic_dev->intr_coalesce[q_id];

[Severity: Medium]
If the interface is down, this early return skips updating the software
configuration struct.

Will this cause the new coalesce parameters to be silently lost instead
of being applied when the interface is later brought up?

> +
> +	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;
> +	spin_unlock(&nic_dev->coal_lock);
> +
> +	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);

[Severity: High]
Does hinic3_set_interrupt_cfg() need to be protected by change_res_mutex
here?

The DIM code path in hinic3_set_interrupt_moder() holds change_res_mutex
when performing this same hardware configuration. Since ethtool commands
and the DIM workqueue can run concurrently, could this lead to a race
condition corrupting the read-modify-write hardware state?

> +	if (err) {
> +		NL_SET_ERR_MSG_FMT_MOD(extack,
> +				       "Failed to set queue%u coalesce",
> +				       q_id);
> +		return err;
> +	}

[Severity: Medium]
If hinic3_set_interrupt_cfg() fails, the software state in intr_coal
has already been updated.

Because hinic3_set_hw_coal_param() runs this in a loop over all queues,
will a hardware configuration failure leave the software state permanently
diverging from the actual physical hardware state?

[ ... ]
> +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;
> +
> +	memset(coal, 0, sizeof(*coal));

[Severity: Medium]
Is it safe to memset the entire ethtool_coalesce structure here?

The core ethtool ioctl code pre-initializes the cmd field to
ETHTOOL_GCOALESCE, so zeroing the entire struct will wipe the cmd field
and break the ethtool UAPI.

> +
> +	interrupt_info = &nic_dev->intr_coalesce[0];
> +
> +	coal->use_adaptive_rx_coalesce = nic_dev->adaptive_rx_coal;
> +
> +	if (nic_dev->adaptive_rx_coal) {
> +		coal->rx_max_coalesced_frames_low =
> +			interrupt_info->rx_pending_limit_low *
> +			COALESCE_PENDING_LIMIT_UNIT;
> +		coal->rx_max_coalesced_frames_high =
> +			interrupt_info->rx_pending_limit_high *
> +			COALESCE_PENDING_LIMIT_UNIT;
> +	} else {
> +		/* 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;
> +	}

[Severity: High]
When adaptive mode is active, the static coalesce fields are left as 0,
and vice versa.

During an ethtool read-modify-write cycle, will the intermediate 0 values
be passed back to hinic3_set_coalesce() and unconditionally overwrite
the driver state, wiping out the inactive configuration?

[ ... ]
> +static int hinic3_set_coalesce(struct net_device *netdev,
> +			       struct ethtool_coalesce *coal,
> +			       struct kernel_ethtool_coalesce *kernel_coal,
> +			       struct netlink_ext_ack *extack)
> +{
[ ... ]
> +	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;

[Severity: High]
When disabling adaptive RX coalesce, is it necessary to cancel any
pending DIM work items before programming the hardware?

If a rxq->dim.work item is already executing concurrently or scheduled,
could it fire after the ethtool command completes and overwrite these newly
configured static limits with stale dynamically calculated values?

> +
> +	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))
> +		NL_SET_ERR_MSG_MOD(extack, "Coalesce will be disabled");
> +
> +	return hinic3_set_hw_coal_param(netdev, &intr_coal, extack);
> +}