Re: [PATCH net-next 1/3] hinic: add mailbox function support

[Jakub Kicinski]

On Tue, 21 Apr 2020 04:56:33 +0000 Luo bin wrote:
> virtual function and physical function can communicate with each
> other through mailbox channel supported by hw
> 
> Signed-off-by: Luo bin <luobin9@xxxxxxxxxx>

> +static int recv_vf_mbox_handler(struct hinic_mbox_func_to_func *func_to_func,
> +				struct hinic_recv_mbox *recv_mbox,
> +				void *buf_out, u16 *out_size)
> +{
> +	hinic_vf_mbox_cb cb;
> +	int ret = 0;
> +
> +	if (recv_mbox->mod >= HINIC_MOD_MAX) {
> +		dev_err(&func_to_func->hwif->pdev->dev, "Receive illegal mbox message, mod = %d\n",
> +			recv_mbox->mod);

You may want to rate limit these, otherwise VF may spam PFs logs.

> +		return -EINVAL;
> +	}

> +static int mbox_func_params_valid(struct hinic_mbox_func_to_func *func_to_func,
> +				  void *buf_in, u16 in_size)
> +{
> +	if (!buf_in || !in_size)
> +		return -EINVAL;

This is defensive programming, we don't do that in the kernel, callers
should not pass NULL buffer and 0 size.

Also you probably want the size to be size_t, otherwise it may get
truncated before the check below.

> +	if (in_size > HINIC_MBOX_DATA_SIZE) {
> +		dev_err(&func_to_func->hwif->pdev->dev,
> +			"Mbox msg len(%d) exceed limit(%d)\n",
> +			in_size, HINIC_MBOX_DATA_SIZE);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +int hinic_mbox_to_pf(struct hinic_hwdev *hwdev,
> +		     enum hinic_mod_type mod, u8 cmd, void *buf_in,
> +		     u16 in_size, void *buf_out, u16 *out_size, u32 timeout)
> +{
> +	struct hinic_mbox_func_to_func *func_to_func = hwdev->func_to_func;
> +	int err = mbox_func_params_valid(func_to_func, buf_in, in_size);
> +
> +	if (err)
> +		return err;
> +
> +	if (!HINIC_IS_VF(hwdev->hwif)) {
> +		dev_err(&hwdev->hwif->pdev->dev, "Params error, func_type: %d\n",
> +			HINIC_FUNC_TYPE(hwdev->hwif));
> +		return -EINVAL;
> +	}
> +
> +	err = hinic_mbox_to_func(func_to_func, mod, cmd,
> +				 hinic_pf_id_of_vf_hw(hwdev->hwif), buf_in,
> +				 in_size, buf_out, out_size, timeout);
> +	return err;

return hinic_mbox_to...

directly

> +}

> +static int comm_pf_mbox_handler(void *handle, u16 vf_id, u8 cmd, void *buf_in,
> +				u16 in_size, void *buf_out, u16 *out_size)
> +{
> +	struct hinic_hwdev *hwdev = handle;
> +	struct hinic_pfhwdev *pfhwdev;
> +	int err = 0;
> +
> +	pfhwdev = container_of(hwdev, struct hinic_pfhwdev, hwdev);
> +
> +	if (cmd == HINIC_COMM_CMD_START_FLR) {
> +		*out_size = 0;
> +	} else {
> +		err = hinic_msg_to_mgmt(&pfhwdev->pf_to_mgmt, HINIC_MOD_COMM,
> +					cmd, buf_in, in_size, buf_out, out_size,
> +					HINIC_MGMT_MSG_SYNC);
> +		if (err  && err != HINIC_MBOX_PF_BUSY_ACTIVE_FW)

Double space, please run checkpatch --strict on the patches

> +			dev_err(&hwdev->hwif->pdev->dev,
> +				"PF mbox common callback handler err: %d\n",
> +				err);
> +	}
> +
> +	return err;
> +}