linux-kernel - Re: [PATCH net-next v06 4/8] hinic3: Command Queue interfaces

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f875faa2-718d-4244-bb86-2178fed55922@redhat.com>
Date: Tue, 1 Jul 2025 14:56:00 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Fan Gong <gongfan1@...wei.com>, Zhu Yikai <zhuyikai1@...artners.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Simon Horman <horms@...nel.org>,
 Andrew Lunn <andrew+netdev@...n.ch>, linux-doc@...r.kernel.org,
 Jonathan Corbet <corbet@....net>, Bjorn Helgaas <helgaas@...nel.org>,
 luosifu <luosifu@...wei.com>, Xin Guo <guoxin09@...wei.com>,
 Shen Chenyang <shenchenyang1@...ilicon.com>,
 Zhou Shuai <zhoushuai28@...wei.com>, Wu Like <wulike1@...wei.com>,
 Shi Jing <shijing34@...wei.com>, Meny Yossefi <meny.yossefi@...wei.com>,
 Gur Stavi <gur.stavi@...wei.com>, Lee Trager <lee@...ger.us>,
 Michael Ellerman <mpe@...erman.id.au>,
 Vadim Fedorenko <vadim.fedorenko@...ux.dev>, Suman Ghosh
 <sumang@...vell.com>, Przemek Kitszel <przemyslaw.kitszel@...el.com>,
 Joe Damato <jdamato@...tly.com>,
 Christophe JAILLET <christophe.jaillet@...adoo.fr>
Subject: Re: [PATCH net-next v06 4/8] hinic3: Command Queue interfaces

On 6/27/25 8:12 AM, Fan Gong wrote:
> +static void cmdq_sync_cmd_handler(struct hinic3_cmdq *cmdq,
> +				  struct cmdq_wqe *wqe, u16 ci)
> +{
> +	spin_lock(&cmdq->cmdq_lock);
> +	cmdq_update_cmd_status(cmdq, ci, wqe);
> +	if (cmdq->cmd_infos[ci].cmpt_code) {
> +		*cmdq->cmd_infos[ci].cmpt_code = CMDQ_DIRECT_SYNC_CMPT_CODE;
> +		cmdq->cmd_infos[ci].cmpt_code = NULL;
> +	}
> +
> +	/* Ensure that completion code has been updated before updating done */
> +	smp_rmb();

There is something off with the above barrier. It's not clear where is
the paired wmb() and the comment looks misleading as this barrier order
reads operation and not writes (as implied by 'updating').

+static int cmdq_sync_cmd_direct_resp(struct hinic3_cmdq *cmdq, u8 mod,
u8 cmd,
> +				     struct hinic3_cmd_buf *buf_in,
> +				     u64 *out_param)
> +{
> +	struct hinic3_cmdq_cmd_info *cmd_info, saved_cmd_info;
> +	int cmpt_code = CMDQ_SEND_CMPT_CODE;
> +	struct cmdq_wqe *curr_wqe, wqe = {};
> +	struct hinic3_wq *wq = &cmdq->wq;
> +	u16 curr_prod_idx, next_prod_idx;
> +	struct completion done;
> +	u64 curr_msg_id;
> +	int errcode;
> +	u8 wrapped;
> +	int err;
> +
> +	spin_lock_bh(&cmdq->cmdq_lock);
> +	curr_wqe = cmdq_get_wqe(wq, &curr_prod_idx);
> +	if (!curr_wqe) {
> +		spin_unlock_bh(&cmdq->cmdq_lock);
> +		return -EBUSY;
> +	}
> +
> +	wrapped = cmdq->wrapped;
> +	next_prod_idx = curr_prod_idx + CMDQ_WQE_NUM_WQEBBS;
> +	if (next_prod_idx >= wq->q_depth) {
> +		cmdq->wrapped ^= 1;
> +		next_prod_idx -= wq->q_depth;
> +	}
> +
> +	cmd_info = &cmdq->cmd_infos[curr_prod_idx];
> +	init_completion(&done);
> +	refcount_inc(&buf_in->ref_cnt);
> +	cmd_info->cmd_type = HINIC3_CMD_TYPE_DIRECT_RESP;
> +	cmd_info->done = &done;
> +	cmd_info->errcode = &errcode;
> +	cmd_info->direct_resp = out_param;
> +	cmd_info->cmpt_code = &cmpt_code;
> +	cmd_info->buf_in = buf_in;
> +	saved_cmd_info = *cmd_info;
> +	cmdq_set_lcmd_wqe(&wqe, CMDQ_CMD_DIRECT_RESP, buf_in, NULL,
> +			  wrapped, mod, cmd, curr_prod_idx);
> +
> +	cmdq_wqe_fill(curr_wqe, &wqe);
> +	(cmd_info->cmdq_msg_id)++;
> +	curr_msg_id = cmd_info->cmdq_msg_id;
> +	cmdq_set_db(cmdq, HINIC3_CMDQ_SYNC, next_prod_idx);
> +	spin_unlock_bh(&cmdq->cmdq_lock);
> +
> +	err = wait_cmdq_sync_cmd_completion(cmdq, cmd_info, &saved_cmd_info,
> +					    curr_msg_id, curr_prod_idx,
> +					    curr_wqe, CMDQ_CMD_TIMEOUT);
> +	if (err) {
> +		dev_err(cmdq->hwdev->dev,
> +			"Cmdq sync command timeout, mod: %u, cmd: %u, prod idx: 0x%x\n",
> +			mod, cmd, curr_prod_idx);
> +		err = -ETIMEDOUT;
> +	}
> +
> +	if (cmpt_code == CMDQ_FORCE_STOP_CMPT_CODE) {
> +		dev_dbg(cmdq->hwdev->dev,
> +			"Force stop cmdq cmd, mod: %u, cmd: %u\n", mod, cmd);
> +		err = -EAGAIN;
> +	}
> +
> +	smp_rmb(); /* read error code after completion */

Isn't the errcode updated under the spinlock protection? Why is this
barrier neeed?

/P