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

[luobin (L)]

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@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;
> > +}
> .