Em Sat, 2 May 2020 08:40:38 +0200
Mauro Carvalho Chehab <mchehab+hua...@kernel.org> escreveu:
> Em Sat, 2 May 2020 00:22:11 -0300
> "Daniel W. S. Almeida" <dwlsalme...@gmail.com> escreveu:
>
> > +u32 vidtv_memcpy(void *to,
> > + const void *from,
> > + size_t len,
> > + u32 offset,
> > + u32 buf_sz)
> > +{
> > + if (buf_sz && offset + len > buf_sz) {
> > + pr_err("%s: overflow detected, skipping. Try increasing the
> > buffer size",
> > + __func__);
> > + return 0;
>
> shouldn't it return an error?
>
> > + }
> > +
> > + memcpy(to, from, len);
> > + return len;
> > +}
When trying to use your memset wrapper, I noticed a few issues there.
The first one is that you should not use __func__ directly at pr_* macros.
Instead, just ensure that you have a pr_fmt() macro that ill be adding
the driver's name and the function, e. g.:
#define pr_fmt(fmt) KBUILD_MODNAME ":%s: " fmt, __func__
Besides that, the parameter order sounded weird:
> > +u32 vidtv_memcpy(void *to,
> > + const void *from,
> > + size_t len,
> > + u32 offset,
> > + u32 buf_sz)
The "to", "offset" and "buf_sz" arguments refer to the "to" buffer,
while "from" and "len" refers to what will be copied from the "from"
into the "to" buffer. Please re-order it, placing first the "to"
args, then "from" arg, and finally the argument that applies to both,
e. g.:
size_t vidtv_memcpy(void *to, size_t to_offset, size_t to_size,
const void *from, size_t len)
(you should notice that I'm using size_t for all args there).
The same is also valid for the memset.
Finally, the places where this function is used require to pass twice
the offset (from patch 08/11):
> + nbytes += vidtv_memcpy(args.dest_buf +
> + args.dest_offset +
> + nbytes,
> + &ts_header,
> + sizeof(ts_header),
> + args.dest_offset + nbytes,
> + args.dest_buf_sz);
That doesn't make much sense. I mean, if the arguments are "buf + offset",
one would expect that the "buf" would be the head of a buffer, and the
function would be adding the offset internally.
So, the best would be to re-define it like:
/**
* vidtv_memcpy() - wrapper routine to be used by MPEG-TS
* generator, in order to avoid going past the
* output buffer.
* @to: Starting element to where a MPEG-TS packet will
* be copied.
* @to_offset: Starting position of the @to buffer to be filled.
* @to_size: Size of the @to buffer.
* @from: Starting element of the buffer to be copied.
* @ten: Number of elements to be copy from @from buffer
* into @to+ @to_offset buffer.
*
* Note:
* Real digital TV demod drivers should not have memcpy
* wrappers. We use it here just because emulating a MPEG-TS
* generation at kernelspace require some extra care.
*
* Return:
* Returns the number of bytes
*/
size_t vidtv_memcpy(void *to, size_t to_offset, size_t to_size,
const void *from, size_t len)
{
if unlikely(to_offset + len > to_size) {
pr_err_ratelimited("overflow detected, skipping. Try
increasing the buffer size\n");
return 0;
}
memcpy(to + to_offset, from, len);
return len;
}
Thanks,
Mauro
