[PATCH 1/3] media: venus: use NULL instead of zero for pointers

[PATCH 1/3] media: venus: use NULL instead of zero for pointers

[Mauro Carvalho Chehab]

As reported by sparse:

	drivers/media/platform/qcom/venus/core.c:227:41: warning: Using plain integer as NULL pointer
	drivers/media/platform/qcom/venus/core.c:228:34: warning: Using plain integer as NULL pointer

Two vars are using zero instead of NULL for pointers. Not really
an issue, but using NULL makes it clearer that the init data is
expecting a pointer.

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

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index f5b88b96f5f7..4451e3c11bc0 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -224,8 +224,8 @@ static void venus_assign_register_offsets(struct venus_core *core)
 		core->cpu_cs_base = core->base + CPU_CS_BASE;
 		core->cpu_ic_base = core->base + CPU_IC_BASE;
 		core->wrapper_base = core->base + WRAPPER_BASE;
-		core->wrapper_tz_base = 0;
-		core->aon_base = 0;
+		core->wrapper_tz_base = NULL;
+		core->aon_base = NULL;
 	}
 }
 
-- 
2.30.2

[PATCH 2/3] media: rdacm21: describe better a truncation

[Mauro Carvalho Chehab]

As warned by sparse:

	drivers/media/i2c/rdacm21.c:348:62: warning: cast truncates bits from constant value (300a becomes a)

the intention of the code is to get just the lowest 8 bits.
So, instead of using a typecast, use a bit and logic.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/i2c/rdacm21.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index dcc21515e5a4..179d107f494c 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -345,7 +345,7 @@ static int ov10640_initialize(struct rdacm21_device *dev)
 	/* Read OV10640 ID to test communications. */
 	ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR, OV490_SCCB_SLAVE_READ);
 	ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH, OV10640_CHIP_ID >> 8);
-	ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, (u8)OV10640_CHIP_ID);
+	ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, OV10640_CHIP_ID & 0xff);
 
 	/* Trigger SCCB slave transaction and give it some time to complete. */
 	ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
-- 
2.30.2

[PATCH 3/3] media: venus: don't de-reference NULL pointers at IRQ time

[Mauro Carvalho Chehab]

Smatch is warning that:
	drivers/media/platform/qcom/venus/hfi_venus.c:1100 venus_isr() warn: variable dereferenced before check 'hdev' (see line 1097)

The logic basically does:
	hdev = to_hfi_priv(core);

with is translated to:
	hdev = core->priv;

If the IRQ code can receive a NULL pointer for hdev, there's
a bug there, as it will first try to de-reference the pointer,
and then check if it is null.

After looking at the code, it seems that this indeed can happen:
Basically, the venus IRQ thread is started with:
	devm_request_threaded_irq()
So, it will only be freed after the driver unbinds.

In order to prevent the IRQ code to work with freed data,
the logic at venus_hfi_destroy() sets core->priv to NULL,
which would make the IRQ code to ignore any pending IRQs.

There is, however a race condition, as core->priv is set
to NULL only after being freed. So, we need also to move the
core->priv = NULL to happen earlier.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/qcom/venus/hfi_venus.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index cebb20cf371f..ce98c523b3c6 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -1094,12 +1094,15 @@ static irqreturn_t venus_isr(struct venus_core *core)
 {
 	struct venus_hfi_device *hdev = to_hfi_priv(core);
 	u32 status;
-	void __iomem *cpu_cs_base = hdev->core->cpu_cs_base;
-	void __iomem *wrapper_base = hdev->core->wrapper_base;
+	void __iomem *cpu_cs_base;
+	void __iomem *wrapper_base;
 
 	if (!hdev)
 		return IRQ_NONE;
 
+	cpu_cs_base = hdev->core->cpu_cs_base;
+	wrapper_base = hdev->core->wrapper_base;
+
 	status = readl(wrapper_base + WRAPPER_INTR_STATUS);
 	if (IS_V6(core)) {
 		if (status & WRAPPER_INTR_STATUS_A2H_MASK ||
@@ -1650,10 +1653,10 @@ void venus_hfi_destroy(struct venus_core *core)
 {
 	struct venus_hfi_device *hdev = to_hfi_priv(core);
 
+	core->priv = NULL;
 	venus_interface_queues_release(hdev);
 	mutex_destroy(&hdev->lock);
 	kfree(hdev);
-	core->priv = NULL;
 	core->ops = NULL;
 }
 
-- 
2.30.2

Re: [PATCH 2/3] media: rdacm21: describe better a truncation

[Jacopo Mondi]

Hi Mauro

On Thu, Apr 08, 2021 at 09:40:03AM +0200, Mauro Carvalho Chehab wrote:
> As warned by sparse:
>
> 	drivers/media/i2c/rdacm21.c:348:62: warning: cast truncates bits from constant value (300a becomes a)
>
> the intention of the code is to get just the lowest 8 bits.
> So, instead of using a typecast, use a bit and logic.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Please see
https://patchwork.linuxtv.org/project/linux-media/patch/20210319164148.199192-11-jacopo+renesas@jmondi.org/

Whatever gets in first it's fine, so you can add my
Acked-by: Jacopo Mondi <jacopo@jmondi.org>
to this one too

Thanks
  j

> ---
>  drivers/media/i2c/rdacm21.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index dcc21515e5a4..179d107f494c 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -345,7 +345,7 @@ static int ov10640_initialize(struct rdacm21_device *dev)
>  	/* Read OV10640 ID to test communications. */
>  	ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR, OV490_SCCB_SLAVE_READ);
>  	ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH, OV10640_CHIP_ID >> 8);
> -	ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, (u8)OV10640_CHIP_ID);
> +	ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, OV10640_CHIP_ID & 0xff);
>
>  	/* Trigger SCCB slave transaction and give it some time to complete. */
>  	ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
> --
> 2.30.2
>

Re: [PATCH 3/3] media: venus: don't de-reference NULL pointers at IRQ time

[Stanimir Varbanov]

Hi Mauro,

On 4/8/21 10:40 AM, Mauro Carvalho Chehab wrote:
> Smatch is warning that:
> 	drivers/media/platform/qcom/venus/hfi_venus.c:1100 venus_isr() warn: variable dereferenced before check 'hdev' (see line 1097)
> 
> The logic basically does:
> 	hdev = to_hfi_priv(core);
> 
> with is translated to:
> 	hdev = core->priv;
> 
> If the IRQ code can receive a NULL pointer for hdev, there's
> a bug there, as it will first try to de-reference the pointer,
> and then check if it is null.
> 
> After looking at the code, it seems that this indeed can happen:
> Basically, the venus IRQ thread is started with:
> 	devm_request_threaded_irq()
> So, it will only be freed after the driver unbinds.
> 
> In order to prevent the IRQ code to work with freed data,
> the logic at venus_hfi_destroy() sets core->priv to NULL,
> which would make the IRQ code to ignore any pending IRQs.
> 
> There is, however a race condition, as core->priv is set
> to NULL only after being freed. So, we need also to move the
> core->priv = NULL to happen earlier.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/media/platform/qcom/venus/hfi_venus.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Acked-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

> 
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index cebb20cf371f..ce98c523b3c6 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -1094,12 +1094,15 @@ static irqreturn_t venus_isr(struct venus_core *core)
>  {
>  	struct venus_hfi_device *hdev = to_hfi_priv(core);
>  	u32 status;
> -	void __iomem *cpu_cs_base = hdev->core->cpu_cs_base;
> -	void __iomem *wrapper_base = hdev->core->wrapper_base;
> +	void __iomem *cpu_cs_base;
> +	void __iomem *wrapper_base;
>  
>  	if (!hdev)
>  		return IRQ_NONE;
>  
> +	cpu_cs_base = hdev->core->cpu_cs_base;
> +	wrapper_base = hdev->core->wrapper_base;
> +
>  	status = readl(wrapper_base + WRAPPER_INTR_STATUS);
>  	if (IS_V6(core)) {
>  		if (status & WRAPPER_INTR_STATUS_A2H_MASK ||
> @@ -1650,10 +1653,10 @@ void venus_hfi_destroy(struct venus_core *core)
>  {
>  	struct venus_hfi_device *hdev = to_hfi_priv(core);
>  
> +	core->priv = NULL;
>  	venus_interface_queues_release(hdev);
>  	mutex_destroy(&hdev->lock);
>  	kfree(hdev);
> -	core->priv = NULL;
>  	core->ops = NULL;
>  }
>  
> 

-- 
regards,
Stan

Re: [PATCH 1/3] media: venus: use NULL instead of zero for pointers

[Stanimir Varbanov]

Hi Mauro,

On 4/8/21 10:40 AM, Mauro Carvalho Chehab wrote:
> As reported by sparse:
> 
> 	drivers/media/platform/qcom/venus/core.c:227:41: warning: Using plain integer as NULL pointer
> 	drivers/media/platform/qcom/venus/core.c:228:34: warning: Using plain integer as NULL pointer
> 
> Two vars are using zero instead of NULL for pointers. Not really
> an issue, but using NULL makes it clearer that the init data is
> expecting a pointer.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/media/platform/qcom/venus/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Acked-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index f5b88b96f5f7..4451e3c11bc0 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -224,8 +224,8 @@ static void venus_assign_register_offsets(struct venus_core *core)
>  		core->cpu_cs_base = core->base + CPU_CS_BASE;
>  		core->cpu_ic_base = core->base + CPU_IC_BASE;
>  		core->wrapper_base = core->base + WRAPPER_BASE;
> -		core->wrapper_tz_base = 0;
> -		core->aon_base = 0;
> +		core->wrapper_tz_base = NULL;
> +		core->aon_base = NULL;
>  	}
>  }
>  
> 

-- 
regards,
Stan