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