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 <dong100@mucse.com>
---
 drivers/net/ethernet/mucse/rnpgbe/Makefile    |   3 +-
 drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h    |   4 +
 .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c    |   1 +
 .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c | 249 ++++++++++++++++++
 .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h | 121 +++++++++
 5 files changed, 377 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h

diff --git a/drivers/net/ethernet/mucse/rnpgbe/Makefile b/drivers/net/ethernet/mucse/rnpgbe/Makefile
index 5fc878ada4b1..de8bcb7772ab 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/Makefile
+++ b/drivers/net/ethernet/mucse/rnpgbe/Makefile
@@ -7,4 +7,5 @@
 obj-$(CONFIG_MGBE) += rnpgbe.o
 rnpgbe-objs := rnpgbe_main.o\
 	       rnpgbe_chip.o\
-	       rnpgbe_mbx.o
+	       rnpgbe_mbx.o\
+	       rnpgbe_mbx_fw.o
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
index ac28502a8860..3a1ad82c24bd 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
@@ -5,6 +5,7 @@
 #define _RNPGBE_H
 
 #include <linux/types.h>
+#include <linux/mutex.h>
 
 enum rnpgbe_boards {
 	board_n500,
@@ -24,6 +25,8 @@ struct mucse_mbx_info {
 	u32 usec_delay;
 	u16 fw_req;
 	u16 fw_ack;
+	/* lock for only one use mbx */
+	struct mutex lock;
 	/* fw <--> pf mbx */
 	u32 fwpf_shm_base;
 	u32 pf2fw_mbx_ctrl;
@@ -34,6 +37,7 @@ struct mucse_mbx_info {
 struct mucse_hw {
 	void __iomem *hw_addr;
 	struct mucse_mbx_info mbx;
+	u8 pfvfnum;
 };
 
 struct mucse {
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
index 9fcafda1bc5b..6ff3655033de 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
@@ -421,5 +421,6 @@ void mucse_init_mbx_params_pf(struct mucse_hw *hw)
 	mbx->stats.msgs_rx = 0;
 	mbx->stats.reqs = 0;
 	mbx->stats.acks = 0;
+	mutex_init(&mbx->lock);
 	mucse_mbx_reset(hw);
 }
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
new file mode 100644
index 000000000000..9b81ee679622
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
@@ -0,0 +1,249 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2020 - 2025 Mucse Corporation. */
+
+#include <linux/if_ether.h>
+#include <linux/bitfield.h>
+
+#include "rnpgbe.h"
+#include "rnpgbe_mbx.h"
+#include "rnpgbe_mbx_fw.h"
+
+/**
+ * mucse_fw_send_cmd_wait_resp - 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_resp sends req to pf-fw mailbox and wait
+ * reply from fw.
+ *
+ * Return: 0 on success, negative errno on failure
+ **/
+static int mucse_fw_send_cmd_wait_resp(struct mucse_hw *hw,
+				       struct mbx_fw_cmd_req *req,
+				       struct mbx_fw_cmd_reply *reply)
+{
+	int len = le16_to_cpu(req->datalen);
+	int retry_cnt = 3;
+	int err;
+
+	mutex_lock(&hw->mbx.lock);
+	err = mucse_write_and_wait_ack_mbx(hw, (u32 *)req, len);
+	if (err)
+		goto out;
+	do {
+		err = mucse_poll_and_read_mbx(hw, (u32 *)reply,
+					      sizeof(*reply));
+		if (err)
+			goto out;
+		/* mucse_write_and_wait_ack_mbx return 0 means fw has
+		 * received request, wait for the expect opcode
+		 * reply with 'retry_cnt' times.
+		 */
+	} while (--retry_cnt >= 0 && reply->opcode != req->opcode);
+out:
+	mutex_unlock(&hw->mbx.lock);
+	if (!err && retry_cnt < 0)
+		return -ETIMEDOUT;
+	if (!err && reply->error_code)
+		return -EIO;
+
+	return err;
+}
+
+/**
+ * build_get_hw_info_req - build req with GET_HW_INFO opcode
+ * @req: pointer to the cmd req structure
+ **/
+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;
+}
+
+/**
+ * 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 = {};
+	struct mucse_hw_info info = {};
+	int err;
+
+	build_get_hw_info_req(&req);
+
+	err = mucse_fw_send_cmd_wait_resp(hw, &req, &reply);
+	if (!err) {
+		memcpy(&info, &reply.hw_info, sizeof(struct mucse_hw_info));
+		mucse_hw_info_update_host_endian(&info);
+		hw->pfvfnum = FIELD_GET(GENMASK_U16(7, 0),
+					le16_to_cpu(info.pfnum));
+	}
+
+	return err;
+}
+
+/**
+ * mucse_mbx_sync_fw - Try to sync with fw
+ * @hw: pointer to the HW structure
+ *
+ * mucse_mbx_sync_fw tries to sync with fw. It is only called in
+ * probe. Nothing (register network) todo if failed.
+ * Try more times to do sync.
+ *
+ * Return: 0 on success, negative errno on failure
+ **/
+int mucse_mbx_sync_fw(struct mucse_hw *hw)
+{
+	int try_cnt = 3;
+	int err;
+
+	do {
+		err = mucse_mbx_get_info(hw);
+		if (err == -ETIMEDOUT)
+			continue;
+		break;
+	} while (try_cnt--);
+
+	return err;
+}
+
+/**
+ * build_powerup - build req with powerup opcode
+ * @req: pointer to the cmd req structure
+ * @is_powerup: true for powerup, false for powerdown
+ **/
+static void build_powerup(struct mbx_fw_cmd_req *req,
+			  bool is_powerup)
+{
+	req->flags = 0;
+	req->opcode = cpu_to_le16(POWER_UP);
+	req->datalen = cpu_to_le16(sizeof(req->powerup) +
+				   MUCSE_MBX_REQ_HDR_LEN);
+	req->reply_lo = 0;
+	req->reply_hi = 0;
+	/* fw needs this to reply correct cmd */
+	req->powerup.version = cpu_to_le32(GENMASK_U32(31, 0));
+	if (is_powerup)
+		req->powerup.status = cpu_to_le32(1);
+	else
+		req->powerup.status = cpu_to_le32(0);
+}
+
+/**
+ * mucse_mbx_powerup - Echo fw to powerup
+ * @hw: pointer to the HW structure
+ * @is_powerup: true for powerup, false for powerdown
+ *
+ * mucse_mbx_powerup echo fw to change working frequency
+ * to normal after received true, and reduce working frequency
+ * if false.
+ *
+ * Return: 0 on success, negative errno on failure
+ **/
+int mucse_mbx_powerup(struct mucse_hw *hw, bool is_powerup)
+{
+	struct mbx_fw_cmd_req req = {};
+	int len;
+	int err;
+
+	build_powerup(&req, is_powerup);
+	len = le16_to_cpu(req.datalen);
+	mutex_lock(&hw->mbx.lock);
+	err = mucse_write_and_wait_ack_mbx(hw, (u32 *)&req, len);
+	mutex_unlock(&hw->mbx.lock);
+
+	return err;
+}
+
+/**
+ * build_reset_hw_req - build req with RESET_HW opcode
+ * @req: pointer to the cmd req structure
+ **/
+static void build_reset_hw_req(struct mbx_fw_cmd_req *req)
+{
+	req->flags = 0;
+	req->opcode = cpu_to_le16(RESET_HW);
+	req->datalen = cpu_to_le16(MUCSE_MBX_REQ_HDR_LEN);
+	req->reply_lo = 0;
+	req->reply_hi = 0;
+}
+
+/**
+ * mucse_mbx_reset_hw - Posts a mbx req to reset hw
+ * @hw: pointer to the HW structure
+ *
+ * mucse_mbx_reset_hw posts a mbx req to firmware to reset hw.
+ * We use mucse_fw_send_cmd_wait_resp to wait hw reset ok.
+ *
+ * Return: 0 on success, negative errno on failure
+ **/
+int mucse_mbx_reset_hw(struct mucse_hw *hw)
+{
+	struct mbx_fw_cmd_reply reply = {};
+	struct mbx_fw_cmd_req req = {};
+
+	build_reset_hw_req(&req);
+
+	return mucse_fw_send_cmd_wait_resp(hw, &req, &reply);
+}
+
+/**
+ * build_get_macaddress_req - build req with get_mac opcode
+ * @req: pointer to the cmd req structure
+ * @port_mask: port valid for this cmd
+ * @pfvfnum: pfvfnum for this cmd
+ **/
+static void build_get_macaddress_req(struct mbx_fw_cmd_req *req,
+				     int port_mask, int pfvfnum)
+{
+	req->flags = 0;
+	req->opcode = cpu_to_le16(GET_MAC_ADDRESS);
+	req->datalen = cpu_to_le16(sizeof(req->get_mac_addr) +
+				   MUCSE_MBX_REQ_HDR_LEN);
+	req->reply_lo = 0;
+	req->reply_hi = 0;
+	req->get_mac_addr.port_mask = cpu_to_le32(port_mask);
+	req->get_mac_addr.pfvf_num = cpu_to_le32(pfvfnum);
+}
+
+/**
+ * mucse_mbx_get_macaddr - Posts a mbx req to request macaddr
+ * @hw: pointer to the HW structure
+ * @pfvfnum: index of pf/vf num
+ * @mac_addr: pointer to store mac_addr
+ * @port: port index
+ *
+ * mucse_mbx_get_macaddr posts a mbx req to firmware to get mac_addr.
+ *
+ * Return: 0 on success, negative errno on failure
+ **/
+int mucse_mbx_get_macaddr(struct mucse_hw *hw, int pfvfnum,
+			  u8 *mac_addr,
+			  int port)
+{
+	struct mbx_fw_cmd_reply reply = {};
+	struct mbx_fw_cmd_req req = {};
+	int err;
+
+	build_get_macaddress_req(&req, BIT(port), pfvfnum);
+	err = mucse_fw_send_cmd_wait_resp(hw, &req, &reply);
+	if (err)
+		return err;
+
+	if (le32_to_cpu(reply.mac_addr.ports) & BIT(port))
+		memcpy(mac_addr, reply.mac_addr.addrs[port].mac, ETH_ALEN);
+	else
+		return -ENODATA;
+
+	return 0;
+}
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h
new file mode 100644
index 000000000000..9dee6029e630
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h
@@ -0,0 +1,121 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright(c) 2020 - 2025 Mucse Corporation. */
+
+#ifndef _RNPGBE_MBX_FW_H
+#define _RNPGBE_MBX_FW_H
+
+#include <linux/types.h>
+
+#include "rnpgbe.h"
+
+#define MUCSE_MBX_REQ_HDR_LEN 24
+
+enum MUCSE_FW_CMD {
+	GET_HW_INFO = 0x0601,
+	GET_MAC_ADDRESS = 0x0602,
+	RESET_HW = 0x0603,
+	POWER_UP = 0x0803,
+};
+
+struct mucse_hw_info {
+	u8 link_stat;
+	u8 port_mask;
+	__le32 speed;
+	__le16 phy_type;
+	__le16 nic_mode;
+	__le16 pfnum;
+	__le32 fw_version;
+	__le32 axi_mhz;
+	union {
+		u8 port_id[4];
+		__le32 port_ids;
+	};
+	__le32 bd_uid;
+	__le32 phy_id;
+	__le32 wol_status;
+	union {
+		__le32 ext_info;
+		struct {
+			u32 valid : 1;
+			u32 wol_en : 1;
+			u32 pci_preset_runtime_en : 1;
+			u32 smbus_en : 1;
+			u32 ncsi_en : 1;
+			u32 rpu_en : 1;
+			u32 v2 : 1;
+			u32 pxe_en : 1;
+			u32 mctp_en : 1;
+			u32 yt8614 : 1;
+			u32 pci_ext_reset : 1;
+			u32 rpu_availble : 1;
+			u32 fw_lldp_ability : 1;
+			u32 lldp_enabled : 1;
+			u32 only_1g : 1;
+			u32 force_down_en: 1;
+		} e_host;
+	};
+} __packed;
+
+/* FW stores extended information in 'ext_info' as a 32-bit
+ * little-endian value. To make these flags easily accessible in the
+ * kernel (via named 'bitfields' instead of raw bitmask operations),
+ * we use the union's 'e_host' struct, which provides named bits
+ * (e.g., 'wol_en', 'smbus_en')
+ */
+static inline void mucse_hw_info_update_host_endian(struct mucse_hw_info *info)
+{
+	u32 host_val = le32_to_cpu(info->ext_info);
+
+	memcpy(&info->e_host, &host_val, sizeof(info->e_host));
+}
+
+struct mbx_fw_cmd_req {
+	__le16 flags;
+	__le16 opcode;
+	__le16 datalen;
+	__le16 ret_value;
+	__le32 cookie_lo;
+	__le32 cookie_hi;
+	__le32 reply_lo;
+	__le32 reply_hi;
+	union {
+		u8 data[32];
+		struct {
+			__le32 version;
+			__le32 status;
+		} powerup;
+		struct {
+			__le32 port_mask;
+			__le32 pfvf_num;
+		} get_mac_addr;
+	};
+} __packed;
+
+struct mbx_fw_cmd_reply {
+	__le16 flags;
+	__le16 opcode;
+	__le16 error_code;
+	__le16 datalen;
+	__le32 cookie_lo;
+	__le32 cookie_hi;
+	union {
+		u8 data[40];
+		struct mac_addr {
+			__le32 ports;
+			struct _addr {
+				/* for macaddr:01:02:03:04:05:06
+				 * mac-hi=0x01020304 mac-lo=0x05060000
+				 */
+				u8 mac[8];
+			} addrs[4];
+		} mac_addr;
+		struct mucse_hw_info hw_info;
+	};
+} __packed;
+
+int mucse_mbx_sync_fw(struct mucse_hw *hw);
+int mucse_mbx_powerup(struct mucse_hw *hw, bool is_powerup);
+int mucse_mbx_reset_hw(struct mucse_hw *hw);
+int mucse_mbx_get_macaddr(struct mucse_hw *hw, int pfvfnum,
+			  u8 *mac_addr, int port);
+#endif /* _RNPGBE_MBX_FW_H */
-- 
2.25.1

On 9/9/2025 5:39 PM, 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 <dong100@mucse.com>
> ---

> +/**
> + * mucse_mbx_sync_fw - Try to sync with fw
> + * @hw: pointer to the HW structure
> + *
> + * mucse_mbx_sync_fw tries to sync with fw. It is only called in
> + * probe. Nothing (register network) todo if failed.
> + * Try more times to do sync.
> + *
> + * Return: 0 on success, negative errno on failure
> + **/
> +int mucse_mbx_sync_fw(struct mucse_hw *hw)
> +{
> +	int try_cnt = 3;
> +	int err;
> +
> +	do {
> +		err = mucse_mbx_get_info(hw);
> +		if (err == -ETIMEDOUT)
> +			continue;
> +		break;
> +	} while (try_cnt--);
> +
> +	return err;
> +}

There's a logical issue in the code. The loop structure attempts to
retry on ETIMEDOUT errors, but the unconditional break statement after
the if-check will always exit the loop after the first attempt,
regardless of the error. The do-while loop will never actually retry
because the break statement is placed outside of the if condition that
checks for timeout errors.

-- 
Thanks and Regards,
Md Danish Anwar

On Tue, 9 Sep 2025 19:59:11 +0530 Anwar, Md Danish wrote:
> > +int mucse_mbx_sync_fw(struct mucse_hw *hw)
> > +{
> > +	int try_cnt = 3;
> > +	int err;
> > +
> > +	do {
> > +		err = mucse_mbx_get_info(hw);
> > +		if (err == -ETIMEDOUT)
> > +			continue;
> > +		break;
> > +	} while (try_cnt--);
> > +
> > +	return err;
> > +}  
> 
> There's a logical issue in the code. The loop structure attempts to
> retry on ETIMEDOUT errors, but the unconditional break statement after
> the if-check will always exit the loop after the first attempt,
> regardless of the error. The do-while loop will never actually retry
> because the break statement is placed outside of the if condition that
> checks for timeout errors.

The other way around. continue; in a do {} while () look does *not*
evaluate the condition. So this can loop forever.

On Tue, Sep 09, 2025 at 01:58:22PM -0700, Jakub Kicinski wrote:
> On Tue, 9 Sep 2025 19:59:11 +0530 Anwar, Md Danish wrote:
> > > +int mucse_mbx_sync_fw(struct mucse_hw *hw)
> > > +{
> > > +	int try_cnt = 3;
> > > +	int err;
> > > +
> > > +	do {
> > > +		err = mucse_mbx_get_info(hw);
> > > +		if (err == -ETIMEDOUT)
> > > +			continue;
> > > +		break;
> > > +	} while (try_cnt--);
> > > +
> > > +	return err;
> > > +}  
> > 
> > There's a logical issue in the code. The loop structure attempts to
> > retry on ETIMEDOUT errors, but the unconditional break statement after
> > the if-check will always exit the loop after the first attempt,
> > regardless of the error. The do-while loop will never actually retry
> > because the break statement is placed outside of the if condition that
> > checks for timeout errors.
> 

What is expected is 'retry on ETIMEDOUT' and 'no retry others'. 
https://lore.kernel.org/netdev/a066746c-2f12-4e70-b63a-7996392a9132@lunn.ch/

> The other way around. continue; in a do {} while () look does *not*
> evaluate the condition. So this can loop forever.
> 

Maybe I can update like this ?

int mucse_mbx_sync_fw(struct mucse_hw *hw)
{
	int try_cnt = 3;
	int err;

	do {
		err = mucse_mbx_get_info(hw);
		if (err != -ETIMEDOUT)
			break;
		/* only retry with ETIMEDOUT, others just return */
	} while (try_cnt--);

	return err;
}  

Thanks for your feedback.

On Wed, 10 Sep 2025 14:08:21 +0800 Yibo Dong wrote:
> 	do {
> 		err = mucse_mbx_get_info(hw);
> 		if (err != -ETIMEDOUT)
> 			break;
> 		/* only retry with ETIMEDOUT, others just return */
> 	} while (try_cnt--);

	do {
		err = bla();
	} while (err == -ETIMEDOUT && try_cnt--);

On Wed, Sep 10, 2025 at 06:03:34PM -0700, Jakub Kicinski wrote:
> On Wed, 10 Sep 2025 14:08:21 +0800 Yibo Dong wrote:
> > 	do {
> > 		err = mucse_mbx_get_info(hw);
> > 		if (err != -ETIMEDOUT)
> > 			break;
> > 		/* only retry with ETIMEDOUT, others just return */
> > 	} while (try_cnt--);
> 
> 	do {
> 		err = bla();
> 	} while (err == -ETIMEDOUT && try_cnt--);
> 

Got it, I will fix it in next version.

Thanks for your feedback.

Yibo Dong schrieb am Mi 10. Sep, 14:08 (+0800):
> On Tue, Sep 09, 2025 at 01:58:22PM -0700, Jakub Kicinski wrote:
> > On Tue, 9 Sep 2025 19:59:11 +0530 Anwar, Md Danish wrote:
> > > > +int mucse_mbx_sync_fw(struct mucse_hw *hw)
> > > > +{
> > > > +	int try_cnt = 3;
> > > > +	int err;
> > > > +
> > > > +	do {
> > > > +		err = mucse_mbx_get_info(hw);
> > > > +		if (err == -ETIMEDOUT)
> > > > +			continue;
> > > > +		break;
> > > > +	} while (try_cnt--);
> > > > +
> > > > +	return err;
> > > > +}  
> > > 
> > > There's a logical issue in the code. The loop structure attempts to
> > > retry on ETIMEDOUT errors, but the unconditional break statement after
> > > the if-check will always exit the loop after the first attempt,
> > > regardless of the error. The do-while loop will never actually retry
> > > because the break statement is placed outside of the if condition that
> > > checks for timeout errors.
> > 
> 
> What is expected is 'retry on ETIMEDOUT' and 'no retry others'. 
> https://lore.kernel.org/netdev/a066746c-2f12-4e70-b63a-7996392a9132@lunn.ch/
> 
> > The other way around. continue; in a do {} while () look does *not*
> > evaluate the condition. So this can loop forever.
> > 
> 
> Maybe I can update like this ?
> 
> int mucse_mbx_sync_fw(struct mucse_hw *hw)
> {
> 	int try_cnt = 3;
> 	int err;
> 
> 	do {
> 		err = mucse_mbx_get_info(hw);
> 		if (err != -ETIMEDOUT)
> 			break;
> 		/* only retry with ETIMEDOUT, others just return */
> 	} while (try_cnt--);
> 
> 	return err;
> }  

How about something like this?

int mucse_mbx_sync_fw(struct mucse_hw *hw)
{
	for (int try = 3; try; --try) {
		int err = mucse_mbx_get_info(hw);
		if (err != -ETIMEDOUT)
			return err;
	}

	return ETIMEDOUT;
}


My 2cent.

Regards Jörg

-- 
„Es wurden und werden zu viele sprachlose Bücher gedruckt, nach deren
schon flüchtiger Lektüre man all die Bäume um Vergebung bitten möchte,
die für den Schund ihr Leben lassen mussten.“ (Michael Jürgs,
                      Seichtgebiete – Warum wir hemmungslos verblöden)

On Wed, Sep 10, 2025 at 10:21:58AM +0200, Jörg Sommer wrote:
> Yibo Dong schrieb am Mi 10. Sep, 14:08 (+0800):
> > On Tue, Sep 09, 2025 at 01:58:22PM -0700, Jakub Kicinski wrote:
> > > On Tue, 9 Sep 2025 19:59:11 +0530 Anwar, Md Danish wrote:
> > > > > +int mucse_mbx_sync_fw(struct mucse_hw *hw)
> > > > > +{
> > > > > +	int try_cnt = 3;
> > > > > +	int err;
> > > > > +
> > > > > +	do {
> > > > > +		err = mucse_mbx_get_info(hw);
> > > > > +		if (err == -ETIMEDOUT)
> > > > > +			continue;
> > > > > +		break;
> > > > > +	} while (try_cnt--);
> > > > > +
> > > > > +	return err;
> > > > > +}  
> > > > 
> > > > There's a logical issue in the code. The loop structure attempts to
> > > > retry on ETIMEDOUT errors, but the unconditional break statement after
> > > > the if-check will always exit the loop after the first attempt,
> > > > regardless of the error. The do-while loop will never actually retry
> > > > because the break statement is placed outside of the if condition that
> > > > checks for timeout errors.
> > > 
> > 
> > What is expected is 'retry on ETIMEDOUT' and 'no retry others'. 
> > https://lore.kernel.org/netdev/a066746c-2f12-4e70-b63a-7996392a9132@lunn.ch/
> > 
> > > The other way around. continue; in a do {} while () look does *not*
> > > evaluate the condition. So this can loop forever.
> > > 
> > 
> > Maybe I can update like this ?
> > 
> > int mucse_mbx_sync_fw(struct mucse_hw *hw)
> > {
> > 	int try_cnt = 3;
> > 	int err;
> > 
> > 	do {
> > 		err = mucse_mbx_get_info(hw);
> > 		if (err != -ETIMEDOUT)
> > 			break;
> > 		/* only retry with ETIMEDOUT, others just return */
> > 	} while (try_cnt--);
> > 
> > 	return err;
> > }  
> 
> How about something like this?
> 
> int mucse_mbx_sync_fw(struct mucse_hw *hw)
> {
> 	for (int try = 3; try; --try) {
> 		int err = mucse_mbx_get_info(hw);
> 		if (err != -ETIMEDOUT)
> 			return err;
> 	}
> 
> 	return ETIMEDOUT;
> }
> 

I think Jakub Kicinski's suggestion is clearer, right?
	do {
		err = mucse_mbx_get_info(hw);
	} while (err == -ETIMEDOUT && try_cnt--);

> 
> My 2cent.
> 
> Regards Jörg
> 
> -- 
> „Es wurden und werden zu viele sprachlose Bücher gedruckt, nach deren
> schon flüchtiger Lektüre man all die Bäume um Vergebung bitten möchte,
> die für den Schund ihr Leben lassen mussten.“ (Michael Jürgs,
>                       Seichtgebiete – Warum wir hemmungslos verblöden)

Thanks for your feedback.