Re: [PATCH 04/15] net: rnpgbe: Add get_capability mbx_fw ops support

On Fri, Jul 04, 2025 at 08:25:12PM +0200, Andrew Lunn wrote:

quoted

+/**
+ * mucse_fw_send_cmd_wait - Send cmd req and wait for response
+ * @hw: Pointer to the HW structure
+ * @req: Pointer to the cmd req structure
+ * @reply: Pointer to the fw reply structure
+ *
+ * mucse_fw_send_cmd_wait sends req to pf-cm3 mailbox and wait
+ * reply from fw.
+ *
+ * Returns 0 on success, negative on failure
+ **/
+static int mucse_fw_send_cmd_wait(struct mucse_hw *hw,
+				  struct mbx_fw_cmd_req *req,
+				  struct mbx_fw_cmd_reply *reply)
+{
+	int err;
+	int retry_cnt = 3;
+
+	if (!hw || !req || !reply || !hw->mbx.ops.read_posted)

Can this happen?

If this is not supposed to happen, it is better the driver opps, so
you get a stack trace and find where the driver is broken.

Yes, it is not supposed to happen. So, you means I should remove this
check in order to get opps when this condition happen?

quoted

+		return -EINVAL;
+
+	/* if pcie off, nothing todo */
+	if (pci_channel_offline(hw->pdev))
+		return -EIO;

What can cause it to go offline? Is this to do with PCIe hotplug?

Yes, I try to get a PCIe hotplug condition by 'pci_channel_offline'.
If that happens, driver should never do bar-read/bar-write, so return
here.

quoted

+
+	if (mutex_lock_interruptible(&hw->mbx.lock))
+		return -EAGAIN;

mutex_lock_interruptable() returns -EINTR, which is what you should
return, not -EAGAIN.

Got it, I should return '-EINTR' here.

quoted

+
+	err = hw->mbx.ops.write_posted(hw, (u32 *)req,
+				       L_WD(req->datalen + MBX_REQ_HDR_LEN),
+				       MBX_FW);
+	if (err) {
+		mutex_unlock(&hw->mbx.lock);
+		return err;
+	}
+
+retry:
+	retry_cnt--;
+	if (retry_cnt < 0)
+		return -EIO;
+
+	err = hw->mbx.ops.read_posted(hw, (u32 *)reply,
+				      L_WD(sizeof(*reply)),
+				      MBX_FW);
+	if (err) {
+		mutex_unlock(&hw->mbx.lock);
+		return err;
+	}
+
+	if (reply->opcode != req->opcode)
+		goto retry;
+
+	mutex_unlock(&hw->mbx.lock);
+
+	if (reply->error_code)
+		return -reply->error_code;

The mbox is using linux error codes? 

It is used only between driver and fw, yay be just samply like this: 
0     -- no error
not 0 -- error
So, it is not using linux error codes.

quoted

+#define FLAGS_DD BIT(0) /* driver clear 0, FW must set 1 */
+/* driver clear 0, FW must set only if it reporting an error */
+#define FLAGS_ERR BIT(2)
+
+/* req is little endian. bigendian should be conserened */
+struct mbx_fw_cmd_req {
+	u16 flags; /* 0-1 */
+	u16 opcode; /* 2-3 enum GENERIC_CMD */
+	u16 datalen; /* 4-5 */
+	u16 ret_value; /* 6-7 */

If this is little endian, please use __le16, __le32 etc, so that the
static analysers will tell you if you are missing cpu_to_le32 etc.

	Andrew

Got it, I will fix it.

Thanks for your feedback.