linux-kernel - Re: [PATCH net-next v12 4/5] net: rnpgbe: Add basic mbx

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: <3E609187B0C91716+20250919011733.GA174153@nic-Precision-5820-Tower>
Date: Fri, 19 Sep 2025 09:17:33 +0800
From: Yibo Dong <dong100@...se.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Cc: andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, horms@...nel.org,
	corbet@....net, gur.stavi@...wei.com, maddy@...ux.ibm.com,
	mpe@...erman.id.au, danishanwar@...com, lee@...ger.us,
	gongfan1@...wei.com, lorenzo@...nel.org, geert+renesas@...der.be,
	Parthiban.Veerasooran@...rochip.com, lukas.bulwahn@...hat.com,
	alexanderduyck@...com, richardcochran@...il.com, kees@...nel.org,
	gustavoars@...nel.org, rdunlap@...radead.org, joerg@...so.de,
	netdev@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH net-next v12 4/5] net: rnpgbe: Add basic mbx_fw support

On Wed, Sep 17, 2025 at 03:02:13PM +0100, Vadim Fedorenko wrote:
> On 17/09/2025 12:05, Yibo Dong wrote:
> > On Wed, Sep 17, 2025 at 11:45:31AM +0100, Vadim Fedorenko wrote:
> > > On 16/09/2025 12:29, Dong Yibo wrote:
> > > > Add fundamental firmware (FW) communication operations via PF-FW
> > > > mailbox, including:
> > > > - FW sync (via HW info query with retries)
> > > > - HW reset (post FW command to reset hardware)
> > > > - MAC address retrieval (request FW for port-specific MAC)
> > > > - Power management (powerup/powerdown notification to FW)
> > > > 
> > > > Signed-off-by: Dong Yibo <dong100@...se.com>
> > > 
> > > Reviewed-by: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
> > > 
> > > small nits below
> > > 
> > > 
> > > > +static void build_get_hw_info_req(struct mbx_fw_cmd_req *req)
> > > > +{
> > > > +	req->flags = 0;
> > > > +	req->opcode = cpu_to_le16(GET_HW_INFO);
> > > > +	req->datalen = cpu_to_le16(MUCSE_MBX_REQ_HDR_LEN);
> > > > +	req->reply_lo = 0;
> > > > +	req->reply_hi = 0;
> > > > +}
> > > 
> > > All these build*() functions re-init flags and reply to 0, but all
> > > mbx_fw_cmd_req are zero-inited on the stack. Might be better clean
> > > things assignments, but no strong opinion because the code is explicit
> > > 
> > > If you will think of refactoring this part, it might be a good idea to
> > > avoid build*() functions at all and do proper initialization of
> > > mbx_fw_cmd_req in callers?
> > > 
> > > > +
> > > > +/**
> > > > + * mucse_mbx_get_info - Get hw info from fw
> > > > + * @hw: pointer to the HW structure
> > > > + *
> > > > + * mucse_mbx_get_info tries to get hw info from hw.
> > > > + *
> > > > + * Return: 0 on success, negative errno on failure
> > > > + **/
> > > > +static int mucse_mbx_get_info(struct mucse_hw *hw)
> > > > +{
> > > > +	struct mbx_fw_cmd_reply reply = {};
> > > > +	struct mbx_fw_cmd_req req = {};
> > > 
> > > something like:
> > > 
> > > struct mbx_fw_cmd_req req =
> > > 	{
> > > 	  .opcode = cpu_to_le16(GET_HW_INFO),
> > > 	  .datalen = cpu_to_le16(MUCSE_MBX_REQ_HDR_LEN),
> > > 	}
> > > 
> > > 
> > > 
> > 
> > That's a good idea! That makes the code more compact.
> > I think I should update this as your suggestion.
> > 
> > Regarding adding your "Reviewed-by" tag in the next version:
> > Would it be acceptable to include it when I submit the updated patch (with
> > the initialization logic adjusted), or should I wait for your further
> > review of the modified code first?
> 
> If you will submit another version with this refactoring, I'll better do
> another review.
> 

I see, I will submit another version later, with this refactoring.
Looking forward to your next review.

Thanks for your feedback.