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: <20250703004433.11160-1-gongfan1@huawei.com>
Date: Thu, 3 Jul 2025 08:44:33 +0800
From: Fan Gong <gongfan1@...wei.com>
To: <pabeni@...hat.com>
CC: <andrew+netdev@...n.ch>, <christophe.jaillet@...adoo.fr>,
	<corbet@....net>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<gongfan1@...wei.com>, <guoxin09@...wei.com>, <gur.stavi@...wei.com>,
	<helgaas@...nel.org>, <horms@...nel.org>, <jdamato@...tly.com>,
	<kuba@...nel.org>, <lee@...ger.us>, <linux-doc@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <luosifu@...wei.com>,
	<meny.yossefi@...wei.com>, <mpe@...erman.id.au>, <netdev@...r.kernel.org>,
	<przemyslaw.kitszel@...el.com>, <shenchenyang1@...ilicon.com>,
	<shijing34@...wei.com>, <sumang@...vell.com>, <vadim.fedorenko@...ux.dev>,
	<wulike1@...wei.com>, <zhoushuai28@...wei.com>, <zhuyikai1@...artners.com>
Subject: Re: [PATCH net-next v06 4/8] hinic3: Command Queue interfaces

> > +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').

Thanks for your reviewing. The comment is right and using read barrier here
is wrong. I will correct smp_rmb to smp_wmb in next version.

> > +	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?

"spin_unlock_bh(&cmdq->cmdq_lock)" is executed before the errcode assignment.
So the errcode is updated out of the spinlock protection and we need this barrier.