[PATCH 0/3] media: i2c: Fix some uninitialized return values

[PATCH 0/3] media: i2c: Fix some uninitialized return values

[Dan Carpenter]

These were a couple uninitialized variable bugs detected by Smatch.

Dan Carpenter (3):
  media: i2c: imx334: uninitialized variable in imx334_update_exp_gain()
  media: i2c: ds90ub960: Fix uninitialized variable in
    ub960_serializer_temp_ramp()
  media: i2c: ds90ub960: Fix uninitialized variable in
    ub960_rxport_bc_ser_config()

 drivers/media/i2c/ds90ub960.c | 4 ++--
 drivers/media/i2c/imx334.c    | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

-- 
2.47.2

[PATCH 1/3] media: i2c: imx334: uninitialized variable in imx334_update_exp_gain()

[Dan Carpenter]

The "ret" variable is not initialized on the success path.  Set it to
zero.

Fixes: 7b19b0fc8ac8 ("media: i2c: imx334: Convert to CCI register access helpers")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/media/i2c/imx334.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index fc875072f859..846b9928d4e8 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -536,7 +536,8 @@ static int imx334_update_controls(struct imx334 *imx334,
 static int imx334_update_exp_gain(struct imx334 *imx334, u32 exposure, u32 gain)
 {
 	u32 lpfr, shutter;
-	int ret, ret_hold;
+	int ret_hold;
+	int ret = 0;
 
 	lpfr = imx334->vblank + imx334->cur_mode->height;
 	shutter = lpfr - exposure;
-- 
2.47.2

Re: [PATCH 1/3] media: i2c: imx334: uninitialized variable in imx334_update_exp_gain()

[Tarang Raval]

Hi Dan,

> The "ret" variable is not initialized on the success path.  Set it to
> zero.
>
> Fixes: 7b19b0fc8ac8 ("media: i2c: imx334: Convert to CCI register access helpers")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  drivers/media/i2c/imx334.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index fc875072f859..846b9928d4e8 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -536,7 +536,8 @@ static int imx334_update_controls(struct imx334 *imx334,
>  static int imx334_update_exp_gain(struct imx334 *imx334, u32 exposure, u32 gain)
>  {
>         u32 lpfr, shutter;
> -       int ret, ret_hold;
> +       int ret_hold;
> +       int ret = 0;

I think this initialization may not really be necessary.

If all of those cci_write are skipped, then yes, using ret uninitialized 
would be a problem.

However, I don’t see any case where they would be skipped in the 
current implementation.

So, is this initialization really needed?

Best Regards,
Tarang

>         lpfr = imx334->vblank + imx334->cur_mode->height;
>         shutter = lpfr - exposure;
> --
> 2.47.2

Re: [PATCH 1/3] media: i2c: imx334: uninitialized variable in imx334_update_exp_gain()

[Dan Carpenter]

On Wed, Apr 30, 2025 at 09:47:44AM +0000, Tarang Raval wrote:
> Hi Dan,
> 
> > The "ret" variable is not initialized on the success path.  Set it to
> > zero.
> >
> > Fixes: 7b19b0fc8ac8 ("media: i2c: imx334: Convert to CCI register access helpers")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> >  drivers/media/i2c/imx334.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > index fc875072f859..846b9928d4e8 100644
> > --- a/drivers/media/i2c/imx334.c
> > +++ b/drivers/media/i2c/imx334.c
> > @@ -536,7 +536,8 @@ static int imx334_update_controls(struct imx334 *imx334,
> >  static int imx334_update_exp_gain(struct imx334 *imx334, u32 exposure, u32 gain)
> >  {
> >         u32 lpfr, shutter;
> > -       int ret, ret_hold;
> > +       int ret_hold;
> > +       int ret = 0;
> 
> I think this initialization may not really be necessary.
> 
> If all of those cci_write are skipped, then yes, using ret uninitialized 
> would be a problem.
> 
> However, I don’t see any case where they would be skipped in the 
> current implementation.
> 
> So, is this initialization really needed?
> 
> Best Regards,
> Tarang

This is a new bug that was introduced in linux-next...

drivers/media/i2c/imx334.c
   536  static int imx334_update_exp_gain(struct imx334 *imx334, u32 exposure, u32 gain)
   537  {
   538          u32 lpfr, shutter;
   539          int ret, ret_hold;
   540  
   541          lpfr = imx334->vblank + imx334->cur_mode->height;
   542          shutter = lpfr - exposure;
   543  
   544          dev_dbg(imx334->dev, "Set long exp %u analog gain %u sh0 %u lpfr %u\n",
   545                  exposure, gain, shutter, lpfr);
   546  
   547          cci_write(imx334->cci, IMX334_REG_HOLD, 1, &ret);

This first call will do an unitialized read of ret to check if it holds
an error code.

   548          cci_write(imx334->cci, IMX334_REG_VMAX, lpfr, &ret);
   549          cci_write(imx334->cci, IMX334_REG_SHUTTER, shutter, &ret);
   550          cci_write(imx334->cci, IMX334_REG_AGAIN, gain, &ret);

cci_write() is designed to preserve the error codes from previous calls.
It will only write error codes to "ret", it will not write success.  If
everything succeeds then "ret" is uninitialized.

   551  
   552          ret_hold = cci_write(imx334->cci, IMX334_REG_HOLD, 0, NULL);
   553          if (ret_hold)
   554                  return ret_hold;
   555  
   556          return ret;
   557  }

In production then everyone should run with INIT_STACK_ALL_ZERO.  In that
case everything works fine.  However some older distributions do not use
this option.  Also in testing, I would encourage everyone to run with
INIT_STACK_ALL_PATTERN.

regards,
dan carpenter

Re: [PATCH 1/3] media: i2c: imx334: uninitialized variable in imx334_update_exp_gain()

[Tarang Raval]

> On Wed, Apr 30, 2025 at 09:47:44AM +0000, Tarang Raval wrote:
> > Hi Dan,
> >
> > > The "ret" variable is not initialized on the success path.  Set it to
> > > zero.
> > >
> > > Fixes: 7b19b0fc8ac8 ("media: i2c: imx334: Convert to CCI register access helpers")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > ---
> > >  drivers/media/i2c/imx334.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > > index fc875072f859..846b9928d4e8 100644
> > > --- a/drivers/media/i2c/imx334.c
> > > +++ b/drivers/media/i2c/imx334.c
> > > @@ -536,7 +536,8 @@ static int imx334_update_controls(struct imx334 *imx334,
> > >  static int imx334_update_exp_gain(struct imx334 *imx334, u32 exposure, u32 gain)
> > >  {
> > >         u32 lpfr, shutter;
> > > -       int ret, ret_hold;
> > > +       int ret_hold;
> > > +       int ret = 0;
> >
> > I think this initialization may not really be necessary.
> >
> > If all of those cci_write are skipped, then yes, using ret uninitialized
> > would be a problem.
> >
> > However, I don’t see any case where they would be skipped in the
> > current implementation.
> >
> > So, is this initialization really needed?
> >
> > Best Regards,
> > Tarang
> 
> This is a new bug that was introduced in linux-next...
> 
> drivers/media/i2c/imx334.c
>    536  static int imx334_update_exp_gain(struct imx334 *imx334, u32 exposure, u32 gain)
>    537  {
>    538          u32 lpfr, shutter;
>    539          int ret, ret_hold;
>    540
>    541          lpfr = imx334->vblank + imx334->cur_mode->height;
>    542          shutter = lpfr - exposure;
>    543
>    544          dev_dbg(imx334->dev, "Set long exp %u analog gain %u sh0 %u lpfr %u\n",
>    545                  exposure, gain, shutter, lpfr);
>    546
>    547          cci_write(imx334->cci, IMX334_REG_HOLD, 1, &ret);
> 
> This first call will do an unitialized read of ret to check if it holds
> an error code.
> 
>    548          cci_write(imx334->cci, IMX334_REG_VMAX, lpfr, &ret);
>    549          cci_write(imx334->cci, IMX334_REG_SHUTTER, shutter, &ret);
>    550          cci_write(imx334->cci, IMX334_REG_AGAIN, gain, &ret);
> 
> cci_write() is designed to preserve the error codes from previous calls.
> It will only write error codes to "ret", it will not write success.  If
> everything succeeds then "ret" is uninitialized.

Yes, correct
 
>    551
>    552          ret_hold = cci_write(imx334->cci, IMX334_REG_HOLD, 0, NULL);
>    553          if (ret_hold)
>    554                  return ret_hold;
>    555
>    556          return ret;
>    557  }
> 
> In production then everyone should run with INIT_STACK_ALL_ZERO.  In that
> case everything works fine.  However some older distributions do not use
> this option.  Also in testing, I would encourage everyone to run with
> INIT_STACK_ALL_PATTERN.

I am not aware of the INIT_STACK_ALL_ZERO and INIT_STACK_ALL_PATTERN options.
I’ll read more about them

Best Regards,
Tarang

[PATCH 2/3] media: i2c: ds90ub960: Fix uninitialized variable in ub960_serializer_temp_ramp()

[Dan Carpenter]

The "ret" variable is not initialized on the success path.

Fixes: a05744749600 ("media: i2c: ds90ub9xx: Set serializer temperature ramp")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/media/i2c/ds90ub960.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index 8075bffbac2b..cf104461b9a7 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -2058,7 +2058,7 @@ static int ub960_serializer_temp_ramp(struct ub960_rxport *rxport)
 	u8 temp_dynamic_cfg;
 	u8 nport = rxport->nport;
 	u8 ser_temp_code;
-	int ret;
+	int ret = 0;
 
 	/* Configure temp ramp only on UB953 */
 	if (!fwnode_device_is_compatible(rxport->ser.fwnode, "ti,ds90ub953-q1"))
-- 
2.47.2

Re: [PATCH 2/3] media: i2c: ds90ub960: Fix uninitialized variable in ub960_serializer_temp_ramp()

[Jai Luthra]

[-- Attachment #1: Type: text/plain, Size: 1038 bytes --]

Hi Dan,

On Apr 30, 2025 at 11:27:36 +0300, Dan Carpenter wrote:
> The "ret" variable is not initialized on the success path.
> 
> Fixes: a05744749600 ("media: i2c: ds90ub9xx: Set serializer temperature ramp")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  drivers/media/i2c/ds90ub960.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
> index 8075bffbac2b..cf104461b9a7 100644
> --- a/drivers/media/i2c/ds90ub960.c
> +++ b/drivers/media/i2c/ds90ub960.c
> @@ -2058,7 +2058,7 @@ static int ub960_serializer_temp_ramp(struct ub960_rxport *rxport)
>  	u8 temp_dynamic_cfg;
>  	u8 nport = rxport->nport;
>  	u8 ser_temp_code;
> -	int ret;
> +	int ret = 0;

Oops, thanks for the fix.

Reviewed-By: Jai Luthra <jai.luthra@ideasonboard.com>

>  
>  	/* Configure temp ramp only on UB953 */
>  	if (!fwnode_device_is_compatible(rxport->ser.fwnode, "ti,ds90ub953-q1"))
> -- 
> 2.47.2
> 

-- 
Thanks,
Jai

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/3] media: i2c: ds90ub960: Fix uninitialized variable in ub960_rxport_bc_ser_config()

[Dan Carpenter]

The "ret" variable is not initialized on success.  Set it to zero.

Fixes: e2a3b695bc5f ("media: i2c: ds90ub960: Configure serializer using back-channel")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/media/i2c/ds90ub960.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index cf104461b9a7..ed2cf9d247d1 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -2133,7 +2133,7 @@ static int ub960_rxport_bc_ser_config(struct ub960_rxport *rxport)
 	struct ub960_data *priv = rxport->priv;
 	struct device *dev = &priv->client->dev;
 	u8 nport = rxport->nport;
-	int ret;
+	int ret = 0;
 
 	/* Skip port if serializer's address is not known */
 	if (rxport->ser.addr < 0) {
-- 
2.47.2

Re: [PATCH 3/3] media: i2c: ds90ub960: Fix uninitialized variable in ub960_rxport_bc_ser_config()

[Jai Luthra]

[-- Attachment #1: Type: text/plain, Size: 1024 bytes --]

On Apr 30, 2025 at 11:27:42 +0300, Dan Carpenter wrote:
> The "ret" variable is not initialized on success.  Set it to zero.
> 
> Fixes: e2a3b695bc5f ("media: i2c: ds90ub960: Configure serializer using back-channel")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Reviewed-By: Jai Luthra <jai.luthra@ideasonboard.com>

> ---
>  drivers/media/i2c/ds90ub960.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
> index cf104461b9a7..ed2cf9d247d1 100644
> --- a/drivers/media/i2c/ds90ub960.c
> +++ b/drivers/media/i2c/ds90ub960.c
> @@ -2133,7 +2133,7 @@ static int ub960_rxport_bc_ser_config(struct ub960_rxport *rxport)
>  	struct ub960_data *priv = rxport->priv;
>  	struct device *dev = &priv->client->dev;
>  	u8 nport = rxport->nport;
> -	int ret;
> +	int ret = 0;
>  
>  	/* Skip port if serializer's address is not known */
>  	if (rxport->ser.addr < 0) {
> -- 
> 2.47.2
> 

-- 
Thanks,
Jai

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/3] media: i2c: Fix some uninitialized return values

[Tarang Raval]

Hi Dan,

> These were a couple uninitialized variable bugs detected by Smatch.
>
> Dan Carpenter (3):
>   media: i2c: imx334: uninitialized variable in imx334_update_exp_gain()
>   media: i2c: ds90ub960: Fix uninitialized variable in
>     ub960_serializer_temp_ramp()
>   media: i2c: ds90ub960: Fix uninitialized variable in
>     ub960_rxport_bc_ser_config()

Regarding patches 2/3 and 3/3, I believe these are also similar cases.

Best Regards,
Tarang   

>  drivers/media/i2c/ds90ub960.c | 4 ++--
>  drivers/media/i2c/imx334.c    | 3 ++-
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> --    
> 2.47.2