Skip to contentSkip to conversation

iio: adc: handle unknow of_device_id data

PATCH In review Arnd Bergmann 3 participants 6 messages Feb 3, 2017 – Feb 8, 2017

Subsystems (3)
IIO Arm/RISC-V/Renesas architecture Renesas R-Car gyroadc driver
lore
Share & tools
lore

Arnd Bergmann

author
#0 lore raw

PATCH 1/1 iio: adc: handle unknow of_device_id data

If we get an unknown 'childmode' value, a number of variables are not initialized properly:

drivers/iio/adc/rcar-gyroadc.c: In function 'rcar_gyroadc_probe':
drivers/iio/adc/rcar-gyroadc.c:390:5: error: 'num_channels' may be used uninitialized in this function [-Werror=maybe-uninitialized] drivers/iio/adc/rcar-gyroadc.c:426:22: error: 'sample_width' may be used uninitialized in this function [-Werror=maybe-uninitialized] drivers/iio/adc/rcar-gyroadc.c:428:23: error: 'channels' may be used uninitialized in this function [-Werror=maybe-uninitialized]

The driver is currently correct, but handling this properly is more robust for possible modifications.

There is also a false-positive warning about adcmode being possibly uninitialized, but that cannot happen as we also check the 'first' flag:

drivers/iio/adc/rcar-gyroadc.c:398:26: error: 'adcmode' may be used uninitialized in this function [-Werror=maybe-uninitialized]

This adds an initialization for 'adcmode' and bails out for any unknown childmode.

Fixes 059c53b32329 ("iio: adc: Add Renesas GyroADC driver")
Signed-off-by Arnd Bergmann <[email protected]>
---
 drivers/iio/adc/rcar-gyroadc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/iio/adc/rcar-gyroadc.c +4 -1
index 0c44f72c32a8..331ff9a673be 100644
@@ -336,7 +336,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
 	struct device_node *child;
 	struct regulator *vref;
 	unsigned int reg;
-	unsigned int adcmode, childmode;
+	unsigned int adcmode = -1, childmode;
 	unsigned int sample_width;
 	unsigned int num_channels;
 	int ret, first = 1;

@@ -366,6 +366,9 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
 			channels = rcar_gyroadc_iio_channels_3;
 			num_channels = ARRAY_SIZE(rcar_gyroadc_iio_channels_3);
 			break;
+		default:
+			dev_err(dev, "unknown device type");
+			return -EINVAL;
 		}
 
 		/*
Signature · 1 line
2.9.0
#1 lore raw
Quoted 10 lines · Arnd Bergmann

If we get an unknown 'childmode' value, a number of variables are not initialized properly:

drivers/iio/adc/rcar-gyroadc.c: In function 'rcar_gyroadc_probe':
drivers/iio/adc/rcar-gyroadc.c:390:5: error: 'num_channels' may be used uninitialized in this function [-Werror=maybe-uninitialized] drivers/iio/adc/rcar-gyroadc.c:426:22: error: 'sample_width' may be used uninitialized in this function [-Werror=maybe-uninitialized] drivers/iio/adc/rcar-gyroadc.c:428:23: error: 'channels' may be used uninitialized in this function [-Werror=maybe-uninitialized]

The driver is currently correct, but handling this properly is more robust for possible modifications.

Yeah, the OF match will make sure this case is never triggered. But then again, it triggers kernel ci, so it should be fixed. Thanks for the patch :)

Quoted 27 lines

There is also a false-positive warning about adcmode being possibly uninitialized, but that cannot happen as we also check the 'first' flag:

drivers/iio/adc/rcar-gyroadc.c:398:26: error: 'adcmode' may be used uninitialized in this function [-Werror=maybe-uninitialized]

This adds an initialization for 'adcmode' and bails out for any unknown childmode.

Fixes 059c53b32329 ("iio: adc: Add Renesas GyroADC driver")
Signed-off-by Arnd Bergmann <[email protected]>
---
 drivers/iio/adc/rcar-gyroadc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/iio/adc/rcar-gyroadc.c +1 -1
index 0c44f72c32a8..331ff9a673be 100644
@@ -336,7 +336,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
 	struct device_node *child;
 	struct regulator *vref;
 	unsigned int reg;
-	unsigned int adcmode, childmode;
+	unsigned int adcmode = -1, childmode;
 	unsigned int sample_width;
 	unsigned int num_channels;
 	int ret, first = 1;

drivers/iio/adc/rcar-gyroadc.c static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev) · line 370

Quoted 5 lines
drivers/iio/adc/rcar-gyroadc.c +2 -0
@@ -366,6 +366,9 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
 			channels = rcar_gyroadc_iio_channels_3;
 			num_channels = ARRAY_SIZE(rcar_gyroadc_iio_channels_3);
 			break;
+		default:
+			dev_err(dev, "unknown device type");

Is this verbose output really needed ?

Quoted 5 lines
drivers/iio/adc/rcar-gyroadc.c +1 -0
+			return -EINVAL;
 		}
 
 		/*
Signature · 2 lines
Best regards,
Marek Vasut
author
#2 lore raw
Quoted 14 lines · Marek Vasut
Quoted 13 lines · Arnd Bergmann
drivers/iio/adc/rcar-gyroadc.c +1 -1
index 0c44f72c32a8..331ff9a673be 100644
@@ -336,7 +336,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
      struct device_node *child;
      struct regulator *vref;
      unsigned int reg;
-     unsigned int adcmode, childmode;
+     unsigned int adcmode = -1, childmode;
      unsigned int sample_width;
      unsigned int num_channels;
      int ret, first = 1;
Quoted 8 lines

drivers/iio/adc/rcar-gyroadc.c static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev) · line 370

Quoted 6 lines
drivers/iio/adc/rcar-gyroadc.c +2 -0
@@ -366,6 +366,9 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
                      channels = rcar_gyroadc_iio_channels_3;
                      num_channels = ARRAY_SIZE(rcar_gyroadc_iio_channels_3);
                      break;
+             default:
+                     dev_err(dev, "unknown device type");

Is this verbose output really needed ?

I don't care much either way, I was just trying to follow what the function does for the other failure cases. We can probably drop it though, as this would indicate
a bug in the driver implementation, while the others are about incorrect DT data.

Arnd

Acked-by
#3 lore raw
Quoted 24 lines · Arnd Bergmann
Quoted 14 lines · Marek Vasut
Quoted 13 lines · Arnd Bergmann
drivers/iio/adc/rcar-gyroadc.c +1 -1
index 0c44f72c32a8..331ff9a673be 100644
@@ -336,7 +336,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
      struct device_node *child;
      struct regulator *vref;
      unsigned int reg;
-     unsigned int adcmode, childmode;
+     unsigned int adcmode = -1, childmode;
      unsigned int sample_width;
      unsigned int num_channels;
      int ret, first = 1;
Quoted 8 lines

drivers/iio/adc/rcar-gyroadc.c static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev) · line 370

Quoted 6 lines
drivers/iio/adc/rcar-gyroadc.c +2 -0
@@ -366,6 +366,9 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
                      channels = rcar_gyroadc_iio_channels_3;
                      num_channels = ARRAY_SIZE(rcar_gyroadc_iio_channels_3);
                      break;
+             default:
+                     dev_err(dev, "unknown device type");

Is this verbose output really needed ?

Quoted 5 lines

I don't care much either way, I was just trying to follow what the function does for the other failure cases. We can probably drop it though, as this would indicate
a bug in the driver implementation, while the others are about incorrect DT data.

I don't really want to pester you too much with minor details, let's wait for JIC's opinion. If you're resending, please add my Ack, otherwise consider this patch:

Acked-by Marek Vasut <[email protected]>
Signature · 2 lines
Best regards,
Marek Vasut
maintainer
#4 lore raw
Quoted 31 lines · Marek Vasut
Quoted 24 lines · Arnd Bergmann
Quoted 14 lines · Marek Vasut
Quoted 13 lines · Arnd Bergmann
drivers/iio/adc/rcar-gyroadc.c +1 -1
index 0c44f72c32a8..331ff9a673be 100644
@@ -336,7 +336,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
      struct device_node *child;
      struct regulator *vref;
      unsigned int reg;
-     unsigned int adcmode, childmode;
+     unsigned int adcmode = -1, childmode;
      unsigned int sample_width;
      unsigned int num_channels;
      int ret, first = 1;
Quoted 8 lines

drivers/iio/adc/rcar-gyroadc.c static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev) · line 370

Quoted 6 lines
drivers/iio/adc/rcar-gyroadc.c +2 -0
@@ -366,6 +366,9 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
                      channels = rcar_gyroadc_iio_channels_3;
                      num_channels = ARRAY_SIZE(rcar_gyroadc_iio_channels_3);
                      break;
+             default:
+                     dev_err(dev, "unknown device type");

Is this verbose output really needed ?

Quoted 5 lines

I don't care much either way, I was just trying to follow what the function does for the other failure cases. We can probably drop it though, as this would indicate
a bug in the driver implementation, while the others are about incorrect DT data.

Quoted 6 lines

I don't really want to pester you too much with minor details, let's wait for JIC's opinion. If you're resending, please add my Ack, otherwise consider this patch:

Acked-by Marek Vasut <[email protected]>

I've applied with the dev_err dropped. Didn't care that much either way :)

Anyhow, applied to the fixes-togreg-post-rc1 branch of iio.git as I might not get another pull request out pre the merge window. (if I do I'll apply it with that).

Thanks,

Jonathan

#5 lore raw
Quoted 32 lines · Jonathan Cameron
Quoted 31 lines · Marek Vasut
Quoted 29 lines · Arnd Bergmann
Quoted 14 lines · Marek Vasut
Quoted 13 lines · Arnd Bergmann
drivers/iio/adc/rcar-gyroadc.c +1 -1
index 0c44f72c32a8..331ff9a673be 100644
@@ -336,7 +336,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
      struct device_node *child;
      struct regulator *vref;
      unsigned int reg;
-     unsigned int adcmode, childmode;
+     unsigned int adcmode = -1, childmode;
      unsigned int sample_width;
      unsigned int num_channels;
      int ret, first = 1;
Quoted 8 lines

drivers/iio/adc/rcar-gyroadc.c static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev) · line 370

Quoted 6 lines
drivers/iio/adc/rcar-gyroadc.c +2 -0
@@ -366,6 +366,9 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
                      channels = rcar_gyroadc_iio_channels_3;
                      num_channels = ARRAY_SIZE(rcar_gyroadc_iio_channels_3);
                      break;
+             default:
+                     dev_err(dev, "unknown device type");

Is this verbose output really needed ?

I don't care much either way, I was just trying to follow what the function does for the other failure cases. We can probably drop it though, as this would indicate
a bug in the driver implementation, while the others are about incorrect DT data.

Quoted 8 lines
Quoted 6 lines

I don't really want to pester you too much with minor details, let's wait for JIC's opinion. If you're resending, please add my Ack, otherwise consider this patch:

Acked-by Marek Vasut <[email protected]>

I've applied with the dev_err dropped. Didn't care that much either way :)

Noone seems to care much, but everyone is slightly opinionated, hehe :)

Quoted 8 lines

Anyhow, applied to the fixes-togreg-post-rc1 branch of iio.git as I might not get another pull request out pre the merge window. (if I do I'll apply it with that).

Thanks,

Jonathan

Thanks

Signature · 2 lines
Best regards,
Marek Vasut