[PATCH 0/6] sparse/smatch fixes

[PATCH 0/6] sparse/smatch fixes

[Hans Verkuil]

Various sparse and smatch fixes.

Together with https://patchwork.linuxtv.org/patch/53375/ and
https://patchwork.linuxtv.org/patch/54237/ we have a clean bill
of health. The vsp1 patch supersedes this by now outdated old patch:
https://patchwork.linuxtv.org/patch/49263/

Kieran, Laurent, please review the uvc, vsp1 and omap3isp patches.

Sakari, Mauro, the pxa_camera patch supersedes this older patch:
https://patchwork.linuxtv.org/patch/53378/ and has Sakari's
comments included.

Regards,

	Hans

Hans Verkuil (6):
  hdpvr: fix smatch warning
  vim2m: fix smatch warning
  uvc: fix smatch warning
  vsp1: fix smatch warning
  omap3isp: fix sparse warning
  pxa_camera: fix smatch warning

 drivers/media/platform/omap3isp/ispvideo.c |  5 +++--
 drivers/media/platform/pxa_camera.c        |  8 +++++---
 drivers/media/platform/vim2m.c             |  2 +-
 drivers/media/platform/vsp1/vsp1_drm.c     |  6 +++---
 drivers/media/usb/hdpvr/hdpvr-i2c.c        | 14 +++++++-------
 drivers/media/usb/uvc/uvcvideo.h           |  6 ++++--
 6 files changed, 23 insertions(+), 18 deletions(-)

-- 
2.20.1

[PATCH 1/6] hdpvr: fix smatch warning

[Hans Verkuil]

drivers/media/usb/hdpvr/hdpvr-i2c.c: drivers/media/usb/hdpvr/hdpvr-i2c.c:78 hdpvr_i2c_read() warn: 'dev->i2c_buf' 4216624615462223872 can't fit into 127 '*data'

dev->i2c_buf is a char array, so you can just use dev->i2c_buf to get the
start address, no need to do &dev->i2c_buf, even though it is the same
address in C. It only confuses smatch.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/usb/hdpvr/hdpvr-i2c.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/media/usb/hdpvr/hdpvr-i2c.c b/drivers/media/usb/hdpvr/hdpvr-i2c.c
index 5a3cb614a211..d76173f1ced1 100644
--- a/drivers/media/usb/hdpvr/hdpvr-i2c.c
+++ b/drivers/media/usb/hdpvr/hdpvr-i2c.c
@@ -61,10 +61,10 @@ static int hdpvr_i2c_read(struct hdpvr_device *dev, int bus,
 		return -EINVAL;
 
 	if (wlen) {
-		memcpy(&dev->i2c_buf, wdata, wlen);
+		memcpy(dev->i2c_buf, wdata, wlen);
 		ret = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
 				      REQTYPE_I2C_WRITE, CTRL_WRITE_REQUEST,
-				      (bus << 8) | addr, 0, &dev->i2c_buf,
+				      (bus << 8) | addr, 0, dev->i2c_buf,
 				      wlen, 1000);
 		if (ret < 0)
 			return ret;
@@ -72,10 +72,10 @@ static int hdpvr_i2c_read(struct hdpvr_device *dev, int bus,
 
 	ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
 			      REQTYPE_I2C_READ, CTRL_READ_REQUEST,
-			      (bus << 8) | addr, 0, &dev->i2c_buf, len, 1000);
+			      (bus << 8) | addr, 0, dev->i2c_buf, len, 1000);
 
 	if (ret == len) {
-		memcpy(data, &dev->i2c_buf, len);
+		memcpy(data, dev->i2c_buf, len);
 		ret = 0;
 	} else if (ret >= 0)
 		ret = -EIO;
@@ -91,17 +91,17 @@ static int hdpvr_i2c_write(struct hdpvr_device *dev, int bus,
 	if (len > sizeof(dev->i2c_buf))
 		return -EINVAL;
 
-	memcpy(&dev->i2c_buf, data, len);
+	memcpy(dev->i2c_buf, data, len);
 	ret = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
 			      REQTYPE_I2C_WRITE, CTRL_WRITE_REQUEST,
-			      (bus << 8) | addr, 0, &dev->i2c_buf, len, 1000);
+			      (bus << 8) | addr, 0, dev->i2c_buf, len, 1000);
 
 	if (ret < 0)
 		return ret;
 
 	ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
 			      REQTYPE_I2C_WRITE_STATT, CTRL_READ_REQUEST,
-			      0, 0, &dev->i2c_buf, 2, 1000);
+			      0, 0, dev->i2c_buf, 2, 1000);
 
 	if ((ret == 2) && (dev->i2c_buf[1] == (len - 1)))
 		ret = 0;
-- 
2.20.1

[PATCH 2/6] vim2m: fix smatch warning

[Hans Verkuil]

drivers/media/platform/vim2m.c: drivers/media/platform/vim2m.c:525 device_work() warn: variable dereferenced before check 'curr_ctx' (see line 523)

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/platform/vim2m.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
index e31c14c7d37f..910727982230 100644
--- a/drivers/media/platform/vim2m.c
+++ b/drivers/media/platform/vim2m.c
@@ -520,13 +520,13 @@ static void device_work(struct work_struct *w)
 	unsigned long flags;
 
 	curr_ctx = container_of(w, struct vim2m_ctx, work_run.work);
-	vim2m_dev = curr_ctx->dev;
 
 	if (NULL == curr_ctx) {
 		pr_err("Instance released before the end of transaction\n");
 		return;
 	}
 
+	vim2m_dev = curr_ctx->dev;
 	src_vb = v4l2_m2m_src_buf_remove(curr_ctx->fh.m2m_ctx);
 	dst_vb = v4l2_m2m_dst_buf_remove(curr_ctx->fh.m2m_ctx);
 
-- 
2.20.1

[PATCH 3/6] uvc: fix smatch warning

[Hans Verkuil]

drivers/media/usb/uvc/uvc_video.c: drivers/media/usb/uvc/uvc_video.c:1893 uvc_video_start_transfer() warn: argument 2 to %u specifier is cast from pointer

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/usb/uvc/uvcvideo.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 9b41b14ce076..c7c1baa90dea 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -620,8 +620,10 @@ struct uvc_streaming {
 	     (uvc_urb) < &(uvc_streaming)->uvc_urb[UVC_URBS]; \
 	     ++(uvc_urb))
 
-#define uvc_urb_index(uvc_urb) \
-	(unsigned int)((uvc_urb) - (&(uvc_urb)->stream->uvc_urb[0]))
+static inline u32 uvc_urb_index(const struct uvc_urb *uvc_urb)
+{
+	return uvc_urb - &uvc_urb->stream->uvc_urb[0];
+}
 
 struct uvc_device_info {
 	u32	quirks;
-- 
2.20.1

[PATCH 4/6] vsp1: fix smatch warning

[Hans Verkuil]

drivers/media/platform/vsp1/vsp1_drm.c: drivers/media/platform/vsp1/vsp1_drm.c:336 vsp1_du_pipeline_setup_brx() error: we previously assumed 'pipe->brx' could be null (see line 244)

smatch missed that if pipe->brx was NULL, then later on it will be
set with a non-NULL value. But it is easier to just use the brx pointer
so smatch doesn't get confused.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/platform/vsp1/vsp1_drm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index 8d86f618ec77..84895385d2e5 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -333,19 +333,19 @@ static int vsp1_du_pipeline_setup_brx(struct vsp1_device *vsp1,
 	 * on the BRx sink pad 0 and propagated inside the entity, not on the
 	 * source pad.
 	 */
-	format.pad = pipe->brx->source_pad;
+	format.pad = brx->source_pad;
 	format.format.width = drm_pipe->width;
 	format.format.height = drm_pipe->height;
 	format.format.field = V4L2_FIELD_NONE;
 
-	ret = v4l2_subdev_call(&pipe->brx->subdev, pad, set_fmt, NULL,
+	ret = v4l2_subdev_call(&brx->subdev, pad, set_fmt, NULL,
 			       &format);
 	if (ret < 0)
 		return ret;
 
 	dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on %s pad %u\n",
 		__func__, format.format.width, format.format.height,
-		format.format.code, BRX_NAME(pipe->brx), pipe->brx->source_pad);
+		format.format.code, BRX_NAME(brx), brx->source_pad);
 
 	if (format.format.width != drm_pipe->width ||
 	    format.format.height != drm_pipe->height) {
-- 
2.20.1

[PATCH 5/6] omap3isp: fix sparse warning

[Hans Verkuil]

drivers/media/platform/omap3isp/ispvideo.c:1013:15: warning: unknown expression (4 0)

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/platform/omap3isp/ispvideo.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index 078d64114b24..911dfad90d32 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -974,6 +974,7 @@ static int isp_video_check_external_subdevs(struct isp_video *video,
 	struct media_entity *source = NULL;
 	struct media_entity *sink;
 	struct v4l2_subdev_format fmt;
+	struct v4l2_subdev *sd;
 	struct v4l2_ext_controls ctrls;
 	struct v4l2_ext_control ctrl;
 	unsigned int i;
@@ -1010,8 +1011,8 @@ static int isp_video_check_external_subdevs(struct isp_video *video,
 
 	fmt.pad = source_pad->index;
 	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
-	ret = v4l2_subdev_call(media_entity_to_v4l2_subdev(sink),
-			       pad, get_fmt, NULL, &fmt);
+	sd = media_entity_to_v4l2_subdev(sink);
+	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
 	if (unlikely(ret < 0)) {
 		dev_warn(isp->dev, "get_fmt returned null!\n");
 		return ret;
-- 
2.20.1

[PATCH 6/6] pxa_camera: fix smatch warning

[Hans Verkuil]

drivers/media/platform/pxa_camera.c:2400 pxa_camera_probe() error: we previously assumed 'pcdev->pdata' could be null (see line 2397)

First check if platform data is provided, then check if DT data is provided,
and if neither is provided just return with -ENODEV.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/platform/pxa_camera.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
index 5f930560eb30..3cf3c6390cc8 100644
--- a/drivers/media/platform/pxa_camera.c
+++ b/drivers/media/platform/pxa_camera.c
@@ -2394,15 +2394,17 @@ static int pxa_camera_probe(struct platform_device *pdev)
 	pcdev->res = res;
 
 	pcdev->pdata = pdev->dev.platform_data;
-	if (pdev->dev.of_node && !pcdev->pdata) {
-		err = pxa_camera_pdata_from_dt(&pdev->dev, pcdev, &pcdev->asd);
-	} else {
+	if (pcdev->pdata) {
 		pcdev->platform_flags = pcdev->pdata->flags;
 		pcdev->mclk = pcdev->pdata->mclk_10khz * 10000;
 		pcdev->asd.match_type = V4L2_ASYNC_MATCH_I2C;
 		pcdev->asd.match.i2c.adapter_id =
 			pcdev->pdata->sensor_i2c_adapter_id;
 		pcdev->asd.match.i2c.address = pcdev->pdata->sensor_i2c_address;
+	} else if (pdev->dev.of_node) {
+		err = pxa_camera_pdata_from_dt(&pdev->dev, pcdev, &pcdev->asd);
+	} else {
+		return -ENODEV;
 	}
 	if (err < 0)
 		return err;
-- 
2.20.1

Re: [PATCH 5/6] omap3isp: fix sparse warning

[Hans Verkuil]

On 2/7/19 10:13 AM, Hans Verkuil wrote:
> drivers/media/platform/omap3isp/ispvideo.c:1013:15: warning: unknown expression (4 0)

I should add this text to the commit log:

The combination of the v4l2_subdev_call and media_entity_to_v4l2_subdev macros
became too complex for sparse. So first assign the result of
media_entity_to_v4l2_subdev to a struct v4l2_subdev *sd variable, then
use that in v4l2_subdev_call.

Regards,

	Hans

> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/platform/omap3isp/ispvideo.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
> index 078d64114b24..911dfad90d32 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -974,6 +974,7 @@ static int isp_video_check_external_subdevs(struct isp_video *video,
>  	struct media_entity *source = NULL;
>  	struct media_entity *sink;
>  	struct v4l2_subdev_format fmt;
> +	struct v4l2_subdev *sd;
>  	struct v4l2_ext_controls ctrls;
>  	struct v4l2_ext_control ctrl;
>  	unsigned int i;
> @@ -1010,8 +1011,8 @@ static int isp_video_check_external_subdevs(struct isp_video *video,
>  
>  	fmt.pad = source_pad->index;
>  	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> -	ret = v4l2_subdev_call(media_entity_to_v4l2_subdev(sink),
> -			       pad, get_fmt, NULL, &fmt);
> +	sd = media_entity_to_v4l2_subdev(sink);
> +	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
>  	if (unlikely(ret < 0)) {
>  		dev_warn(isp->dev, "get_fmt returned null!\n");
>  		return ret;
> 

Re: [PATCH 6/6] pxa_camera: fix smatch warning

[Sakari Ailus]

On Thu, Feb 07, 2019 at 10:13:38AM +0100, Hans Verkuil wrote:
> drivers/media/platform/pxa_camera.c:2400 pxa_camera_probe() error: we previously assumed 'pcdev->pdata' could be null (see line 2397)
> 
> First check if platform data is provided, then check if DT data is provided,
> and if neither is provided just return with -ENODEV.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 5/6] omap3isp: fix sparse warning

[Sakari Ailus]

On Thu, Feb 07, 2019 at 10:13:37AM +0100, Hans Verkuil wrote:
> drivers/media/platform/omap3isp/ispvideo.c:1013:15: warning: unknown expression (4 0)
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

[PATCH 7/6] omap4iss: fix sparse warning

[Hans Verkuil]

drivers/staging/media/omap4iss/iss.c:141:15: warning: unknown expression (4 0)
drivers/staging/media/omap4iss/iss.c:141:15: warning: unknown expression (4 0)
drivers/staging/media/omap4iss/iss.c:141:15: warning: unknown expression (4 0)
drivers/staging/media/omap4iss/iss.c:141:15: warning: unknown expression (4 0)

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
Same fix as for omap3isp. I discovered that staging drivers weren't built by the
daily build, so I never noticed these warnings.
---
diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
index c8be1db532ab..fd702947cdb8 100644
--- a/drivers/staging/media/omap4iss/iss.c
+++ b/drivers/staging/media/omap4iss/iss.c
@@ -124,6 +124,7 @@ int omap4iss_get_external_info(struct iss_pipeline *pipe,
 {
 	struct iss_device *iss =
 		container_of(pipe, struct iss_video, pipe)->iss;
+	struct v4l2_subdev *sd;
 	struct v4l2_subdev_format fmt;
 	struct v4l2_ctrl *ctrl;
 	int ret;
@@ -138,8 +139,8 @@ int omap4iss_get_external_info(struct iss_pipeline *pipe,

 	fmt.pad = link->source->index;
 	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
-	ret = v4l2_subdev_call(media_entity_to_v4l2_subdev(link->sink->entity),
-			       pad, get_fmt, NULL, &fmt);
+	sd = media_entity_to_v4l2_subdev(link->sink->entity);
+	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
 	if (ret < 0)
 		return -EPIPE;

Re: [PATCH 7/6] omap4iss: fix sparse warning

[Sakari Ailus]

On Thu, Feb 07, 2019 at 03:38:02PM +0100, Hans Verkuil wrote:
> drivers/staging/media/omap4iss/iss.c:141:15: warning: unknown expression (4 0)
> drivers/staging/media/omap4iss/iss.c:141:15: warning: unknown expression (4 0)
> drivers/staging/media/omap4iss/iss.c:141:15: warning: unknown expression (4 0)
> drivers/staging/media/omap4iss/iss.c:141:15: warning: unknown expression (4 0)
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> Same fix as for omap3isp. I discovered that staging drivers weren't built by the
> daily build, so I never noticed these warnings.

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 3/6] uvc: fix smatch warning

[Kieran Bingham]

Hi Hans,

On 07/02/2019 10:13, Hans Overkill wrote:
> drivers/media/usb/uvc/uvc_video.c: drivers/media/usb/uvc/uvc_video.c:1893 uvc_video_start_transfer() warn: argument 2 to %u specifier is cast from pointer
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

This look fine to me.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  drivers/media/usb/uvc/uvcvideo.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 9b41b14ce076..c7c1baa90dea 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -620,8 +620,10 @@ struct uvc_streaming {
>  	     (uvc_urb) < &(uvc_streaming)->uvc_urb[UVC_URBS]; \
>  	     ++(uvc_urb))
>  
> -#define uvc_urb_index(uvc_urb) \
> -	(unsigned int)((uvc_urb) - (&(uvc_urb)->stream->uvc_urb[0]))
> +static inline u32 uvc_urb_index(const struct uvc_urb *uvc_urb)
> +{
> +	return uvc_urb - &uvc_urb->stream->uvc_urb[0];
> +}>
>  struct uvc_device_info {
>  	u32	quirks;
> 

-- 
Regards
--
Kieran

Re: [PATCH 3/6] uvc: fix smatch warning

[Laurent Pinchart]

Hello,

On Thu, Feb 07, 2019 at 03:57:26PM +0100, Kieran Bingham wrote:
> On 07/02/2019 10:13, Hans Overkill wrote:
> > drivers/media/usb/uvc/uvc_video.c: drivers/media/usb/uvc/uvc_video.c:1893 uvc_video_start_transfer() warn: argument 2 to %u specifier is cast from pointer
> > 
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> This look fine to me.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Even though I believe we should fix tooling instead of code to handle
these issues, the patch for uvcvideo doesn't adversely affect the code,
so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Should I take this in my tree ?

> > ---
> >  drivers/media/usb/uvc/uvcvideo.h | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 9b41b14ce076..c7c1baa90dea 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -620,8 +620,10 @@ struct uvc_streaming {
> >  	     (uvc_urb) < &(uvc_streaming)->uvc_urb[UVC_URBS]; \
> >  	     ++(uvc_urb))
> >  
> > -#define uvc_urb_index(uvc_urb) \
> > -	(unsigned int)((uvc_urb) - (&(uvc_urb)->stream->uvc_urb[0]))
> > +static inline u32 uvc_urb_index(const struct uvc_urb *uvc_urb)
> > +{
> > +	return uvc_urb - &uvc_urb->stream->uvc_urb[0];
> > +}
> > 
> >  struct uvc_device_info {
> >  	u32	quirks;
> > 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/6] omap3isp: fix sparse warning

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Thu, Feb 07, 2019 at 10:13:37AM +0100, Hans Verkuil wrote:
> drivers/media/platform/omap3isp/ispvideo.c:1013:15: warning: unknown expression (4 0)
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/platform/omap3isp/ispvideo.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
> index 078d64114b24..911dfad90d32 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -974,6 +974,7 @@ static int isp_video_check_external_subdevs(struct isp_video *video,
>  	struct media_entity *source = NULL;
>  	struct media_entity *sink;
>  	struct v4l2_subdev_format fmt;
> +	struct v4l2_subdev *sd;
>  	struct v4l2_ext_controls ctrls;
>  	struct v4l2_ext_control ctrl;
>  	unsigned int i;
> @@ -1010,8 +1011,8 @@ static int isp_video_check_external_subdevs(struct isp_video *video,
>  
>  	fmt.pad = source_pad->index;
>  	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> -	ret = v4l2_subdev_call(media_entity_to_v4l2_subdev(sink),
> -			       pad, get_fmt, NULL, &fmt);
> +	sd = media_entity_to_v4l2_subdev(sink);
> +	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);

This is really a workaround for an issue in smath, and I don't like it.
Where will we draw the line if we accept it ? Will we start rejecting
all nested function calls or macros because we have a tooling issue ?
This one really needs to be fixed in smatch, the code is totally fine.

>  	if (unlikely(ret < 0)) {
>  		dev_warn(isp->dev, "get_fmt returned null!\n");
>  		return ret;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/6] omap4iss: fix sparse warning

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Thu, Feb 07, 2019 at 03:38:02PM +0100, Hans Verkuil wrote:
> drivers/staging/media/omap4iss/iss.c:141:15: warning: unknown expression (4 0)
> drivers/staging/media/omap4iss/iss.c:141:15: warning: unknown expression (4 0)
> drivers/staging/media/omap4iss/iss.c:141:15: warning: unknown expression (4 0)
> drivers/staging/media/omap4iss/iss.c:141:15: warning: unknown expression (4 0)
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> Same fix as for omap3isp. I discovered that staging drivers weren't built by the
> daily build, so I never noticed these warnings.

Same answer as for the omap3isp driver then :-) Let's fix the tool
please. Dan Carpenter has been very supportive when we reported smatch
issues in the past, let's work with him to improve the tool instead of
patching all current and future kernel code to work around the problem.

> ---
> diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
> index c8be1db532ab..fd702947cdb8 100644
> --- a/drivers/staging/media/omap4iss/iss.c
> +++ b/drivers/staging/media/omap4iss/iss.c
> @@ -124,6 +124,7 @@ int omap4iss_get_external_info(struct iss_pipeline *pipe,
>  {
>  	struct iss_device *iss =
>  		container_of(pipe, struct iss_video, pipe)->iss;
> +	struct v4l2_subdev *sd;
>  	struct v4l2_subdev_format fmt;
>  	struct v4l2_ctrl *ctrl;
>  	int ret;
> @@ -138,8 +139,8 @@ int omap4iss_get_external_info(struct iss_pipeline *pipe,
> 
>  	fmt.pad = link->source->index;
>  	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> -	ret = v4l2_subdev_call(media_entity_to_v4l2_subdev(link->sink->entity),
> -			       pad, get_fmt, NULL, &fmt);
> +	sd = media_entity_to_v4l2_subdev(link->sink->entity);
> +	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
>  	if (ret < 0)
>  		return -EPIPE;
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/6] uvc: fix smatch warning

[Hans Verkuil]

On 2/7/19 4:41 PM, Laurent Pinchart wrote:
> Hello,
> 
> On Thu, Feb 07, 2019 at 03:57:26PM +0100, Kieran Bingham wrote:
>> On 07/02/2019 10:13, Hans Overkill wrote:
>>> drivers/media/usb/uvc/uvc_video.c: drivers/media/usb/uvc/uvc_video.c:1893 uvc_video_start_transfer() warn: argument 2 to %u specifier is cast from pointer
>>>
>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>
>> This look fine to me.
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Even though I believe we should fix tooling instead of code to handle
> these issues, the patch for uvcvideo doesn't adversely affect the code,
> so
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Should I take this in my tree ?

Yes, go ahead.

Regards,

	Hans

> 
>>> ---
>>>  drivers/media/usb/uvc/uvcvideo.h | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
>>> index 9b41b14ce076..c7c1baa90dea 100644
>>> --- a/drivers/media/usb/uvc/uvcvideo.h
>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>>> @@ -620,8 +620,10 @@ struct uvc_streaming {
>>>  	     (uvc_urb) < &(uvc_streaming)->uvc_urb[UVC_URBS]; \
>>>  	     ++(uvc_urb))
>>>  
>>> -#define uvc_urb_index(uvc_urb) \
>>> -	(unsigned int)((uvc_urb) - (&(uvc_urb)->stream->uvc_urb[0]))
>>> +static inline u32 uvc_urb_index(const struct uvc_urb *uvc_urb)
>>> +{
>>> +	return uvc_urb - &uvc_urb->stream->uvc_urb[0];
>>> +}
>>>
>>>  struct uvc_device_info {
>>>  	u32	quirks;
>>>
> 

Re: [PATCH 4/6] vsp1: fix smatch warning

[Kieran Bingham]

Hi Hans,

On 07/02/2019 09:13, Hans Verkuil wrote:
> drivers/media/platform/vsp1/vsp1_drm.c: drivers/media/platform/vsp1/vsp1_drm.c:336 vsp1_du_pipeline_setup_brx() error: we previously assumed 'pipe->brx' could be null (see line 244)
> 
> smatch missed that if pipe->brx was NULL, then later on it will be
> set with a non-NULL value. But it is easier to just use the brx pointer
> so smatch doesn't get confused.
> 

Aha, my initial reaction to this was "Oh - but we've already looked at
this and it was a false positive ..." And then I looked at your patch ...

So my initial reaction was completely wrong - and you have indeed got a
good patch :)

As this function is to 'setup the brx' I think it's very reasonable to
use the chosen BRX pointer to do the configuration. (once all the
dancing has gone on to swap as necessary)

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

And for sanity, I've tested this with our VSP Tests on both:

  Salvator-XS-H3 ES2.0 : 164 tests: 148 passed, 0 failed, 3 skipped
and
  Salvator-XS-M3N      : 164 tests: 148 passed, 0 failed, 3 skipped


Tested-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Tested-on: Salvator-XS-ES2.0, Salvator-XS-M3N

> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/platform/vsp1/vsp1_drm.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> index 8d86f618ec77..84895385d2e5 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -333,19 +333,19 @@ static int vsp1_du_pipeline_setup_brx(struct vsp1_device *vsp1,
>  	 * on the BRx sink pad 0 and propagated inside the entity, not on the
>  	 * source pad.
>  	 */
> -	format.pad = pipe->brx->source_pad;
> +	format.pad = brx->source_pad;
>  	format.format.width = drm_pipe->width;
>  	format.format.height = drm_pipe->height;
>  	format.format.field = V4L2_FIELD_NONE;
>  
> -	ret = v4l2_subdev_call(&pipe->brx->subdev, pad, set_fmt, NULL,
> +	ret = v4l2_subdev_call(&brx->subdev, pad, set_fmt, NULL,
>  			       &format);
>  	if (ret < 0)
>  		return ret;
>  
>  	dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on %s pad %u\n",
>  		__func__, format.format.width, format.format.height,
> -		format.format.code, BRX_NAME(pipe->brx), pipe->brx->source_pad);
> +		format.format.code, BRX_NAME(brx), brx->source_pad);
>  
>  	if (format.format.width != drm_pipe->width ||
>  	    format.format.height != drm_pipe->height) {
> 

-- 
Regards
--
Kieran

Re: [PATCH 4/6] vsp1: fix smatch warning

[Laurent Pinchart]

On Tue, Feb 12, 2019 at 01:48:00PM +0000, Kieran Bingham wrote:
> On 07/02/2019 09:13, Hans Verkuil wrote:
> > drivers/media/platform/vsp1/vsp1_drm.c: drivers/media/platform/vsp1/vsp1_drm.c:336 vsp1_du_pipeline_setup_brx() error: we previously assumed 'pipe->brx' could be null (see line 244)
> > 
> > smatch missed that if pipe->brx was NULL, then later on it will be
> > set with a non-NULL value. But it is easier to just use the brx pointer
> > so smatch doesn't get confused.
> > 
> 
> Aha, my initial reaction to this was "Oh - but we've already looked at
> this and it was a false positive ..." And then I looked at your patch ...
> 
> So my initial reaction was completely wrong - and you have indeed got a
> good patch :)
> 
> As this function is to 'setup the brx' I think it's very reasonable to
> use the chosen BRX pointer to do the configuration. (once all the
> dancing has gone on to swap as necessary)
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> And for sanity, I've tested this with our VSP Tests on both:
> 
>   Salvator-XS-H3 ES2.0 : 164 tests: 148 passed, 0 failed, 3 skipped
> and
>   Salvator-XS-M3N      : 164 tests: 148 passed, 0 failed, 3 skipped
> 
> 
> Tested-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Tested-on: Salvator-XS-ES2.0, Salvator-XS-M3N

Taken in my tree with all these tags and my

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > ---
> >  drivers/media/platform/vsp1/vsp1_drm.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> > index 8d86f618ec77..84895385d2e5 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > @@ -333,19 +333,19 @@ static int vsp1_du_pipeline_setup_brx(struct vsp1_device *vsp1,
> >  	 * on the BRx sink pad 0 and propagated inside the entity, not on the
> >  	 * source pad.
> >  	 */
> > -	format.pad = pipe->brx->source_pad;
> > +	format.pad = brx->source_pad;
> >  	format.format.width = drm_pipe->width;
> >  	format.format.height = drm_pipe->height;
> >  	format.format.field = V4L2_FIELD_NONE;
> >  
> > -	ret = v4l2_subdev_call(&pipe->brx->subdev, pad, set_fmt, NULL,
> > +	ret = v4l2_subdev_call(&brx->subdev, pad, set_fmt, NULL,
> >  			       &format);
> >  	if (ret < 0)
> >  		return ret;
> >  
> >  	dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on %s pad %u\n",
> >  		__func__, format.format.width, format.format.height,
> > -		format.format.code, BRX_NAME(pipe->brx), pipe->brx->source_pad);
> > +		format.format.code, BRX_NAME(brx), brx->source_pad);
> >  
> >  	if (format.format.width != drm_pipe->width ||
> >  	    format.format.height != drm_pipe->height) {

-- 
Regards,

Laurent Pinchart