Re: [RFC, WIP, v4 06/11] media: vidtv: add wrappers for memcpy and memset

From: Mauro Carvalho Chehab
Date: Sun May 03 2020 - 03:06:58 EST


Em Sat, 2 May 2020 08:40:38 +0200
Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> escreveu:

> Em Sat, 2 May 2020 00:22:11 -0300
> "Daniel W. S. Almeida" <dwlsalmeida@xxxxxxxxx> 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