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

On Thu, Sep 04, 2025 at 02:35:06PM +0200, Andrew Lunn wrote:

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

quoted

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

Got it, I will add comment.

...
    /*
     * 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.
     */
    mucse_init_mbx_params_pf(hw);
    err = hw->ops->mbx_send_notify(hw, true, mucse_fw_powerup);
    if (err) {
        dev_warn(&pdev->dev, "Failed to send power-up notification to firmware: %d\n", err);
        dev_warn(&pdev->dev, "Performance may be limited\n");
    }

    /*
     * Step 2: Synchronize mailbox communication with firmware (requires response)
     * After power-up, confirm firmware is ready to process requests with responses.
     * This ensures subsequent request/response interactions work reliably.
     */
    err = mucse_mbx_sync_fw(hw);
...
And lower lever function names will be consided to be renamed.

Thanks for your feedback.