[PATCH 0/5] Fix smatch warnings

[PATCH 0/5] Fix smatch warnings

[Mauro Carvalho Chehab]

Fix smatch warnings on media subsystem.

Mauro Carvalho Chehab (5):
  media: cx2341x: replace badly designed macros
  media: pwc-ctl: pChoose can't be NULL
  media: sti/delta: remove uneeded check
  media: rcar-dma: p_set can't be NULL
  media: hfi_parser: don't trick gcc with a wrong expected size

 drivers/media/common/cx2341x.c                | 151 +++++++++++-------
 .../media/platform/qcom/venus/hfi_helper.h    |   4 +-
 drivers/media/platform/rcar-vin/rcar-dma.c    |   2 +-
 drivers/media/platform/sti/delta/delta-ipc.c  |   6 +-
 drivers/media/usb/pwc/pwc-ctrl.c              |  17 +-
 5 files changed, 107 insertions(+), 73 deletions(-)

-- 
2.20.1

[PATCH 1/5] media: cx2341x: replace badly designed macros

[Mauro Carvalho Chehab]

There are some macros at cx2341x_update() with seemed to
be introduced in order to ensure that lines would be less
than 80 columns.

Well, the thing is that they make the code harder to be analized,
not only by humans, but also for static code analyzers:

	drivers/media/common/cx2341x.c:1116 cx2341x_update() error: we previously assumed 'old' could be null (see line 1047)

So, remove the "force" var, and replace the NEQ macro to a
better designed one that makes clearer about what it is doing.

While here, also remove the "temporal" var, as it is just another
way of doing the same type of check as the new CMP_FIELD() macro
already does.

Finally, fix coding style at the block code.
 remove such macros.

Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
 drivers/media/common/cx2341x.c | 151 ++++++++++++++++++++-------------
 1 file changed, 93 insertions(+), 58 deletions(-)

diff --git a/drivers/media/common/cx2341x.c b/drivers/media/common/cx2341x.c
index 1dcc39b87bb7..121cda73ff88 100644
--- a/drivers/media/common/cx2341x.c
+++ b/drivers/media/common/cx2341x.c
@@ -1028,7 +1028,7 @@ static int cx2341x_api(void *priv, cx2341x_mbox_func func,
 	return func(priv, cmd, args, 0, data);
 }
 
-#define NEQ(field) (old->field != new->field)
+#define CMP_FIELD(__old, __new, __field) (__old->__field != __new->__field)
 
 int cx2341x_update(void *priv, cx2341x_mbox_func func,
 		   const struct cx2341x_mpeg_params *old,
@@ -1042,20 +1042,22 @@ int cx2341x_update(void *priv, cx2341x_mbox_func func,
 		11,	/* VCD */
 		12,	/* SVCD */
 	};
-
-	int err = 0;
-	int force = (old == NULL);
-	u16 temporal = new->video_temporal_filter;
+	int err;
 
 	cx2341x_api(priv, func, CX2341X_ENC_SET_OUTPUT_PORT, 2, new->port, 0);
 
-	if (force || NEQ(is_50hz)) {
+	if (!old ||
+	    CMP_FIELD(old, new, is_50hz)) {
 		err = cx2341x_api(priv, func, CX2341X_ENC_SET_FRAME_RATE, 1,
 				  new->is_50hz);
-		if (err) return err;
+		if (err)
+			return err;
 	}
 
-	if (force || NEQ(width) || NEQ(height) || NEQ(video_encoding)) {
+	if (!old ||
+	    CMP_FIELD(old, new, width) ||
+	    CMP_FIELD(old, new, height) ||
+	    CMP_FIELD(old, new, video_encoding)) {
 		u16 w = new->width;
 		u16 h = new->height;
 
@@ -1065,94 +1067,127 @@ int cx2341x_update(void *priv, cx2341x_mbox_func func,
 		}
 		err = cx2341x_api(priv, func, CX2341X_ENC_SET_FRAME_SIZE, 2,
 				  h, w);
-		if (err) return err;
+		if (err)
+			return err;
 	}
-	if (force || NEQ(stream_type)) {
+	if (!old ||
+	    CMP_FIELD(old, new, stream_type)) {
 		err = cx2341x_api(priv, func, CX2341X_ENC_SET_STREAM_TYPE, 1,
 				  mpeg_stream_type[new->stream_type]);
-		if (err) return err;
+		if (err)
+			return err;
 	}
-	if (force || NEQ(video_aspect)) {
+	if (!old ||
+	    CMP_FIELD(old, new, video_aspect)) {
 		err = cx2341x_api(priv, func, CX2341X_ENC_SET_ASPECT_RATIO, 1,
 				  1 + new->video_aspect);
-		if (err) return err;
+		if (err)
+			return err;
 	}
-	if (force || NEQ(video_b_frames) || NEQ(video_gop_size)) {
+	if (!old ||
+	    CMP_FIELD(old, new, video_b_frames) ||
+	    CMP_FIELD(old, new, video_gop_size)) {
 		err = cx2341x_api(priv, func, CX2341X_ENC_SET_GOP_PROPERTIES, 2,
-				new->video_gop_size, new->video_b_frames + 1);
-		if (err) return err;
+				  new->video_gop_size, new->video_b_frames + 1);
+		if (err)
+			return err;
 	}
-	if (force || NEQ(video_gop_closure)) {
+	if (!old ||
+	    CMP_FIELD(old, new, video_gop_closure)) {
 		err = cx2341x_api(priv, func, CX2341X_ENC_SET_GOP_CLOSURE, 1,
 				  new->video_gop_closure);
-		if (err) return err;
+		if (err)
+			return err;
 	}
-	if (force || NEQ(audio_properties)) {
+	if (!old ||
+	    CMP_FIELD(old, new, audio_properties)) {
 		err = cx2341x_api(priv, func, CX2341X_ENC_SET_AUDIO_PROPERTIES,
 				  1, new->audio_properties);
-		if (err) return err;
+		if (err)
+			return err;
 	}
-	if (force || NEQ(audio_mute)) {
+	if (!old ||
+	    CMP_FIELD(old, new, audio_mute)) {
 		err = cx2341x_api(priv, func, CX2341X_ENC_MUTE_AUDIO, 1,
 				  new->audio_mute);
-		if (err) return err;
+		if (err)
+			return err;
 	}
-	if (force || NEQ(video_bitrate_mode) || NEQ(video_bitrate) ||
-						NEQ(video_bitrate_peak)) {
+	if (!old ||
+	    CMP_FIELD(old, new, video_bitrate_mode) ||
+	    CMP_FIELD(old, new, video_bitrate) ||
+	    CMP_FIELD(old, new, video_bitrate_peak)) {
 		err = cx2341x_api(priv, func, CX2341X_ENC_SET_BIT_RATE, 5,
-				new->video_bitrate_mode, new->video_bitrate,
-				new->video_bitrate_peak / 400, 0, 0);
-		if (err) return err;
+				  new->video_bitrate_mode, new->video_bitrate,
+				  new->video_bitrate_peak / 400, 0, 0);
+		if (err)
+			return err;
 	}
-	if (force || NEQ(video_spatial_filter_mode) ||
-		     NEQ(video_temporal_filter_mode) ||
-		     NEQ(video_median_filter_type)) {
+	if (!old ||
+	    CMP_FIELD(old, new, video_spatial_filter_mode) ||
+	    CMP_FIELD(old, new, video_temporal_filter_mode) ||
+	    CMP_FIELD(old, new, video_median_filter_type)) {
 		err = cx2341x_api(priv, func, CX2341X_ENC_SET_DNR_FILTER_MODE,
-				  2, new->video_spatial_filter_mode |
+				  2,
+				  new->video_spatial_filter_mode |
 					(new->video_temporal_filter_mode << 1),
-				new->video_median_filter_type);
-		if (err) return err;
+				  new->video_median_filter_type);
+		if (err)
+			return err;
 	}
-	if (force || NEQ(video_luma_median_filter_bottom) ||
-		     NEQ(video_luma_median_filter_top) ||
-		     NEQ(video_chroma_median_filter_bottom) ||
-		     NEQ(video_chroma_median_filter_top)) {
+	if (!old ||
+	    CMP_FIELD(old, new, video_luma_median_filter_bottom) ||
+	    CMP_FIELD(old, new, video_luma_median_filter_top) ||
+	    CMP_FIELD(old, new, video_chroma_median_filter_bottom) ||
+	    CMP_FIELD(old, new, video_chroma_median_filter_top)) {
 		err = cx2341x_api(priv, func, CX2341X_ENC_SET_CORING_LEVELS, 4,
-				new->video_luma_median_filter_bottom,
-				new->video_luma_median_filter_top,
-				new->video_chroma_median_filter_bottom,
-				new->video_chroma_median_filter_top);
-		if (err) return err;
+				  new->video_luma_median_filter_bottom,
+				  new->video_luma_median_filter_top,
+				  new->video_chroma_median_filter_bottom,
+				  new->video_chroma_median_filter_top);
+		if (err)
+			return err;
 	}
-	if (force || NEQ(video_luma_spatial_filter_type) ||
-		     NEQ(video_chroma_spatial_filter_type)) {
+	if (!old ||
+	    CMP_FIELD(old, new, video_luma_spatial_filter_type) ||
+	    CMP_FIELD(old, new, video_chroma_spatial_filter_type)) {
 		err = cx2341x_api(priv, func,
 				  CX2341X_ENC_SET_SPATIAL_FILTER_TYPE,
 				  2, new->video_luma_spatial_filter_type,
 				  new->video_chroma_spatial_filter_type);
-		if (err) return err;
+		if (err)
+			return err;
 	}
-	if (force || NEQ(video_spatial_filter) ||
-		     old->video_temporal_filter != temporal) {
+	if (!old ||
+	    CMP_FIELD(old, new, video_spatial_filter) ||
+	    CMP_FIELD(old, new, video_temporal_filter)) {
 		err = cx2341x_api(priv, func, CX2341X_ENC_SET_DNR_FILTER_PROPS,
-				  2, new->video_spatial_filter, temporal);
-		if (err) return err;
+				  2, new->video_spatial_filter,
+				  new->video_temporal_filter);
+		if (err)
+			return err;
 	}
-	if (force || NEQ(video_temporal_decimation)) {
+	if (!old ||
+	    CMP_FIELD(old, new, video_temporal_decimation)) {
 		err = cx2341x_api(priv, func, CX2341X_ENC_SET_FRAME_DROP_RATE,
 				  1, new->video_temporal_decimation);
-		if (err) return err;
+		if (err)
+			return err;
 	}
-	if (force || NEQ(video_mute) ||
-		(new->video_mute && NEQ(video_mute_yuv))) {
+	if (!old ||
+	    CMP_FIELD(old, new, video_mute) ||
+	    (new->video_mute && CMP_FIELD(old, new, video_mute_yuv))) {
 		err = cx2341x_api(priv, func, CX2341X_ENC_MUTE_VIDEO, 1,
-				new->video_mute | (new->video_mute_yuv << 8));
-		if (err) return err;
+				  new->video_mute | (new->video_mute_yuv << 8));
+		if (err)
+			return err;
 	}
-	if (force || NEQ(stream_insert_nav_packets)) {
+	if (!old ||
+	    CMP_FIELD(old, new, stream_insert_nav_packets)) {
 		err = cx2341x_api(priv, func, CX2341X_ENC_MISC, 2,
-				7, new->stream_insert_nav_packets);
-		if (err) return err;
+				  7, new->stream_insert_nav_packets);
+		if (err)
+			return err;
 	}
 	return 0;
 }
-- 
2.20.1

[PATCH 2/5] media: pwc-ctl: pChoose can't be NULL

[Mauro Carvalho Chehab]

The way the code works, compression will be a valid value (less or equal to 3)
on both set_video_mode_foo() calls at the beginning of the while() loop.

So, the value for pChoose can't be NULL.

Solves those smatch warnings:

	drivers/media/usb/pwc/pwc-ctrl.c: drivers/media/usb/pwc/pwc-ctrl.c:252 set_video_mode_Timon() warn: variable dereferenced before check 'pChoose' (see line 248)
	drivers/media/usb/pwc/pwc-ctrl.c: drivers/media/usb/pwc/pwc-ctrl.c:302 set_video_mode_Kiara() warn: variable dereferenced before check 'pChoose' (see line 298)

and simplifies the code a little bit.

Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
 drivers/media/usb/pwc/pwc-ctrl.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/pwc/pwc-ctrl.c b/drivers/media/usb/pwc/pwc-ctrl.c
index 655cef39eb3d..b681a184ef87 100644
--- a/drivers/media/usb/pwc/pwc-ctrl.c
+++ b/drivers/media/usb/pwc/pwc-ctrl.c
@@ -242,14 +242,14 @@ static int set_video_mode_Timon(struct pwc_device *pdev, int size, int pixfmt,
 	fps = (frames / 5) - 1;
 
 	/* Find a supported framerate with progressively higher compression */
-	pChoose = NULL;
-	while (*compression <= 3) {
+	do {
 		pChoose = &Timon_table[size][fps][*compression];
 		if (pChoose->alternate != 0)
 			break;
 		(*compression)++;
-	}
-	if (pChoose == NULL || pChoose->alternate == 0)
+	} while (*compression <= 3);
+
+	if (pChoose->alternate == 0)
 		return -ENOENT; /* Not supported. */
 
 	if (send_to_cam)
@@ -279,7 +279,7 @@ static int set_video_mode_Timon(struct pwc_device *pdev, int size, int pixfmt,
 static int set_video_mode_Kiara(struct pwc_device *pdev, int size, int pixfmt,
 				int frames, int *compression, int send_to_cam)
 {
-	const struct Kiara_table_entry *pChoose = NULL;
+	const struct Kiara_table_entry *pChoose;
 	int fps, ret = 0;
 
 	if (size >= PSZ_MAX || *compression < 0 || *compression > 3)
@@ -293,13 +293,14 @@ static int set_video_mode_Kiara(struct pwc_device *pdev, int size, int pixfmt,
 	fps = (frames / 5) - 1;
 
 	/* Find a supported framerate with progressively higher compression */
-	while (*compression <= 3) {
+	do {
 		pChoose = &Kiara_table[size][fps][*compression];
 		if (pChoose->alternate != 0)
 			break;
 		(*compression)++;
-	}
-	if (pChoose == NULL || pChoose->alternate == 0)
+	} while (*compression <= 3);
+
+	if (pChoose->alternate == 0)
 		return -ENOENT; /* Not supported. */
 
 	/* Firmware bug: video endpoint is 5, but commands are sent to endpoint 4 */
-- 
2.20.1

[PATCH 3/5] media: sti/delta: remove uneeded check

[Mauro Carvalho Chehab]

At the error logic, ipc_buf was already asigned to &ctx->ipc_buf_struct,
with can't be null, as warned by smatch:

	drivers/media/platform/sti/delta/delta-ipc.c:223 delta_ipc_open() warn: variable dereferenced before check 'ctx->ipc_buf' (see line 183)

So, remove the uneeded check.

Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
 drivers/media/platform/sti/delta/delta-ipc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/sti/delta/delta-ipc.c b/drivers/media/platform/sti/delta/delta-ipc.c
index a4603d573c34..186d88f02ecd 100644
--- a/drivers/media/platform/sti/delta/delta-ipc.c
+++ b/drivers/media/platform/sti/delta/delta-ipc.c
@@ -220,10 +220,8 @@ int delta_ipc_open(struct delta_ctx *pctx, const char *name,
 
 err:
 	pctx->sys_errors++;
-	if (ctx->ipc_buf) {
-		hw_free(pctx, ctx->ipc_buf);
-		ctx->ipc_buf = NULL;
-	}
+	hw_free(pctx, ctx->ipc_buf);
+	ctx->ipc_buf = NULL;
 
 	return ret;
 };
-- 
2.20.1

[PATCH 4/5] media: rcar-dma: p_set can't be NULL

[Mauro Carvalho Chehab]

The only way for p_set to be NULL would be if vin_coef_set would be an
empty array.

On such case, the driver will OOPS, as it would try to de-reference a
NULL value. So, the check if p_set is not NULL doesn't make any sense.

Solves those two smatch warnings:

	drivers/media/platform/rcar-vin/rcar-dma.c:489 rvin_set_coeff() warn: variable dereferenced before check 'p_set' (see line 484)
	drivers/media/platform/rcar-vin/rcar-dma.c:494 rvin_set_coeff() error: we previously assumed 'p_set' could be null (see line 489)

Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 2207a31d355e..91ab064404a1 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -486,7 +486,7 @@ static void rvin_set_coeff(struct rvin_dev *vin, unsigned short xs)
 	}
 
 	/* Use previous value if its XS value is closer */
-	if (p_prev_set && p_set &&
+	if (p_prev_set &&
 	    xs - p_prev_set->xs_value < p_set->xs_value - xs)
 		p_set = p_prev_set;
 
-- 
2.20.1

[PATCH 5/5] media: hfi_parser: don't trick gcc with a wrong expected size

[Mauro Carvalho Chehab]

Smatch warns about small size on two structs:

	drivers/media/platform/qcom/venus/hfi_parser.c:103 parse_profile_level() error: memcpy() 'proflevel' too small (8 vs 128)
	drivers/media/platform/qcom/venus/hfi_parser.c: drivers/media/platform/qcom/venus/hfi_parser.c:129 parse_caps() error: memcpy() 'cap' too small (16 vs 512)

The reason is that the hfi_parser actually expects:
    - multiple data entries on hfi_capabilities
    - multiple profile_level on hfi_profile_level_supported

However, the structs trick gcc, making it to believe that
there's just one value for each.

Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
 drivers/media/platform/qcom/venus/hfi_helper.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/hfi_helper.h b/drivers/media/platform/qcom/venus/hfi_helper.h
index 15804ad7e65d..34ea503a9842 100644
--- a/drivers/media/platform/qcom/venus/hfi_helper.h
+++ b/drivers/media/platform/qcom/venus/hfi_helper.h
@@ -569,7 +569,7 @@ struct hfi_capability {
 
 struct hfi_capabilities {
 	u32 num_capabilities;
-	struct hfi_capability data[1];
+	struct hfi_capability *data;
 };
 
 #define HFI_DEBUG_MSG_LOW	0x01
@@ -726,7 +726,7 @@ struct hfi_profile_level {
 
 struct hfi_profile_level_supported {
 	u32 profile_count;
-	struct hfi_profile_level profile_level[1];
+	struct hfi_profile_level *profile_level;
 };
 
 struct hfi_quality_vs_speed {
-- 
2.20.1

Re: [PATCH 4/5] media: rcar-dma: p_set can't be NULL

[Sergei Shtylyov]

Hello!

    I'd have used instead (or at least mentioned) the driver's name (rcar-vin) 
in the subject.

On 26.03.2019 1:03, Mauro Carvalho Chehab wrote:
> The only way for p_set to be NULL would be if vin_coef_set would be an
> empty array.
> 
> On such case, the driver will OOPS, as it would try to de-reference a
> NULL value. So, the check if p_set is not NULL doesn't make any sense.
> 
> Solves those two smatch warnings:
> 
> 	drivers/media/platform/rcar-vin/rcar-dma.c:489 rvin_set_coeff() warn: variable dereferenced before check 'p_set' (see line 484)
> 	drivers/media/platform/rcar-vin/rcar-dma.c:494 rvin_set_coeff() error: we previously assumed 'p_set' could be null (see line 489)
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
>   drivers/media/platform/rcar-vin/rcar-dma.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 2207a31d355e..91ab064404a1 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -486,7 +486,7 @@ static void rvin_set_coeff(struct rvin_dev *vin, unsigned short xs)
>   	}
>   
>   	/* Use previous value if its XS value is closer */
> -	if (p_prev_set && p_set &&
> +	if (p_prev_set &&
>   	    xs - p_prev_set->xs_value < p_set->xs_value - xs)
>   		p_set = p_prev_set;
>   

MBR, Sergei