Re: [PATCH net-next v04 1/6] hinic3: Add ethtool queue 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.

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

[ ... ]