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:

quoted

+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.

quoted

+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.

quoted

 	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.