netdev - Re: [PATCH v3 4/5] net: rnpgbe: Add basic mbx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2ef60880-f85d-4a1a-b50c-9ea73fee70b0@linux.dev>
Date: Wed, 13 Aug 2025 14:33:39 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Yibo Dong <dong100@...se.com>
Cc: andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, horms@...nel.org, corbet@....net,
 gur.stavi@...wei.com, maddy@...ux.ibm.com, mpe@...erman.id.au,
 danishanwar@...com, lee@...ger.us, gongfan1@...wei.com, lorenzo@...nel.org,
 geert+renesas@...der.be, Parthiban.Veerasooran@...rochip.com,
 lukas.bulwahn@...hat.com, alexanderduyck@...com, richardcochran@...il.com,
 netdev@...r.kernel.org, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 4/5] net: rnpgbe: Add basic mbx_fw support

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