Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support

[Andrew Lunn <andrew@lunn.ch>]

On Sun, Aug 24, 2025 at 12:10:52PM +0800, Yibo Dong wrote:
> On Sat, Aug 23, 2025 at 05:17:45PM +0200, Andrew Lunn wrote:
> > On Sat, Aug 23, 2025 at 09:58:24AM +0800, Yibo Dong wrote:
> > > On Fri, Aug 22, 2025 at 04:43:16PM +0200, Andrew Lunn wrote:
> > > > > +/**
> > > > > + * 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.
> > 
> > > 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?
> > 
> > > 4. -EIO: return by mucse_fw_send_cmd_wait. Failed when error_code in
> > > response.
> > 
> > Which should be fatal. No retries necessary.
> > 
> > > 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?
> 

> 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:

> > > > 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.

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