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

This is just min(size, mbx->size). There's no need to open code min().

+
+       if (!mbx->ops.read)
+               return -EIO;
+
+       return mbx->ops.read(hw, msg, size, mbx_id);
+}
+
+/**
+ * mucse_write_mbx - Write a message to the mailbox
+ * @hw: Pointer to the HW structure
+ * @msg: The message buffer
+ * @size: Length of buffer
+ * @mbx_id: Id of vf/fw to write
+ *
+ * @return: 0 on success, negative on failure
+ **/
+int mucse_write_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
+                   enum MBX_ID mbx_id)
+{
+       struct mucse_mbx_info *mbx = &hw->mbx;
+
+       if (size > mbx->size)
+               return -EINVAL;
+
+       if (!mbx->ops.write)
+               return -EIO;
+
+       return mbx->ops.write(hw, msg, size, mbx_id);
+}
+
+/**
+ * mucse_mbx_get_req - Read req from reg
+ * @hw: Pointer to the HW structure
+ * @reg: Register to read
+ *
+ * @return: the req value
+ **/
+static u16 mucse_mbx_get_req(struct mucse_hw *hw, int reg)
+{
+       /* force memory barrier */
+       mb();
+       return ioread32(hw->hw_addr + reg) & GENMASK(15, 0);
+}
+
+/**
+ * mucse_mbx_get_ack - Read ack from reg
+ * @hw: Pointer to the HW structure
+ * @reg: Register to read
+ *
+ * @return: the ack value
+ **/
+static u16 mucse_mbx_get_ack(struct mucse_hw *hw, int reg)
+{
+       /* force memory barrier */
+       mb();
+       return (mbx_rd32(hw, reg) >> 16);
+}
+
+/**
+ * mucse_mbx_inc_pf_req - Increase req
+ * @hw: Pointer to the HW structure
+ * @mbx_id: Id of vf/fw to read
+ *
+ * mucse_mbx_inc_pf_req read pf_req from hw, then write
+ * new value back after increase
+ **/
+static void mucse_mbx_inc_pf_req(struct mucse_hw *hw,
+                                enum MBX_ID mbx_id)
+{
+       struct mucse_mbx_info *mbx = &hw->mbx;
+       u32 reg, v;
+       u16 req;
+
+       reg = (mbx_id == MBX_FW) ? PF2FW_COUNTER(mbx) :
+                                  PF2VF_COUNTER(mbx, mbx_id);
+       v = mbx_rd32(hw, reg);
+       req = (v & GENMASK(15, 0));
+       req++;
+       v &= GENMASK(31, 16);
+       v |= req;
+       /* force before write to hw */
+       mb();
+       mbx_wr32(hw, reg, v);
+       /* update stats */

Nit, but this comment is unnecessary. Same comment for all of the other 
identical comments. They are just repeating "what" you are doing.

+       hw->mbx.stats.msgs_tx++;
+}
+
+/**
+ * mucse_mbx_inc_pf_ack - Increase ack
+ * @hw: Pointer to the HW structure
+ * @mbx_id: Id of vf/fw to read
+ *
+ * mucse_mbx_inc_pf_ack read pf_ack from hw, then write
+ * new value back after increase
+ **/
+static void mucse_mbx_inc_pf_ack(struct mucse_hw *hw,
+                                enum MBX_ID mbx_id)
+{
+       struct mucse_mbx_info *mbx = &hw->mbx;
+       u32 reg, v;
+       u16 ack;
+
+       reg = (mbx_id == MBX_FW) ? PF2FW_COUNTER(mbx) :
+                                  PF2VF_COUNTER(mbx, mbx_id);
+       v = mbx_rd32(hw, reg);
+       ack = (v >> 16) & GENMASK(15, 0);
+       ack++;
+       v &= GENMASK(15, 0);
+       v |= (ack << 16);
+       /* force before write to hw */
+       mb();
+       mbx_wr32(hw, reg, v);
+       /* update stats */
+       hw->mbx.stats.msgs_rx++;
+}
+
+/**
+ * mucse_check_for_msg - Checks to see if vf/fw sent us mail
+ * @hw: Pointer to the HW structure
+ * @mbx_id: Id of vf/fw to check
+ *
+ * @return: 0 on success, negative on failure
+ **/
+int mucse_check_for_msg(struct mucse_hw *hw, enum MBX_ID mbx_id)
+{
+       struct mucse_mbx_info *mbx = &hw->mbx;
+
+       if (!mbx->ops.check_for_msg)
+               return -EIO;
+
+       return mbx->ops.check_for_msg(hw, mbx_id);
+}
+
+/**
+ * mucse_check_for_ack - Checks to see if vf/fw sent us ACK
+ * @hw: Pointer to the HW structure
+ * @mbx_id: Id of vf/fw to check
+ *
+ * @return: 0 on success, negative on failure
+ **/
+int mucse_check_for_ack(struct mucse_hw *hw, enum MBX_ID mbx_id)
+{
+       struct mucse_mbx_info *mbx = &hw->mbx;
+
+       if (!mbx->ops.check_for_ack)
+               return -EIO;
+       return mbx->ops.check_for_ack(hw, mbx_id);
+}
+
+/**
+ * mucse_poll_for_msg - Wait for message notification
+ * @hw: Pointer to the HW structure
+ * @mbx_id: Id of vf/fw to poll
+ *
+ * @return: 0 on success, negative on failure
+ **/
+static int mucse_poll_for_msg(struct mucse_hw *hw, enum MBX_ID mbx_id)
+{
+       struct mucse_mbx_info *mbx = &hw->mbx;
+       int countdown = mbx->timeout;
+       int val;
+
+       if (!countdown || !mbx->ops.check_for_msg)
+               return -EIO;
+
+       return read_poll_timeout(mbx->ops.check_for_msg,
+                                val, val == 0, mbx->usec_delay,
+                                countdown * mbx->usec_delay,
+                                false, hw, mbx_id);
+}
+
+/**
+ * mucse_poll_for_ack - Wait for message acknowledgment
+ * @hw: Pointer to the HW structure
+ * @mbx_id: Id of vf/fw to poll
+ *
+ * @return: 0 if it successfully received a message acknowledgment
+ **/
+static int mucse_poll_for_ack(struct mucse_hw *hw, enum MBX_ID mbx_id)
+{
+       struct mucse_mbx_info *mbx = &hw->mbx;
+       int countdown = mbx->timeout;
+       int val;
+
+       if (!countdown || !mbx->ops.check_for_ack)
+               return -EIO;
+
+       return read_poll_timeout(mbx->ops.check_for_ack,
+                                val, val == 0, mbx->usec_delay,
+                                countdown * mbx->usec_delay,
+                                false, hw, mbx_id);
+}
+
+/**
+ * mucse_read_posted_mbx - Wait for message notification and receive message
+ * @hw: Pointer to the HW structure
+ * @msg: The message buffer
+ * @size: Length of buffer
+ * @mbx_id: Id of vf/fw to read
+ *
+ * @return: 0 if it successfully received a message notification and
+ * copied it into the receive buffer.
+ **/
+static int mucse_read_posted_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
+                                enum MBX_ID mbx_id)
+{
+       struct mucse_mbx_info *mbx = &hw->mbx;
+       int ret_val;
+
+       if (!mbx->ops.read)
+               return -EIO;
+
+       ret_val = mucse_poll_for_msg(hw, mbx_id);
+
+       /* if ack received read message, otherwise we timed out */
+       if (!ret_val)
+               ret_val = mbx->ops.read(hw, msg, size, mbx_id);
+
+       return ret_val;

IMO a more typical flow would be the following:

ret_val = mucse_poll_for_msg();
if (ret_val)
	return ret_val;

return mbx->ops.read();

+}
+
+/**
+ * mucse_write_posted_mbx - Write a message to the mailbox, wait for ack
+ * @hw: Pointer to the HW structure
+ * @msg: The message buffer
+ * @size: Length of buffer
+ * @mbx_id: Id of vf/fw to write
+ *
+ * @return: 0 if it successfully copied message into the buffer and
+ * received an ack to that message within delay * timeout period
+ **/
+static int mucse_write_posted_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
+                                 enum MBX_ID mbx_id)
+{
+       struct mucse_mbx_info *mbx = &hw->mbx;
+       int ret_val;
+
+       /* exit if either we can't write or there isn't a defined timeout */
+       if (!mbx->ops.write || !mbx->timeout)
+               return -EIO;
+
+       /* send msg and hold buffer lock */
+       ret_val = mbx->ops.write(hw, msg, size, mbx_id);
+
+       /* if msg sent wait until we receive an ack */
+       if (!ret_val)
+               ret_val = mucse_poll_for_ack(hw, mbx_id);
+
+       return ret_val;

IMO a more typical flow would be the following:

ret_val = mbx->ops.write();
if (ret_val)
	return ret_val;

return mucse_poll_for_ack();

+}
+
+/**
+ * mucse_check_for_msg_pf - checks to see if the vf/fw has sent mail
+ * @hw: Pointer to the HW structure
+ * @mbx_id: Id of vf/fw to check
+ *
+ * @return: 0 if the vf/fw has set the Status bit or else
+ * -EIO
+ **/
+static int mucse_check_for_msg_pf(struct mucse_hw *hw,
+                                 enum MBX_ID mbx_id)
+{
+       struct mucse_mbx_info *mbx = &hw->mbx;
+       u16 hw_req_count = 0;
+       int ret_val = -EIO;
+
+       if (mbx_id == MBX_FW) {
+               hw_req_count = mucse_mbx_get_req(hw, FW2PF_COUNTER(mbx));
+               /* reg in hw should avoid 0 check */

This comment explains "what" you are doing/preventing, but if the 
comment is necessary it should explain "why" it's being done.

+               if (mbx->mbx_feature & MBX_FEATURE_NO_ZERO) {
+                       if (hw_req_count != 0 &&
+                           hw_req_count != hw->mbx.fw_req) {
+                               ret_val = 0;
+                               hw->mbx.stats.reqs++;
+                       }
+               } else {
+                       if (hw_req_count != hw->mbx.fw_req) {
+                               ret_val = 0;
+                               hw->mbx.stats.reqs++;
+                       }
+               }
+       } else {
+               if (mucse_mbx_get_req(hw, VF2PF_COUNTER(mbx, mbx_id)) !=
+                   hw->mbx.vf_req[mbx_id]) {
+                       ret_val = 0;
+                       hw->mbx.stats.reqs++;
+               }
+       }
+
+       return ret_val;

Nit, but ret_val isn't really needed in this function. You can just 
return 0 in all of the places where you set ret_val to 0. If you get 
here you can "return -EIO".

+}
+
+/**
+ * mucse_check_for_ack_pf - checks to see if the VF has ACKed
+ * @hw: Pointer to the HW structure
+ * @mbx_id: Id of vf/fw to check
+ *
+ * @return: 0 if the vf/fw has set the Status bit or else
+ * -EIO
+ **/
+static int mucse_check_for_ack_pf(struct mucse_hw *hw, enum MBX_ID mbx_id)
+{
+       struct mucse_mbx_info *mbx = &hw->mbx;
+       int ret_val = -EIO;
+       u16 hw_fw_ack;
+
+       if (mbx_id == MBX_FW) {
+               hw_fw_ack = mucse_mbx_get_ack(hw, FW2PF_COUNTER(mbx));
+               if (hw_fw_ack != 0 &&
+                   hw_fw_ack != hw->mbx.fw_ack) {
+                       ret_val = 0;
+                       hw->mbx.stats.acks++;
+               }
+       } else {
+               if (mucse_mbx_get_ack(hw, VF2PF_COUNTER(mbx, mbx_id)) !=
+                   hw->mbx.vf_ack[mbx_id]) {
+                       ret_val = 0;
+                       hw->mbx.stats.acks++;
+               }
+       }
+
+       return ret_val;

Ditto on ret_val being unnecessary.

+}
+
+/**
+ * mucse_obtain_mbx_lock_pf - obtain mailbox lock
+ * @hw: pointer to the HW structure
+ * @mbx_id: Id of vf/fw to obtain
+ *
+ * This function maybe used in an irq handler.

Nit, s/maybe/may be/

+ *
+ * @return: 0 if we obtained the mailbox lock
+ **/
+static int mucse_obtain_mbx_lock_pf(struct mucse_hw *hw, enum MBX_ID mbx_id)
+{
+       struct mucse_mbx_info *mbx = &hw->mbx;
+       int try_cnt = 5000, ret;
+       u32 reg;
+
+       reg = (mbx_id == MBX_FW) ? PF2FW_MBOX_CTRL(mbx) :
+                                  PF2VF_MBOX_CTRL(mbx, mbx_id);
+       while (try_cnt-- > 0) {
+               /* Take ownership of the buffer */
+               mbx_wr32(hw, reg, MBOX_PF_HOLD);
+               /* force write back before check */
+               wmb();
+               if (mbx_rd32(hw, reg) & MBOX_PF_HOLD)
+                       return 0;
+               udelay(100);
+       }
+       return ret;

Just as Andrew said, this is uninitialized. I don't think ret is needed 
at all in this function.

Please think about whether local variables are needed or not in the rest 
of the patches.

In this case you return 0 right away on success and can return 
-ETIMEDOUT (or whatever is appropriate) on failure.

+}
+
+/**
+ * mucse_write_mbx_pf - Places a message in the mailbox
+ * @hw: pointer to the HW structure
+ * @msg: The message buffer
+ * @size: Length of buffer
+ * @mbx_id: Id of vf/fw to write
+ *
+ * This function maybe used in an irq handler.
+ *
+ * @return: 0 if it successfully copied message into the buffer
+ **/
+static int mucse_write_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size,
+                             enum MBX_ID mbx_id)
+{
+       struct mucse_mbx_info *mbx = &hw->mbx;
+       u32 data_reg, ctrl_reg;
+       int ret_val = 0;
+       u16 i;
+
+       data_reg = (mbx_id == MBX_FW) ? FW_PF_SHM_DATA(mbx) :
+                                       PF_VF_SHM_DATA(mbx, mbx_id);
+       ctrl_reg = (mbx_id == MBX_FW) ? PF2FW_MBOX_CTRL(mbx) :
+                                       PF2VF_MBOX_CTRL(mbx, mbx_id);
+       if (size > MUCSE_VFMAILBOX_SIZE)
+               return -EINVAL;
+
+       /* lock the mailbox to prevent pf/vf/fw race condition */
+       ret_val = mucse_obtain_mbx_lock_pf(hw, mbx_id);
+       if (ret_val)
+               goto out_no_write;

Just return directly here. No need for a goto.

+
+       /* copy the caller specified message to the mailbox memory buffer */
+       for (i = 0; i < size; i++)
+               mbx_wr32(hw, data_reg + i * 4, msg[i]);
+
+       /* flush msg and acks as we are overwriting the message buffer */
+       if (mbx_id == MBX_FW) {
+               hw->mbx.fw_ack = mucse_mbx_get_ack(hw, FW2PF_COUNTER(mbx));
+       } else {
+               hw->mbx.vf_ack[mbx_id] =
+                       mucse_mbx_get_ack(hw, VF2PF_COUNTER(mbx, mbx_id));
+       }
+       mucse_mbx_inc_pf_req(hw, mbx_id);
+
+       /* Interrupt VF/FW to tell it a message
+        * has been sent and release buffer
+        */
+       if (mbx->mbx_feature & MBX_FEATURE_WRITE_DELAY)
+               udelay(300);

This delay seems arbitrary. How do you know 300us is sufficient?

+       mbx_wr32(hw, ctrl_reg, MBOX_CTRL_REQ);
+
+out_no_write:

This label isn't required, unless it's used in a future patch. If that's 
the case introduce it in the future patch.

+       return ret_val;
+}
+
+/**
+ * mucse_read_mbx_pf - Read a message from the mailbox
+ * @hw: pointer to the HW structure
+ * @msg: The message buffer
+ * @size: Length of buffer
+ * @mbx_id: Id of vf/fw to read
+ *
+ * This function copies a message from the mailbox buffer to the caller's
+ * memory buffer.  The presumption is that the caller knows that there was
+ * a message due to a vf/fw request so no polling for message is needed.
+ *
+ * @return: 0 on success, negative on failure
+ **/
+static int mucse_read_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size,
+                            enum MBX_ID mbx_id)
+{
+       struct mucse_mbx_info *mbx = &hw->mbx;
+       u32 data_reg, ctrl_reg;
+       int ret_val;
+       u32 i;
+
+       data_reg = (mbx_id == MBX_FW) ? FW_PF_SHM_DATA(mbx) :
+                                       PF_VF_SHM_DATA(mbx, mbx_id);
+       ctrl_reg = (mbx_id == MBX_FW) ? PF2FW_MBOX_CTRL(mbx) :
+                                       PF2VF_MBOX_CTRL(mbx, mbx_id);
+
+       if (size > MUCSE_VFMAILBOX_SIZE)
+               return -EINVAL;
+       /* lock the mailbox to prevent pf/vf race condition */
+       ret_val = mucse_obtain_mbx_lock_pf(hw, mbx_id);
+       if (ret_val)
+               goto out_no_read;
+
+       /* we need this */

This comment doesn't seem useful at all. Why do you need this comment?

+       mb();
+       /* copy the message from the mailbox memory buffer */
+       for (i = 0; i < size; i++)
+               msg[i] = mbx_rd32(hw, data_reg + 4 * i);
+       mbx_wr32(hw, data_reg, 0);
+
+       /* update req */
+       if (mbx_id == MBX_FW) {
+               hw->mbx.fw_req = mucse_mbx_get_req(hw, FW2PF_COUNTER(mbx));
+       } else {
+               hw->mbx.vf_req[mbx_id] =
+                       mucse_mbx_get_req(hw, VF2PF_COUNTER(mbx, mbx_id));
+       }
+       /* Acknowledge receipt and release mailbox, then we're done */
+       mucse_mbx_inc_pf_ack(hw, mbx_id);
+       /* free ownership of the buffer */
+       mbx_wr32(hw, ctrl_reg, 0);
+
+out_no_read:
+       return ret_val;
+}
+
+/**
+ * mucse_mbx_reset - reset mbx info, sync info from regs
+ * @hw: Pointer to the HW structure
+ *
+ * This function reset all mbx variables to default.
+ **/
+static void mucse_mbx_reset(struct mucse_hw *hw)
+{
+       struct mucse_mbx_info *mbx = &hw->mbx;
+       int idx, v;
+
+       for (idx = 0; idx < hw->max_vfs; idx++) {
+               v = mbx_rd32(hw, VF2PF_COUNTER(mbx, idx));
+               hw->mbx.vf_req[idx] = v & GENMASK(15, 0);
+               hw->mbx.vf_ack[idx] = (v >> 16) & GENMASK(15, 0);
+               mbx_wr32(hw, PF2VF_MBOX_CTRL(mbx, idx), 0);
+       }
+       v = mbx_rd32(hw, FW2PF_COUNTER(mbx));
+       hw->mbx.fw_req = v & GENMASK(15, 0);
+       hw->mbx.fw_ack = (v >> 16) & GENMASK(15, 0);
+
+       mbx_wr32(hw, PF2FW_MBOX_CTRL(mbx), 0);
+
+       if (PF_VF_MBOX_MASK_LO(mbx))
+               mbx_wr32(hw, PF_VF_MBOX_MASK_LO(mbx), 0);
+       if (PF_VF_MBOX_MASK_HI(mbx))
+               mbx_wr32(hw, PF_VF_MBOX_MASK_HI(mbx), 0);
+
+       mbx_wr32(hw, FW_PF_MBOX_MASK(mbx), GENMASK(31, 16));
+}
+
+/**
+ * mucse_mbx_configure_pf - configure mbx to use nr_vec interrupt
+ * @hw: Pointer to the HW structure
+ * @nr_vec: Vector number for mbx
+ * @enable: TRUE for enable, FALSE for disable
+ *
+ * This function configure mbx to use interrupt nr_vec.
+ **/
+static void mucse_mbx_configure_pf(struct mucse_hw *hw, int nr_vec,
+                                  bool enable)
+{
+       struct mucse_mbx_info *mbx = &hw->mbx;
+       int idx = 0;

Nit, you don't need to initialize idx. It's always initialized before 
it's used.

Instead of the comment and the name, wouldn't it make more sense to 
incorporate "words" into the define? Something like:

#define MUCSE_VFMAILBOX_WORDS 14

+/* ================ PF <--> VF mailbox ================ */
+#define SHARE_MEM_BYTES 64
+static inline u32 PF_VF_SHM(struct mucse_mbx_info *mbx, int vf)
+{
+       return mbx->pf_vf_shm_base + mbx->mbx_mem_size * vf;
+}
+
+#define PF2VF_COUNTER(mbx, vf) (PF_VF_SHM(mbx, vf) + 0)
+#define VF2PF_COUNTER(mbx, vf) (PF_VF_SHM(mbx, vf) + 4)
+#define PF_VF_SHM_DATA(mbx, vf) (PF_VF_SHM(mbx, vf) + 8)
+#define VF2PF_MBOX_VEC(mbx, vf) ((mbx)->vf2pf_mbox_vec_base + 4 * (vf))
+#define PF2VF_MBOX_CTRL(mbx, vf) ((mbx)->pf2vf_mbox_ctrl_base + 4 * (vf))
+#define PF_VF_MBOX_MASK_LO(mbx) ((mbx)->pf_vf_mbox_mask_lo)
+#define PF_VF_MBOX_MASK_HI(mbx) ((mbx)->pf_vf_mbox_mask_hi)
+/* ================ PF <--> FW mailbox ================ */
+#define FW_PF_SHM(mbx) ((mbx)->fw_pf_shm_base)
+#define FW2PF_COUNTER(mbx) (FW_PF_SHM(mbx) + 0)
+#define PF2FW_COUNTER(mbx) (FW_PF_SHM(mbx) + 4)
+#define FW_PF_SHM_DATA(mbx) (FW_PF_SHM(mbx) + 8)
+#define FW2PF_MBOX_VEC(mbx) ((mbx)->fw2pf_mbox_vec)
+#define PF2FW_MBOX_CTRL(mbx) ((mbx)->pf2fw_mbox_ctrl)
+#define FW_PF_MBOX_MASK(mbx) ((mbx)->fw_pf_mbox_mask)
+#define MBOX_CTRL_REQ BIT(0) /* WO */
+#define MBOX_PF_HOLD (BIT(3)) /* VF:RO, PF:WR */
+#define MBOX_IRQ_EN 0
+#define MBOX_IRQ_DISABLE 1
+#define mbx_rd32(hw, reg) m_rd_reg((hw)->hw_addr + (reg))
+#define mbx_wr32(hw, reg, val) m_wr_reg((hw)->hw_addr + (reg), (val))
+
+extern struct mucse_mbx_operations mucse_mbx_ops_generic;
+
+int mucse_read_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
+                  enum MBX_ID mbx_id);
+int mucse_write_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
+                   enum MBX_ID mbx_id);
+int mucse_check_for_msg(struct mucse_hw *hw, enum MBX_ID mbx_id);
+int mucse_check_for_ack(struct mucse_hw *hw, enum MBX_ID mbx_id);
+#endif /* _RNPGBE_MBX_H */
--
2.25.1