Re: [PATCH net-next v04 3/5] hinic3: Add NIC configuration ops

[Simon Horman <horms@kernel.org>]

On Wed, Oct 29, 2025 at 02:16:27PM +0800, Fan Gong wrote:

...

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_hw_comm.c b/drivers/net/ethernet/huawei/hinic3/hinic3_hw_comm.c
> index 09dae2ef610c..0efb5a843964 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_hw_comm.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_hw_comm.c
> @@ -9,6 +9,36 @@
>  #include "hinic3_hwif.h"
>  #include "hinic3_mbox.h"
>  
> +static int hinic3_get_interrupt_cfg(struct hinic3_hwdev *hwdev,
> +				    struct hinic3_interrupt_info *info)
> +{
> +	struct comm_cmd_cfg_msix_ctrl_reg msix_cfg = {};
> +	struct mgmt_msg_params msg_params = {};
> +	int err;
> +
> +	msix_cfg.func_id = hinic3_global_func_id(hwdev);
> +	msix_cfg.msix_index = info->msix_index;
> +	msix_cfg.opcode = MGMT_MSG_CMD_OP_GET;
> +
> +	mgmt_msg_params_init_default(&msg_params, &msix_cfg, sizeof(msix_cfg));
> +
> +	err = hinic3_send_mbox_to_mgmt(hwdev, MGMT_MOD_COMM,
> +				       COMM_CMD_CFG_MSIX_CTRL_REG, &msg_params);
> +	if (err || msix_cfg.head.status) {
> +		dev_err(hwdev->dev, "Failed to get interrupt config, err: %d, status: 0x%x\n",
> +			err, msix_cfg.head.status);
> +		return -EFAULT;
> +	}
> +
> +	info->lli_credit_limit = msix_cfg.lli_credit_cnt;
> +	info->lli_timer_cfg = msix_cfg.lli_timer_cnt;
> +	info->pending_limit = msix_cfg.pending_cnt;
> +	info->coalesc_timer_cfg = msix_cfg.coalesce_timer_cnt;
> +	info->resend_timer_cfg = msix_cfg.resend_timer_cnt;
> +
> +	return 0;
> +}
> +
>  int hinic3_set_interrupt_cfg_direct(struct hinic3_hwdev *hwdev,
>  				    const struct hinic3_interrupt_info *info)
>  {
> @@ -40,6 +70,30 @@ int hinic3_set_interrupt_cfg_direct(struct hinic3_hwdev *hwdev,
>  	return 0;
>  }
>  
> +int hinic3_set_interrupt_cfg(struct hinic3_hwdev *hwdev,
> +			     struct hinic3_interrupt_info info)
> +{
> +	struct hinic3_interrupt_info temp_info;
> +	int err;
> +
> +	temp_info.msix_index = info.msix_index;
> +
> +	err = hinic3_get_interrupt_cfg(hwdev, &temp_info);
> +	if (err)
> +		return -EINVAL;

Maybe I am missing something. It seems to me thaat this error value will
propagate up to be the return value of the probe value. And it seems to me
that would be a bit more intuitive, and possibly lead to a better user
experience, if the return value of hinic3_get_interrupt_cfg() was
propagated here. And, in turn, if hinic3_get_interrupt_cfg() propagated the
return value of hinic3_send_mbox_to_mgmt(). These values differ from
-EINVAL.

> +
> +	info.lli_credit_limit = temp_info.lli_credit_limit;
> +	info.lli_timer_cfg = temp_info.lli_timer_cfg;
> +
> +	if (!info.interrupt_coalesc_set) {
> +		info.pending_limit = temp_info.pending_limit;
> +		info.coalesc_timer_cfg = temp_info.coalesc_timer_cfg;
> +		info.resend_timer_cfg = temp_info.resend_timer_cfg;
> +	}
> +
> +	return hinic3_set_interrupt_cfg_direct(hwdev, &info);
> +}

...