Re: [PATCH net-next v07 7/8] hinic3: Mailbox management interfaces

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

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.