Initialize basic mbx_fw ops, such as get_capability, reset phy
and so on.
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_fw.c | 275 ++++++++++++++++++
.../net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h | 201 +++++++++++++
4 files changed, 482 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 05830bb73d3e..6cb14b79cbfe 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
@@ -88,9 +88,13 @@ struct mucse_mbx_info {
struct mucse_hw {
void *back;
+ u8 pfvfnum;
void __iomem *hw_addr;
void __iomem *ring_msix_base;
struct pci_dev *pdev;
+ u32 fw_version;
+ u32 axi_mhz;
+ u32 bd_uid;
enum rnpgbe_hw_type hw_type;
struct mucse_dma_info dma;
struct mucse_eth_info eth;
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..374eb9df1369
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
@@ -0,0 +1,275 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2020 - 2025 Mucse Corporation. */
+
+#include <linux/pci.h>
+
+#include "rnpgbe.h"
+#include "rnpgbe_hw.h"
+#include "rnpgbe_mbx.h"
+#include "rnpgbe_mbx_fw.h"
+
+/**
+ * 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-fw mailbox and wait
+ * reply from fw.
+ *
+ * @return: 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 len = le16_to_cpu(req->datalen) + MBX_REQ_HDR_LEN;
+ int retry_cnt = 3;
+ int err;
+
+ err = mutex_lock_interruptible(&hw->mbx.lock);
+ if (err)
+ return err;
+ err = hw->mbx.ops->write_posted(hw, (u32 *)req,
+ L_WD(len));
+ if (err) {
+ mutex_unlock(&hw->mbx.lock);
+ return err;
+ }
+ do {
+ err = hw->mbx.ops->read_posted(hw, (u32 *)reply,
+ L_WD(sizeof(*reply)));
+ if (err) {
+ mutex_unlock(&hw->mbx.lock);
+ return err;
+ }
+ } while (--retry_cnt >= 0 && reply->opcode != req->opcode);
+ mutex_unlock(&hw->mbx.lock);
+ if (retry_cnt < 0 || reply->error_code)
+ return -EIO;
+ 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;
+
+ 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)
+ 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;
+
+ memset(&ability, 0, sizeof(ability));
+ while (try_cnt--) {
+ err = mucse_fw_get_capability(hw, &ability);
+ if (err)
+ continue;
+ hw->pfvfnum = le16_to_cpu(ability.pfnum);
+ hw->fw_version = le32_to_cpu(ability.fw_version);
+ hw->axi_mhz = le32_to_cpu(ability.axi_mhz);
+ hw->bd_uid = le32_to_cpu(ability.bd_uid);
+ return 0;
+ }
+ return err;
+}
+
+/**
+ * mbx_cookie_zalloc - Alloc a cookie structure
+ * @priv_len: private length for this cookie
+ *
+ * @return: cookie structure on success
+ **/
+static struct mbx_req_cookie *mbx_cookie_zalloc(int priv_len)
+{
+ struct mbx_req_cookie *cookie;
+
+ cookie = kzalloc(struct_size(cookie, priv, priv_len), GFP_KERNEL);
+ if (cookie) {
+ cookie->timeout_jiffes = 30 * HZ;
+ cookie->magic = COOKIE_MAGIC;
+ cookie->priv_len = priv_len;
+ }
+ return cookie;
+}
+
+/**
+ * mucse_mbx_fw_post_req - Posts a mbx req to firmware and wait reply
+ * @hw: pointer to the HW structure
+ * @req: pointer to the cmd req structure
+ * @cookie: pointer to the req cookie
+ *
+ * mucse_mbx_fw_post_req posts a mbx req to firmware and wait for the
+ * reply. cookie->wait will be set in irq handler.
+ *
+ * @return: 0 on success, negative on failure
+ **/
+static int mucse_mbx_fw_post_req(struct mucse_hw *hw,
+ struct mbx_fw_cmd_req *req,
+ struct mbx_req_cookie *cookie)
+{
+ int len = le16_to_cpu(req->datalen) + MBX_REQ_HDR_LEN;
+ int err;
+
+ cookie->errcode = 0;
+ cookie->done = 0;
+ init_waitqueue_head(&cookie->wait);
+ err = mutex_lock_interruptible(&hw->mbx.lock);
+ if (err)
+ return err;
+ err = mucse_write_mbx(hw, (u32 *)req,
+ L_WD(len));
+ if (err) {
+ mutex_unlock(&hw->mbx.lock);
+ return err;
+ }
+ do {
+ err = wait_event_interruptible_timeout(cookie->wait,
+ cookie->done == 1,
+ cookie->timeout_jiffes);
+ } while (err == -ERESTARTSYS);
+
+ mutex_unlock(&hw->mbx.lock);
+ if (!err)
+ err = -ETIME;
+ else
+ err = 0;
+ if (!err && cookie->errcode)
+ err = cookie->errcode;
+
+ return err;
+}
+
+/**
+ * mucse_mbx_ifinsmod - Echo driver insmod status to hw
+ * @hw: pointer to the HW structure
+ * @status: true for insmod, false for rmmod
+ *
+ * @return: 0 on success, negative on failure
+ **/
+int mucse_mbx_ifinsmod(struct mucse_hw *hw, int status)
+{
+ struct mbx_fw_cmd_reply reply;
+ struct mbx_fw_cmd_req req;
+ int len;
+ int err;
+
+ memset(&req, 0, sizeof(req));
+ memset(&reply, 0, sizeof(reply));
+ build_ifinsmod(&req, hw->driver_version, status);
+ len = le16_to_cpu(req.datalen) + MBX_REQ_HDR_LEN;
+ err = mutex_lock_interruptible(&hw->mbx.lock);
+ if (err)
+ return err;
+
+ if (status) {
+ err = hw->mbx.ops->write_posted(hw, (u32 *)&req,
+ L_WD(len));
+ } else {
+ err = hw->mbx.ops->write(hw, (u32 *)&req,
+ L_WD(len));
+ }
+
+ mutex_unlock(&hw->mbx.lock);
+ return err;
+}
+
+/**
+ * mucse_mbx_fw_reset_phy - Posts a mbx req to reset hw
+ * @hw: pointer to the HW structure
+ *
+ * mucse_mbx_fw_reset_phy posts a mbx req to firmware to reset hw.
+ * It uses mucse_fw_send_cmd_wait if no irq, and mucse_mbx_fw_post_req
+ * if other irq is registered.
+ *
+ * @return: 0 on success, negative on failure
+ **/
+int mucse_mbx_fw_reset_phy(struct mucse_hw *hw)
+{
+ struct mbx_fw_cmd_reply reply;
+ struct mbx_fw_cmd_req req;
+ int ret;
+
+ memset(&req, 0, sizeof(req));
+ memset(&reply, 0, sizeof(reply));
+ if (hw->mbx.irq_enabled) {
+ struct mbx_req_cookie *cookie = mbx_cookie_zalloc(0);
+
+ if (!cookie)
+ return -ENOMEM;
+
+ build_reset_phy_req(&req, cookie);
+ ret = mucse_mbx_fw_post_req(hw, &req, cookie);
+ kfree(cookie);
+ return ret;
+ }
+
+ build_reset_phy_req(&req, &req);
+ return mucse_fw_send_cmd_wait(hw, &req, &reply);
+}
+
+/**
+ * mucse_fw_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
+ * @lane: lane index
+ *
+ * mucse_fw_get_macaddr posts a mbx req to firmware to get mac_addr.
+ * It uses mucse_fw_send_cmd_wait if no irq, and mucse_mbx_fw_post_req
+ * if other irq is registered.
+ *
+ * @return: 0 on success, negative on failure
+ **/
+int mucse_fw_get_macaddr(struct mucse_hw *hw, int pfvfnum,
+ u8 *mac_addr,
+ int lane)
+{
+ struct mbx_fw_cmd_reply reply;
+ struct mbx_fw_cmd_req req;
+ int err;
+
+ memset(&req, 0, sizeof(req));
+ memset(&reply, 0, sizeof(reply));
+ build_get_macaddress_req(&req, 1 << lane, pfvfnum, &req);
+ err = mucse_fw_send_cmd_wait(hw, &req, &reply);
+ if (err)
+ return err;
+
+ if ((1 << lane) & le32_to_cpu(reply.mac_addr.lanes))
+ memcpy(mac_addr, reply.mac_addr.addrs[lane].mac, 6);
+ 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..2e33073bfe9b
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h
@@ -0,0 +1,201 @@
+/* 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 <linux/errno.h>
+#include <linux/wait.h>
+
+#include "rnpgbe.h"
+
+#define MBX_REQ_HDR_LEN 24
+#define L_WD(x) ((x) / 4)
+#define M_DEFAULT_DUMY 0xa0000000
+#define M_DUMY_MASK 0x0f000f11
+#define EVT_LINK_UP 1
+
+struct mbx_fw_cmd_reply;
+typedef void (*cookie_cb)(struct mbx_fw_cmd_reply *reply, void *priv);
+
+struct mbx_req_cookie {
+ int magic;
+#define COOKIE_MAGIC 0xCE
+ cookie_cb cb;
+ int timeout_jiffes;
+ int errcode;
+ wait_queue_head_t wait;
+ int done;
+ int priv_len;
+ char priv[];
+};
+
+enum MUCSE_FW_CMD {
+ GET_PHY_ABALITY = 0x0601,
+ GET_MAC_ADDRES = 0x0602,
+ RESET_PHY = 0x0603,
+ DRIVER_INSMOD = 0x0803,
+};
+
+struct hw_abilities {
+ u8 link_stat;
+ u8 lane_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_ability;
+ 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;
+
+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;
+}
+
+#define FLAGS_DD BIT(0)
+#define FLAGS_ERR BIT(2)
+
+/* Request is in little-endian format. Big-endian systems should be considered */
+struct mbx_fw_cmd_req {
+ __le16 flags;
+ __le16 opcode;
+ __le16 datalen;
+ __le16 ret_value;
+ union {
+ struct {
+ __le32 cookie_lo;
+ __le32 cookie_hi;
+ };
+
+ void *cookie;
+ };
+ __le32 reply_lo;
+ __le32 reply_hi;
+ union {
+ u8 data[32];
+ struct {
+ __le32 lane;
+ __le32 status;
+ } ifinsmod;
+ struct {
+ __le32 lane_mask;
+ __le32 pfvf_num;
+ } get_mac_addr;
+ };
+} __packed;
+
+struct mbx_fw_cmd_reply {
+ __le16 flags;
+ __le16 opcode;
+ __le16 error_code;
+ __le16 datalen;
+ union {
+ struct {
+ __le32 cookie_lo;
+ __le32 cookie_hi;
+ };
+ void *cookie;
+ };
+ union {
+ u8 data[40];
+ 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 hw_abilities hw_abilities;
+ };
+} __packed;
+
+static inline void build_phy_abalities_req(struct mbx_fw_cmd_req *req,
+ void *cookie)
+{
+ req->flags = 0;
+ req->opcode = cpu_to_le16(GET_PHY_ABALITY);
+ req->datalen = 0;
+ req->reply_lo = 0;
+ req->reply_hi = 0;
+ req->cookie = cookie;
+}
+
+static inline void build_ifinsmod(struct mbx_fw_cmd_req *req,
+ unsigned int lane,
+ int status)
+{
+ req->flags = 0;
+ req->opcode = cpu_to_le16(DRIVER_INSMOD);
+ req->datalen = cpu_to_le16(sizeof(req->ifinsmod));
+ req->cookie = NULL;
+ req->reply_lo = 0;
+ req->reply_hi = 0;
+ req->ifinsmod.lane = cpu_to_le32(lane);
+ req->ifinsmod.status = cpu_to_le32(status);
+}
+
+static inline void build_reset_phy_req(struct mbx_fw_cmd_req *req,
+ void *cookie)
+{
+ req->flags = 0;
+ req->opcode = cpu_to_le16(RESET_PHY);
+ req->datalen = 0;
+ req->reply_lo = 0;
+ req->reply_hi = 0;
+ req->cookie = cookie;
+}
+
+static inline void build_get_macaddress_req(struct mbx_fw_cmd_req *req,
+ int lane_mask, int pfvfnum,
+ void *cookie)
+{
+ req->flags = 0;
+ req->opcode = cpu_to_le16(GET_MAC_ADDRES);
+ req->datalen = cpu_to_le16(sizeof(req->get_mac_addr));
+ req->cookie = cookie;
+ req->reply_lo = 0;
+ req->reply_hi = 0;
+ req->get_mac_addr.lane_mask = cpu_to_le32(lane_mask);
+ req->get_mac_addr.pfvf_num = cpu_to_le32(pfvfnum);
+}
+
+int mucse_mbx_get_capability(struct mucse_hw *hw);
+int mucse_mbx_ifinsmod(struct mucse_hw *hw, int status);
+int mucse_mbx_fw_reset_phy(struct mucse_hw *hw);
+int mucse_fw_get_macaddr(struct mucse_hw *hw, int pfvfnum,
+ u8 *mac_addr, int lane);
+#endif /* _RNPGBE_MBX_FW_H */
--
2.25.1
On 12/08/25 3:09 pm, Dong Yibo wrote:
> +/**
> + * 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;
> +
> + memset(&req, 0, sizeof(req));
> + memset(&reply, 0, sizeof(reply));
> + build_phy_abalities_req(&req, &req);
Typo in function name. You probably meant "build_phy_abilities_req".
> + err = mucse_fw_send_cmd_wait(hw, &req, &reply);
> + if (!err)
> + 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.
> + *
Typo in comment: "tries to some capabities" should be "tries to get
capabilities"
> + * @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;
> +
> + memset(&ability, 0, sizeof(ability));
> + while (try_cnt--) {
> + err = mucse_fw_get_capability(hw, &ability);
> + if (err)
> + continue;
> + hw->pfvfnum = le16_to_cpu(ability.pfnum);
> + hw->fw_version = le32_to_cpu(ability.fw_version);
> + hw->axi_mhz = le32_to_cpu(ability.axi_mhz);
> + hw->bd_uid = le32_to_cpu(ability.bd_uid);
> + return 0;
> + }
> + return err;
> +}
Missing initialization of err variable before the last return, which
could lead to undefined behavior if all attempts fail.
> +
> +/**
> + * mbx_cookie_zalloc - Alloc a cookie structure
> + * @priv_len: private length for this cookie
> + *
--
Thanks and Regards,
Danish
On Wed, Aug 13, 2025 at 01:50:08PM +0530, MD Danish Anwar wrote:
> On 12/08/25 3:09 pm, Dong Yibo wrote:
> > +/**
> > + * 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;
> > +
> > + memset(&req, 0, sizeof(req));
> > + memset(&reply, 0, sizeof(reply));
> > + build_phy_abalities_req(&req, &req);
>
> Typo in function name. You probably meant "build_phy_abilities_req".
>
You are right, I will update it.
> > + err = mucse_fw_send_cmd_wait(hw, &req, &reply);
> > + if (!err)
> > + 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.
> > + *
>
> Typo in comment: "tries to some capabities" should be "tries to get
> capabilities"
>
Got it, I will fix it.
> > + * @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;
> > +
> > + memset(&ability, 0, sizeof(ability));
> > + while (try_cnt--) {
> > + err = mucse_fw_get_capability(hw, &ability);
> > + if (err)
> > + continue;
> > + hw->pfvfnum = le16_to_cpu(ability.pfnum);
> > + hw->fw_version = le32_to_cpu(ability.fw_version);
> > + hw->axi_mhz = le32_to_cpu(ability.axi_mhz);
> > + hw->bd_uid = le32_to_cpu(ability.bd_uid);
> > + return 0;
> > + }
> > + return err;
> > +}
>
>
> Missing initialization of err variable before the last return, which
> could lead to undefined behavior if all attempts fail.
>
Got it, I will init it by 'int err = -EIO'.
> > +
> > +/**
> > + * mbx_cookie_zalloc - Alloc a cookie structure
> > + * @priv_len: private length for this cookie
> > + *
>
>
> --
> Thanks and Regards,
> Danish
>
>
Thanks for your feedback.
On 12/08/2025 10:39, Dong Yibo wrote:
> Initialize basic mbx_fw ops, such as get_capability, reset phy
> and so on.
>
> Signed-off-by: Dong Yibo <dong100@mucse.com>
> +static int mucse_fw_send_cmd_wait(struct mucse_hw *hw,
> + struct mbx_fw_cmd_req *req,
> + struct mbx_fw_cmd_reply *reply)
> +{
> + int len = le16_to_cpu(req->datalen) + MBX_REQ_HDR_LEN;
> + int retry_cnt = 3;
> + int err;
> +
> + err = mutex_lock_interruptible(&hw->mbx.lock);
> + if (err)
> + return err;
> + err = hw->mbx.ops->write_posted(hw, (u32 *)req,
> + L_WD(len));
> + if (err) {> + mutex_unlock(&hw->mbx.lock);
> + return err;
> + }
it might look a bit cleaner if you add error label and have unlock code
once in the end of the function...
> + do {
> + err = hw->mbx.ops->read_posted(hw, (u32 *)reply,
> + L_WD(sizeof(*reply)));
> + if (err) {
> + mutex_unlock(&hw->mbx.lock);
> + return err;
> + }
> + } while (--retry_cnt >= 0 && reply->opcode != req->opcode);
> + mutex_unlock(&hw->mbx.lock);
> + if (retry_cnt < 0 || reply->error_code)
> + return -EIO;
> + 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;
> +
> + memset(&req, 0, sizeof(req));
> + memset(&reply, 0, sizeof(reply));
probably
struct mbx_fw_cmd_reply reply = {};
struct mbx_fw_cmd_req req = {};
will look better. in the other functions as well..
On Tue, Aug 12, 2025 at 05:14:15PM +0100, Vadim Fedorenko wrote:
> On 12/08/2025 10:39, Dong Yibo wrote:
> > Initialize basic mbx_fw ops, such as get_capability, reset phy
> > and so on.
> >
> > Signed-off-by: Dong Yibo <dong100@mucse.com>
> > +static int mucse_fw_send_cmd_wait(struct mucse_hw *hw,
> > + struct mbx_fw_cmd_req *req,
> > + struct mbx_fw_cmd_reply *reply)
> > +{
> > + int len = le16_to_cpu(req->datalen) + MBX_REQ_HDR_LEN;
> > + int retry_cnt = 3;
> > + int err;
> > +
> > + err = mutex_lock_interruptible(&hw->mbx.lock);
> > + if (err)
> > + return err;
> > + err = hw->mbx.ops->write_posted(hw, (u32 *)req,
> > + L_WD(len));
> > + if (err) {> + mutex_unlock(&hw->mbx.lock);
> > + return err;
> > + }
>
> it might look a bit cleaner if you add error label and have unlock code
> once in the end of the function...
>
If it is more cleaner bellow?
static int mucse_fw_send_cmd_wait(struct mucse_hw *hw,
struct mbx_fw_cmd_req *req,
struct mbx_fw_cmd_reply *reply)
{
int len = le16_to_cpu(req->datalen) + MBX_REQ_HDR_LEN;
int retry_cnt = 3;
int err;
err = mutex_lock_interruptible(&hw->mbx.lock);
if (err)
return err;
err = hw->mbx.ops->write_posted(hw, (u32 *)req,
L_WD(len));
if (err)
goto quit;
do {
err = hw->mbx.ops->read_posted(hw, (u32 *)reply,
L_WD(sizeof(*reply)));
if (err)
goto quit;
} while (--retry_cnt >= 0 && reply->opcode != req->opcode);
mutex_unlock(&hw->mbx.lock);
if (retry_cnt < 0)
return -ETIMEDOUT;
if (reply->error_code)
return -EIO;
return 0;
quit:
mutex_unlock(&hw->mbx.lock);
return err;
}
> If it is more cleaner bellow?
>
> static int mucse_fw_send_cmd_wait(struct mucse_hw *hw,
> struct mbx_fw_cmd_req *req,
> struct mbx_fw_cmd_reply *reply)
> {
> int len = le16_to_cpu(req->datalen) + MBX_REQ_HDR_LEN;
> int retry_cnt = 3;
> int err;
>
> err = mutex_lock_interruptible(&hw->mbx.lock);
> if (err)
> return err;
> err = hw->mbx.ops->write_posted(hw, (u32 *)req,
> L_WD(len));
> if (err)
> goto quit;
> do {
> err = hw->mbx.ops->read_posted(hw, (u32 *)reply,
> L_WD(sizeof(*reply)));
> if (err)
> goto quit;
> } while (--retry_cnt >= 0 && reply->opcode != req->opcode);
>
> mutex_unlock(&hw->mbx.lock);
> if (retry_cnt < 0)
> return -ETIMEDOUT;
> if (reply->error_code)
> return -EIO;
> return 0;
> quit:
> mutex_unlock(&hw->mbx.lock);
> return err;
> }
You might want a read a few other drivers in mailine. Look at the
naming. I doubt you will find many using "quit" for a label. "out" or
"unlock" is more popular.
When it comes to locks, it is better to have one lock statement and
one unlock statement. It then becomes easy to see all paths lead to
the unlock.
Andrew
On Fri, Aug 15, 2025 at 02:04:57AM +0200, Andrew Lunn wrote:
> > If it is more cleaner bellow?
> >
> > static int mucse_fw_send_cmd_wait(struct mucse_hw *hw,
> > struct mbx_fw_cmd_req *req,
> > struct mbx_fw_cmd_reply *reply)
> > {
> > int len = le16_to_cpu(req->datalen) + MBX_REQ_HDR_LEN;
> > int retry_cnt = 3;
> > int err;
> >
> > err = mutex_lock_interruptible(&hw->mbx.lock);
> > if (err)
> > return err;
> > err = hw->mbx.ops->write_posted(hw, (u32 *)req,
> > L_WD(len));
> > if (err)
> > goto quit;
> > do {
> > err = hw->mbx.ops->read_posted(hw, (u32 *)reply,
> > L_WD(sizeof(*reply)));
> > if (err)
> > goto quit;
> > } while (--retry_cnt >= 0 && reply->opcode != req->opcode);
> >
> > mutex_unlock(&hw->mbx.lock);
> > if (retry_cnt < 0)
> > return -ETIMEDOUT;
> > if (reply->error_code)
> > return -EIO;
> > return 0;
> > quit:
> > mutex_unlock(&hw->mbx.lock);
> > return err;
> > }
>
> You might want a read a few other drivers in mailine. Look at the
> naming. I doubt you will find many using "quit" for a label. "out" or
> "unlock" is more popular.
>
> When it comes to locks, it is better to have one lock statement and
> one unlock statement. It then becomes easy to see all paths lead to
> the unlock.
>
> Andrew
>
Got it, I will change label 'quit' to 'out'.
And I will try to keep 'one lock statement and one unlock statement'
principle in mind.
Thanks for your feedback.
On 13/08/2025 10:52, Yibo Dong wrote:
> On Tue, Aug 12, 2025 at 05:14:15PM +0100, Vadim Fedorenko wrote:
>> On 12/08/2025 10:39, Dong Yibo wrote:
>>> Initialize basic mbx_fw ops, such as get_capability, reset phy
>>> and so on.
>>>
>>> Signed-off-by: Dong Yibo <dong100@mucse.com>
>>> +static int mucse_fw_send_cmd_wait(struct mucse_hw *hw,
>>> + struct mbx_fw_cmd_req *req,
>>> + struct mbx_fw_cmd_reply *reply)
>>> +{
>>> + int len = le16_to_cpu(req->datalen) + MBX_REQ_HDR_LEN;
>>> + int retry_cnt = 3;
>>> + int err;
>>> +
>>> + err = mutex_lock_interruptible(&hw->mbx.lock);
>>> + if (err)
>>> + return err;
>>> + err = hw->mbx.ops->write_posted(hw, (u32 *)req,
>>> + L_WD(len));
>>> + if (err) {> + mutex_unlock(&hw->mbx.lock);
>>> + return err;
>>> + }
>>
>> it might look a bit cleaner if you add error label and have unlock code
>> once in the end of the function...
>>
>
> If it is more cleaner bellow?
>
> static int mucse_fw_send_cmd_wait(struct mucse_hw *hw,
> struct mbx_fw_cmd_req *req,
> struct mbx_fw_cmd_reply *reply)
> {
> int len = le16_to_cpu(req->datalen) + MBX_REQ_HDR_LEN;
> int retry_cnt = 3;
> int err;
>
> err = mutex_lock_interruptible(&hw->mbx.lock);
> if (err)
> return err;
> err = hw->mbx.ops->write_posted(hw, (u32 *)req,
> L_WD(len));
> if (err)
> goto quit;
> do {
> err = hw->mbx.ops->read_posted(hw, (u32 *)reply,
> L_WD(sizeof(*reply)));
> if (err)
> goto quit;
> } while (--retry_cnt >= 0 && reply->opcode != req->opcode);
>
> mutex_unlock(&hw->mbx.lock);
> if (retry_cnt < 0)
> return -ETIMEDOUT;
> if (reply->error_code)
> return -EIO;
> return 0;
> quit:
> mutex_unlock(&hw->mbx.lock);
> return err;
> }
>
Maybe:
} while (--retry_cnt >= 0 && reply->opcode != req->opcode);
quit:
mutex_unlock(&hw->mbx.lock);
if (!err && retry_cnt < 0)
return -ETIMEDOUT;
if (!err && reply->error_code)
return -EIO;
return err;
looks cleaner
On Wed, Aug 13, 2025 at 02:33:39PM +0100, Vadim Fedorenko wrote:
> On 13/08/2025 10:52, Yibo Dong wrote:
> > On Tue, Aug 12, 2025 at 05:14:15PM +0100, Vadim Fedorenko wrote:
> > > On 12/08/2025 10:39, Dong Yibo wrote:
> > > > Initialize basic mbx_fw ops, such as get_capability, reset phy
> > > > and so on.
> > > >
> > > > Signed-off-by: Dong Yibo <dong100@mucse.com>
> > > > +static int mucse_fw_send_cmd_wait(struct mucse_hw *hw,
> > > > + struct mbx_fw_cmd_req *req,
> > > > + struct mbx_fw_cmd_reply *reply)
> > > > +{
> > > > + int len = le16_to_cpu(req->datalen) + MBX_REQ_HDR_LEN;
> > > > + int retry_cnt = 3;
> > > > + int err;
> > > > +
> > > > + err = mutex_lock_interruptible(&hw->mbx.lock);
> > > > + if (err)
> > > > + return err;
> > > > + err = hw->mbx.ops->write_posted(hw, (u32 *)req,
> > > > + L_WD(len));
> > > > + if (err) {> + mutex_unlock(&hw->mbx.lock);
> > > > + return err;
> > > > + }
> > >
> > > it might look a bit cleaner if you add error label and have unlock code
> > > once in the end of the function...
> > >
> >
> > If it is more cleaner bellow?
> >
> > static int mucse_fw_send_cmd_wait(struct mucse_hw *hw,
> > struct mbx_fw_cmd_req *req,
> > struct mbx_fw_cmd_reply *reply)
> > {
> > int len = le16_to_cpu(req->datalen) + MBX_REQ_HDR_LEN;
> > int retry_cnt = 3;
> > int err;
> >
> > err = mutex_lock_interruptible(&hw->mbx.lock);
> > if (err)
> > return err;
> > err = hw->mbx.ops->write_posted(hw, (u32 *)req,
> > L_WD(len));
> > if (err)
> > goto quit;
> > do {
> > err = hw->mbx.ops->read_posted(hw, (u32 *)reply,
> > L_WD(sizeof(*reply)));
> > if (err)
> > goto quit;
> > } while (--retry_cnt >= 0 && reply->opcode != req->opcode);
> >
> > mutex_unlock(&hw->mbx.lock);
> > if (retry_cnt < 0)
> > return -ETIMEDOUT;
> > if (reply->error_code)
> > return -EIO;
> > return 0;
> > quit:
> > mutex_unlock(&hw->mbx.lock);
> > return err;
> > }
> >
>
> Maybe:
>
> } while (--retry_cnt >= 0 && reply->opcode != req->opcode);
>
> quit:
> mutex_unlock(&hw->mbx.lock);
> if (!err && retry_cnt < 0)
> return -ETIMEDOUT;
> if (!err && reply->error_code)
> return -EIO;
> return err;
>
>
> looks cleaner
>
>
Got it, I will update this, thanks.
Hi Yibo,
On Wed, 13 Aug 2025 at 11:52, Yibo Dong <dong100@mucse.com> wrote:
> On Tue, Aug 12, 2025 at 05:14:15PM +0100, Vadim Fedorenko wrote:
> > On 12/08/2025 10:39, Dong Yibo wrote:
> > > Initialize basic mbx_fw ops, such as get_capability, reset phy
> > > and so on.
> > >
> > > Signed-off-by: Dong Yibo <dong100@mucse.com>
> > > +static int mucse_fw_send_cmd_wait(struct mucse_hw *hw,
> > > + struct mbx_fw_cmd_req *req,
> > > + struct mbx_fw_cmd_reply *reply)
> > > +{
> > > + int len = le16_to_cpu(req->datalen) + MBX_REQ_HDR_LEN;
> > > + int retry_cnt = 3;
> > > + int err;
> > > +
> > > + err = mutex_lock_interruptible(&hw->mbx.lock);
> > > + if (err)
> > > + return err;
> > > + err = hw->mbx.ops->write_posted(hw, (u32 *)req,
> > > + L_WD(len));
> > > + if (err) {> + mutex_unlock(&hw->mbx.lock);
> > > + return err;
> > > + }
> >
> > it might look a bit cleaner if you add error label and have unlock code
> > once in the end of the function...
> >
>
> If it is more cleaner bellow?
>
> static int mucse_fw_send_cmd_wait(struct mucse_hw *hw,
> struct mbx_fw_cmd_req *req,
> struct mbx_fw_cmd_reply *reply)
> {
> int len = le16_to_cpu(req->datalen) + MBX_REQ_HDR_LEN;
> int retry_cnt = 3;
> int err;
>
> err = mutex_lock_interruptible(&hw->mbx.lock);
> if (err)
> return err;
> err = hw->mbx.ops->write_posted(hw, (u32 *)req,
> L_WD(len));
> if (err)
> goto quit;
> do {
> err = hw->mbx.ops->read_posted(hw, (u32 *)reply,
> L_WD(sizeof(*reply)));
> if (err)
> goto quit;
> } while (--retry_cnt >= 0 && reply->opcode != req->opcode);
>
> mutex_unlock(&hw->mbx.lock);
> if (retry_cnt < 0)
> return -ETIMEDOUT;
> if (reply->error_code)
> return -EIO;
> return 0;
> quit:
> mutex_unlock(&hw->mbx.lock);
> return err;
> }
Or use scoped_cond_guard(mutex_intr, ...) { ... }?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Wed, 13 Aug 2025 13:05:36 +0200 Geert Uytterhoeven wrote:
> Or use scoped_cond_guard(mutex_intr, ...) { ... }?
Hard no on that in networking.
On Tue, Aug 12, 2025 at 05:14:15PM +0100, Vadim Fedorenko wrote:
> On 12/08/2025 10:39, Dong Yibo wrote:
> > Initialize basic mbx_fw ops, such as get_capability, reset phy
> > and so on.
> >
> > Signed-off-by: Dong Yibo <dong100@mucse.com>
> > +static int mucse_fw_send_cmd_wait(struct mucse_hw *hw,
> > + struct mbx_fw_cmd_req *req,
> > + struct mbx_fw_cmd_reply *reply)
> > +{
> > + int len = le16_to_cpu(req->datalen) + MBX_REQ_HDR_LEN;
> > + int retry_cnt = 3;
> > + int err;
> > +
> > + err = mutex_lock_interruptible(&hw->mbx.lock);
> > + if (err)
> > + return err;
> > + err = hw->mbx.ops->write_posted(hw, (u32 *)req,
> > + L_WD(len));
> > + if (err) {> + mutex_unlock(&hw->mbx.lock);
> > + return err;
> > + }
>
> it might look a bit cleaner if you add error label and have unlock code
> once in the end of the function...
>
Ok, I will try to update this.
> > + do {
> > + err = hw->mbx.ops->read_posted(hw, (u32 *)reply,
> > + L_WD(sizeof(*reply)));
> > + if (err) {
> > + mutex_unlock(&hw->mbx.lock);
> > + return err;
> > + }
> > + } while (--retry_cnt >= 0 && reply->opcode != req->opcode);
> > + mutex_unlock(&hw->mbx.lock);
> > + if (retry_cnt < 0 || reply->error_code)
> > + return -EIO;
> > + 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;
> > +
> > + memset(&req, 0, sizeof(req));
> > + memset(&reply, 0, sizeof(reply));
>
> probably
>
> struct mbx_fw_cmd_reply reply = {};
> struct mbx_fw_cmd_req req = {};
>
> will look better. in the other functions as well..
>
>
>
Got it, I will update it.
Thanks for your feedback.
© 2016 - 2025 Red Hat, Inc.