netdev - 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250709083233.27344-1-gongfan1@huawei.com>
Date: Wed, 9 Jul 2025 16:32:33 +0800
From: Fan Gong <gongfan1@...wei.com>
To: <vadim.fedorenko@...ux.dev>
CC: <andrew+netdev@...n.ch>, <christophe.jaillet@...adoo.fr>,
	<corbet@....net>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<gongfan1@...wei.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

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.