Re: [PATCH 04/15] net: rnpgbe: Add get_capability mbx_fw ops support

[Andrew Lunn]

> +/**
> + * 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