[PATCH 00/17] media: sparse/smatch fixes

[PATCH 00/17] media: sparse/smatch fixes

[Hans Verkuil]

We have way too many sparse/smatch warnings and errors today, making
it hard to tell when new issues are introduced.

This series will get rid of most of them.

Regards,

	Hans

Hans Verkuil (17):
  media: visl: make visl_qops static
  media: davinci/vpif.c: drop unnecessary cast
  media: i2c: s5c73m3: return 0 instead of 'ret'.
  media: dvb-frontends: cxd2880: return 0 instead of 'ret'.
  media: usb: dvb-usb-v2: af9015.c: return 0 instead of 'ret'.
  media: dvb-frontends: cxd2880: return 0 instead of 'ret'.
  media: marvell: change return to goto for proper unwind
  media: dvb-frontends: drx39xyj: replace return with goto for proper
    unwind
  media: nxp: imx-jpeg: replace dummy gotos by returns
  media: mediatek: mdp3: replace return by goto for proper unwind
  media: mediatek: vcodec/venc: return 0 instead of 'ret'.
  media: ti: davinci: vpbe_display.c: return 0 instead of 'ret'.
  media: i2c: ov7670: 0 instead of -EINVAL was returned
  media: usb: go7007: add second unwind label
  media: i2c: adp1653: introduce 'no_child' label
  media: st: delta: introduce 'err_too_many_comps' label
  media: dvb-frontends: mb86a16.c: always use the same error path

 .../dvb-frontends/cxd2880/cxd2880_tnrdmd.c    |  4 ++--
 .../cxd2880/cxd2880_tnrdmd_dvbt.c             | 14 +++++++-------
 .../cxd2880/cxd2880_tnrdmd_dvbt2.c            | 14 +++++++-------
 drivers/media/dvb-frontends/drx39xyj/drxj.c   |  9 ++++++---
 drivers/media/dvb-frontends/mb86a16.c         |  9 ++++++---
 drivers/media/i2c/adp1653.c                   |  5 +++--
 drivers/media/i2c/ov7670.c                    |  2 +-
 drivers/media/i2c/s5c73m3/s5c73m3-core.c      |  2 +-
 drivers/media/platform/marvell/mmp-driver.c   |  2 +-
 .../platform/mediatek/mdp3/mtk-mdp3-comp.c    |  3 ++-
 .../mediatek/vcodec/venc/venc_h264_if.c       |  4 ++--
 .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 19 ++++++-------------
 .../platform/st/sti/delta/delta-mjpeg-hdr.c   | 16 +++++++++-------
 .../media/platform/ti/davinci/vpbe_display.c  |  2 +-
 drivers/media/platform/ti/davinci/vpif.c      |  2 +-
 drivers/media/test-drivers/visl/visl-video.c  |  2 +-
 drivers/media/usb/dvb-usb-v2/af9015.c         |  4 ++--
 drivers/media/usb/go7007/go7007-usb.c         | 11 +++++++----
 18 files changed, 65 insertions(+), 59 deletions(-)

-- 
2.39.0

[PATCH 01/17] media: visl: make visl_qops static

[Hans Verkuil]

This struct can be static. This fixes a sparse warning:

visl-video.c:690:22: warning: symbol 'visl_qops' was not declared. Should it be static?

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Daniel Almeida <daniel.almeida@collabora.com>
---
 drivers/media/test-drivers/visl/visl-video.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/test-drivers/visl/visl-video.c b/drivers/media/test-drivers/visl/visl-video.c
index b08664dfbe5f..7cac6a6456eb 100644
--- a/drivers/media/test-drivers/visl/visl-video.c
+++ b/drivers/media/test-drivers/visl/visl-video.c
@@ -687,7 +687,7 @@ static void visl_buf_request_complete(struct vb2_buffer *vb)
 	v4l2_ctrl_request_complete(vb->req_obj.req, &ctx->hdl);
 }
 
-const struct vb2_ops visl_qops = {
+static const struct vb2_ops visl_qops = {
 	.queue_setup          = visl_queue_setup,
 	.buf_out_validate     = visl_buf_out_validate,
 	.buf_prepare          = visl_buf_prepare,
-- 
2.39.0

[PATCH 02/17] media: davinci/vpif.c: drop unnecessary cast

[Hans Verkuil]

DEFINE_RES_IRQ_NAMED already casts to (struct resource), so no
need to do it again.

This fixes a sparse warning:

vpif.c:483:20: warning: cast to non-scalar

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
---
 drivers/media/platform/ti/davinci/vpif.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/ti/davinci/vpif.c b/drivers/media/platform/ti/davinci/vpif.c
index da27da4c165a..832489822706 100644
--- a/drivers/media/platform/ti/davinci/vpif.c
+++ b/drivers/media/platform/ti/davinci/vpif.c
@@ -480,7 +480,7 @@ static int vpif_probe(struct platform_device *pdev)
 		ret = irq;
 		goto err_put_rpm;
 	}
-	res_irq = (struct resource)DEFINE_RES_IRQ_NAMED(irq, of_node_full_name(pdev->dev.of_node));
+	res_irq = DEFINE_RES_IRQ_NAMED(irq, of_node_full_name(pdev->dev.of_node));
 	res_irq.flags |= irq_get_trigger_type(irq);
 
 	pdev_capture = kzalloc(sizeof(*pdev_capture), GFP_KERNEL);
-- 
2.39.0

Re: [PATCH 02/17] media: davinci/vpif.c: drop unnecessary cast

[Lad, Prabhakar]

Hi Hans,

Thank you for the patch.

On Thu, Jan 26, 2023 at 3:07 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> DEFINE_RES_IRQ_NAMED already casts to (struct resource), so no
> need to do it again.
>
> This fixes a sparse warning:
>
> vpif.c:483:20: warning: cast to non-scalar
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Cc: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
> ---
>  drivers/media/platform/ti/davinci/vpif.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
Reviewed-by: Lad Prabhakar <prabhakar.csengg@gmail.com>

Cheers,
Prabhakar

> diff --git a/drivers/media/platform/ti/davinci/vpif.c b/drivers/media/platform/ti/davinci/vpif.c
> index da27da4c165a..832489822706 100644
> --- a/drivers/media/platform/ti/davinci/vpif.c
> +++ b/drivers/media/platform/ti/davinci/vpif.c
> @@ -480,7 +480,7 @@ static int vpif_probe(struct platform_device *pdev)
>                 ret = irq;
>                 goto err_put_rpm;
>         }
> -       res_irq = (struct resource)DEFINE_RES_IRQ_NAMED(irq, of_node_full_name(pdev->dev.of_node));
> +       res_irq = DEFINE_RES_IRQ_NAMED(irq, of_node_full_name(pdev->dev.of_node));
>         res_irq.flags |= irq_get_trigger_type(irq);
>
>         pdev_capture = kzalloc(sizeof(*pdev_capture), GFP_KERNEL);
> --
> 2.39.0
>

[PATCH 03/17] media: i2c: s5c73m3: return 0 instead of 'ret'.

[Hans Verkuil]

Since 'ret' is known to be 0, just return '0'. This fixes a smatch warning:

s5c73m3-core.c:439 __s5c73m3_s_stream() warn: missing error code? 'ret'

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
 drivers/media/i2c/s5c73m3/s5c73m3-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
index 59b03b0860d5..85d5f76386be 100644
--- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c
+++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
@@ -436,7 +436,7 @@ static int __s5c73m3_s_stream(struct s5c73m3 *state, struct v4l2_subdev *sd,
 	state->streaming = !!on;
 
 	if (!on)
-		return ret;
+		return 0;
 
 	if (state->apply_fiv) {
 		ret = s5c73m3_set_frame_rate(state);
-- 
2.39.0

[PATCH 04/17] media: dvb-frontends: cxd2880: return 0 instead of 'ret'.

[Hans Verkuil]

Since 'ret' is known to be 0, just return '0'. This fixes two smatch warnings:

cxd2880_tnrdmd.c:2165 cxd2880_tnrdmd_check_internal_cpu_status() warn: missing error code? 'ret'
cxd2880_tnrdmd.c:2169 cxd2880_tnrdmd_check_internal_cpu_status() warn: missing error code? 'ret'

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Yasunari Takiguchi <Yasunari.Takiguchi@sony.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
---
 drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd.c b/drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd.c
index 4cf2d7cfd3f5..0a1f3899d72c 100644
--- a/drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd.c
+++ b/drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd.c
@@ -2162,11 +2162,11 @@ int cxd2880_tnrdmd_check_internal_cpu_status(struct cxd2880_tnrdmd
 		else
 			*task_completed = 0;
 
-		return ret;
+		return 0;
 	}
 	if (cpu_status != 0) {
 		*task_completed = 0;
-		return ret;
+		return 0;
 	}
 
 	ret = cxd2880_tnrdmd_mon_internal_cpu_status_sub(tnr_dmd, &cpu_status);
-- 
2.39.0

[PATCH 05/17] media: usb: dvb-usb-v2: af9015.c: return 0 instead of 'ret'.

[Hans Verkuil]

Since 'ret' is known to be 0, just return '0'. This fixes two smatch warnings:

af9015.c:1168 af9015_rc_query() warn: missing error code? 'ret'
af9015.c:1177 af9015_rc_query() warn: missing error code? 'ret'

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Antti Palosaari <crope@iki.fi>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
---
 drivers/media/usb/dvb-usb-v2/af9015.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/dvb-usb-v2/af9015.c b/drivers/media/usb/dvb-usb-v2/af9015.c
index d33514acc2b5..4014f7d07330 100644
--- a/drivers/media/usb/dvb-usb-v2/af9015.c
+++ b/drivers/media/usb/dvb-usb-v2/af9015.c
@@ -1165,7 +1165,7 @@ static int af9015_rc_query(struct dvb_usb_device *d)
 	/* If any of these are non-zero, assume invalid data */
 	if (buf[1] || buf[2] || buf[3]) {
 		dev_dbg(&intf->dev, "invalid data\n");
-		return ret;
+		return 0;
 	}
 
 	/* Check for repeat of previous code */
@@ -1174,7 +1174,7 @@ static int af9015_rc_query(struct dvb_usb_device *d)
 		dev_dbg(&intf->dev, "key repeated\n");
 		rc_repeat(d->rc_dev);
 		state->rc_repeat = buf[6];
-		return ret;
+		return 0;
 	}
 
 	/* Only process key if canary killed */
-- 
2.39.0

[PATCH 06/17] media: dvb-frontends: cxd2880: return 0 instead of 'ret'.

[Hans Verkuil]

Since 'ret' is known to be 0, just return '0'. This fixes 10 smatch warnings:

cxd2880_tnrdmd_dvbt.c:836 cxd2880_tnrdmd_dvbt_check_demod_lock() warn: missing error code? 'ret'
cxd2880_tnrdmd_dvbt.c:841 cxd2880_tnrdmd_dvbt_check_demod_lock() warn: missing error code? 'ret'
cxd2880_tnrdmd_dvbt.c:896 cxd2880_tnrdmd_dvbt_check_ts_lock() warn: missing error code? 'ret'
cxd2880_tnrdmd_dvbt.c:901 cxd2880_tnrdmd_dvbt_check_ts_lock() warn: missing error code? 'ret'
cxd2880_tnrdmd_dvbt.c:904 cxd2880_tnrdmd_dvbt_check_ts_lock() warn: missing error code? 'ret'
cxd2880_tnrdmd_dvbt2.c:1027 cxd2880_tnrdmd_dvbt2_check_demod_lock() warn: missing error code? 'ret'
cxd2880_tnrdmd_dvbt2.c:1032 cxd2880_tnrdmd_dvbt2_check_demod_lock() warn: missing error code? 'ret'
cxd2880_tnrdmd_dvbt2.c:1087 cxd2880_tnrdmd_dvbt2_check_ts_lock() warn: missing error code? 'ret'
cxd2880_tnrdmd_dvbt2.c:1092 cxd2880_tnrdmd_dvbt2_check_ts_lock() warn: missing error code? 'ret'
cxd2880_tnrdmd_dvbt2.c:1095 cxd2880_tnrdmd_dvbt2_check_ts_lock() warn: missing error code? 'ret'

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Yasunari Takiguchi <Yasunari.Takiguchi@sony.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
---
 .../dvb-frontends/cxd2880/cxd2880_tnrdmd_dvbt.c    | 14 +++++++-------
 .../dvb-frontends/cxd2880/cxd2880_tnrdmd_dvbt2.c   | 14 +++++++-------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_dvbt.c b/drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_dvbt.c
index fe3c6f8b1b3e..c7e79da8c432 100644
--- a/drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_dvbt.c
+++ b/drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_dvbt.c
@@ -833,12 +833,12 @@ int cxd2880_tnrdmd_dvbt_check_demod_lock(struct cxd2880_tnrdmd
 		else
 			*lock = CXD2880_TNRDMD_LOCK_RESULT_NOTDETECT;
 
-		return ret;
+		return 0;
 	}
 
 	if (sync_stat == 6) {
 		*lock = CXD2880_TNRDMD_LOCK_RESULT_LOCKED;
-		return ret;
+		return 0;
 	}
 
 	ret =
@@ -854,7 +854,7 @@ int cxd2880_tnrdmd_dvbt_check_demod_lock(struct cxd2880_tnrdmd
 	else
 		*lock = CXD2880_TNRDMD_LOCK_RESULT_NOTDETECT;
 
-	return ret;
+	return 0;
 }
 
 int cxd2880_tnrdmd_dvbt_check_ts_lock(struct cxd2880_tnrdmd
@@ -893,15 +893,15 @@ int cxd2880_tnrdmd_dvbt_check_ts_lock(struct cxd2880_tnrdmd
 		else
 			*lock = CXD2880_TNRDMD_LOCK_RESULT_NOTDETECT;
 
-		return ret;
+		return 0;
 	}
 
 	if (ts_lock) {
 		*lock = CXD2880_TNRDMD_LOCK_RESULT_LOCKED;
-		return ret;
+		return 0;
 	} else if (!unlock_detected) {
 		*lock = CXD2880_TNRDMD_LOCK_RESULT_NOTDETECT;
-		return ret;
+		return 0;
 	}
 
 	ret =
@@ -915,5 +915,5 @@ int cxd2880_tnrdmd_dvbt_check_ts_lock(struct cxd2880_tnrdmd
 	else
 		*lock = CXD2880_TNRDMD_LOCK_RESULT_NOTDETECT;
 
-	return ret;
+	return 0;
 }
diff --git a/drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_dvbt2.c b/drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_dvbt2.c
index dd32004a12d8..a9ab983348c8 100644
--- a/drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_dvbt2.c
+++ b/drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_dvbt2.c
@@ -1024,12 +1024,12 @@ int cxd2880_tnrdmd_dvbt2_check_demod_lock(struct cxd2880_tnrdmd
 		else
 			*lock = CXD2880_TNRDMD_LOCK_RESULT_NOTDETECT;
 
-		return ret;
+		return 0;
 	}
 
 	if (sync_stat == 6) {
 		*lock = CXD2880_TNRDMD_LOCK_RESULT_LOCKED;
-		return ret;
+		return 0;
 	}
 
 	ret =
@@ -1045,7 +1045,7 @@ int cxd2880_tnrdmd_dvbt2_check_demod_lock(struct cxd2880_tnrdmd
 	else
 		*lock = CXD2880_TNRDMD_LOCK_RESULT_NOTDETECT;
 
-	return ret;
+	return 0;
 }
 
 int cxd2880_tnrdmd_dvbt2_check_ts_lock(struct cxd2880_tnrdmd
@@ -1084,15 +1084,15 @@ int cxd2880_tnrdmd_dvbt2_check_ts_lock(struct cxd2880_tnrdmd
 		else
 			*lock = CXD2880_TNRDMD_LOCK_RESULT_NOTDETECT;
 
-		return ret;
+		return 0;
 	}
 
 	if (ts_lock) {
 		*lock = CXD2880_TNRDMD_LOCK_RESULT_LOCKED;
-		return ret;
+		return 0;
 	} else if (!unlock_detected) {
 		*lock = CXD2880_TNRDMD_LOCK_RESULT_NOTDETECT;
-		return ret;
+		return 0;
 	}
 
 	ret =
@@ -1106,7 +1106,7 @@ int cxd2880_tnrdmd_dvbt2_check_ts_lock(struct cxd2880_tnrdmd
 	else
 		*lock = CXD2880_TNRDMD_LOCK_RESULT_NOTDETECT;
 
-	return ret;
+	return 0;
 }
 
 int cxd2880_tnrdmd_dvbt2_set_plp_cfg(struct cxd2880_tnrdmd
-- 
2.39.0

[PATCH 07/17] media: marvell: change return to goto for proper unwind

[Hans Verkuil]

The probe function used 'goto out' everywhere, except in one
place where it returned an error. That too should be a 'goto out'.

This fixes a smatch warning:

mmp-driver.c:257 mmpcam_probe() warn: missing unwind goto?

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

diff --git a/drivers/media/platform/marvell/mmp-driver.c b/drivers/media/platform/marvell/mmp-driver.c
index df16899ab1cb..ef22bf8f276c 100644
--- a/drivers/media/platform/marvell/mmp-driver.c
+++ b/drivers/media/platform/marvell/mmp-driver.c
@@ -254,7 +254,7 @@ static int mmpcam_probe(struct platform_device *pdev)
 	 */
 	ret = mccic_register(mcam);
 	if (ret)
-		return ret;
+		goto out;
 
 	/*
 	 * Add OF clock provider.
-- 
2.39.0

[PATCH 08/17] media: dvb-frontends: drx39xyj: replace return with goto for proper unwind

[Hans Verkuil]

In three places there was a return instead of a goto to the unwind
code.

This fixes three smatch warnings:

drxj.c:9542 ctrl_get_qam_sig_quality() warn: missing unwind goto?
drxj.c:10919 ctrl_set_standard() warn: missing unwind goto?
drxj.c:11466 drxj_open() warn: missing unwind goto?

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
---
 drivers/media/dvb-frontends/drx39xyj/drxj.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/media/dvb-frontends/drx39xyj/drxj.c b/drivers/media/dvb-frontends/drx39xyj/drxj.c
index 1dff59ca21a1..6bf6559b127f 100644
--- a/drivers/media/dvb-frontends/drx39xyj/drxj.c
+++ b/drivers/media/dvb-frontends/drx39xyj/drxj.c
@@ -9539,7 +9539,8 @@ ctrl_get_qam_sig_quality(struct drx_demod_instance *demod)
 		qam_sl_sig_power = DRXJ_QAM_SL_SIG_POWER_QAM256 << 2;
 		break;
 	default:
-		return -EIO;
+		rc = -EIO;
+		goto rw_error;
 	}
 
 	/* ------------------------------ */
@@ -10916,7 +10917,8 @@ ctrl_set_standard(struct drx_demod_instance *demod, enum drx_standard *standard)
 		break;
 	case DRX_STANDARD_AUTO:
 	default:
-		return -EINVAL;
+		rc = -EINVAL;
+		goto rw_error;
 	}
 
 	/*
@@ -11463,7 +11465,8 @@ static int drxj_open(struct drx_demod_instance *demod)
 
 		if (DRX_ISPOWERDOWNMODE(demod->my_common_attr->current_power_mode)) {
 			pr_err("Should powerup before loading the firmware.");
-			return -EINVAL;
+			rc = -EINVAL;
+			goto rw_error;
 		}
 
 		rc = drx_ctrl_u_code(demod, &ucode_info, UCODE_UPLOAD);
-- 
2.39.0

[PATCH 09/17] media: nxp: imx-jpeg: replace dummy gotos by returns

[Hans Verkuil]

The err_irq and err_clk labels just did a 'return ret'. So
drop these and replace them by just a return.

This fixes a smatch warning:

mxc-jpeg.c:2492 mxc_jpeg_probe() warn: missing unwind goto?

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Mirela Rabulea <mirela.rabulea@nxp.com>
---
 .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
index 6cd015a35f7c..552d0900cb26 100644
--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
@@ -2445,7 +2445,7 @@ static int mxc_jpeg_probe(struct platform_device *pdev)
 	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
 	if (ret) {
 		dev_err(&pdev->dev, "No suitable DMA available.\n");
-		goto err_irq;
+		return ret;
 	}
 
 	jpeg->base_reg = devm_platform_ioremap_resource(pdev, 0);
@@ -2454,16 +2454,14 @@ static int mxc_jpeg_probe(struct platform_device *pdev)
 
 	for (slot = 0; slot < MXC_MAX_SLOTS; slot++) {
 		dec_irq = platform_get_irq(pdev, slot);
-		if (dec_irq < 0) {
-			ret = dec_irq;
-			goto err_irq;
-		}
+		if (dec_irq < 0)
+			return dec_irq;
 		ret = devm_request_irq(&pdev->dev, dec_irq, mxc_jpeg_dec_irq,
 				       0, pdev->name, jpeg);
 		if (ret) {
 			dev_err(&pdev->dev, "Failed to request irq %d (%d)\n",
 				dec_irq, ret);
-			goto err_irq;
+			return ret;
 		}
 	}
 
@@ -2475,15 +2473,13 @@ static int mxc_jpeg_probe(struct platform_device *pdev)
 	jpeg->clk_ipg = devm_clk_get(dev, "ipg");
 	if (IS_ERR(jpeg->clk_ipg)) {
 		dev_err(dev, "failed to get clock: ipg\n");
-		ret = PTR_ERR(jpeg->clk_ipg);
-		goto err_clk;
+		return PTR_ERR(jpeg->clk_ipg);
 	}
 
 	jpeg->clk_per = devm_clk_get(dev, "per");
 	if (IS_ERR(jpeg->clk_per)) {
 		dev_err(dev, "failed to get clock: per\n");
-		ret = PTR_ERR(jpeg->clk_per);
-		goto err_clk;
+		return PTR_ERR(jpeg->clk_per);
 	}
 
 	ret = mxc_jpeg_attach_pm_domains(jpeg);
@@ -2569,9 +2565,6 @@ static int mxc_jpeg_probe(struct platform_device *pdev)
 
 err_register:
 	mxc_jpeg_detach_pm_domains(jpeg);
-
-err_irq:
-err_clk:
 	return ret;
 }
 
-- 
2.39.0

Re: [EXT] [PATCH 09/17] media: nxp: imx-jpeg: replace dummy gotos by returns

[Mirela Rabulea]

On 26.01.2023 17:06, Hans Verkuil wrote:
> 
> The err_irq and err_clk labels just did a 'return ret'. So
> drop these and replace them by just a return.
> 
> This fixes a smatch warning:
> 
> mxc-jpeg.c:2492 mxc_jpeg_probe() warn: missing unwind goto?
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Reviewed-by: Mirela Rabulea <mirela.rabulea@gmail.com>

> Cc: Mirela Rabulea <mirela.rabulea@nxp.com>
> ---
>   .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 19 ++++++-------------
>   1 file changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> index 6cd015a35f7c..552d0900cb26 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> @@ -2445,7 +2445,7 @@ static int mxc_jpeg_probe(struct platform_device *pdev)
>          ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>          if (ret) {
>                  dev_err(&pdev->dev, "No suitable DMA available.\n");
> -               goto err_irq;
> +               return ret;
>          }
> 
>          jpeg->base_reg = devm_platform_ioremap_resource(pdev, 0);
> @@ -2454,16 +2454,14 @@ static int mxc_jpeg_probe(struct platform_device *pdev)
> 
>          for (slot = 0; slot < MXC_MAX_SLOTS; slot++) {
>                  dec_irq = platform_get_irq(pdev, slot);
> -               if (dec_irq < 0) {
> -                       ret = dec_irq;
> -                       goto err_irq;
> -               }
> +               if (dec_irq < 0)
> +                       return dec_irq;
>                  ret = devm_request_irq(&pdev->dev, dec_irq, mxc_jpeg_dec_irq,
>                                         0, pdev->name, jpeg);
>                  if (ret) {
>                          dev_err(&pdev->dev, "Failed to request irq %d (%d)\n",
>                                  dec_irq, ret);
> -                       goto err_irq;
> +                       return ret;
>                  }
>          }
> 
> @@ -2475,15 +2473,13 @@ static int mxc_jpeg_probe(struct platform_device *pdev)
>          jpeg->clk_ipg = devm_clk_get(dev, "ipg");
>          if (IS_ERR(jpeg->clk_ipg)) {
>                  dev_err(dev, "failed to get clock: ipg\n");
> -               ret = PTR_ERR(jpeg->clk_ipg);
> -               goto err_clk;
> +               return PTR_ERR(jpeg->clk_ipg);
>          }
> 
>          jpeg->clk_per = devm_clk_get(dev, "per");
>          if (IS_ERR(jpeg->clk_per)) {
>                  dev_err(dev, "failed to get clock: per\n");
> -               ret = PTR_ERR(jpeg->clk_per);
> -               goto err_clk;
> +               return PTR_ERR(jpeg->clk_per);
>          }
> 
>          ret = mxc_jpeg_attach_pm_domains(jpeg);
> @@ -2569,9 +2565,6 @@ static int mxc_jpeg_probe(struct platform_device *pdev)
> 
>   err_register:
>          mxc_jpeg_detach_pm_domains(jpeg);
> -
> -err_irq:
> -err_clk:
>          return ret;
>   }
> 
> --
> 2.39.0
> 

[PATCH 10/17] media: mediatek: mdp3: replace return by goto for proper unwind

[Hans Verkuil]

An error was returned at one point without going through the
goto label for proper unwinding.

This fixes a smatch warning:

mtk-mdp3-comp.c:1005 mdp_comp_config() warn: missing unwind goto?

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Moudy Ho <moudy.ho@mediatek.com>
---
 drivers/media/platform/mediatek/mdp3/mtk-mdp3-comp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-comp.c b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-comp.c
index 7bc05f42a23c..091a68685590 100644
--- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-comp.c
+++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-comp.c
@@ -1002,7 +1002,8 @@ int mdp_comp_config(struct mdp_dev *mdp)
 		if (!pdev) {
 			dev_warn(dev, "can't find platform device of node:%s\n",
 				 node->name);
-			return -ENODEV;
+			ret = -ENODEV;
+			goto err_init_comps;
 		}
 
 		comp->comp_dev = &pdev->dev;
-- 
2.39.0

Re: [PATCH 10/17] media: mediatek: mdp3: replace return by goto for proper unwind

[Mauro Carvalho Chehab]

Em Thu, 26 Jan 2023 16:06:50 +0100
Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:

> An error was returned at one point without going through the
> goto label for proper unwinding.
> 
> This fixes a smatch warning:
> 
> mtk-mdp3-comp.c:1005 mdp_comp_config() warn: missing unwind goto?

This patch doesn't apply, as the logic it is meant to touch
changed a lot. I'm simply discarding it from your PR.

Please work on a new version if still needed.

Regards,
Mauro

Thanks,
Mauro

[PATCH 11/17] media: mediatek: vcodec/venc: return 0 instead of 'ret'.

[Hans Verkuil]

Since 'ret' is known to be 0, just return '0'. This fixes a smatch warning:

venc_h264_if.c:568 h264_encode_frame() warn: missing error code? 'ret'

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Andrew-CT Chen <andrew-ct.chen@mediatek.com>
Cc: Yunfei Dong <yunfei.dong@mediatek.com>
---
 drivers/media/platform/mediatek/vcodec/venc/venc_h264_if.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/venc/venc_h264_if.c b/drivers/media/platform/mediatek/vcodec/venc/venc_h264_if.c
index 13c4f860fa69..60fd165c0d94 100644
--- a/drivers/media/platform/mediatek/vcodec/venc/venc_h264_if.c
+++ b/drivers/media/platform/mediatek/vcodec/venc/venc_h264_if.c
@@ -565,7 +565,7 @@ static int h264_encode_frame(struct venc_h264_inst *inst,
 		       *bs_size);
 		++inst->frm_cnt;
 		++inst->skip_frm_cnt;
-		return ret;
+		return 0;
 	}
 
 	irq_status = h264_enc_wait_venc_done(inst);
@@ -580,7 +580,7 @@ static int h264_encode_frame(struct venc_h264_inst *inst,
 	mtk_vcodec_debug(inst, "frm %d bs_size %d key_frm %d <-",
 			 inst->frm_cnt, *bs_size, inst->vpu_inst.is_key_frm);
 
-	return ret;
+	return 0;
 }
 
 static void h264_encode_filler(struct venc_h264_inst *inst, void *buf,
-- 
2.39.0

[PATCH 12/17] media: ti: davinci: vpbe_display.c: return 0 instead of 'ret'.

[Hans Verkuil]

Since 'ret' is known to be 0, just return '0'. This fixes a smatch warning:

vpbe_display.c:1152 vpbe_display_open() warn: missing error code? 'err'

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
---
 drivers/media/platform/ti/davinci/vpbe_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/ti/davinci/vpbe_display.c b/drivers/media/platform/ti/davinci/vpbe_display.c
index 9ea70817538e..ea2d0795d1e2 100644
--- a/drivers/media/platform/ti/davinci/vpbe_display.c
+++ b/drivers/media/platform/ti/davinci/vpbe_display.c
@@ -1149,7 +1149,7 @@ static int vpbe_display_open(struct file *file)
 
 	/* leaving if layer is already initialized */
 	if (!v4l2_fh_is_singular_file(file))
-		return err;
+		return 0;
 
 	if (!layer->usrs) {
 		if (mutex_lock_interruptible(&layer->opslock))
-- 
2.39.0

Re: [PATCH 12/17] media: ti: davinci: vpbe_display.c: return 0 instead of 'ret'.

[Lad, Prabhakar]

Hi Hans,

Thank you for the patch.

On Thu, Jan 26, 2023 at 3:07 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> Since 'ret' is known to be 0, just return '0'. This fixes a smatch warning:
>
> vpbe_display.c:1152 vpbe_display_open() warn: missing error code? 'err'
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Cc: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
> ---
>  drivers/media/platform/ti/davinci/vpbe_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
Reviewed-by: Lad Prabhakar <prabhakar.csengg@gmail.com>

Cheers,
Prabhakar

> diff --git a/drivers/media/platform/ti/davinci/vpbe_display.c b/drivers/media/platform/ti/davinci/vpbe_display.c
> index 9ea70817538e..ea2d0795d1e2 100644
> --- a/drivers/media/platform/ti/davinci/vpbe_display.c
> +++ b/drivers/media/platform/ti/davinci/vpbe_display.c
> @@ -1149,7 +1149,7 @@ static int vpbe_display_open(struct file *file)
>
>         /* leaving if layer is already initialized */
>         if (!v4l2_fh_is_singular_file(file))
> -               return err;
> +               return 0;
>
>         if (!layer->usrs) {
>                 if (mutex_lock_interruptible(&layer->opslock))
> --
> 2.39.0
>

[PATCH 13/17] media: i2c: ov7670: 0 instead of -EINVAL was returned

[Hans Verkuil]

If the media bus is unsupported, then return -EINVAL. Instead it
returned 'ret' which happened to be 0.

This fixes a smatch warning:

ov7670.c:1843 ov7670_parse_dt() warn: missing error code? 'ret'

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/ov7670.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
index 27db0a07de1f..b1bb0833571e 100644
--- a/drivers/media/i2c/ov7670.c
+++ b/drivers/media/i2c/ov7670.c
@@ -1840,7 +1840,7 @@ static int ov7670_parse_dt(struct device *dev,
 
 	if (bus_cfg.bus_type != V4L2_MBUS_PARALLEL) {
 		dev_err(dev, "Unsupported media bus type\n");
-		return ret;
+		return -EINVAL;
 	}
 	info->mbus_config = bus_cfg.bus.parallel.flags;
 
-- 
2.39.0

Re: [PATCH 13/17] media: i2c: ov7670: 0 instead of -EINVAL was returned

[Sakari Ailus]

Hi Hans,

On Thu, Jan 26, 2023 at 04:06:53PM +0100, Hans Verkuil wrote:
> If the media bus is unsupported, then return -EINVAL. Instead it
> returned 'ret' which happened to be 0.
> 
> This fixes a smatch warning:
> 
> ov7670.c:1843 ov7670_parse_dt() warn: missing error code? 'ret'
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>

Thanks for the fix.

Could you add:

Fixes: 01b8444828fc ("media: v4l2: i2c: ov7670: Implement OF mbus configuration")
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus

[PATCH 14/17] media: usb: go7007: add second unwind label

[Hans Verkuil]

Most of the time the same unwind code was used for errors,
except in one case where it just returned. But smatch didn't like
that, so make a new goto label for that situation.

This fixes this smatch warning:

go7007-usb.c:702 go7007_usb_ezusb_write_interrupt() warn: missing unwind goto?

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

diff --git a/drivers/media/usb/go7007/go7007-usb.c b/drivers/media/usb/go7007/go7007-usb.c
index eeb85981e02b..d8524d761c32 100644
--- a/drivers/media/usb/go7007/go7007-usb.c
+++ b/drivers/media/usb/go7007/go7007-usb.c
@@ -697,10 +697,9 @@ static int go7007_usb_ezusb_write_interrupt(struct go7007 *go,
 	}
 	if (r < 0)
 		goto write_int_error;
-	if (i == 100) {
-		dev_err(go->dev, "device is hung, status reg = 0x%04x\n", status_reg);
-		return -1;
-	}
+	if (i == 100)
+		goto dev_is_hung;
+
 	r = usb_control_msg(usb->usbdev, usb_sndctrlpipe(usb->usbdev, 0), 0x12,
 			USB_TYPE_VENDOR | USB_RECIP_DEVICE, data,
 			INT_PARAM_ADDR, NULL, 0, timeout);
@@ -716,6 +715,10 @@ static int go7007_usb_ezusb_write_interrupt(struct go7007 *go,
 write_int_error:
 	dev_err(go->dev, "error in WriteInterrupt: %d\n", r);
 	return r;
+
+dev_is_hung:
+	dev_err(go->dev, "device is hung, status reg = 0x%04x\n", status_reg);
+	return -1;
 }
 
 static int go7007_usb_onboard_write_interrupt(struct go7007 *go,
-- 
2.39.0

[PATCH 15/17] media: i2c: adp1653: introduce 'no_child' label

[Hans Verkuil]

The code mixed gotos and returns, which confused smatch. Add a no_child
label before the 'return -EINVAL;' to help smatch understand this.
It's a bit more consistent as well.

This fixes this smatch warning:

adp1653.c:444 adp1653_of_init() warn: missing unwind goto?

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/adp1653.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index a61a77de6eee..136bca801db7 100644
--- a/drivers/media/i2c/adp1653.c
+++ b/drivers/media/i2c/adp1653.c
@@ -420,7 +420,7 @@ static int adp1653_of_init(struct i2c_client *client,
 
 	child = of_get_child_by_name(node, "flash");
 	if (!child)
-		return -EINVAL;
+		goto no_child;
 
 	if (of_property_read_u32(child, "flash-timeout-us",
 				 &pd->max_flash_timeout))
@@ -441,7 +441,7 @@ static int adp1653_of_init(struct i2c_client *client,
 
 	child = of_get_child_by_name(node, "indicator");
 	if (!child)
-		return -EINVAL;
+		goto no_child;
 
 	if (of_property_read_u32(child, "led-max-microamp",
 				 &pd->max_indicator_intensity))
@@ -459,6 +459,7 @@ static int adp1653_of_init(struct i2c_client *client,
 err:
 	dev_err(&client->dev, "Required property not found\n");
 	of_node_put(child);
+no_child:
 	return -EINVAL;
 }
 
-- 
2.39.0

Re: [PATCH 15/17] media: i2c: adp1653: introduce 'no_child' label

[Sakari Ailus]

Hi Hans,

On Thu, Jan 26, 2023 at 04:06:55PM +0100, Hans Verkuil wrote:
> The code mixed gotos and returns, which confused smatch. Add a no_child
> label before the 'return -EINVAL;' to help smatch understand this.
> It's a bit more consistent as well.
> 
> This fixes this smatch warning:
> 
> adp1653.c:444 adp1653_of_init() warn: missing unwind goto?

This is clearly a false positive. There's also no reason to just have a
label where you simply return a value.

Would it be possible to just fix smatch instead?

> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/i2c/adp1653.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
> index a61a77de6eee..136bca801db7 100644
> --- a/drivers/media/i2c/adp1653.c
> +++ b/drivers/media/i2c/adp1653.c
> @@ -420,7 +420,7 @@ static int adp1653_of_init(struct i2c_client *client,
>  
>  	child = of_get_child_by_name(node, "flash");
>  	if (!child)
> -		return -EINVAL;
> +		goto no_child;
>  
>  	if (of_property_read_u32(child, "flash-timeout-us",
>  				 &pd->max_flash_timeout))
> @@ -441,7 +441,7 @@ static int adp1653_of_init(struct i2c_client *client,
>  
>  	child = of_get_child_by_name(node, "indicator");
>  	if (!child)
> -		return -EINVAL;
> +		goto no_child;
>  
>  	if (of_property_read_u32(child, "led-max-microamp",
>  				 &pd->max_indicator_intensity))
> @@ -459,6 +459,7 @@ static int adp1653_of_init(struct i2c_client *client,
>  err:
>  	dev_err(&client->dev, "Required property not found\n");
>  	of_node_put(child);
> +no_child:
>  	return -EINVAL;
>  }
>  

-- 
Kind regards,

Sakari Ailus

Can smatch handle this better? (was: [PATCH 15/17] media: i2c: adp1653: introduce 'no_child' label)

[Hans Verkuil]

Hi Dan,

While trying to clean up smatch warnings in the media subsystem I came
across a number of 'warn: missing unwind goto?' warnings that all had
the same root cause as illustrated by this patch: the 'unwind' path
just has a variation on printk(), it is not actually cleaning up anything.

As Sakari suggested, is this something that smatch can be improved for?
These false positives are a bit annoying.

You can see the whole series here if you are interested:

https://patchwork.linuxtv.org/project/linux-media/list/?series=9747

Regards,

	Hans

On 26/01/2023 16:19, Sakari Ailus wrote:
> Hi Hans,
> 
> On Thu, Jan 26, 2023 at 04:06:55PM +0100, Hans Verkuil wrote:
>> The code mixed gotos and returns, which confused smatch. Add a no_child
>> label before the 'return -EINVAL;' to help smatch understand this.
>> It's a bit more consistent as well.
>>
>> This fixes this smatch warning:
>>
>> adp1653.c:444 adp1653_of_init() warn: missing unwind goto?
> 
> This is clearly a false positive. There's also no reason to just have a
> label where you simply return a value.
> 
> Would it be possible to just fix smatch instead?
> 
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>>  drivers/media/i2c/adp1653.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
>> index a61a77de6eee..136bca801db7 100644
>> --- a/drivers/media/i2c/adp1653.c
>> +++ b/drivers/media/i2c/adp1653.c
>> @@ -420,7 +420,7 @@ static int adp1653_of_init(struct i2c_client *client,
>>  
>>  	child = of_get_child_by_name(node, "flash");
>>  	if (!child)
>> -		return -EINVAL;
>> +		goto no_child;
>>  
>>  	if (of_property_read_u32(child, "flash-timeout-us",
>>  				 &pd->max_flash_timeout))
>> @@ -441,7 +441,7 @@ static int adp1653_of_init(struct i2c_client *client,
>>  
>>  	child = of_get_child_by_name(node, "indicator");
>>  	if (!child)
>> -		return -EINVAL;
>> +		goto no_child;
>>  
>>  	if (of_property_read_u32(child, "led-max-microamp",
>>  				 &pd->max_indicator_intensity))
>> @@ -459,6 +459,7 @@ static int adp1653_of_init(struct i2c_client *client,
>>  err:
>>  	dev_err(&client->dev, "Required property not found\n");
>>  	of_node_put(child);
>> +no_child:
>>  	return -EINVAL;
>>  }
>>  
> 

Re: Can smatch handle this better? (was: [PATCH 15/17] media: i2c: adp1653: introduce 'no_child' label)

[Dan Carpenter]

On Fri, Jan 27, 2023 at 08:43:51AM +0100, Hans Verkuil wrote:
> Hi Dan,
> 
> While trying to clean up smatch warnings in the media subsystem I came
> across a number of 'warn: missing unwind goto?' warnings that all had
> the same root cause as illustrated by this patch: the 'unwind' path
> just has a variation on printk(), it is not actually cleaning up anything.
> 
> As Sakari suggested, is this something that smatch can be improved for?
> These false positives are a bit annoying.
> 
> You can see the whole series here if you are interested:
> 
> https://patchwork.linuxtv.org/project/linux-media/list/?series=9747
> 
> Regards,
> 

Oh wow.  I really hate do nothing gotos.  The canonical bug for do
nothing gotos is forgetting to set the error code.

I like that check because it finds a lot of error handling bugs.

It's not just about the printk().  I could and probably should make the
check ignore printks.  There is also an of_node_put(child);

The check doesn't look at what the error handling does, only that there
is a direct returns surrounded by gotos.

I could make the check so that it's only enabled when --spammy is turned
on.

I guess another option would be to only enable the warning if there were
more than one label in the cleanup section at the end of the function.
I can make that a warning and if there is only one label, then make that
disable unless --spammy is used.

regards,
dan carpenter

Re: Can smatch handle this better? (was: [PATCH 15/17] media: i2c: adp1653: introduce 'no_child' label)

[Hans Verkuil]

On 27/01/2023 13:41, Dan Carpenter wrote:
> On Fri, Jan 27, 2023 at 08:43:51AM +0100, Hans Verkuil wrote:
>> Hi Dan,
>>
>> While trying to clean up smatch warnings in the media subsystem I came
>> across a number of 'warn: missing unwind goto?' warnings that all had
>> the same root cause as illustrated by this patch: the 'unwind' path
>> just has a variation on printk(), it is not actually cleaning up anything.
>>
>> As Sakari suggested, is this something that smatch can be improved for?
>> These false positives are a bit annoying.
>>
>> You can see the whole series here if you are interested:
>>
>> https://patchwork.linuxtv.org/project/linux-media/list/?series=9747
>>
>> Regards,
>>
> 
> Oh wow.  I really hate do nothing gotos.  The canonical bug for do
> nothing gotos is forgetting to set the error code.
> 
> I like that check because it finds a lot of error handling bugs.

I do too, I did find some real bugs with this as well.

> 
> It's not just about the printk().  I could and probably should make the
> check ignore printks.  There is also an of_node_put(child);

This are other examples in my patch series where there is only an
error message:

https://patchwork.linuxtv.org/project/linux-media/patch/20230126150657.367921-15-hverkuil-cisco@xs4all.nl/
https://patchwork.linuxtv.org/project/linux-media/patch/20230126150657.367921-17-hverkuil-cisco@xs4all.nl/

> 
> The check doesn't look at what the error handling does, only that there
> is a direct returns surrounded by gotos.
> 
> I could make the check so that it's only enabled when --spammy is turned
> on.
> 
> I guess another option would be to only enable the warning if there were
> more than one label in the cleanup section at the end of the function.
> I can make that a warning and if there is only one label, then make that
> disable unless --spammy is used.

That would cause us to miss a real bug like this:

https://patchwork.linuxtv.org/project/linux-media/patch/20230126150657.367921-8-hverkuil-cisco@xs4all.nl/

I think that if you were able to check that all the code after a goto label
consisted of variations on printk, then that would solve most of the
false positives I've seen in the media subsystem.

The few remaining ones we can work around ourselves.

Regards,

	Hans

> 
> regards,
> dan carpenter
> 

Re: Can smatch handle this better? (was: [PATCH 15/17] media: i2c: adp1653: introduce 'no_child' label)

[Dan Carpenter]

On Fri, Jan 27, 2023 at 02:00:51PM +0100, Hans Verkuil wrote:
> I think that if you were able to check that all the code after a goto label
> consisted of variations on printk, then that would solve most of the
> false positives I've seen in the media subsystem.

Ok.  That is doable.

regards,
dan carpenter

[PATCH 16/17] media: st: delta: introduce 'err_too_many_comps' label

[Hans Verkuil]

The code mixed gotos and returns, which confused smatch. Add a
err_too_many_comps label to handle the error instead of a return,
this helps smatch understand the code, and it's a bit more consistent
as well.

This fixes this smatch warning:

delta-mjpeg-hdr.c:67 delta_mjpeg_read_sof() warn: missing unwind goto?

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Hugues Fruchet <hugues.fruchet@foss.st.com>
---
 .../platform/st/sti/delta/delta-mjpeg-hdr.c      | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/st/sti/delta/delta-mjpeg-hdr.c b/drivers/media/platform/st/sti/delta/delta-mjpeg-hdr.c
index 90e5b2f72c82..c132487637d3 100644
--- a/drivers/media/platform/st/sti/delta/delta-mjpeg-hdr.c
+++ b/drivers/media/platform/st/sti/delta/delta-mjpeg-hdr.c
@@ -59,13 +59,8 @@ static int delta_mjpeg_read_sof(struct delta_ctx *pctx,
 	header->nb_of_components = *(u8 *)(data + offset);
 	offset += sizeof(u8);
 
-	if (header->nb_of_components >= MJPEG_MAX_COMPONENTS) {
-		dev_err(delta->dev,
-			"%s   unsupported number of components (%d > %d)\n",
-			pctx->name, header->nb_of_components,
-			MJPEG_MAX_COMPONENTS);
-		return -EINVAL;
-	}
+	if (header->nb_of_components >= MJPEG_MAX_COMPONENTS)
+		goto err_too_many_comps;
 
 	if ((offset + header->nb_of_components *
 	     sizeof(header->components[0])) > size)
@@ -78,6 +73,13 @@ static int delta_mjpeg_read_sof(struct delta_ctx *pctx,
 		"%s   sof: reached end of %d size input stream\n",
 		pctx->name, size);
 	return -ENODATA;
+
+err_too_many_comps:
+	dev_err(delta->dev,
+		"%s   unsupported number of components (%d > %d)\n",
+		pctx->name, header->nb_of_components,
+		MJPEG_MAX_COMPONENTS);
+	return -EINVAL;
 }
 
 int delta_mjpeg_read_header(struct delta_ctx *pctx,
-- 
2.39.0

[PATCH 17/17] media: dvb-frontends: mb86a16.c: always use the same error path

[Hans Verkuil]

If the message length was wrong, the dprintk() after the 'err' label
was bypassed. Fix that, and fix a smatch warning at the same time:

mb86a16.c:1514 mb86a16_send_diseqc_msg() warn: missing unwind goto?

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
---
 drivers/media/dvb-frontends/mb86a16.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/media/dvb-frontends/mb86a16.c b/drivers/media/dvb-frontends/mb86a16.c
index 2505f1e5794e..d3e29937cf4c 100644
--- a/drivers/media/dvb-frontends/mb86a16.c
+++ b/drivers/media/dvb-frontends/mb86a16.c
@@ -1498,6 +1498,7 @@ static int mb86a16_send_diseqc_msg(struct dvb_frontend *fe,
 				   struct dvb_diseqc_master_cmd *cmd)
 {
 	struct mb86a16_state *state = fe->demodulator_priv;
+	int ret = -EREMOTEIO;
 	int i;
 	u8 regs;
 
@@ -1510,8 +1511,10 @@ static int mb86a16_send_diseqc_msg(struct dvb_frontend *fe,
 
 	regs = 0x18;
 
-	if (cmd->msg_len > 5 || cmd->msg_len < 4)
-		return -EINVAL;
+	if (cmd->msg_len > 5 || cmd->msg_len < 4) {
+		ret = -EINVAL;
+		goto err;
+	}
 
 	for (i = 0; i < cmd->msg_len; i++) {
 		if (mb86a16_write(state, regs, cmd->msg[i]) < 0)
@@ -1532,7 +1535,7 @@ static int mb86a16_send_diseqc_msg(struct dvb_frontend *fe,
 
 err:
 	dprintk(verbose, MB86A16_ERROR, 1, "I2C transfer error");
-	return -EREMOTEIO;
+	return ret;
 }
 
 static int mb86a16_send_diseqc_burst(struct dvb_frontend *fe,
-- 
2.39.0