Re: [PATCH net-next v09 1/5] 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.
Full review at: https://sashiko.dev
---
hinic3: Add ethtool queue ops

This commit implements ethtool callback functions .get_ringparam and
.set_ringparam. These callbacks allow users to utilize ethtool for detailed
queue depth configuration and monitoring.

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c b/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
> index 90fc16288de95..be9992a235f7b 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
[ ... ]
> @@ -409,6 +410,96 @@ hinic3_get_link_ksettings(struct net_device *netdev,
>  	return 0;
>  }
>  
> +static void hinic3_get_ringparam(struct net_device *netdev,
> +				 struct ethtool_ringparam *ring,
> +				 struct kernel_ethtool_ringparam *kernel_ring,
> +				 struct netlink_ext_ack *extack)
> +{
> +	struct hinic3_nic_dev *nic_dev = netdev_priv(netdev);
> +
> +	ring->rx_max_pending = HINIC3_MAX_RX_QUEUE_DEPTH;
> +	ring->tx_max_pending = HINIC3_MAX_TX_QUEUE_DEPTH;
> +	ring->rx_pending = nic_dev->q_params.rq_depth;
> +	ring->rx_pending = nic_dev->q_params.sq_depth;

[Severity: High]
Does this code accidentally assign rx_pending twice and omit tx_pending?
Querying the interface will return 0 for the Tx queue depth and an
incorrect value for the Rx queue depth, which also breaks partial
ring updates.

> +}
> +
[ ... ]
> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_irq.c b/drivers/net/ethernet/huawei/hinic3/hinic3_irq.c
> index e7d6c2033b451..bc4d879f9be4e 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_irq.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_irq.c
> @@ -137,7 +137,8 @@ static int hinic3_set_interrupt_moder(struct net_device *netdev, u16 q_id,
>  	struct hinic3_interrupt_info info = {};
>  	int err;
>  
> -	if (q_id >= nic_dev->q_params.num_qps)
> +	if (q_id >= nic_dev->q_params.num_qps ||
> +	    !mutex_trylock(&nic_dev->change_res_mutex))
>  		return 0;

[Severity: High]
Is it safe to silently return 0 (success) without applying the interrupt
moderation if the mutex is contended? This appears to drop hardware
configuration updates without informing the caller.

>  
>  	info.interrupt_coalesc_set = 1;
[ ... ]
> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_main.c b/drivers/net/ethernet/huawei/hinic3/hinic3_main.c
> index 0a888fe4c975b..c87624a5e5dcd 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_main.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_main.c
[ ... ]
> @@ -315,6 +316,9 @@ static void hinic3_link_status_change(struct net_device *netdev,
>  {
>  	struct hinic3_nic_dev *nic_dev = netdev_priv(netdev);
>  
> +	if (!mutex_trylock(&nic_dev->change_res_mutex))
> +		return;
> +

[Severity: High]
Can this usage of mutex_trylock cause physical link status events to be
discarded? If the lock is held (e.g., during an ethtool ring size change),
the OS link state could permanently diverge from the physical link state.

>  	if (link_status_up) {
>  		if (netif_carrier_ok(netdev))
>  			return;

[Severity: Critical]
Does this early return path leak the mutex? We acquired change_res_mutex
earlier, but it looks like we exit without unlocking it, which would
cause subsequent deadlocks.

> @@ -330,6 +334,8 @@ static void hinic3_link_status_change(struct net_device *netdev,
>  		netif_carrier_off(netdev);
>  		netdev_dbg(netdev, "Link is down\n");
>  	}
> +
> +	mutex_unlock(&nic_dev->change_res_mutex);
>  }

[Severity: Critical]
There is another early return just above this code:
        if (!netif_carrier_ok(netdev))
                return;
Similarly, does this early return also forget to call mutex_unlock()?

>  
>  static void hinic3_port_module_event_handler(struct net_device *netdev,
> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c b/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
> index da73811641a9d..047214cfc7532 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
[ ... ]
> @@ -428,6 +439,74 @@ 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)
> +{
[ ... ]
> +	err = hinic3_vport_up(netdev);
> +	if (err)
> +		goto err_close_channel;
> +
> +	hinic3_free_channel_resources(netdev, &cur_qp_params, &cur_trxq_params);
> +
> +	mutex_unlock(&nic_dev->change_res_mutex);
> +
> +	return 0;
> +
> +err_close_channel:
> +	hinic3_close_channel(netdev);
> +err_qp_irq_reset:
> +	nic_dev->q_params = cur_trxq_params;
> +
> +	if (trxq_params->num_qps > cur_trxq_params.num_qps)
> +		hinic3_qp_irq_change(netdev, cur_trxq_params.num_qps);
> +	hinic3_free_qp_ctxts(nic_dev);

[Severity: High]
Is hinic3_free_qp_ctxts() called twice sequentially in this error path?
The err_close_channel block calls hinic3_close_channel(), which internally
invokes hinic3_free_qp_ctxts(). The flow then falls through to
err_qp_irq_reset which explicitly calls hinic3_free_qp_ctxts() again.

> +err_uninit_qps:
> +	hinic3_get_cur_qps(nic_dev, &new_qp_params);
> +	hinic3_free_channel_resources(netdev, &new_qp_params, trxq_params);
> +	hinic3_free_channel_resources(netdev, &cur_qp_params, &cur_trxq_params);

[Severity: Critical]
Does this error path destroy both the new and the old working channel
resources? By calling hinic3_free_channel_resources() on cur_qp_params,
we leave dangling pointers in nic_dev->q_params and nic_io. It seems
subsequent packet transmissions would trigger a use-after-free, and
closing the interface later would trigger a double-free.

> +	mutex_unlock(&nic_dev->change_res_mutex);
> +
> +	return err;
> +}
> +
>  static int hinic3_open(struct net_device *netdev)
>  {
>  	struct hinic3_nic_dev *nic_dev = netdev_priv(netdev);
> @@ -458,6 +537,10 @@ static int hinic3_open(struct net_device *netdev)
>  
>  	hinic3_init_qps(nic_dev, &qp_params);
>  
> +	err = hinic3_prepare_channel(netdev, &nic_dev->q_params);
> +	if (err)
> +		goto err_uninit_qps;
> +
>  	err = hinic3_open_channel(netdev);
>  	if (err)
>  		goto err_uninit_qps;

[Severity: High]
Does jumping to err_uninit_qps leak the QP contexts allocated by
hinic3_prepare_channel()?

Since hinic3_prepare_channel() calls hinic3_init_qp_ctxts(), it looks like
we bypass the cleanup that happens inside hinic3_close_channel(), which
would permanently leak the root context configuration and associated DMA
memory.