linux-kernel - Re: [PATCH net-next v07 7/8] hinic3: Mailbox management 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] [day] [month] [year] [list]

Message-ID: <b6b78ea8-4235-4fd5-ab19-133bc04e4188@linux.dev>
Date: Wed, 9 Jul 2025 14:15:27 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Fan Gong <gongfan1@...wei.com>
Cc: andrew+netdev@...n.ch, christophe.jaillet@...adoo.fr, corbet@....net,
 davem@...emloft.net, edumazet@...gle.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,
 pabeni@...hat.com, przemyslaw.kitszel@...el.com,
 shenchenyang1@...ilicon.com, shijing34@...wei.com, sumang@...vell.com,
 wulike1@...wei.com, zhoushuai28@...wei.com, zhuyikai1@...artners.com
Subject: Re: [PATCH net-next v07 7/8] hinic3: Mailbox management interfaces

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.