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

[Jakub Kicinski]

On Sat, 8 Nov 2025 14:41:38 +0800 Fan Gong wrote:
> Add ops to configure NIC feature(lro, vlan, csum...).

This patch really does too many things at once, please split it.
You are allowed to have up to 15 patches in a series, having a list 
of things in a commit message is a strong indication that the patch 
is doing too many things at once.

> +static void hinic3_auto_moderation_work(struct work_struct *work)
> +{
> +	u64 rx_packets, rx_bytes, rx_pkt_diff, rx_rate, avg_pkt_size;
> +	u64 tx_packets, tx_bytes, tx_pkt_diff, tx_rate;
> +	struct hinic3_nic_dev *nic_dev;
> +	struct delayed_work *delay;
> +	struct net_device *netdev;
> +	unsigned long period;
> +	u16 qid;

Please use the DIM infrastructure kernel already has.
If it's not good enough - improve it.

> +static void hinic3_tx_timeout(struct net_device *netdev, unsigned int txqueue)
> +{
> +	struct hinic3_nic_dev *nic_dev = netdev_priv(netdev);
> +	struct hinic3_io_queue *sq;
> +	bool hw_err = false;
> +	u16 sw_pi, hw_ci;
> +	u8 q_id;
> +
> +	HINIC3_NIC_STATS_INC(nic_dev, netdev_tx_timeout);

kernel already has a counter for this

> +	netdev_err(netdev, "Tx timeout\n");

and prints an error

Please don't duplicate.

> +	for (q_id = 0; q_id < nic_dev->q_params.num_qps; q_id++) {
> +		if (!netif_xmit_stopped(netdev_get_tx_queue(netdev, q_id)))
> +			continue;

Why are you scanning the queues? kernel passes @txqueue to tell you
which queue has stalled.

> +		sq = nic_dev->txqs[q_id].sq;
> +		sw_pi = hinic3_get_sq_local_pi(sq);
> +		hw_ci = hinic3_get_sq_hw_ci(sq);
> +		netdev_dbg(netdev,
> +			   "txq%u: sw_pi: %u, hw_ci: %u, sw_ci: %u, napi->state: 0x%lx.\n",
> +			   q_id, sw_pi, hw_ci, hinic3_get_sq_local_ci(sq),
> +			   nic_dev->q_params.irq_cfg[q_id].napi.state);
> +
> +		if (sw_pi != hw_ci)
> +			hw_err = true;
> +	}
> +
> +	if (hw_err)
> +		set_bit(HINIC3_EVENT_WORK_TX_TIMEOUT, &nic_dev->event_flag);
> +}

> +struct hinic3_nic_stats {
> +	u64                   netdev_tx_timeout;
> +
> +	/* Subdivision statistics show in private tool */
> +	u64                   tx_carrier_off_drop;
> +	u64                   tx_invalid_qid;

I don't see these being used in the series.

> +	struct u64_stats_sync syncp;
> +};
-- 
pw-bot: cr