Re: [PATCH v18 05/15] media: amphion: implement vpu core communication based on mailbox

[Dan Carpenter]

On Thu, Feb 24, 2022 at 11:10:03AM +0800, Ming Qian wrote:
> +static bool vpu_inst_receive_msg(struct vpu_inst *inst, struct vpu_rpc_event *pkt)
> +{
> +	u32 bytes = sizeof(struct vpu_rpc_event_header);
> +	u32 ret;
> +
> +	memset(pkt, 0, sizeof(*pkt));
> +	if (kfifo_len(&inst->msg_fifo) < bytes)
> +		return false;
> +
> +	ret = kfifo_out(&inst->msg_fifo, pkt, bytes);
> +	if (ret != bytes)
> +		return false;
> +
> +	if (pkt->hdr.num > 0) {
> +		bytes = pkt->hdr.num * sizeof(u32);

Imagine if we had declared "bytes" as unsigned long, then on 64 bit
systems we wouldn't have to worry about integer overflows here.  The u32
type has a lot of negatives and none of the positives that unsigned long
has.

> +		ret = kfifo_out(&inst->msg_fifo, pkt->data, bytes);
> +		if (ret != bytes)
> +			return false;
> +	}
> +
> +	return true;
> +}

[ Snip ]

> +void vpu_msg_delayed_work(struct work_struct *work)
> +{
> +	struct vpu_core *core;
> +	struct delayed_work *dwork;
> +	u32 bytes = sizeof(bytes);

LOL...  You got jokes.

> +	u32 i;
> +
> +	if (!work)
> +		return;
> +
> +	dwork = to_delayed_work(work);
> +	core = container_of(dwork, struct vpu_core, msg_delayed_work);
> +	if (kfifo_len(&core->msg_fifo) >= bytes)
> +		vpu_core_run_msg_work(core);
> +
> +	bytes = sizeof(struct vpu_rpc_event_header);
> +	for (i = 0; i < core->supported_instance_count; i++) {
> +		struct vpu_inst *inst = vpu_core_find_instance(core, i);
> +
> +		if (!inst)
> +			continue;
> +
> +		if (inst->workqueue && kfifo_len(&inst->msg_fifo) >= bytes)
> +			queue_work(inst->workqueue, &inst->msg_work);
> +
> +		vpu_inst_put(inst);
> +	}
> +}

regards,
dan carpenter