Thanks for your reviewing.
> > +static int send_mbox_msg(struct hinic3_mbox *mbox, u8 mod, u16 cmd,
> > + const void *msg, u32 msg_len, u16 dst_func,
> > + enum mbox_msg_direction_type direction,
> > + enum mbox_msg_ack_type ack_type,
> > + struct mbox_msg_info *msg_info)
> > +{
> > + enum mbox_msg_data_type data_type = MBOX_MSG_DATA_INLINE;
> > + struct hinic3_hwdev *hwdev = mbox->hwdev;
> > + struct mbox_dma_msg dma_msg;
> > + u32 seg_len = MBOX_SEG_LEN;
> > + u64 header = 0;
> > + u32 seq_id = 0;
> > + u16 rsp_aeq_id;
> > + u8 *msg_seg;
> > + int err = 0;
> > + u32 left;
> > +
> > + if (hwdev->hwif->attr.num_aeqs > MBOX_MSG_AEQ_FOR_MBOX)
> > + rsp_aeq_id = MBOX_MSG_AEQ_FOR_MBOX;
> > + else
> > + rsp_aeq_id = 0;
> > +
> > + mutex_lock(&mbox->msg_send_lock);
>
> this function is always called under mbox->mbox_send_lock, why do you
> need another mutex? From the experience, a double-locking schema usually
> brings more troubles than benefits...
In the current patch, send_mbox_msg is only used in mbox sending process.
But send_mbox_msg will be used in other functions like mbox response in the
future patch, so msg_send_lock is necessary to cover the remaining scenes.
> > int hinic3_send_mbox_to_mgmt(struct hinic3_hwdev *hwdev, u8 mod, u16 cmd,
> > const struct mgmt_msg_params *msg_params)
> > {
> > - /* Completed by later submission due to LoC limit. */
> > - return -EFAULT;
> > + struct hinic3_mbox *mbox = hwdev->mbox;
> > + struct mbox_msg_info msg_info = {};
> > + struct hinic3_msg_desc *msg_desc;
> > + int err;
> > +
> > + /* expect response message */
> > + msg_desc = get_mbox_msg_desc(mbox, MBOX_MSG_RESP, MBOX_MGMT_FUNC_ID);
> > + mutex_lock(&mbox->mbox_send_lock);
> > + msg_info.msg_id = (msg_info.msg_id + 1) & 0xF;
>
> msg_id is constant 1 here as msg_info is initialized to all zeroes a
> couple of lines above. It looks like a mistake to me and
> mbox->send_msg_id should be used instead.
This is our mistake. We will fix this error in the next version's patch.
