On 09/07/2025 09:32, Fan Gong wrote:
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.
I would still suggest you to implement it with one locking primitive as
it will be safer and easier to maintain in the future
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.
