Fix uses of uninitialized variables, reported by smatch.
Signed-off-by: Tomi Valkeinen <[email protected]>
---
Tomi Valkeinen (2):
media: i2c: ds90ub9x3: Fix use of uninitialized variables
media: i2c: ds90ub960: Fix PLL config for 1200 MHz CSI rate
drivers/media/i2c/ds90ub913.c | 2 +-
drivers/media/i2c/ds90ub953.c | 6 +++---
drivers/media/i2c/ds90ub960.c | 1 +
3 files changed, 5 insertions(+), 4 deletions(-)
---
base-commit: a0e657a03ffbd26332f316f13c3e5dbc98cb1fca
change-id: 20230803-ub9xx-uninit-vars-733337ba1051
Best regards,
--
Tomi Valkeinen <[email protected]>
smatch reports:
drivers/media/i2c/ds90ub960.c:1788 ub960_init_tx_ports() error: uninitialized symbol 'pll_div'.
This is caused by 'pll_div' not being set for 1200 MHz CSI rate. Set the
'pll_div' correctly.
Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver")
Reported-by: Hans Verkuil <[email protected]>
Closes: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Tomi Valkeinen <[email protected]>
---
drivers/media/i2c/ds90ub960.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index 4833b39b9178..4ab45e326d80 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -1763,6 +1763,7 @@ static int ub960_init_tx_ports(struct ub960_data *priv)
break;
case MHZ(1200):
speed_select = 1;
+ pll_div = 0x18;
break;
case MHZ(800):
speed_select = 2;
--
2.34.1
smatch reports some uninitialized variables:
drivers/media/i2c/ds90ub913.c:481 ub913_log_status() error: uninitialized symbol 'v1'.
drivers/media/i2c/ds90ub913.c:481 ub913_log_status() error: uninitialized symbol 'v2'.
drivers/media/i2c/ds90ub953.c:655 ub953_log_status() error: uninitialized symbol 'gpio_local_data'.
drivers/media/i2c/ds90ub953.c:655 ub953_log_status() error: uninitialized symbol 'gpio_input_ctrl'.
drivers/media/i2c/ds90ub953.c:655 ub953_log_status() error: uninitialized symbol 'gpio_pin_sts'.
These are used only for printing debug information, and the use of an
uninitialized variable only happens if an i2c transaction has failed,
which will print an error. Thus, fix the errors just by initializing the
variables to 0.
Fixes: 6363db1c9d45 ("media: i2c: add DS90UB953 driver")
Fixes: c158d0d4ff15 ("media: i2c: add DS90UB913 driver")
Reported-by: Hans Verkuil <[email protected]>
Closes: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Tomi Valkeinen <[email protected]>
---
drivers/media/i2c/ds90ub913.c | 2 +-
drivers/media/i2c/ds90ub953.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
index 80d9cf6dd945..b2115e3519e2 100644
--- a/drivers/media/i2c/ds90ub913.c
+++ b/drivers/media/i2c/ds90ub913.c
@@ -469,7 +469,7 @@ static int ub913_log_status(struct v4l2_subdev *sd)
{
struct ub913_data *priv = sd_to_ub913(sd);
struct device *dev = &priv->client->dev;
- u8 v = 0, v1, v2;
+ u8 v = 0, v1 = 0, v2 = 0;
ub913_read(priv, UB913_REG_MODE_SEL, &v);
dev_info(dev, "MODE_SEL %#02x\n", v);
diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
index cadf75eb0773..27471249a62a 100644
--- a/drivers/media/i2c/ds90ub953.c
+++ b/drivers/media/i2c/ds90ub953.c
@@ -593,9 +593,9 @@ static int ub953_log_status(struct v4l2_subdev *sd)
u8 v = 0, v1 = 0, v2 = 0;
unsigned int i;
char id[UB953_REG_FPD3_RX_ID_LEN];
- u8 gpio_local_data;
- u8 gpio_input_ctrl;
- u8 gpio_pin_sts;
+ u8 gpio_local_data = 0;
+ u8 gpio_input_ctrl = 0;
+ u8 gpio_pin_sts = 0;
for (i = 0; i < sizeof(id); i++)
ub953_read(priv, UB953_REG_FPD3_RX_ID(i), &id[i]);
--
2.34.1
On Thu, Aug 03, 2023 at 11:41:38AM +0300, Tomi Valkeinen wrote:
> smatch reports some uninitialized variables:
>
> drivers/media/i2c/ds90ub913.c:481 ub913_log_status() error: uninitialized symbol 'v1'.
> drivers/media/i2c/ds90ub913.c:481 ub913_log_status() error: uninitialized symbol 'v2'.
> drivers/media/i2c/ds90ub953.c:655 ub953_log_status() error: uninitialized symbol 'gpio_local_data'.
> drivers/media/i2c/ds90ub953.c:655 ub953_log_status() error: uninitialized symbol 'gpio_input_ctrl'.
> drivers/media/i2c/ds90ub953.c:655 ub953_log_status() error: uninitialized symbol 'gpio_pin_sts'.
>
> These are used only for printing debug information, and the use of an
> uninitialized variable only happens if an i2c transaction has failed,
> which will print an error. Thus, fix the errors just by initializing the
> variables to 0.
>
> Fixes: 6363db1c9d45 ("media: i2c: add DS90UB953 driver")
> Fixes: c158d0d4ff15 ("media: i2c: add DS90UB913 driver")
I would prefer two separate changes on per driver basis. This is a good
practice to make backporting easier (generally speaking).
--
With Best Regards,
Andy Shevchenko
On 03/08/2023 14:57, Andy Shevchenko wrote:
> On Thu, Aug 03, 2023 at 11:41:38AM +0300, Tomi Valkeinen wrote:
>> smatch reports some uninitialized variables:
>>
>> drivers/media/i2c/ds90ub913.c:481 ub913_log_status() error: uninitialized symbol 'v1'.
>> drivers/media/i2c/ds90ub913.c:481 ub913_log_status() error: uninitialized symbol 'v2'.
>> drivers/media/i2c/ds90ub953.c:655 ub953_log_status() error: uninitialized symbol 'gpio_local_data'.
>> drivers/media/i2c/ds90ub953.c:655 ub953_log_status() error: uninitialized symbol 'gpio_input_ctrl'.
>> drivers/media/i2c/ds90ub953.c:655 ub953_log_status() error: uninitialized symbol 'gpio_pin_sts'.
>>
>> These are used only for printing debug information, and the use of an
>> uninitialized variable only happens if an i2c transaction has failed,
>> which will print an error. Thus, fix the errors just by initializing the
>> variables to 0.
>>
>> Fixes: 6363db1c9d45 ("media: i2c: add DS90UB953 driver")
>> Fixes: c158d0d4ff15 ("media: i2c: add DS90UB913 driver")
>
> I would prefer two separate changes on per driver basis. This is a good
> practice to make backporting easier (generally speaking).
Yes, I almost did that, but then somehow got hit by acute laziness...
I'll send a v2 with split patches.
Tomi
Hi Tomi,
Thank you for the patch.
On Thu, Aug 03, 2023 at 11:41:39AM +0300, Tomi Valkeinen wrote:
> smatch reports:
>
> drivers/media/i2c/ds90ub960.c:1788 ub960_init_tx_ports() error: uninitialized symbol 'pll_div'.
>
> This is caused by 'pll_div' not being set for 1200 MHz CSI rate. Set the
> 'pll_div' correctly.
I'm surprised gcc didn't catch it.
Reviewed-by: Laurent Pinchart <[email protected]>
> Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver")
> Reported-by: Hans Verkuil <[email protected]>
> Closes: https://lore.kernel.org/all/[email protected]/
> Signed-off-by: Tomi Valkeinen <[email protected]>
> ---
> drivers/media/i2c/ds90ub960.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
> index 4833b39b9178..4ab45e326d80 100644
> --- a/drivers/media/i2c/ds90ub960.c
> +++ b/drivers/media/i2c/ds90ub960.c
> @@ -1763,6 +1763,7 @@ static int ub960_init_tx_ports(struct ub960_data *priv)
> break;
> case MHZ(1200):
> speed_select = 1;
> + pll_div = 0x18;
> break;
> case MHZ(800):
> speed_select = 2;
>
--
Regards,
Laurent Pinchart
Hi Tomi,
Thank you for the patch.
On Thu, Aug 03, 2023 at 11:41:38AM +0300, Tomi Valkeinen wrote:
> smatch reports some uninitialized variables:
>
> drivers/media/i2c/ds90ub913.c:481 ub913_log_status() error: uninitialized symbol 'v1'.
> drivers/media/i2c/ds90ub913.c:481 ub913_log_status() error: uninitialized symbol 'v2'.
> drivers/media/i2c/ds90ub953.c:655 ub953_log_status() error: uninitialized symbol 'gpio_local_data'.
> drivers/media/i2c/ds90ub953.c:655 ub953_log_status() error: uninitialized symbol 'gpio_input_ctrl'.
> drivers/media/i2c/ds90ub953.c:655 ub953_log_status() error: uninitialized symbol 'gpio_pin_sts'.
>
> These are used only for printing debug information, and the use of an
> uninitialized variable only happens if an i2c transaction has failed,
> which will print an error. Thus, fix the errors just by initializing the
> variables to 0.
>
> Fixes: 6363db1c9d45 ("media: i2c: add DS90UB953 driver")
> Fixes: c158d0d4ff15 ("media: i2c: add DS90UB913 driver")
> Reported-by: Hans Verkuil <[email protected]>
> Closes: https://lore.kernel.org/all/[email protected]/
> Signed-off-by: Tomi Valkeinen <[email protected]>
> ---
> drivers/media/i2c/ds90ub913.c | 2 +-
> drivers/media/i2c/ds90ub953.c | 6 +++---
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
> index 80d9cf6dd945..b2115e3519e2 100644
> --- a/drivers/media/i2c/ds90ub913.c
> +++ b/drivers/media/i2c/ds90ub913.c
> @@ -469,7 +469,7 @@ static int ub913_log_status(struct v4l2_subdev *sd)
> {
> struct ub913_data *priv = sd_to_ub913(sd);
> struct device *dev = &priv->client->dev;
> - u8 v = 0, v1, v2;
> + u8 v = 0, v1 = 0, v2 = 0;
This seems to work around the lack of error checking when calling
ub913_read(). Wouldn't it be better to check for errors there ? Or,
because this is ub913_log_status(), do you consider that we can print an
invalid CRC errors count, given that the ub913_read() function will have
printed an error message before ?
>
> ub913_read(priv, UB913_REG_MODE_SEL, &v);
> dev_info(dev, "MODE_SEL %#02x\n", v);
> diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
> index cadf75eb0773..27471249a62a 100644
> --- a/drivers/media/i2c/ds90ub953.c
> +++ b/drivers/media/i2c/ds90ub953.c
> @@ -593,9 +593,9 @@ static int ub953_log_status(struct v4l2_subdev *sd)
> u8 v = 0, v1 = 0, v2 = 0;
> unsigned int i;
> char id[UB953_REG_FPD3_RX_ID_LEN];
> - u8 gpio_local_data;
> - u8 gpio_input_ctrl;
> - u8 gpio_pin_sts;
> + u8 gpio_local_data = 0;
> + u8 gpio_input_ctrl = 0;
> + u8 gpio_pin_sts = 0;
>
> for (i = 0; i < sizeof(id); i++)
> ub953_read(priv, UB953_REG_FPD3_RX_ID(i), &id[i]);
>
--
Regards,
Laurent Pinchart
On 04/08/2023 00:46, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the patch.
>
> On Thu, Aug 03, 2023 at 11:41:38AM +0300, Tomi Valkeinen wrote:
>> smatch reports some uninitialized variables:
>>
>> drivers/media/i2c/ds90ub913.c:481 ub913_log_status() error: uninitialized symbol 'v1'.
>> drivers/media/i2c/ds90ub913.c:481 ub913_log_status() error: uninitialized symbol 'v2'.
>> drivers/media/i2c/ds90ub953.c:655 ub953_log_status() error: uninitialized symbol 'gpio_local_data'.
>> drivers/media/i2c/ds90ub953.c:655 ub953_log_status() error: uninitialized symbol 'gpio_input_ctrl'.
>> drivers/media/i2c/ds90ub953.c:655 ub953_log_status() error: uninitialized symbol 'gpio_pin_sts'.
>>
>> These are used only for printing debug information, and the use of an
>> uninitialized variable only happens if an i2c transaction has failed,
>> which will print an error. Thus, fix the errors just by initializing the
>> variables to 0.
>>
>> Fixes: 6363db1c9d45 ("media: i2c: add DS90UB953 driver")
>> Fixes: c158d0d4ff15 ("media: i2c: add DS90UB913 driver")
>> Reported-by: Hans Verkuil <[email protected]>
>> Closes: https://lore.kernel.org/all/[email protected]/
>> Signed-off-by: Tomi Valkeinen <[email protected]>
>> ---
>> drivers/media/i2c/ds90ub913.c | 2 +-
>> drivers/media/i2c/ds90ub953.c | 6 +++---
>> 2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
>> index 80d9cf6dd945..b2115e3519e2 100644
>> --- a/drivers/media/i2c/ds90ub913.c
>> +++ b/drivers/media/i2c/ds90ub913.c
>> @@ -469,7 +469,7 @@ static int ub913_log_status(struct v4l2_subdev *sd)
>> {
>> struct ub913_data *priv = sd_to_ub913(sd);
>> struct device *dev = &priv->client->dev;
>> - u8 v = 0, v1, v2;
>> + u8 v = 0, v1 = 0, v2 = 0;
>
> This seems to work around the lack of error checking when calling
Yes.
> ub913_read(). Wouldn't it be better to check for errors there ? Or,
> because this is ub913_log_status(), do you consider that we can print an
> invalid CRC errors count, given that the ub913_read() function will have
> printed an error message before ?
Yes, that was my thinking. Adding proper error handling would complicate
the function (more visibly so in ub953 and ub960, which have more
printing done), and what would be the benefit? Not much, in my opinion.
If the i2c transactions start to fail, we're in a bad situation anyway
(and, as you mention, ub913_read() will print errors).
However, I guess the "benefit" depends on the use a bit. If log status
is used as a debug aid, I think my reasoning is fine. But if it's used
by some automated script, to collect data, it may be more difficult for
the script to detect that an error has happened in the log status.
That said, I have to say this ignore-errors code somewhat bugs me, so
maybe I'll improve the log-status functions later. But I think these are
acceptable fixes to get rid of the smatch errors.
Tomi
On Fri, Aug 04, 2023 at 08:49:28AM +0300, Tomi Valkeinen wrote:
> On 04/08/2023 00:46, Laurent Pinchart wrote:
> > Hi Tomi,
> >
> > Thank you for the patch.
> >
> > On Thu, Aug 03, 2023 at 11:41:38AM +0300, Tomi Valkeinen wrote:
> >> smatch reports some uninitialized variables:
> >>
> >> drivers/media/i2c/ds90ub913.c:481 ub913_log_status() error: uninitialized symbol 'v1'.
> >> drivers/media/i2c/ds90ub913.c:481 ub913_log_status() error: uninitialized symbol 'v2'.
> >> drivers/media/i2c/ds90ub953.c:655 ub953_log_status() error: uninitialized symbol 'gpio_local_data'.
> >> drivers/media/i2c/ds90ub953.c:655 ub953_log_status() error: uninitialized symbol 'gpio_input_ctrl'.
> >> drivers/media/i2c/ds90ub953.c:655 ub953_log_status() error: uninitialized symbol 'gpio_pin_sts'.
> >>
> >> These are used only for printing debug information, and the use of an
> >> uninitialized variable only happens if an i2c transaction has failed,
> >> which will print an error. Thus, fix the errors just by initializing the
> >> variables to 0.
> >>
> >> Fixes: 6363db1c9d45 ("media: i2c: add DS90UB953 driver")
> >> Fixes: c158d0d4ff15 ("media: i2c: add DS90UB913 driver")
> >> Reported-by: Hans Verkuil <[email protected]>
> >> Closes: https://lore.kernel.org/all/[email protected]/
> >> Signed-off-by: Tomi Valkeinen <[email protected]>
> >> ---
> >> drivers/media/i2c/ds90ub913.c | 2 +-
> >> drivers/media/i2c/ds90ub953.c | 6 +++---
> >> 2 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
> >> index 80d9cf6dd945..b2115e3519e2 100644
> >> --- a/drivers/media/i2c/ds90ub913.c
> >> +++ b/drivers/media/i2c/ds90ub913.c
> >> @@ -469,7 +469,7 @@ static int ub913_log_status(struct v4l2_subdev *sd)
> >> {
> >> struct ub913_data *priv = sd_to_ub913(sd);
> >> struct device *dev = &priv->client->dev;
> >> - u8 v = 0, v1, v2;
> >> + u8 v = 0, v1 = 0, v2 = 0;
> >
> > This seems to work around the lack of error checking when calling
>
> Yes.
>
> > ub913_read(). Wouldn't it be better to check for errors there ? Or,
> > because this is ub913_log_status(), do you consider that we can print an
> > invalid CRC errors count, given that the ub913_read() function will have
> > printed an error message before ?
>
> Yes, that was my thinking. Adding proper error handling would complicate
> the function (more visibly so in ub953 and ub960, which have more
> printing done), and what would be the benefit? Not much, in my opinion.
> If the i2c transactions start to fail, we're in a bad situation anyway
> (and, as you mention, ub913_read() will print errors).
>
> However, I guess the "benefit" depends on the use a bit. If log status
> is used as a debug aid, I think my reasoning is fine. But if it's used
> by some automated script, to collect data, it may be more difficult for
> the script to detect that an error has happened in the log status.
I see log status as a debugging aid only, so I'm fine with your
reasoning.
> That said, I have to say this ignore-errors code somewhat bugs me, so
> maybe I'll improve the log-status functions later. But I think these are
> acceptable fixes to get rid of the smatch errors.
--
Regards,
Laurent Pinchart