Re: [PATCH v3 4/5] net: rnpgbe: Add basic mbx_fw support

[Vadim Fedorenko <vadim.fedorenko@xxxxxxxxx>]

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@xxxxxxxxx>
> > > +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