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

[Ming Qian <ming.qian@xxxxxxx>]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx]
> Sent: Wednesday, March 9, 2022 7:34 PM
> To: Ming Qian <ming.qian@xxxxxxx>
> Cc: mchehab@xxxxxxxxxx; shawnguo@xxxxxxxxxx; robh+dt@xxxxxxxxxx;
> s.hauer@xxxxxxxxxxxxxx; hverkuil-cisco@xxxxxxxxx; kernel@xxxxxxxxxxxxxx;
> festevam@xxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; Aisheng Dong
> <aisheng.dong@xxxxxxx>; linux-media@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Subject: [EXT] Re: [PATCH v18 06/15] media: amphion: add vpu v4l2 m2m
> support
> 
> Caution: EXT Email
> 
> 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.

OK, I think you're right, I'll check and fix it according to your comments
Thanks very much for your reivew

> 
> > +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