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

["luobin (L)" <luobin9@huawei.com>]

Thank you for your review. Will fix.

On 2020/4/22 2:13, Jakub Kicinski wrote:
> 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@huawei.com>
>> +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;
>> +}
> .