Re: [PATCH net-next v04 1/6] hinic3: Add ethtool queue ops
From: Simon Horman
Date: Fri Apr 10 2026 - 16:13:28 EST
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 queue ops
This commit implements .get_ringparam and .set_ringparam ethtool callbacks,
allowing users to configure and monitor queue depths.
> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c b/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
> index 90fc16288de9..e47c3f43e7b9 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
[ ... ]
> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_irq.c b/drivers/net/ethernet/huawei/hinic3/hinic3_irq.c
> index e7d6c2033b45..d3b3927b5408 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_irq.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_irq.c
[ ... ]
> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_main.c b/drivers/net/ethernet/huawei/hinic3/hinic3_main.c
> index 0a888fe4c975..3b470978714a 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_main.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_main.c
[ ... ]
> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c b/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
> index da73811641a9..cec501a9dd43 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
> @@ -428,6 +428,84 @@ static void hinic3_vport_down(struct net_device *netdev)
> }
> }
>
> +int
> +hinic3_change_channel_settings(struct net_device *netdev,
> + struct hinic3_dyna_txrxq_params *trxq_params)
> +{
> + struct hinic3_nic_dev *nic_dev = netdev_priv(netdev);
> + struct hinic3_dyna_txrxq_params old_qp_params = {};
> + struct hinic3_dyna_qp_params new_qp_params = {};
> + struct hinic3_dyna_qp_params cur_qp_params = {};
> + bool need_teardown = false;
> + unsigned long flags;
> + int err;
> +
> + mutex_lock(&nic_dev->channel_cfg_lock);
> +
> + hinic3_config_num_qps(netdev, trxq_params);
> +
> + err = hinic3_alloc_channel_resources(netdev, &new_qp_params,
> + trxq_params);
> + if (err) {
> + netdev_err(netdev, "Failed to alloc channel resources\n");
> + mutex_unlock(&nic_dev->channel_cfg_lock);
> + return err;
> + }
> +
> + spin_lock_irqsave(&nic_dev->channel_res_lock, flags);
> + if (!test_and_set_bit(HINIC3_CHANGE_RES_INVALID, &nic_dev->flags))
> + need_teardown = true;
> + spin_unlock_irqrestore(&nic_dev->channel_res_lock, flags);
> +
> + if (need_teardown) {
> + hinic3_vport_down(netdev);
> + hinic3_close_channel(netdev);
> + hinic3_uninit_qps(nic_dev, &cur_qp_params);
> + hinic3_free_channel_resources(netdev, &cur_qp_params,
> + &nic_dev->q_params);
> + }
> +
> + if (nic_dev->num_qp_irq > trxq_params->num_qps)
> + hinic3_qp_irq_change(netdev, trxq_params->num_qps);
> +
> + spin_lock_irqsave(&nic_dev->channel_res_lock, flags);
> + old_qp_params = nic_dev->q_params;
> + nic_dev->q_params = *trxq_params;
> + spin_unlock_irqrestore(&nic_dev->channel_res_lock, flags);
> +
> + hinic3_init_qps(nic_dev, &new_qp_params);
> +
> + err = hinic3_open_channel(netdev);
> + if (err)
> + goto err_uninit_qps;
> +
> + err = hinic3_vport_up(netdev);
> + if (err)
> + goto err_close_channel;
> +
> + spin_lock_irqsave(&nic_dev->channel_res_lock, flags);
> + clear_bit(HINIC3_CHANGE_RES_INVALID, &nic_dev->flags);
> + spin_unlock_irqrestore(&nic_dev->channel_res_lock, flags);
> +
> + mutex_unlock(&nic_dev->channel_cfg_lock);
> +
> + return 0;
> +
> +err_close_channel:
> + hinic3_close_channel(netdev);
> +err_uninit_qps:
> + spin_lock_irqsave(&nic_dev->channel_res_lock, flags);
> + nic_dev->q_params = old_qp_params;
> + spin_unlock_irqrestore(&nic_dev->channel_res_lock, flags);
> +
> + hinic3_uninit_qps(nic_dev, &new_qp_params);
> + hinic3_free_channel_resources(netdev, &new_qp_params, trxq_params);
> +
> + mutex_unlock(&nic_dev->channel_cfg_lock);
> +
> + return err;
> +}
Does the error path properly clear the HINIC3_CHANGE_RES_INVALID flag?
On the success path, clear_bit(HINIC3_CHANGE_RES_INVALID) is called under
channel_res_lock. However, both error paths (err_close_channel and
err_uninit_qps) skip this, leaving the flag permanently set.
After a failed ring parameter change, the device enters an unrecoverable
state:
1. HINIC3_INTF_UP remains set, HINIC3_CHANGE_RES_INVALID remains set.
2. User does ifdown: hinic3_close() clears HINIC3_INTF_UP but skips
teardown since HINIC3_CHANGE_RES_INVALID is already set:
hinic3_close() {
...
spin_lock_irqsave(&nic_dev->channel_res_lock, flags);
if (!test_and_set_bit(HINIC3_CHANGE_RES_INVALID, &nic_dev->flags))
need_teardown = true; // Already set, need_teardown = false
spin_unlock_irqrestore(&nic_dev->channel_res_lock, flags);
if (need_teardown) {
// Skipped
}
}
3. User does ifup: hinic3_open() allocates fresh resources and sets
HINIC3_INTF_UP, but never clears HINIC3_CHANGE_RES_INVALID.
4. HINIC3_CHANNEL_RES_VALID() now returns false permanently:
#define HINIC3_CHANNEL_RES_VALID(nic_dev) \
(test_bit(HINIC3_INTF_UP, &(nic_dev)->flags) && \
!test_bit(HINIC3_CHANGE_RES_INVALID, &(nic_dev)->flags))
5. hinic3_link_status_change() in hinic3_main.c silently returns, so the
link never comes up:
static void hinic3_link_status_change(struct net_device *netdev,
bool link_status_up)
{
...
spin_lock_irqsave(&nic_dev->channel_res_lock, flags);
valid = HINIC3_CHANNEL_RES_VALID(nic_dev);
spin_unlock_irqrestore(&nic_dev->channel_res_lock, flags);
if (!valid)
return; // Always returns here
...
}
6. hinic3_set_interrupt_moder() in hinic3_irq.c returns 0 without
configuring interrupts:
static int hinic3_set_interrupt_moder(struct net_device *netdev, ...)
{
...
spin_lock_irqsave(&nic_dev->channel_res_lock, flags);
if (!HINIC3_CHANNEL_RES_VALID(nic_dev) ||
q_id >= nic_dev->q_params.num_qps) {
spin_unlock_irqrestore(&nic_dev->channel_res_lock, flags);
return 0; // Always returns here
}
...
}
The device appears up but is permanently non-functional until module reload.
Should the error paths clear HINIC3_CHANGE_RES_INVALID under
channel_res_lock before returning?
> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_nic_dev.h b/drivers/net/ethernet/huawei/hinic3/hinic3_nic_dev.h
> index 9502293ff710..55b280888ad8 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_nic_dev.h
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_nic_dev.h
[ ... ]
> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_nic_io.h b/drivers/net/ethernet/huawei/hinic3/hinic3_nic_io.h
> index 12eefabcf1db..3791b9bc865b 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_nic_io.h
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_nic_io.h
[ ... ]