linux-kernel - Re: [PATCH net-next v13 5/5] net: rnpgbe: Add register

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: <CB1199BE018DD3DE+20250924025058.GB292859@nic-Precision-5820-Tower>
Date: Wed, 24 Sep 2025 10:50:58 +0800
From: Yibo Dong <dong100@...se.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
	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,
	vadim.fedorenko@...ux.dev, 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 v13 5/5] net: rnpgbe: Add register_netdev

Hi, Jakub:

On Tue, Sep 23, 2025 at 06:16:39PM -0700, Jakub Kicinski wrote:
> On Mon, 22 Sep 2025 09:41:11 +0800 Dong Yibo wrote:
> > +static const struct mucse_hw_operations rnpgbe_hw_ops = {
> > +	.reset_hw = rnpgbe_reset,
> > +	.get_perm_mac = rnpgbe_get_permanent_mac,
> > +	.mbx_send_notify = rnpgbe_mbx_send_notify,
> 
> Please don't add abstraction layers, you only have one set of ops right
> now call them directly. The abstractions layers make the code harder to
> follow.
> 

Ok, remove abstraction layers in this series. I will add abstraction
layers when adding more sets of ops.

> > +static netdev_tx_t rnpgbe_xmit_frame(struct sk_buff *skb,
> > +				     struct net_device *netdev)
> > +{
> > +	dev_kfree_skb_any(skb);
> > +	netdev->stats.tx_dropped++;
> 
> Please add your own stats, the stats in struct net_device
> are deprecated and should not be used by new drivers.
> 

Got it, I will fix this.

> >  	err = rnpgbe_init_hw(hw, board_type);
> >  	if (err) {
> >  		dev_err(&pdev->dev, "Init hw err %d\n", err);
> >  		goto err_free_net;
> >  	}
> > +	/* Step 1: Send power-up notification to firmware (no response expected)
> > +	 * This informs firmware to initialize hardware power state, but
> > +	 * firmware only acknowledges receipt without returning data. Must be
> > +	 * done before synchronization as firmware may be in low-power idle
> > +	 * state initially.
> > +	 */
> > +	err = hw->ops->mbx_send_notify(hw, true, mucse_fw_powerup);
> > +	if (err) {
> 
> Don't you have to power it down on errors later in this function?

I will add an lable err_powerdown to handle errors later in this function.

err_powerdown:
         /* notify powerdown only powerup ok */
         if (!err_notify) {
                 err_notify = rnpgbe_send_notify(hw, false, mucse_fw_powerup);
                 if (err_notify)
                         dev_warn(&pdev->dev, "Send powerdown to hw failed %d\n",
                                  err_notify);
         }

> -- 
> pw-bot: cr
> 

thanks for your feedback.