linux-kernel - Re: [PATCH 04/15] net: rnpgbe: Add get_capability 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 for Android: free password hash cracker in your pocket

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

Message-ID: <57497e14-3f9a-4da8-9892-ed794aadbf47@lunn.ch>
Date: Fri, 4 Jul 2025 20:25:12 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Dong Yibo <dong100@...se.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, horms@...nel.org, corbet@....net,
	andrew+netdev@...n.ch, 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, netdev@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 04/15] net: rnpgbe: Add get_capability mbx_fw ops support

> +/**
> + * mucse_fw_send_cmd_wait - Send cmd req and wait for response
> + * @hw: Pointer to the HW structure
> + * @req: Pointer to the cmd req structure
> + * @reply: Pointer to the fw reply structure
> + *
> + * mucse_fw_send_cmd_wait sends req to pf-cm3 mailbox and wait
> + * reply from fw.
> + *
> + * Returns 0 on success, negative on failure
> + **/
> +static int mucse_fw_send_cmd_wait(struct mucse_hw *hw,
> +				  struct mbx_fw_cmd_req *req,
> +				  struct mbx_fw_cmd_reply *reply)
> +{
> +	int err;
> +	int retry_cnt = 3;
> +
> +	if (!hw || !req || !reply || !hw->mbx.ops.read_posted)

Can this happen?

If this is not supposed to happen, it is better the driver opps, so
you get a stack trace and find where the driver is broken.

> +		return -EINVAL;
> +
> +	/* if pcie off, nothing todo */
> +	if (pci_channel_offline(hw->pdev))
> +		return -EIO;

What can cause it to go offline? Is this to do with PCIe hotplug?

> +
> +	if (mutex_lock_interruptible(&hw->mbx.lock))
> +		return -EAGAIN;

mutex_lock_interruptable() returns -EINTR, which is what you should
return, not -EAGAIN.

> +
> +	err = hw->mbx.ops.write_posted(hw, (u32 *)req,
> +				       L_WD(req->datalen + MBX_REQ_HDR_LEN),
> +				       MBX_FW);
> +	if (err) {
> +		mutex_unlock(&hw->mbx.lock);
> +		return err;
> +	}
> +
> +retry:
> +	retry_cnt--;
> +	if (retry_cnt < 0)
> +		return -EIO;
> +
> +	err = hw->mbx.ops.read_posted(hw, (u32 *)reply,
> +				      L_WD(sizeof(*reply)),
> +				      MBX_FW);
> +	if (err) {
> +		mutex_unlock(&hw->mbx.lock);
> +		return err;
> +	}
> +
> +	if (reply->opcode != req->opcode)
> +		goto retry;
> +
> +	mutex_unlock(&hw->mbx.lock);
> +
> +	if (reply->error_code)
> +		return -reply->error_code;

The mbox is using linux error codes? 

> +#define FLAGS_DD BIT(0) /* driver clear 0, FW must set 1 */
> +/* driver clear 0, FW must set only if it reporting an error */
> +#define FLAGS_ERR BIT(2)
> +
> +/* req is little endian. bigendian should be conserened */
> +struct mbx_fw_cmd_req {
> +	u16 flags; /* 0-1 */
> +	u16 opcode; /* 2-3 enum GENERIC_CMD */
> +	u16 datalen; /* 4-5 */
> +	u16 ret_value; /* 6-7 */

If this is little endian, please use __le16, __le32 etc, so that the
static analysers will tell you if you are missing cpu_to_le32 etc.

	Andrew