Re: [PATCH v18 06/15] media: amphion: add vpu v4l2 m2m support

[Dan Carpenter <dan.carpenter@oracle.com>]

This code has a serious case of the u32 pox.  There are times where u32
is specified in the hardware or network spec.  That's when a u32 is
appropriate.  Also for bit masks.  Otherwise "int" is normally the
correct type.  If it's a size value then unsigned long, long, or
unsigned long long is probably correct.

INT_MAX is just over 2 billion.  If you make a number line then most
numbers are going to be near the zero.  You have 10 fingers.  You have
2 phones.  2 cars.  3 monitors connected to your computer.  200 error
codes.  You're never going to even get close to the 2 billion limit.

For situations where the numbers get very large, then the band on the
number line between 2 and 4 billion is very narrow.  I can name people
who have over a billion dollars but I cannot name even one who falls
exactly between 2 and 4 billion.

In other words u32 is almost useless for describing anything.  If
something cannot fit in a int then it's not going to fit into a u32
either and you should use a u64 instead.

Some people think that unsigned values are more safe than signed values.
It is true, in certain limited cases that the invisible side effects of
unsigned math can protect you.  But mostly the invisible side effects
create surprises and bugs.  And again if you have to pick an unsigned
type pick an u64 because it is harder to have an integer overflow on a
64 bit type vs a 32 bit type.

Avoid u32 types where ever you can, they only cause bugs.

> +u32 vpu_helper_copy_from_stream_buffer(struct vpu_buffer *stream_buffer,
> +				       u32 *rptr, u32 size, void *dst)
> +{
> +	u32 offset;
> +	u32 start;
> +	u32 end;
> +	void *virt;
> +
> +	if (!stream_buffer || !rptr || !dst)
> +		return -EINVAL;

This function returns negatives.

> +
> +	if (!size)
> +		return 0;
> +
> +	offset = *rptr;
> +	start = stream_buffer->phys;
> +	end = start + stream_buffer->length;
> +	virt = stream_buffer->virt;
> +
> +	if (offset < start || offset > end)
> +		return -EINVAL;
> +
> +	if (offset + size <= end) {

Check for integer overflows?


> +		memcpy(dst, virt + (offset - start), size);
> +	} else {
> +		memcpy(dst, virt + (offset - start), end - offset);
> +		memcpy(dst + end - offset, virt, size + offset - end);
> +	}
> +
> +	*rptr = vpu_helper_step_walk(stream_buffer, offset, size);
> +	return size;

This function always returns size on success.  Just return 0 on success.

> +}
> +
> +u32 vpu_helper_copy_to_stream_buffer(struct vpu_buffer *stream_buffer,
> +				     u32 *wptr, u32 size, void *src)
> +{
> +	u32 offset;
> +	u32 start;
> +	u32 end;
> +	void *virt;
> +
> +	if (!stream_buffer || !wptr || !src)
> +		return -EINVAL;

Signedness bug.

> +
> +	if (!size)
> +		return 0;
> +
> +	offset = *wptr;
> +	start = stream_buffer->phys;
> +	end = start + stream_buffer->length;
> +	virt = stream_buffer->virt;
> +	if (offset < start || offset > end)
> +		return -EINVAL;

Signedness.

> +
> +	if (offset + size <= end) {

Check for integer overflow?

> +		memcpy(virt + (offset - start), src, size);
> +	} else {
> +		memcpy(virt + (offset - start), src, end - offset);
> +		memcpy(virt, src + end - offset, size + offset - end);
> +	}
> +
> +	*wptr = vpu_helper_step_walk(stream_buffer, offset, size);
> +
> +	return size;

Just return zero on success.  No need to return a known parameter.

> +}
> +
> +u32 vpu_helper_memset_stream_buffer(struct vpu_buffer *stream_buffer,
> +				    u32 *wptr, u8 val, u32 size)
> +{
> +	u32 offset;
> +	u32 start;
> +	u32 end;
> +	void *virt;
> +
> +	if (!stream_buffer || !wptr)
> +		return -EINVAL;

Signedness.

> +
> +	if (!size)
> +		return 0;
> +
> +	offset = *wptr;
> +	start = stream_buffer->phys;
> +	end = start + stream_buffer->length;
> +	virt = stream_buffer->virt;
> +	if (offset < start || offset > end)
> +		return -EINVAL;
> +
> +	if (offset + size <= end) {

Check for overflow?

> +		memset(virt + (offset - start), val, size);
> +	} else {
> +		memset(virt + (offset - start), val, end - offset);
> +		memset(virt, val, size + offset - end);
> +	}
> +
> +	offset += size;
> +	if (offset >= end)
> +		offset -= stream_buffer->length;
> +
> +	*wptr = offset;
> +
> +	return size;
> +}

regards,
dan carpenter