Re: [PATCH net-next v12 4/5] net: rnpgbe: Add basic mbx_fw support

[Vadim Fedorenko]

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 <dong...@mucse.com>

Reviewed-by: Vadim Fedorenko <vadim.fedore...@linux.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),
        }