[PATCH v3 1/2] iio: adc: ad7192: fix null pointer de-reference crash during probe

[PATCH v3 1/2] iio: adc: ad7192: fix null pointer de-reference crash during probe

[Alexandru Ardelean]

When the 'spi_device_id' table was removed, it omitted to cleanup/fix the
assignment:
   'indio_dev->name = spi_get_device_id(spi)->name;'

After that patch 'spi_get_device_id(spi)' returns NULL, so this crashes
during probe with null de-ref.

This change fixes this by introducing an ad7192_chip_info struct, and
defines all part-names [that should be assigned to indio_dev->name] in a
'ad7192_chip_info_tbl' table.

With this change, the old 'st->devid' is also moved to be a
'chip_info->chip_id'. And the old 'ID_AD719X' macros have been renamed to
'CHIPID_AD719X'. Tld identifiers have been re-purposed to be enum/index
values in the new 'ad7192_chip_info_tbl'.

This should fix the bug, and maintain the ABI for the 'indio_dev->name'
field.

Fixes: 66614ab2be38 ("staging: iio: adc: ad7192: removed spi_device_id")
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---

Changelog v2 -> v3:
* reworked patch to introduce a chip_info struct for the part-name
* added 2nd patch to move of-table closer to the end of the file; this
  patch is more cosmetic; has no fixes tag, but is on top of the previous

Changelog v1 -> v2:
* fix colon for Fixes tag
* updated commit title a bit; to make it longer

 drivers/iio/adc/ad7192.c | 61 ++++++++++++++++++++++++++++++----------
 1 file changed, 46 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 8ec28aa8fa8a..7e8662c5cb0e 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -125,10 +125,10 @@
 #define AD7193_CH_AINCOM	0x600 /* AINCOM - AINCOM */
 
 /* ID Register Bit Designations (AD7192_REG_ID) */
-#define ID_AD7190		0x4
-#define ID_AD7192		0x0
-#define ID_AD7193		0x2
-#define ID_AD7195		0x6
+#define CHIPID_AD7190		0x4
+#define CHIPID_AD7192		0x0
+#define CHIPID_AD7193		0x2
+#define CHIPID_AD7195		0x6
 #define AD7192_ID_MASK		0x0F
 
 /* GPOCON Register Bit Designations (AD7192_REG_GPOCON) */
@@ -161,7 +161,20 @@ enum {
    AD7192_SYSCALIB_FULL_SCALE,
 };
 
+enum {
+	ID_AD7190,
+	ID_AD7192,
+	ID_AD7193,
+	ID_AD7195,
+};
+
+struct ad7192_chip_info {
+	unsigned int			chip_id;
+	const char			*name;
+};
+
 struct ad7192_state {
+	const struct ad7192_chip_info	*chip_info;
 	struct regulator		*avdd;
 	struct regulator		*dvdd;
 	struct clk			*mclk;
@@ -172,7 +185,6 @@ struct ad7192_state {
 	u32				conf;
 	u32				scale_avail[8][2];
 	u8				gpocon;
-	u8				devid;
 	u8				clock_sel;
 	struct mutex			lock;	/* protect sensor state */
 	u8				syscalib_mode[8];
@@ -348,7 +360,7 @@ static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
 
 	id &= AD7192_ID_MASK;
 
-	if (id != st->devid)
+	if (id != st->chip_info->chip_id)
 		dev_warn(&st->sd.spi->dev, "device ID query failed (0x%X)\n",
 			 id);
 
@@ -363,7 +375,7 @@ static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
 		st->mode |= AD7192_MODE_REJ60;
 
 	refin2_en = of_property_read_bool(np, "adi,refin2-pins-enable");
-	if (refin2_en && st->devid != ID_AD7195)
+	if (refin2_en && st->chip_info->chip_id != CHIPID_AD7195)
 		st->conf |= AD7192_CONF_REFSEL;
 
 	st->conf &= ~AD7192_CONF_CHOP;
@@ -859,11 +871,30 @@ static const struct iio_chan_spec ad7193_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(14),
 };
 
+static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
+	[ID_AD7190] = {
+		.chip_id = CHIPID_AD7190,
+		.name = "ad7190",
+	},
+	[ID_AD7192] = {
+		.chip_id = CHIPID_AD7192,
+		.name = "ad7192",
+	},
+	[ID_AD7193] = {
+		.chip_id = CHIPID_AD7193,
+		.name = "ad7193",
+	},
+	[ID_AD7195] = {
+		.chip_id = CHIPID_AD7195,
+		.name = "ad7195",
+	},
+};
+
 static int ad7192_channels_config(struct iio_dev *indio_dev)
 {
 	struct ad7192_state *st = iio_priv(indio_dev);
 
-	switch (st->devid) {
+	switch (st->chip_info->chip_id) {
 	case ID_AD7193:
 		indio_dev->channels = ad7193_channels;
 		indio_dev->num_channels = ARRAY_SIZE(ad7193_channels);
@@ -878,10 +909,10 @@ static int ad7192_channels_config(struct iio_dev *indio_dev)
 }
 
 static const struct of_device_id ad7192_of_match[] = {
-	{ .compatible = "adi,ad7190", .data = (void *)ID_AD7190 },
-	{ .compatible = "adi,ad7192", .data = (void *)ID_AD7192 },
-	{ .compatible = "adi,ad7193", .data = (void *)ID_AD7193 },
-	{ .compatible = "adi,ad7195", .data = (void *)ID_AD7195 },
+	{ .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
+	{ .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
+	{ .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
+	{ .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
 	{}
 };
 MODULE_DEVICE_TABLE(of, ad7192_of_match);
@@ -938,16 +969,16 @@ static int ad7192_probe(struct spi_device *spi)
 	}
 
 	spi_set_drvdata(spi, indio_dev);
-	st->devid = (unsigned long)of_device_get_match_data(&spi->dev);
+	st->chip_info = of_device_get_match_data(&spi->dev);
 	indio_dev->dev.parent = &spi->dev;
-	indio_dev->name = spi_get_device_id(spi)->name;
+	indio_dev->name = st->chip_info->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	ret = ad7192_channels_config(indio_dev);
 	if (ret < 0)
 		goto error_disable_dvdd;
 
-	if (st->devid == ID_AD7195)
+	if (st->chip_info->chip_id == CHIPID_AD7195)
 		indio_dev->info = &ad7195_info;
 	else
 		indio_dev->info = &ad7192_info;
-- 
2.17.1

[PATCH v3 2/2] iio: adc: ad7192: move ad7192_of_match table closer to the end of the file

[Alexandru Ardelean]

The change is more cosmetic. There is no need to reference this table in
the probe function since 'of_device_get_match_data' is used, which obtains
this information from the driver object.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/adc/ad7192.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 7e8662c5cb0e..cef61f962e60 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -908,15 +908,6 @@ static int ad7192_channels_config(struct iio_dev *indio_dev)
 	return 0;
 }
 
-static const struct of_device_id ad7192_of_match[] = {
-	{ .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
-	{ .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
-	{ .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
-	{ .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
-	{}
-};
-MODULE_DEVICE_TABLE(of, ad7192_of_match);
-
 static int ad7192_probe(struct spi_device *spi)
 {
 	struct ad7192_state *st;
@@ -1050,6 +1041,15 @@ static int ad7192_remove(struct spi_device *spi)
 	return 0;
 }
 
+static const struct of_device_id ad7192_of_match[] = {
+	{ .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
+	{ .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
+	{ .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
+	{ .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ad7192_of_match);
+
 static struct spi_driver ad7192_driver = {
 	.driver = {
 		.name	= "ad7192",
-- 
2.17.1

Re: [PATCH v3 1/2] iio: adc: ad7192: fix null pointer de-reference crash during probe

[Jonathan Cameron]

On Mon, 13 Apr 2020 11:20:43 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> When the 'spi_device_id' table was removed, it omitted to cleanup/fix the
> assignment:
>    'indio_dev->name = spi_get_device_id(spi)->name;'
> 
> After that patch 'spi_get_device_id(spi)' returns NULL, so this crashes
> during probe with null de-ref.
> 
> This change fixes this by introducing an ad7192_chip_info struct, and
> defines all part-names [that should be assigned to indio_dev->name] in a
> 'ad7192_chip_info_tbl' table.
> 
> With this change, the old 'st->devid' is also moved to be a
> 'chip_info->chip_id'. And the old 'ID_AD719X' macros have been renamed to
> 'CHIPID_AD719X'. Tld identifiers have been re-purposed to be enum/index
> values in the new 'ad7192_chip_info_tbl'.
> 
> This should fix the bug, and maintain the ABI for the 'indio_dev->name'
> field.
> 
> Fixes: 66614ab2be38 ("staging: iio: adc: ad7192: removed spi_device_id")
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied to the fixes-togreg branch of iio.git.

Thanks,

Jonathan

> ---
> 
> Changelog v2 -> v3:
> * reworked patch to introduce a chip_info struct for the part-name
> * added 2nd patch to move of-table closer to the end of the file; this
>   patch is more cosmetic; has no fixes tag, but is on top of the previous
> 
> Changelog v1 -> v2:
> * fix colon for Fixes tag
> * updated commit title a bit; to make it longer
> 
>  drivers/iio/adc/ad7192.c | 61 ++++++++++++++++++++++++++++++----------
>  1 file changed, 46 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 8ec28aa8fa8a..7e8662c5cb0e 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -125,10 +125,10 @@
>  #define AD7193_CH_AINCOM	0x600 /* AINCOM - AINCOM */
>  
>  /* ID Register Bit Designations (AD7192_REG_ID) */
> -#define ID_AD7190		0x4
> -#define ID_AD7192		0x0
> -#define ID_AD7193		0x2
> -#define ID_AD7195		0x6
> +#define CHIPID_AD7190		0x4
> +#define CHIPID_AD7192		0x0
> +#define CHIPID_AD7193		0x2
> +#define CHIPID_AD7195		0x6
>  #define AD7192_ID_MASK		0x0F
>  
>  /* GPOCON Register Bit Designations (AD7192_REG_GPOCON) */
> @@ -161,7 +161,20 @@ enum {
>     AD7192_SYSCALIB_FULL_SCALE,
>  };
>  
> +enum {
> +	ID_AD7190,
> +	ID_AD7192,
> +	ID_AD7193,
> +	ID_AD7195,
> +};
> +
> +struct ad7192_chip_info {
> +	unsigned int			chip_id;
> +	const char			*name;
> +};
> +
>  struct ad7192_state {
> +	const struct ad7192_chip_info	*chip_info;
>  	struct regulator		*avdd;
>  	struct regulator		*dvdd;
>  	struct clk			*mclk;
> @@ -172,7 +185,6 @@ struct ad7192_state {
>  	u32				conf;
>  	u32				scale_avail[8][2];
>  	u8				gpocon;
> -	u8				devid;
>  	u8				clock_sel;
>  	struct mutex			lock;	/* protect sensor state */
>  	u8				syscalib_mode[8];
> @@ -348,7 +360,7 @@ static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
>  
>  	id &= AD7192_ID_MASK;
>  
> -	if (id != st->devid)
> +	if (id != st->chip_info->chip_id)
>  		dev_warn(&st->sd.spi->dev, "device ID query failed (0x%X)\n",
>  			 id);
>  
> @@ -363,7 +375,7 @@ static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
>  		st->mode |= AD7192_MODE_REJ60;
>  
>  	refin2_en = of_property_read_bool(np, "adi,refin2-pins-enable");
> -	if (refin2_en && st->devid != ID_AD7195)
> +	if (refin2_en && st->chip_info->chip_id != CHIPID_AD7195)
>  		st->conf |= AD7192_CONF_REFSEL;
>  
>  	st->conf &= ~AD7192_CONF_CHOP;
> @@ -859,11 +871,30 @@ static const struct iio_chan_spec ad7193_channels[] = {
>  	IIO_CHAN_SOFT_TIMESTAMP(14),
>  };
>  
> +static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
> +	[ID_AD7190] = {
> +		.chip_id = CHIPID_AD7190,
> +		.name = "ad7190",
> +	},
> +	[ID_AD7192] = {
> +		.chip_id = CHIPID_AD7192,
> +		.name = "ad7192",
> +	},
> +	[ID_AD7193] = {
> +		.chip_id = CHIPID_AD7193,
> +		.name = "ad7193",
> +	},
> +	[ID_AD7195] = {
> +		.chip_id = CHIPID_AD7195,
> +		.name = "ad7195",
> +	},
> +};
> +
>  static int ad7192_channels_config(struct iio_dev *indio_dev)
>  {
>  	struct ad7192_state *st = iio_priv(indio_dev);
>  
> -	switch (st->devid) {
> +	switch (st->chip_info->chip_id) {
>  	case ID_AD7193:
>  		indio_dev->channels = ad7193_channels;
>  		indio_dev->num_channels = ARRAY_SIZE(ad7193_channels);
> @@ -878,10 +909,10 @@ static int ad7192_channels_config(struct iio_dev *indio_dev)
>  }
>  
>  static const struct of_device_id ad7192_of_match[] = {
> -	{ .compatible = "adi,ad7190", .data = (void *)ID_AD7190 },
> -	{ .compatible = "adi,ad7192", .data = (void *)ID_AD7192 },
> -	{ .compatible = "adi,ad7193", .data = (void *)ID_AD7193 },
> -	{ .compatible = "adi,ad7195", .data = (void *)ID_AD7195 },
> +	{ .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
> +	{ .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
> +	{ .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
> +	{ .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, ad7192_of_match);
> @@ -938,16 +969,16 @@ static int ad7192_probe(struct spi_device *spi)
>  	}
>  
>  	spi_set_drvdata(spi, indio_dev);
> -	st->devid = (unsigned long)of_device_get_match_data(&spi->dev);
> +	st->chip_info = of_device_get_match_data(&spi->dev);
>  	indio_dev->dev.parent = &spi->dev;
> -	indio_dev->name = spi_get_device_id(spi)->name;
> +	indio_dev->name = st->chip_info->name;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
>  	ret = ad7192_channels_config(indio_dev);
>  	if (ret < 0)
>  		goto error_disable_dvdd;
>  
> -	if (st->devid == ID_AD7195)
> +	if (st->chip_info->chip_id == CHIPID_AD7195)
>  		indio_dev->info = &ad7195_info;
>  	else
>  		indio_dev->info = &ad7192_info;

Re: [PATCH v3 2/2] iio: adc: ad7192: move ad7192_of_match table closer to the end of the file

[Jonathan Cameron]

On Mon, 13 Apr 2020 11:20:44 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> The change is more cosmetic. There is no need to reference this table in
> the probe function since 'of_device_get_match_data' is used, which obtains
> this information from the driver object.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
I'm obviously not going to take this one as a fix.  It's a reasonable patch
so feel free to remind me of it's existence once the fix has worked its
way around to be in my togreg/testing branch.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ad7192.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 7e8662c5cb0e..cef61f962e60 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -908,15 +908,6 @@ static int ad7192_channels_config(struct iio_dev *indio_dev)
>  	return 0;
>  }
>  
> -static const struct of_device_id ad7192_of_match[] = {
> -	{ .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
> -	{ .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
> -	{ .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
> -	{ .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
> -	{}
> -};
> -MODULE_DEVICE_TABLE(of, ad7192_of_match);
> -
>  static int ad7192_probe(struct spi_device *spi)
>  {
>  	struct ad7192_state *st;
> @@ -1050,6 +1041,15 @@ static int ad7192_remove(struct spi_device *spi)
>  	return 0;
>  }
>  
> +static const struct of_device_id ad7192_of_match[] = {
> +	{ .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
> +	{ .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
> +	{ .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
> +	{ .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, ad7192_of_match);
> +
>  static struct spi_driver ad7192_driver = {
>  	.driver = {
>  		.name	= "ad7192",

Re: [PATCH v3 1/2] iio: adc: ad7192: fix null pointer de-reference crash during probe

[Jeremy Fertic]

On Mon, Apr 13, 2020 at 11:20:43AM +0300, Alexandru Ardelean wrote:
> When the 'spi_device_id' table was removed, it omitted to cleanup/fix the
> assignment:
>    'indio_dev->name = spi_get_device_id(spi)->name;'
> 
> After that patch 'spi_get_device_id(spi)' returns NULL, so this crashes
> during probe with null de-ref.
> 
> This change fixes this by introducing an ad7192_chip_info struct, and
> defines all part-names [that should be assigned to indio_dev->name] in a
> 'ad7192_chip_info_tbl' table.
> 
> With this change, the old 'st->devid' is also moved to be a
> 'chip_info->chip_id'. And the old 'ID_AD719X' macros have been renamed to
> 'CHIPID_AD719X'. Tld identifiers have been re-purposed to be enum/index
> values in the new 'ad7192_chip_info_tbl'.
> 
> This should fix the bug, and maintain the ABI for the 'indio_dev->name'
> field.
> 
> Fixes: 66614ab2be38 ("staging: iio: adc: ad7192: removed spi_device_id")
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> 
> Changelog v2 -> v3:
> * reworked patch to introduce a chip_info struct for the part-name
> * added 2nd patch to move of-table closer to the end of the file; this
>   patch is more cosmetic; has no fixes tag, but is on top of the previous
> 
> Changelog v1 -> v2:
> * fix colon for Fixes tag
> * updated commit title a bit; to make it longer
> 
>  drivers/iio/adc/ad7192.c | 61 ++++++++++++++++++++++++++++++----------
>  1 file changed, 46 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 8ec28aa8fa8a..7e8662c5cb0e 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -125,10 +125,10 @@
>  #define AD7193_CH_AINCOM	0x600 /* AINCOM - AINCOM */
>  
>  /* ID Register Bit Designations (AD7192_REG_ID) */
> -#define ID_AD7190		0x4
> -#define ID_AD7192		0x0
> -#define ID_AD7193		0x2
> -#define ID_AD7195		0x6
> +#define CHIPID_AD7190		0x4
> +#define CHIPID_AD7192		0x0
> +#define CHIPID_AD7193		0x2
> +#define CHIPID_AD7195		0x6
>  #define AD7192_ID_MASK		0x0F
>  
>  /* GPOCON Register Bit Designations (AD7192_REG_GPOCON) */
> @@ -161,7 +161,20 @@ enum {
>     AD7192_SYSCALIB_FULL_SCALE,
>  };
>  
> +enum {
> +	ID_AD7190,
> +	ID_AD7192,
> +	ID_AD7193,
> +	ID_AD7195,
> +};
> +
> +struct ad7192_chip_info {
> +	unsigned int			chip_id;
> +	const char			*name;
> +};
> +
>  struct ad7192_state {
> +	const struct ad7192_chip_info	*chip_info;
>  	struct regulator		*avdd;
>  	struct regulator		*dvdd;
>  	struct clk			*mclk;
> @@ -172,7 +185,6 @@ struct ad7192_state {
>  	u32				conf;
>  	u32				scale_avail[8][2];
>  	u8				gpocon;
> -	u8				devid;
>  	u8				clock_sel;
>  	struct mutex			lock;	/* protect sensor state */
>  	u8				syscalib_mode[8];
> @@ -348,7 +360,7 @@ static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
>  
>  	id &= AD7192_ID_MASK;
>  
> -	if (id != st->devid)
> +	if (id != st->chip_info->chip_id)
>  		dev_warn(&st->sd.spi->dev, "device ID query failed (0x%X)\n",
>  			 id);
>  
> @@ -363,7 +375,7 @@ static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
>  		st->mode |= AD7192_MODE_REJ60;
>  
>  	refin2_en = of_property_read_bool(np, "adi,refin2-pins-enable");
> -	if (refin2_en && st->devid != ID_AD7195)
> +	if (refin2_en && st->chip_info->chip_id != CHIPID_AD7195)
>  		st->conf |= AD7192_CONF_REFSEL;
>  
>  	st->conf &= ~AD7192_CONF_CHOP;
> @@ -859,11 +871,30 @@ static const struct iio_chan_spec ad7193_channels[] = {
>  	IIO_CHAN_SOFT_TIMESTAMP(14),
>  };
>  
> +static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
> +	[ID_AD7190] = {
> +		.chip_id = CHIPID_AD7190,
> +		.name = "ad7190",
> +	},
> +	[ID_AD7192] = {
> +		.chip_id = CHIPID_AD7192,
> +		.name = "ad7192",
> +	},
> +	[ID_AD7193] = {
> +		.chip_id = CHIPID_AD7193,
> +		.name = "ad7193",
> +	},
> +	[ID_AD7195] = {
> +		.chip_id = CHIPID_AD7195,
> +		.name = "ad7195",
> +	},
> +};
> +
>  static int ad7192_channels_config(struct iio_dev *indio_dev)
>  {
>  	struct ad7192_state *st = iio_priv(indio_dev);
>  
> -	switch (st->devid) {
> +	switch (st->chip_info->chip_id) {
>  	case ID_AD7193:

Would it be better to change this to CHIPID_AD7193? It looks like it works
the way you have it, but only by coincidence since it happens to be true
that ID_AD7193 == CHIPID_AD7193.

Jeremy

>  		indio_dev->channels = ad7193_channels;
>  		indio_dev->num_channels = ARRAY_SIZE(ad7193_channels);
> @@ -878,10 +909,10 @@ static int ad7192_channels_config(struct iio_dev *indio_dev)
>  }
>  
>  static const struct of_device_id ad7192_of_match[] = {
> -	{ .compatible = "adi,ad7190", .data = (void *)ID_AD7190 },
> -	{ .compatible = "adi,ad7192", .data = (void *)ID_AD7192 },
> -	{ .compatible = "adi,ad7193", .data = (void *)ID_AD7193 },
> -	{ .compatible = "adi,ad7195", .data = (void *)ID_AD7195 },
> +	{ .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
> +	{ .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
> +	{ .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
> +	{ .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, ad7192_of_match);
> @@ -938,16 +969,16 @@ static int ad7192_probe(struct spi_device *spi)
>  	}
>  
>  	spi_set_drvdata(spi, indio_dev);
> -	st->devid = (unsigned long)of_device_get_match_data(&spi->dev);
> +	st->chip_info = of_device_get_match_data(&spi->dev);
>  	indio_dev->dev.parent = &spi->dev;
> -	indio_dev->name = spi_get_device_id(spi)->name;
> +	indio_dev->name = st->chip_info->name;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
>  	ret = ad7192_channels_config(indio_dev);
>  	if (ret < 0)
>  		goto error_disable_dvdd;
>  
> -	if (st->devid == ID_AD7195)
> +	if (st->chip_info->chip_id == CHIPID_AD7195)
>  		indio_dev->info = &ad7195_info;
>  	else
>  		indio_dev->info = &ad7192_info;
> -- 
> 2.17.1
> 

Re: [PATCH v3 2/2] iio: adc: ad7192: move ad7192_of_match table closer to the end of the file

[Ardelean, Alexandru]

On Mon, 2020-04-13 at 17:14 +0100, Jonathan Cameron wrote:
> On Mon, 13 Apr 2020 11:20:44 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> 
> > The change is more cosmetic. There is no need to reference this table in
> > the probe function since 'of_device_get_match_data' is used, which obtains
> > this information from the driver object.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> I'm obviously not going to take this one as a fix.  It's a reasonable patch
> so feel free to remind me of it's existence once the fix has worked its
> way around to be in my togreg/testing branch.

Ack.

Thanks
Alex

> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/adc/ad7192.c | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> > index 7e8662c5cb0e..cef61f962e60 100644
> > --- a/drivers/iio/adc/ad7192.c
> > +++ b/drivers/iio/adc/ad7192.c
> > @@ -908,15 +908,6 @@ static int ad7192_channels_config(struct iio_dev
> > *indio_dev)
> >  	return 0;
> >  }
> >  
> > -static const struct of_device_id ad7192_of_match[] = {
> > -	{ .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190]
> > },
> > -	{ .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192]
> > },
> > -	{ .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193]
> > },
> > -	{ .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195]
> > },
> > -	{}
> > -};
> > -MODULE_DEVICE_TABLE(of, ad7192_of_match);
> > -
> >  static int ad7192_probe(struct spi_device *spi)
> >  {
> >  	struct ad7192_state *st;
> > @@ -1050,6 +1041,15 @@ static int ad7192_remove(struct spi_device *spi)
> >  	return 0;
> >  }
> >  
> > +static const struct of_device_id ad7192_of_match[] = {
> > +	{ .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190]
> > },
> > +	{ .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192]
> > },
> > +	{ .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193]
> > },
> > +	{ .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195]
> > },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, ad7192_of_match);
> > +
> >  static struct spi_driver ad7192_driver = {
> >  	.driver = {
> >  		.name	= "ad7192",

Re: [PATCH v3 1/2] iio: adc: ad7192: fix null pointer de-reference crash during probe

[Jonathan Cameron]

On Mon, 13 Apr 2020 16:06:31 -0600
Jeremy Fertic <jeremyfertic@gmail.com> wrote:

> On Mon, Apr 13, 2020 at 11:20:43AM +0300, Alexandru Ardelean wrote:
> > When the 'spi_device_id' table was removed, it omitted to cleanup/fix the
> > assignment:
> >    'indio_dev->name = spi_get_device_id(spi)->name;'
> > 
> > After that patch 'spi_get_device_id(spi)' returns NULL, so this crashes
> > during probe with null de-ref.
> > 
> > This change fixes this by introducing an ad7192_chip_info struct, and
> > defines all part-names [that should be assigned to indio_dev->name] in a
> > 'ad7192_chip_info_tbl' table.
> > 
> > With this change, the old 'st->devid' is also moved to be a
> > 'chip_info->chip_id'. And the old 'ID_AD719X' macros have been renamed to
> > 'CHIPID_AD719X'. Tld identifiers have been re-purposed to be enum/index
> > values in the new 'ad7192_chip_info_tbl'.
> > 
> > This should fix the bug, and maintain the ABI for the 'indio_dev->name'
> > field.
> > 
> > Fixes: 66614ab2be38 ("staging: iio: adc: ad7192: removed spi_device_id")
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> > 
> > Changelog v2 -> v3:
> > * reworked patch to introduce a chip_info struct for the part-name
> > * added 2nd patch to move of-table closer to the end of the file; this
> >   patch is more cosmetic; has no fixes tag, but is on top of the previous
> > 
> > Changelog v1 -> v2:
> > * fix colon for Fixes tag
> > * updated commit title a bit; to make it longer
> > 
> >  drivers/iio/adc/ad7192.c | 61 ++++++++++++++++++++++++++++++----------
> >  1 file changed, 46 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> > index 8ec28aa8fa8a..7e8662c5cb0e 100644
> > --- a/drivers/iio/adc/ad7192.c
> > +++ b/drivers/iio/adc/ad7192.c
> > @@ -125,10 +125,10 @@
> >  #define AD7193_CH_AINCOM	0x600 /* AINCOM - AINCOM */
> >  
> >  /* ID Register Bit Designations (AD7192_REG_ID) */
> > -#define ID_AD7190		0x4
> > -#define ID_AD7192		0x0
> > -#define ID_AD7193		0x2
> > -#define ID_AD7195		0x6
> > +#define CHIPID_AD7190		0x4
> > +#define CHIPID_AD7192		0x0
> > +#define CHIPID_AD7193		0x2
> > +#define CHIPID_AD7195		0x6
> >  #define AD7192_ID_MASK		0x0F
> >  
> >  /* GPOCON Register Bit Designations (AD7192_REG_GPOCON) */
> > @@ -161,7 +161,20 @@ enum {
> >     AD7192_SYSCALIB_FULL_SCALE,
> >  };
> >  
> > +enum {
> > +	ID_AD7190,
> > +	ID_AD7192,
> > +	ID_AD7193,
> > +	ID_AD7195,
> > +};
> > +
> > +struct ad7192_chip_info {
> > +	unsigned int			chip_id;
> > +	const char			*name;
> > +};
> > +
> >  struct ad7192_state {
> > +	const struct ad7192_chip_info	*chip_info;
> >  	struct regulator		*avdd;
> >  	struct regulator		*dvdd;
> >  	struct clk			*mclk;
> > @@ -172,7 +185,6 @@ struct ad7192_state {
> >  	u32				conf;
> >  	u32				scale_avail[8][2];
> >  	u8				gpocon;
> > -	u8				devid;
> >  	u8				clock_sel;
> >  	struct mutex			lock;	/* protect sensor state */
> >  	u8				syscalib_mode[8];
> > @@ -348,7 +360,7 @@ static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
> >  
> >  	id &= AD7192_ID_MASK;
> >  
> > -	if (id != st->devid)
> > +	if (id != st->chip_info->chip_id)
> >  		dev_warn(&st->sd.spi->dev, "device ID query failed (0x%X)\n",
> >  			 id);
> >  
> > @@ -363,7 +375,7 @@ static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
> >  		st->mode |= AD7192_MODE_REJ60;
> >  
> >  	refin2_en = of_property_read_bool(np, "adi,refin2-pins-enable");
> > -	if (refin2_en && st->devid != ID_AD7195)
> > +	if (refin2_en && st->chip_info->chip_id != CHIPID_AD7195)
> >  		st->conf |= AD7192_CONF_REFSEL;
> >  
> >  	st->conf &= ~AD7192_CONF_CHOP;
> > @@ -859,11 +871,30 @@ static const struct iio_chan_spec ad7193_channels[] = {
> >  	IIO_CHAN_SOFT_TIMESTAMP(14),
> >  };
> >  
> > +static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
> > +	[ID_AD7190] = {
> > +		.chip_id = CHIPID_AD7190,
> > +		.name = "ad7190",
> > +	},
> > +	[ID_AD7192] = {
> > +		.chip_id = CHIPID_AD7192,
> > +		.name = "ad7192",
> > +	},
> > +	[ID_AD7193] = {
> > +		.chip_id = CHIPID_AD7193,
> > +		.name = "ad7193",
> > +	},
> > +	[ID_AD7195] = {
> > +		.chip_id = CHIPID_AD7195,
> > +		.name = "ad7195",
> > +	},
> > +};
> > +
> >  static int ad7192_channels_config(struct iio_dev *indio_dev)
> >  {
> >  	struct ad7192_state *st = iio_priv(indio_dev);
> >  
> > -	switch (st->devid) {
> > +	switch (st->chip_info->chip_id) {
> >  	case ID_AD7193:  
> 
> Would it be better to change this to CHIPID_AD7193? It looks like it works
> the way you have it, but only by coincidence since it happens to be true
> that ID_AD7193 == CHIPID_AD7193.
>
dropped patch from my fixes branch whilst this is resolved!

Thanks,

Jonathan
 
> Jeremy
> 
> >  		indio_dev->channels = ad7193_channels;
> >  		indio_dev->num_channels = ARRAY_SIZE(ad7193_channels);
> > @@ -878,10 +909,10 @@ static int ad7192_channels_config(struct iio_dev *indio_dev)
> >  }
> >  
> >  static const struct of_device_id ad7192_of_match[] = {
> > -	{ .compatible = "adi,ad7190", .data = (void *)ID_AD7190 },
> > -	{ .compatible = "adi,ad7192", .data = (void *)ID_AD7192 },
> > -	{ .compatible = "adi,ad7193", .data = (void *)ID_AD7193 },
> > -	{ .compatible = "adi,ad7195", .data = (void *)ID_AD7195 },
> > +	{ .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
> > +	{ .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
> > +	{ .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
> > +	{ .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
> >  	{}
> >  };
> >  MODULE_DEVICE_TABLE(of, ad7192_of_match);
> > @@ -938,16 +969,16 @@ static int ad7192_probe(struct spi_device *spi)
> >  	}
> >  
> >  	spi_set_drvdata(spi, indio_dev);
> > -	st->devid = (unsigned long)of_device_get_match_data(&spi->dev);
> > +	st->chip_info = of_device_get_match_data(&spi->dev);
> >  	indio_dev->dev.parent = &spi->dev;
> > -	indio_dev->name = spi_get_device_id(spi)->name;
> > +	indio_dev->name = st->chip_info->name;
> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> >  
> >  	ret = ad7192_channels_config(indio_dev);
> >  	if (ret < 0)
> >  		goto error_disable_dvdd;
> >  
> > -	if (st->devid == ID_AD7195)
> > +	if (st->chip_info->chip_id == CHIPID_AD7195)
> >  		indio_dev->info = &ad7195_info;
> >  	else
> >  		indio_dev->info = &ad7192_info;
> > -- 
> > 2.17.1
> >   

Re: [PATCH v3 1/2] iio: adc: ad7192: fix null pointer de-reference crash during probe

[Ardelean, Alexandru]

On Mon, 2020-04-13 at 16:06 -0600, Jeremy Fertic wrote:
> [External]
> 
> On Mon, Apr 13, 2020 at 11:20:43AM +0300, Alexandru Ardelean wrote:
> > When the 'spi_device_id' table was removed, it omitted to cleanup/fix the
> > assignment:
> >    'indio_dev->name = spi_get_device_id(spi)->name;'
> > 
> > After that patch 'spi_get_device_id(spi)' returns NULL, so this crashes
> > during probe with null de-ref.
> > 
> > This change fixes this by introducing an ad7192_chip_info struct, and
> > defines all part-names [that should be assigned to indio_dev->name] in a
> > 'ad7192_chip_info_tbl' table.
> > 
> > With this change, the old 'st->devid' is also moved to be a
> > 'chip_info->chip_id'. And the old 'ID_AD719X' macros have been renamed to
> > 'CHIPID_AD719X'. Tld identifiers have been re-purposed to be enum/index
> > values in the new 'ad7192_chip_info_tbl'.
> > 
> > This should fix the bug, and maintain the ABI for the 'indio_dev->name'
> > field.
> > 
> > Fixes: 66614ab2be38 ("staging: iio: adc: ad7192: removed spi_device_id")
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> > 
> > Changelog v2 -> v3:
> > * reworked patch to introduce a chip_info struct for the part-name
> > * added 2nd patch to move of-table closer to the end of the file; this
> >   patch is more cosmetic; has no fixes tag, but is on top of the previous
> > 
> > Changelog v1 -> v2:
> > * fix colon for Fixes tag
> > * updated commit title a bit; to make it longer
> > 
> >  drivers/iio/adc/ad7192.c | 61 ++++++++++++++++++++++++++++++----------
> >  1 file changed, 46 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> > index 8ec28aa8fa8a..7e8662c5cb0e 100644
> > --- a/drivers/iio/adc/ad7192.c
> > +++ b/drivers/iio/adc/ad7192.c
> > @@ -125,10 +125,10 @@
> >  #define AD7193_CH_AINCOM	0x600 /* AINCOM - AINCOM */
> >  
> >  /* ID Register Bit Designations (AD7192_REG_ID) */
> > -#define ID_AD7190		0x4
> > -#define ID_AD7192		0x0
> > -#define ID_AD7193		0x2
> > -#define ID_AD7195		0x6
> > +#define CHIPID_AD7190		0x4
> > +#define CHIPID_AD7192		0x0
> > +#define CHIPID_AD7193		0x2
> > +#define CHIPID_AD7195		0x6
> >  #define AD7192_ID_MASK		0x0F
> >  
> >  /* GPOCON Register Bit Designations (AD7192_REG_GPOCON) */
> > @@ -161,7 +161,20 @@ enum {
> >     AD7192_SYSCALIB_FULL_SCALE,
> >  };
> >  
> > +enum {
> > +	ID_AD7190,
> > +	ID_AD7192,
> > +	ID_AD7193,
> > +	ID_AD7195,
> > +};
> > +
> > +struct ad7192_chip_info {
> > +	unsigned int			chip_id;
> > +	const char			*name;
> > +};
> > +
> >  struct ad7192_state {
> > +	const struct ad7192_chip_info	*chip_info;
> >  	struct regulator		*avdd;
> >  	struct regulator		*dvdd;
> >  	struct clk			*mclk;
> > @@ -172,7 +185,6 @@ struct ad7192_state {
> >  	u32				conf;
> >  	u32				scale_avail[8][2];
> >  	u8				gpocon;
> > -	u8				devid;
> >  	u8				clock_sel;
> >  	struct mutex			lock;	/* protect sensor state */
> >  	u8				syscalib_mode[8];
> > @@ -348,7 +360,7 @@ static int ad7192_setup(struct ad7192_state *st, struct
> > device_node *np)
> >  
> >  	id &= AD7192_ID_MASK;
> >  
> > -	if (id != st->devid)
> > +	if (id != st->chip_info->chip_id)
> >  		dev_warn(&st->sd.spi->dev, "device ID query failed (0x%X)\n",
> >  			 id);
> >  
> > @@ -363,7 +375,7 @@ static int ad7192_setup(struct ad7192_state *st, struct
> > device_node *np)
> >  		st->mode |= AD7192_MODE_REJ60;
> >  
> >  	refin2_en = of_property_read_bool(np, "adi,refin2-pins-enable");
> > -	if (refin2_en && st->devid != ID_AD7195)
> > +	if (refin2_en && st->chip_info->chip_id != CHIPID_AD7195)
> >  		st->conf |= AD7192_CONF_REFSEL;
> >  
> >  	st->conf &= ~AD7192_CONF_CHOP;
> > @@ -859,11 +871,30 @@ static const struct iio_chan_spec ad7193_channels[] =
> > {
> >  	IIO_CHAN_SOFT_TIMESTAMP(14),
> >  };
> >  
> > +static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
> > +	[ID_AD7190] = {
> > +		.chip_id = CHIPID_AD7190,
> > +		.name = "ad7190",
> > +	},
> > +	[ID_AD7192] = {
> > +		.chip_id = CHIPID_AD7192,
> > +		.name = "ad7192",
> > +	},
> > +	[ID_AD7193] = {
> > +		.chip_id = CHIPID_AD7193,
> > +		.name = "ad7193",
> > +	},
> > +	[ID_AD7195] = {
> > +		.chip_id = CHIPID_AD7195,
> > +		.name = "ad7195",
> > +	},
> > +};
> > +
> >  static int ad7192_channels_config(struct iio_dev *indio_dev)
> >  {
> >  	struct ad7192_state *st = iio_priv(indio_dev);
> >  
> > -	switch (st->devid) {
> > +	switch (st->chip_info->chip_id) {
> >  	case ID_AD7193:
> 
> Would it be better to change this to CHIPID_AD7193? It looks like it works
> the way you have it, but only by coincidence since it happens to be true
> that ID_AD7193 == CHIPID_AD7193.

Thanks for catching this.
I'll fix up in a V3.


> 
> Jeremy
> 
> >  		indio_dev->channels = ad7193_channels;
> >  		indio_dev->num_channels = ARRAY_SIZE(ad7193_channels);
> > @@ -878,10 +909,10 @@ static int ad7192_channels_config(struct iio_dev
> > *indio_dev)
> >  }
> >  
> >  static const struct of_device_id ad7192_of_match[] = {
> > -	{ .compatible = "adi,ad7190", .data = (void *)ID_AD7190 },
> > -	{ .compatible = "adi,ad7192", .data = (void *)ID_AD7192 },
> > -	{ .compatible = "adi,ad7193", .data = (void *)ID_AD7193 },
> > -	{ .compatible = "adi,ad7195", .data = (void *)ID_AD7195 },
> > +	{ .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190]
> > },
> > +	{ .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192]
> > },
> > +	{ .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193]
> > },
> > +	{ .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195]
> > },
> >  	{}
> >  };
> >  MODULE_DEVICE_TABLE(of, ad7192_of_match);
> > @@ -938,16 +969,16 @@ static int ad7192_probe(struct spi_device *spi)
> >  	}
> >  
> >  	spi_set_drvdata(spi, indio_dev);
> > -	st->devid = (unsigned long)of_device_get_match_data(&spi->dev);
> > +	st->chip_info = of_device_get_match_data(&spi->dev);
> >  	indio_dev->dev.parent = &spi->dev;
> > -	indio_dev->name = spi_get_device_id(spi)->name;
> > +	indio_dev->name = st->chip_info->name;
> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> >  
> >  	ret = ad7192_channels_config(indio_dev);
> >  	if (ret < 0)
> >  		goto error_disable_dvdd;
> >  
> > -	if (st->devid == ID_AD7195)
> > +	if (st->chip_info->chip_id == CHIPID_AD7195)
> >  		indio_dev->info = &ad7195_info;
> >  	else
> >  		indio_dev->info = &ad7192_info;
> > -- 
> > 2.17.1
> >