Date:   Sun, 3 May 2020 09:06:34 +0200
From:   Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To:     "Daniel W. S. Almeida" <dwlsalmeida@...il.com>
Cc:     sean@...s.org, kstewart@...uxfoundation.org, allison@...utok.net,
        tglx@...utronix.de, linux-media@...r.kernel.org,
        skhan@...uxfoundation.org,
        linux-kernel-mentees@...ts.linuxfoundation.org,
        linux-kernel@...r.kernel.org
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@...nel.org> escreveu:

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

Powered by blists - more mailing lists