Messages in this thread | | | Date | Sun, 3 May 2020 09:06:34 +0200 | From | Mauro Carvalho Chehab <> | Subject | Re: [RFC, WIP, v4 06/11] media: vidtv: add wrappers for memcpy and memset |
| |
Em Sat, 2 May 2020 08:40:38 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> Em Sat, 2 May 2020 00:22:11 -0300 > "Daniel W. S. Almeida" <dwlsalmeida@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
| |