[PATCH 0/8] Various smatch/sparse fixes

[PATCH 0/8] Various smatch/sparse fixes

[Hans Verkuil]

Various fixes to reduce the number of smatch/sparse warnings.

Hans Verkuil (8):
  staging: media: atomisp: initialize settings to 0
  media: rockchip: rga: fix clock cleanup
  media: usb: as102: drop as102_dev NULL check
  media: platform: renesas-ceu: drop buf NULL check
  media: platform: mediatek: vpu: fix NULL ptr dereference
  media: mediatek: vpu: add missing clk_unprepare
  media: pci: tw686x: no need to check 'next'
  staging: media: atomisp: move up sanity checks

 drivers/media/pci/tw686x/tw686x-audio.c           |  2 +-
 drivers/media/platform/mediatek/vpu/mtk_vpu.c     |  7 +++++--
 drivers/media/platform/renesas/renesas-ceu.c      |  9 ---------
 drivers/media/platform/rockchip/rga/rga.c         |  4 ++--
 drivers/media/usb/as102/as102_usb_drv.c           |  6 ++----
 .../media/atomisp/pci/atomisp_gmin_platform.c     |  2 +-
 .../staging/media/atomisp/pci/sh_css_firmware.c   | 15 ++++++++-------
 7 files changed, 19 insertions(+), 26 deletions(-)

-- 
2.39.2

[PATCH 1/8] staging: media: atomisp: initialize settings to 0

[Hans Verkuil]

Fix a compiler warning:

drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c:1525:13: warning: 'settings' may be used uninitialized [-Wmaybe-uninitialized]

The 'settings' variable is actually always initialized, but the
compiler isn't quite able to figure that out. Just initialize it
to 0 to avoid this warning.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index c718a74ea70a..48ba0d0bcfe5 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -1522,7 +1522,7 @@ static int v4l2_acpi_handle_gpio_res(struct acpi_resource *ares, void *_data)
 	const char *name;
 	bool active_low;
 	unsigned int i;
-	u32 settings;
+	u32 settings = 0;
 	u8 pin;
 
 	if (!acpi_gpio_get_io_resource(ares, &agpio))
-- 
2.39.2

Re: [PATCH 1/8] staging: media: atomisp: initialize settings to 0

[Hans de Goede]

Hi Hans,

On 5/24/23 14:11, Hans Verkuil wrote:
> Fix a compiler warning:
> 
> drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c:1525:13: warning: 'settings' may be used uninitialized [-Wmaybe-uninitialized]
> 
> The 'settings' variable is actually always initialized, but the
> compiler isn't quite able to figure that out. Just initialize it
> to 0 to avoid this warning.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Cc: Hans de Goede <hdegoede@redhat.com>

Thank you for these patches. I have merged this one and 8/8
into:

https://git.kernel.org/pub/scm/linux/kernel/git/hansg/linux.git/log/?h=media-atomisp

I'm currently doing a lot of work on the atomisp code,
so merging them through my atomisp branch is best to
avoid conflicts (I already had to resolve a conflict).

So please refrain from merging 1/8 + 8/8 to some
other linux-media tree/branch.

Regards,

Hans





> ---
>  drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> index c718a74ea70a..48ba0d0bcfe5 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> @@ -1522,7 +1522,7 @@ static int v4l2_acpi_handle_gpio_res(struct acpi_resource *ares, void *_data)
>  	const char *name;
>  	bool active_low;
>  	unsigned int i;
> -	u32 settings;
> +	u32 settings = 0;
>  	u8 pin;
>  
>  	if (!acpi_gpio_get_io_resource(ares, &agpio))

Re: [PATCH 1/8] staging: media: atomisp: initialize settings to 0

[Hans Verkuil]

On 24/05/2023 18:04, Hans de Goede wrote:
> Hi Hans,
> 
> On 5/24/23 14:11, Hans Verkuil wrote:
>> Fix a compiler warning:
>>
>> drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c:1525:13: warning: 'settings' may be used uninitialized [-Wmaybe-uninitialized]
>>
>> The 'settings' variable is actually always initialized, but the
>> compiler isn't quite able to figure that out. Just initialize it
>> to 0 to avoid this warning.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> Cc: Hans de Goede <hdegoede@redhat.com>
> 
> Thank you for these patches. I have merged this one and 8/8
> into:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/hansg/linux.git/log/?h=media-atomisp
> 
> I'm currently doing a lot of work on the atomisp code,
> so merging them through my atomisp branch is best to
> avoid conflicts (I already had to resolve a conflict).
> 
> So please refrain from merging 1/8 + 8/8 to some
> other linux-media tree/branch.

I've delegated patches 1 and 8 to you in patchwork.

Regards,

	Hans

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
>> ---
>>  drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
>> index c718a74ea70a..48ba0d0bcfe5 100644
>> --- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
>> +++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
>> @@ -1522,7 +1522,7 @@ static int v4l2_acpi_handle_gpio_res(struct acpi_resource *ares, void *_data)
>>  	const char *name;
>>  	bool active_low;
>>  	unsigned int i;
>> -	u32 settings;
>> +	u32 settings = 0;
>>  	u8 pin;
>>  
>>  	if (!acpi_gpio_get_io_resource(ares, &agpio))
> 

[PATCH 2/8] media: rockchip: rga: fix clock cleanup

[Hans Verkuil]

Fix this smatch warning:

drivers/media/platform/rockchip/rga/rga.c:734 rga_enable_clocks() warn: 'rga->sclk' from clk_prepare_enable() not released on lines: 734.

The reason is that aclk should be disabled/unprepared before
sclk, instead of the other way around.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Jacob Chen <jacob-chen@iotwrt.com>
Cc: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Cc: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/media/platform/rockchip/rga/rga.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
index 67dcf22e5ba3..cbb33dd62fdd 100644
--- a/drivers/media/platform/rockchip/rga/rga.c
+++ b/drivers/media/platform/rockchip/rga/rga.c
@@ -726,10 +726,10 @@ static int rga_enable_clocks(struct rockchip_rga *rga)
 
 	return 0;
 
-err_disable_sclk:
-	clk_disable_unprepare(rga->sclk);
 err_disable_aclk:
 	clk_disable_unprepare(rga->aclk);
+err_disable_sclk:
+	clk_disable_unprepare(rga->sclk);
 
 	return ret;
 }
-- 
2.39.2

Re: [PATCH 2/8] media: rockchip: rga: fix clock cleanup

[Michael Tretter]

On Wed, 24 May 2023 14:11:44 +0200, Hans Verkuil wrote:
> Fix this smatch warning:
> 
> drivers/media/platform/rockchip/rga/rga.c:734 rga_enable_clocks() warn: 'rga->sclk' from clk_prepare_enable() not released on lines: 734.
> 
> The reason is that aclk should be disabled/unprepared before
> sclk, instead of the other way around.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Cc: Jacob Chen <jacob-chen@iotwrt.com>
> Cc: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> Cc: Michael Tretter <m.tretter@pengutronix.de>

Reviewed-by: Michael Tretter <m.tretter@pengutronix.de>

> ---
>  drivers/media/platform/rockchip/rga/rga.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> index 67dcf22e5ba3..cbb33dd62fdd 100644
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
> @@ -726,10 +726,10 @@ static int rga_enable_clocks(struct rockchip_rga *rga)
>  
>  	return 0;
>  
> -err_disable_sclk:
> -	clk_disable_unprepare(rga->sclk);
>  err_disable_aclk:
>  	clk_disable_unprepare(rga->aclk);
> +err_disable_sclk:
> +	clk_disable_unprepare(rga->sclk);
>  
>  	return ret;
>  }
> -- 
> 2.39.2
> 
> 

[PATCH 3/8] media: usb: as102: drop as102_dev NULL check

[Hans Verkuil]

Fixes this smatch warning:

drivers/media/usb/as102/as102_usb_drv.c:306 as102_usb_release() warn: can 'as102_dev' even be NULL?

And indeed, as102_dev can never be NULL, so just drop the NULL check.

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

diff --git a/drivers/media/usb/as102/as102_usb_drv.c b/drivers/media/usb/as102/as102_usb_drv.c
index 50419e8ae56c..6b380144d6c2 100644
--- a/drivers/media/usb/as102/as102_usb_drv.c
+++ b/drivers/media/usb/as102/as102_usb_drv.c
@@ -303,10 +303,8 @@ static void as102_usb_release(struct kref *kref)
 	struct as102_dev_t *as102_dev;
 
 	as102_dev = container_of(kref, struct as102_dev_t, kref);
-	if (as102_dev != NULL) {
-		usb_put_dev(as102_dev->bus_adap.usb_dev);
-		kfree(as102_dev);
-	}
+	usb_put_dev(as102_dev->bus_adap.usb_dev);
+	kfree(as102_dev);
 }
 
 static void as102_usb_disconnect(struct usb_interface *intf)
-- 
2.39.2

[PATCH 4/8] media: platform: renesas-ceu: drop buf NULL check

[Hans Verkuil]

Since start_streaming is only called if there are at least two
buffers queued, the ceudev->capture list will never be empty, so
the check whether there are no buffers can be dropped.

Note that the '!buf' check was wrong in any case, if we wanted to
check for an empty list it should have used list_empty().

This fixes this smatch warning:

drivers/media/platform/renesas/renesas-ceu.c:705 ceu_start_streaming() warn: can 'buf' even be NULL?

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/platform/renesas/renesas-ceu.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/media/platform/renesas/renesas-ceu.c b/drivers/media/platform/renesas/renesas-ceu.c
index 56b9c59cfda8..5c9e27f8c94b 100644
--- a/drivers/media/platform/renesas/renesas-ceu.c
+++ b/drivers/media/platform/renesas/renesas-ceu.c
@@ -702,12 +702,6 @@ static int ceu_start_streaming(struct vb2_queue *vq, unsigned int count)
 	/* Grab the first available buffer and trigger the first capture. */
 	buf = list_first_entry(&ceudev->capture, struct ceu_buffer,
 			       queue);
-	if (!buf) {
-		spin_unlock_irqrestore(&ceudev->lock, irqflags);
-		dev_dbg(ceudev->dev,
-			"No buffer available for capture.\n");
-		goto error_stop_sensor;
-	}
 
 	list_del(&buf->queue);
 	ceudev->active = &buf->vb;
@@ -722,9 +716,6 @@ static int ceu_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	return 0;
 
-error_stop_sensor:
-	v4l2_subdev_call(v4l2_sd, video, s_stream, 0);
-
 error_return_bufs:
 	spin_lock_irqsave(&ceudev->lock, irqflags);
 	list_for_each_entry(buf, &ceudev->capture, queue)
-- 
2.39.2

Re: [PATCH 4/8] media: platform: renesas-ceu: drop buf NULL check

[Jacopo Mondi]

Hi Hans,

On Wed, May 24, 2023 at 02:11:46PM +0200, Hans Verkuil wrote:
> Since start_streaming is only called if there are at least two
> buffers queued, the ceudev->capture list will never be empty, so
> the check whether there are no buffers can be dropped.
>
> Note that the '!buf' check was wrong in any case, if we wanted to
> check for an empty list it should have used list_empty().
>
> This fixes this smatch warning:
>
> drivers/media/platform/renesas/renesas-ceu.c:705 ceu_start_streaming() warn: can 'buf' even be NULL?
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Cc: Jacopo Mondi <jacopo@jmondi.org>

Thank you!
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>


> ---
>  drivers/media/platform/renesas/renesas-ceu.c | 9 ---------
>  1 file changed, 9 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/renesas-ceu.c b/drivers/media/platform/renesas/renesas-ceu.c
> index 56b9c59cfda8..5c9e27f8c94b 100644
> --- a/drivers/media/platform/renesas/renesas-ceu.c
> +++ b/drivers/media/platform/renesas/renesas-ceu.c
> @@ -702,12 +702,6 @@ static int ceu_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	/* Grab the first available buffer and trigger the first capture. */
>  	buf = list_first_entry(&ceudev->capture, struct ceu_buffer,
>  			       queue);
> -	if (!buf) {
> -		spin_unlock_irqrestore(&ceudev->lock, irqflags);
> -		dev_dbg(ceudev->dev,
> -			"No buffer available for capture.\n");
> -		goto error_stop_sensor;
> -	}
>
>  	list_del(&buf->queue);
>  	ceudev->active = &buf->vb;
> @@ -722,9 +716,6 @@ static int ceu_start_streaming(struct vb2_queue *vq, unsigned int count)
>
>  	return 0;
>
> -error_stop_sensor:
> -	v4l2_subdev_call(v4l2_sd, video, s_stream, 0);
> -
>  error_return_bufs:
>  	spin_lock_irqsave(&ceudev->lock, irqflags);
>  	list_for_each_entry(buf, &ceudev->capture, queue)
> --
> 2.39.2
>

[PATCH 5/8] media: platform: mediatek: vpu: fix NULL ptr dereference

[Hans Verkuil]

If pdev is NULL, then it is still dereferenced.

This fixes this smatch warning:

drivers/media/platform/mediatek/vpu/mtk_vpu.c:570 vpu_load_firmware() warn: address of NULL pointer 'pdev'

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Yunfei Dong <yunfei.dong@mediatek.com>
---
 drivers/media/platform/mediatek/vpu/mtk_vpu.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/mediatek/vpu/mtk_vpu.c b/drivers/media/platform/mediatek/vpu/mtk_vpu.c
index 5e2bc286f168..1a95958a1f90 100644
--- a/drivers/media/platform/mediatek/vpu/mtk_vpu.c
+++ b/drivers/media/platform/mediatek/vpu/mtk_vpu.c
@@ -562,15 +562,17 @@ static int load_requested_vpu(struct mtk_vpu *vpu,
 int vpu_load_firmware(struct platform_device *pdev)
 {
 	struct mtk_vpu *vpu;
-	struct device *dev = &pdev->dev;
+	struct device *dev;
 	struct vpu_run *run;
 	int ret;
 
 	if (!pdev) {
-		dev_err(dev, "VPU platform device is invalid\n");
+		pr_err("VPU platform device is invalid\n");
 		return -EINVAL;
 	}
 
+	dev = &pdev->dev;
+
 	vpu = platform_get_drvdata(pdev);
 	run = &vpu->run;
 
-- 
2.39.2

[PATCH 6/8] media: mediatek: vpu: add missing clk_unprepare

[Hans Verkuil]

If vpu_clock_enable() fails, then call clk_unprepare().

This fixes this smatch warning:

drivers/media/platform/mediatek/vpu/mtk_vpu.c:1031 mtk_vpu_resume() warn: 'vpu->clk' from clk_prepare() not released on lines: 1020.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Yunfei Dong <yunfei.dong@mediatek.com>
---
 drivers/media/platform/mediatek/vpu/mtk_vpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/mediatek/vpu/mtk_vpu.c b/drivers/media/platform/mediatek/vpu/mtk_vpu.c
index 1a95958a1f90..4c8f5296d120 100644
--- a/drivers/media/platform/mediatek/vpu/mtk_vpu.c
+++ b/drivers/media/platform/mediatek/vpu/mtk_vpu.c
@@ -1018,6 +1018,7 @@ static int mtk_vpu_resume(struct device *dev)
 	clk_prepare(vpu->clk);
 	ret = vpu_clock_enable(vpu);
 	if (ret) {
+		clk_unprepare(vpu->clk);
 		dev_err(dev, "failed to enable vpu clock\n");
 		return ret;
 	}
-- 
2.39.2

[PATCH 7/8] media: pci: tw686x: no need to check 'next'

[Hans Verkuil]

If 'done' is not NULL, then next can never be NULL, so just drop
the 'next' check.

This fixes this smatch warning:

drivers/media/pci/tw686x/tw686x-audio.c:62 tw686x_audio_irq() warn: can 'next' even be NULL?

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/media/pci/tw686x/tw686x-audio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/pci/tw686x/tw686x-audio.c b/drivers/media/pci/tw686x/tw686x-audio.c
index 74cba1368cfa..1ae3845b6743 100644
--- a/drivers/media/pci/tw686x/tw686x-audio.c
+++ b/drivers/media/pci/tw686x/tw686x-audio.c
@@ -59,7 +59,7 @@ void tw686x_audio_irq(struct tw686x_dev *dev, unsigned long requests,
 		}
 		spin_unlock_irqrestore(&ac->lock, flags);
 
-		if (!done || !next)
+		if (!done)
 			continue;
 		/*
 		 * Checking for a non-nil dma_desc[pb]->virt buffer is
-- 
2.39.2

[PATCH 8/8] staging: media: atomisp: move up sanity checks

[Hans Verkuil]

The sanity checks were done too late, so move them up.

This fixes this smatch warning:

drivers/staging/media/atomisp/pci/sh_css_firmware.c:247 sh_css_load_firmware() warn: variable dereferenced before check 'fw_data' (see line 237)

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/sh_css_firmware.c   | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/sh_css_firmware.c b/drivers/staging/media/atomisp/pci/sh_css_firmware.c
index e7ef578db8ab..fb3f63938882 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_firmware.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_firmware.c
@@ -229,8 +229,16 @@ sh_css_load_firmware(struct device *dev, const char *fw_data,
 	struct sh_css_fw_bi_file_h *file_header;
 	int ret;
 
+	/* some sanity checks */
+	if (!fw_data || fw_size < sizeof(struct sh_css_fw_bi_file_h))
+		return -EINVAL;
+
 	firmware_header = (struct firmware_header *)fw_data;
 	file_header = &firmware_header->file_header;
+
+	if (file_header->h_size != sizeof(struct sh_css_fw_bi_file_h))
+		return -EINVAL;
+
 	binaries = &firmware_header->binary_header;
 	strscpy(FW_rel_ver_name, file_header->version,
 		min(sizeof(FW_rel_ver_name), sizeof(file_header->version)));
@@ -243,13 +251,6 @@ sh_css_load_firmware(struct device *dev, const char *fw_data,
 		IA_CSS_LOG("successfully load firmware version %s", release_version);
 	}
 
-	/* some sanity checks */
-	if (!fw_data || fw_size < sizeof(struct sh_css_fw_bi_file_h))
-		return -EINVAL;
-
-	if (file_header->h_size != sizeof(struct sh_css_fw_bi_file_h))
-		return -EINVAL;
-
 	sh_css_num_binaries = file_header->binary_nr;
 	/* Only allocate memory for ISP blob info */
 	if (sh_css_num_binaries > NUM_OF_SPS) {
-- 
2.39.2