From: Ming Qian <ming.qian@nxp.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: "mchehab@kernel.org" <mchehab@kernel.org>,
"shawnguo@kernel.org" <shawnguo@kernel.org>,
"robh+dt@kernel.org" <robh+dt@kernel.org>,
"s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
"hverkuil-cisco@xs4all.nl" <hverkuil-cisco@xs4all.nl>,
"kernel@pengutronix.de" <kernel@pengutronix.de>,
"festevam@gmail.com" <festevam@gmail.com>,
dl-linux-imx <linux-imx@nxp.com>,
Aisheng Dong <aisheng.dong@nxp.com>,
"linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>
Subject: RE: [EXT] Re: [PATCH v18 06/15] media: amphion: add vpu v4l2 m2m support
Date: Thu, 10 Mar 2022 01:55:33 +0000 [thread overview]
Message-ID: <AM6PR04MB6341451D5139D2EFFDAD44DFE70B9@AM6PR04MB6341.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <20220309113420.GA2592@kili>
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Wednesday, March 9, 2022 7:34 PM
> To: Ming Qian <ming.qian@nxp.com>
> Cc: mchehab@kernel.org; shawnguo@kernel.org; robh+dt@kernel.org;
> s.hauer@pengutronix.de; hverkuil-cisco@xs4all.nl; kernel@pengutronix.de;
> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; Aisheng Dong
> <aisheng.dong@nxp.com>; linux-media@vger.kernel.org;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-03-10 2:11 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-24 3:09 [PATCH v18 00/15] amphion video decoder/encoder driver Ming Qian
2022-02-24 3:09 ` [PATCH v18 01/15] dt-bindings: media: amphion: add amphion video codec bindings Ming Qian
2022-02-24 3:10 ` [PATCH v18 02/15] media: add nv12m_8l128 and nv12m_10be_8l128 video format Ming Qian
2022-02-24 3:10 ` [PATCH v18 03/15] media: amphion: add amphion vpu device driver Ming Qian
2022-02-24 3:10 ` [PATCH v18 04/15] media: amphion: add vpu core driver Ming Qian
2022-03-09 12:06 ` Dan Carpenter
2022-02-24 3:10 ` [PATCH v18 05/15] media: amphion: implement vpu core communication based on mailbox Ming Qian
2022-03-09 12:23 ` Dan Carpenter
2022-02-24 3:10 ` [PATCH v18 06/15] media: amphion: add vpu v4l2 m2m support Ming Qian
2022-03-09 11:34 ` Dan Carpenter
2022-03-10 1:55 ` Ming Qian [this message]
2022-02-24 3:10 ` [PATCH v18 07/15] media: amphion: add v4l2 m2m vpu encoder stateful driver Ming Qian
2022-02-24 3:10 ` [PATCH v18 08/15] media: amphion: add v4l2 m2m vpu decoder " Ming Qian
2022-02-24 3:10 ` [PATCH v18 09/15] media: amphion: implement windsor encoder rpc interface Ming Qian
2022-02-24 3:10 ` [PATCH v18 10/15] media: amphion: implement malone decoder " Ming Qian
2022-03-09 11:44 ` Dan Carpenter
2022-02-24 3:10 ` [PATCH v18 11/15] arm64: dts: freescale: imx8q: add imx vpu codec entries Ming Qian
2022-02-24 3:10 ` [PATCH v18 12/15] firmware: imx: scu-pd: imx8q: add vpu mu resources Ming Qian
2022-02-24 3:10 ` [PATCH v18 13/15] MAINTAINERS: add AMPHION VPU CODEC V4L2 driver entry Ming Qian
2022-02-24 3:10 ` [PATCH v18 14/15] arm64: defconfig: amphion: enable vpu driver Ming Qian
2022-02-24 3:10 ` [PATCH v18 15/15] media: amphion: add amphion vpu entry in Kconfig and Makefile Ming Qian
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AM6PR04MB6341451D5139D2EFFDAD44DFE70B9@AM6PR04MB6341.eurprd04.prod.outlook.com \
--to=ming.qian@nxp.com \
--cc=aisheng.dong@nxp.com \
--cc=dan.carpenter@oracle.com \
--cc=devicetree@vger.kernel.org \
--cc=festevam@gmail.com \
--cc=hverkuil-cisco@xs4all.nl \
--cc=kernel@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-imx@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=robh+dt@kernel.org \
--cc=s.hauer@pengutronix.de \
--cc=shawnguo@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).