Re: [PATCH net-next v10 5/5] net: rnpgbe: Add register_netdev

On Thu, Sep 04, 2025 at 11:06:21AM +0800, Yibo Dong wrote:

On Thu, Sep 04, 2025 at 12:53:27AM +0200, Andrew Lunn wrote:

quoted

quoted

  * rnpgbe_add_adapter - Add netdev for this pci_dev
  * @pdev: PCI device information structure

@@ -78,6 +129,38 @@ static int rnpgbe_add_adapter(struct pci_dev *pdev,
 
 	hw->hw_addr = hw_addr;
 	info->init(hw);
+	mucse_init_mbx_params_pf(hw);
+	err = hw->ops->echo_fw_status(hw, true, mucse_fw_powerup);
+	if (err) {
+		dev_warn(&pdev->dev, "Send powerup to hw failed %d\n", err);
+		dev_warn(&pdev->dev, "Maybe low performance\n");
+	}
+
+	err = mucse_mbx_sync_fw(hw);
+	if (err) {
+		dev_err(&pdev->dev, "Sync fw failed! %d\n", err);
+		goto err_free_net;
+	}

The order here seems odd. Don't you want to synchronise the mbox
before you power up? If your are out of sync, the power up could fail,
and you keep in lower power mode? 

As I explained before, powerup sends mbx and wait fw read out, but
without response data from fw. mucse_mbx_sync_fw sends mbx and wait for
the corect response from fw, after mucse_mbx_sync_fw, driver->fw
request and fw->driver response will be both ok.

Because this is logically the wrong order, this deserves a comment.

You choice of function names for the lower level functions also does
not help. It is not so easy to look at the function used to know if it
is a request/response to the firmware, or just a request without a
response.

	Andrew