On Sun, Aug 24, 2025 at 05:15:25PM +0200, Andrew Lunn wrote:

On Sun, Aug 24, 2025 at 12:10:52PM +0800, Yibo Dong wrote:

quoted

On Sat, Aug 23, 2025 at 05:17:45PM +0200, Andrew Lunn wrote:

quoted

On Sat, Aug 23, 2025 at 09:58:24AM +0800, Yibo Dong wrote:

quoted

On Fri, Aug 22, 2025 at 04:43:16PM +0200, Andrew Lunn wrote:

quoted

quoted

+/**
+ * mucse_mbx_get_capability - Get hw abilities from fw
+ * @hw: pointer to the HW structure
+ *
+ * mucse_mbx_get_capability tries to get 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 = -EIO;
+
+	while (try_cnt--) {
+		err = mucse_fw_get_capability(hw, &ability);
+		if (err)
+			continue;
+		hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
+		return 0;
+	}
+	return err;
+}

Please could you add an explanation why it would fail? Is this to do
with getting the driver and firmware in sync? Maybe you should make
this explicit, add a function mucse_mbx_sync() with a comment that
this is used once during probe to synchronise communication with the
firmware. You can then remove this loop here.

It is just get some fw capability(or info such as fw version).
It is failed maybe:
1. -EIO: return by mucse_obtain_mbx_lock_pf. The function tries to get
pf-fw lock(in chip register, not driver), failed when fw hold the lock.

If it cannot get the lock, isn't that fatal? You cannot do anything
without the lock.

quoted

2. -ETIMEDOUT: return by mucse_poll_for_xx. Failed when timeout.
3. -ETIMEDOUT: return by mucse_fw_send_cmd_wait. Failed when wait
response timeout.

If its dead, its dead. Why would it suddenly start responding?

quoted

4. -EIO: return by mucse_fw_send_cmd_wait. Failed when error_code in
response.

Which should be fatal. No retries necessary.

quoted

5. err return by mutex_lock_interruptible.

So you want the user to have to ^C three times?

And is mucse_mbx_get_capability() special, or will all interactions
with the firmware have three retries?

quoted

It is the first 'cmd with response' from fw when probe. If it failed,
return err and nothing else todo (no registe netdev ...). So, we design
to give retry for it.
fatal with no retry, maybe like this? 

 
Quoting myself:

quoted

quoted

quoted

quoted

Is this to do
with getting the driver and firmware in sync? Maybe you should make
this explicit, add a function mucse_mbx_sync() with a comment that
this is used once during probe to synchronise communication with the
firmware. You can then remove this loop here.

'mucse_mbx_get_capability' is used once during probe in fact, and won't be
used anywhere.

Does the firmware offer a NOP command? Or one to get the firmware
version?  If you are trying to get the driver and firmware in sync, it
make sense to use an operation which is low value and won't be used
anywhere else.

	Andrew

No NOP command.. 'mucse_mbx_get_capability' can get the firmware version
and in fact only used in probe, maybe I should rename it to 'mucse_mbx_sync',
and add comment 'only be used once during probe'?
Or keep the name with that comment?

Thanks for your feedback.