On 7/21/2025 4:32 AM, Dong Yibo wrote:

Do you want to know what the "err" value was? Should that be included in 
the error message?

You aren't releasing the lock here, was that intentional?  Also, would 
-ETIMEDOUT make more sense?

+
+       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;

It seems like this block could be achieved with some sort of loop 
condition instead of a goto retry. Something like:

do {
} while (--retry_cnt >= 0 && reply->opcode != req->opcode);

mutex_unlock();

if (retry_cnt < 0)
	return -ETIMEDOUT;

if (reply->error_code)
	return -EIO;

return 0;

+
+       mutex_unlock(&hw->mbx.lock);
+
+       if (reply->error_code)
+               return -EIO;

Do you want to lose the "error_code" here?

+
+       return 0;
+}
+
+/**
+ * mucse_fw_get_capability - Get hw abilities from fw
+ * @hw: Pointer to the HW structure
+ * @abil: Pointer to the hw_abilities structure
+ *
+ * mucse_fw_get_capability tries to get hw abilities from
+ * hw.
+ *
+ * @return: 0 on success, negative on failure
+ **/
+static int mucse_fw_get_capability(struct mucse_hw *hw,
+                                  struct hw_abilities *abil)
+{
+       struct mbx_fw_cmd_reply reply;
+       struct mbx_fw_cmd_req req;
+       int err = 0;

Nit, you don't need to initialize this because it will always be set 
when calling mucse_fw_send_cmd_wait().

+
+       memset(&req, 0, sizeof(req));
+       memset(&reply, 0, sizeof(reply));
+       build_phy_abalities_req(&req, &req);
+       err = mucse_fw_send_cmd_wait(hw, &req, &reply);
+       if (err == 0)

Nit, but typically just "if (!err)" is used.

+               memcpy(abil, &reply.hw_abilities, sizeof(*abil));
+
+       return err;
+}
+
+/**
+ * mucse_mbx_get_capability - Get hw abilities from fw
+ * @hw: Pointer to the HW structure
+ *
+ * mucse_mbx_get_capability tries to some capabities from
+ * hw. Many retrys will do if it is failed.
+ *
+ * @return: 0 on success, negative on failure
+ **/
+int mucse_mbx_get_capability(struct mucse_hw *hw)
+{
+       struct hw_abilities ability;
+       int try_cnt = 3;
+       int err = 0;

Don't initialize err as it's always set before being used. Please look 
through all your patches and apply this same comment/thought process 
before reposting.

+
+       memset(&ability, 0, sizeof(ability));
+
+       while (try_cnt--) {
+               err = mucse_fw_get_capability(hw, &ability);
+               if (err == 0) {

Typically the following is done to reduce indentation.

while (condition) {
	err = func();
         if (err)
		continue;

	/* handle non-err case */
         return 0;
}

return err;

Do the /* <bit_#> */ comments provide any value here?

+       };
+} __packed;
+
+static inline void ability_update_host_endian(struct hw_abilities *abi)
+{
+       u32 host_val = le32_to_cpu(abi->ext_ability);
+
+       abi->e_host = *(typeof(abi->e_host) *)&host_val;
+}
+
+struct phy_pause_data {
+       u32 pause_mode;
+};
+
+struct lane_stat_data {
+       u8 nr_lane;
+       u8 pci_gen : 4;
+       u8 pci_lanes : 4;
+       u8 pma_type;
+       u8 phy_type;
+       __le16 linkup : 1;
+       __le16 duplex : 1;
+       __le16 autoneg : 1;
+       __le16 fec : 1;
+       __le16 an : 1;
+       __le16 link_traing : 1;
+       __le16 media_availble : 1;
+       __le16 is_sgmii : 1;
+       __le16 link_fault : 4;
+#define LINK_LINK_FAULT BIT(0)
+#define LINK_TX_FAULT BIT(1)
+#define LINK_RX_FAULT BIT(2)
+#define LINK_REMOTE_FAULT BIT(3)
+       __le16 is_backplane : 1;
+       __le16 tp_mdx : 2;
+       union {
+               u8 phy_addr;
+               struct {
+                       u8 mod_abs : 1;
+                       u8 fault : 1;
+                       u8 tx_dis : 1;
+                       u8 los : 1;
+               } sfp;
+       };
+       u8 sfp_connector;
+       __le32 speed;
+       __le32 si_main;
+       __le32 si_pre;
+       __le32 si_post;
+       __le32 si_tx_boost;
+       __le32 supported_link;
+       __le32 phy_id;
+       __le32 advertised_link;
+} __packed;
+
+struct yt_phy_statistics {
+       __le32 pkg_ib_valid; /* rx crc good and length 64-1518 */
+       __le32 pkg_ib_os_good; /* rx crc good and length >1518 */
+       __le32 pkg_ib_us_good; /* rx crc good and length <64 */
+       __le16 pkg_ib_err; /* rx crc wrong and length 64-1518 */
+       __le16 pkg_ib_os_bad; /* rx crc wrong and length >1518 */
+       __le16 pkg_ib_frag; /* rx crc wrong and length <64 */
+       __le16 pkg_ib_nosfd; /* rx sfd missed */
+       __le32 pkg_ob_valid; /* tx crc good and length 64-1518 */
+       __le32 pkg_ob_os_good; /* tx crc good and length >1518 */
+       __le32 pkg_ob_us_good; /* tx crc good and length <64 */
+       __le16 pkg_ob_err; /* tx crc wrong and length 64-1518 */
+       __le16 pkg_ob_os_bad; /* tx crc wrong and length >1518 */
+       __le16 pkg_ob_frag; /* tx crc wrong and length <64 */
+       __le16 pkg_ob_nosfd; /* tx sfd missed */
+} __packed;
+
+struct phy_statistics {
+       union {
+               struct yt_phy_statistics yt;
+       };
+} __packed;
+
+struct port_stat {
+       u8 phyid;
+       u8 duplex : 1;
+       u8 autoneg : 1;
+       u8 fec : 1;
+       __le16 speed;
+       union {
+               __le16 stat;
+               struct {
+                       __le16 pause : 4;
+                       __le16 local_eee : 3;
+                       __le16 partner_eee : 3;
+                       __le16 tp_mdx : 2;
+                       __le16 lldp_status : 1;
+                       __le16 revs : 3;
+               } v;
+               struct {
+                       u16 pause : 4;
+                       u16 local_eee : 3;
+                       u16 partner_eee : 3;
+                       u16 tp_mdx : 2;
+                       u16 lldp_status : 1;
+                       u16 revs : 3;
+               } v_host;
+       };
+} __packed;
+
+#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 */

Some typos here. Please fix.

+struct mbx_fw_cmd_req {
+       __le16 flags; /* 0-1 */
+       __le16 opcode; /* 2-3 enum GENERIC_CMD */
+       __le16 datalen; /* 4-5 */
+       __le16 ret_value; /* 6-7 */
+       union {
+               struct {
+                       __le32 cookie_lo; /* 8-11 */
+                       __le32 cookie_hi; /* 12-15 */
+               };
+
+               void *cookie;
+       };
+       __le32 reply_lo; /* 16-19 5dw */
+       __le32 reply_hi; /* 20-23 */
+       union {
+               u8 data[32];
+               struct {
+                       __le32 addr;
+                       __le32 bytes;
+               } r_reg;
+
+               struct {
+                       __le32 addr;
+                       __le32 bytes;
+                       __le32 data[4];
+               } w_reg;
+
+               struct {
+                       __le32 lanes;
+               } ptp;
+
+               struct {
+                       __le32 lane;
+                       __le32 up;
+               } ifup;
+
+               struct {
+                       __le32 sec;
+                       __le32 nanosec;
+
+               } tstamps;
+
+               struct {
+                       __le32 lane;
+                       __le32 status;
+               } ifinsmod;
+
+               struct {
+                       __le32 lane;
+                       __le32 status;
+               } ifforce;
+
+               struct {
+                       __le32 lane;
+                       __le32 status;
+               } ifsuspuse;
+
+               struct {
+                       __le32 nr_lane;
+               } get_lane_st;
+
+               struct {
+                       __le32 nr_lane;
+                       __le32 func;
+#define LANE_FUN_AN 0
+#define LANE_FUN_LINK_TRAING 1
+#define LANE_FUN_FEC 2
+#define LANE_FUN_SI 3
+#define LANE_FUN_SFP_TX_DISABLE 4
+#define LANE_FUN_PCI_LANE 5
+#define LANE_FUN_PRBS 6
+#define LANE_FUN_SPEED_CHANGE 7
+                       __le32 value0;
+                       __le32 value1;
+                       __le32 value2;
+                       __le32 value3;
+               } set_lane_fun;
+
+               struct {
+                       __le32 flag;
+                       __le32 nr_lane;
+               } set_dump;
+
+               struct {
+                       __le32 lane;
+                       __le32 enable;
+               } wol;
+
+               struct {
+                       __le32 lane;
+                       __le32 mode;
+               } gephy_test;
+
+               struct {
+                       __le32 lane;
+                       __le32 op;
+                       __le32 enable;
+                       __le32 inteval;
+               } lldp_tx;
+
+               struct {
+                       __le32 bytes;
+                       __le32 nr_lane;
+                       __le32 bin_offset;
+                       __le32 no_use;
+               } get_dump;
+
+               struct {
+                       __le32 nr_lane;
+                       __le32 value;
+#define LED_IDENTIFY_INACTIVE 0
+#define LED_IDENTIFY_ACTIVE 1
+#define LED_IDENTIFY_ON 2
+#define LED_IDENTIFY_OFF 3
+               } led_set;
+
+               struct {
+                       __le32 addr;
+                       __le32 data;
+                       __le32 mask;
+               } modify_reg;
+
+               struct {
+                       __le32 adv_speed_mask;
+                       __le32 autoneg;
+                       __le32 speed;
+                       __le32 duplex;
+                       __le32 nr_lane;
+                       __le32 tp_mdix_ctrl;
+               } phy_link_set;
+
+               struct {
+                       __le32 pause_mode;
+                       __le32 nr_lane;
+               } phy_pause_set;
+
+               struct {
+                       __le32 pause_mode;
+                       __le32 nr_lane;
+               } phy_pause_get;
+
+               struct {
+                       __le32 local_eee;
+                       __le32 tx_lpi_timer;
+                       __le32 nr_lane;
+               } phy_eee_set;
+
+               struct {
+                       __le32 nr_lane;
+                       __le32 sfp_adr; /* 0xa0 or 0xa2 */
+                       __le32 reg;
+                       __le32 cnt;
+               } sfp_read;
+
+               struct {
+                       __le32 nr_lane;
+                       __le32 sfp_adr; /* 0xa0 or 0xa2 */
+                       __le32 reg;
+                       __le32 val;
+               } sfp_write;
+
+               struct {
+                       __le32 nr_lane; /* 0-3 */
+               } get_linkstat;
+
+               struct {
+                       __le16 changed_lanes;
+                       __le16 lane_status;
+                       __le32 port_st_magic;
+#define SPEED_VALID_MAGIC 0xa4a6a8a9
+                       struct port_stat st[4];
+               } link_stat; /* FW->RC */
+
+               struct {
+                       __le16 enable_stat;
+                       __le16 event_mask;
+               } stat_event_mask;
+
+               struct {
+                       __le32 cmd;
+                       __le32 arg0;
+                       __le32 req_bytes;
+                       __le32 reply_bytes;
+                       __le32 ddr_lo;
+                       __le32 ddr_hi;
+               } maintain;
+
+               struct { /* set phy register */
+                       u8 phy_interface;
+                       union {
+                               u8 page_num;
+                               u8 external_phy_addr;
+                       };
+                       __le32 phy_reg_addr;
+                       __le32 phy_w_data;
+                       __le32 reg_addr;
+                       __le32 w_data;
+                       /* 1 = ignore page_num, use last QSFP */
+                       u8 recall_qsfp_page : 1;
+                       /* page value */
+                       /* 0 = use page_num for QSFP */
+                       u8 nr_lane;
+               } set_phy_reg;
+
+               struct {
+                       __le32 lane_mask;
+                       __le32 pfvf_num;
+               } get_mac_addr;
+
+               struct {
+                       u8 phy_interface;
+                       union {
+                               u8 page_num;
+                               u8 external_phy_addr;
+                       };
+                       __le32 phy_reg_addr;
+                       u8 nr_lane;
+               } get_phy_reg;
+
+               struct {
+                       __le32 nr_lane;
+               } phy_statistics;
+
+               struct {
+                       u8 paration;
+                       __le32 bytes;
+                       __le32 bin_phy_lo;
+                       __le32 bin_phy_hi;
+               } fw_update;
+       };
+} __packed;
+
+#define EEE_1000BT BIT(2)
+#define EEE_100BT BIT(1)
+
+struct rnpgbe_eee_cap {
+       __le32 local_capability;
+       __le32 local_eee;
+       __le32 partner_eee;
+};
+
+/* firmware -> driver */
+struct mbx_fw_cmd_reply {
+       /* fw must set: DD, CMP, Error(if error), copy value */
+       __le16 flags;
+       /* from command: LB,RD,VFC,BUF,SI,EI,FE */
+       __le16 opcode; /* 2-3: copy from req */
+       __le16 error_code; /* 4-5: 0 if no error */
+       __le16 datalen; /* 6-7: */
+       union {
+               struct {
+                       __le32 cookie_lo; /* 8-11: */
+                       __le32 cookie_hi; /* 12-15: */
+               };
+               void *cookie;
+       };
+       /* ===== data ==== [16-64] */
+       union {
+               u8 data[40];
+
+               struct version {
+                       __le32 major;
+                       __le32 sub;
+                       __le32 modify;
+               } version;
+
+               struct {
+                       __le32 value[4];
+               } r_reg;
+
+               struct {
+                       __le32 new_value;
+               } modify_reg;
+
+               struct get_temp {
+                       __le32 temp;
+                       __le32 volatage;
+               } get_temp;
+
+               struct {
+#define MBX_SFP_READ_MAX_CNT 32
+                       u8 value[MBX_SFP_READ_MAX_CNT];
+               } sfp_read;
+
+               struct mac_addr {
+                       __le32 lanes;
+                       struct _addr {
+                               /*
+                                * for macaddr:01:02:03:04:05:06
+                                * mac-hi=0x01020304 mac-lo=0x05060000
+                                */
+                               u8 mac[8];
+                       } addrs[4];
+               } mac_addr;
+
+               struct get_dump_reply {
+                       __le32 flags;
+                       __le32 version;
+                       __le32 bytes;
+                       __le32 data[4];
+               } get_dump;
+
+               struct get_lldp_reply {
+                       __le32 value;
+                       __le32 inteval;
+               } get_lldp;
+
+               struct rnpgbe_eee_cap phy_eee_abilities;
+               struct lane_stat_data lanestat;
+               struct hw_abilities hw_abilities;
+               struct phy_statistics phy_statistics;
+       };
+} __packed;
+
+static inline void build_phy_abalities_req(struct mbx_fw_cmd_req *req,
+                                          void *cookie)
+{
+       req->flags = 0;
+       req->opcode = cpu_to_le32(GET_PHY_ABALITY);
+       req->datalen = 0;
+       req->reply_lo = 0;
+       req->reply_hi = 0;
+       req->cookie = cookie;
+}
+
+int mucse_mbx_get_capability(struct mucse_hw *hw);
+
+#endif /* _RNPGBE_MBX_FW_H */
--
2.25.1