[PATCH v2 0/4] iio: initialise ddata for iio_get_acpi_device_name_and_data()

[PATCH v2 0/4] iio: initialise ddata for iio_get_acpi_device_name_and_data()

[Andy Shevchenko]

Dan reported that ddata might be used uninitialised in some cases.
Let's initialise it to NULL (patches 1 - 3). With that, update one driver
to drop an unneeded anymore check (included in patch 3).

While at it, one more cleanup to kxcjk-1013 (patch 4) is added.

Jonathan, dunno if you want to rebase at this stage (probably not),
but if you do, feel free to fold the patches 1-3 to the initial code.

In v2:
- rewritten patches 1-3 to do the job on the caller side (Jonathan)

Andy Shevchenko (4):
  iio: light: ltr501: Assing ddata to NULL
  iio: light: isl29018: Assing ddata to NULL
  iio: accel: kxcjk-1013: Assing ddata to NULL instead of NULL check
  iio: accel: kxcjk-1013: Deduplicate ODR startup time array

 drivers/iio/accel/kxcjk-1013.c | 30 ++++++------------------------
 drivers/iio/light/isl29018.c   |  2 +-
 drivers/iio/light/ltr501.c     |  2 +-
 3 files changed, 8 insertions(+), 26 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 1/4] iio: light: ltr501: Assing ddata to NULL

[Andy Shevchenko]

When iio_get_acpi_device_name_and_data() fails, the ddata may be left
uninitialised. Initialise it to NULL.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/54fac4a7-b601-40ce-8c00-d94807f5e214@stanley.mountain
Fixes: 61da460b813d ("iio: light: ltr501: Replace a variant of iio_get_acpi_device_name_and_data()")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/light/ltr501.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
index 7228611d4c59..616dc6921702 100644
--- a/drivers/iio/light/ltr501.c
+++ b/drivers/iio/light/ltr501.c
@@ -1424,8 +1424,8 @@ static int ltr501_probe(struct i2c_client *client)
 	struct ltr501_data *data;
 	struct iio_dev *indio_dev;
 	struct regmap *regmap;
+	const void *ddata = NULL;
 	int partid, chip_idx;
-	const void *ddata;
 	const char *name;
 	int ret;
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v2 1/4] iio: light: ltr501: Assing ddata to NULL

[Markus Elfring]

> When iio_get_acpi_device_name_and_data() fails, the ddata may be left
> uninitialised. Initialise it to NULL.

Would you like to use the summary phrase “Assign NULL to ddata in ltr501_probe()” accordingly?

Regards,
Markus

Re: [PATCH v2 1/4] iio: light: ltr501: Assing ddata to NULL

[Markus Elfring]

> When iio_get_acpi_device_name_and_data() fails, the ddata may be left
> uninitialised. Initialise it to NULL.

How do you think about to perform the variable assignment only in
a corresponding else branch?

Can it be that this adjustment does not really matter here because of
the following statement?

	if (!name)
		return -ENODEV;


Regards,
Markus

[PATCH v2 2/4] iio: light: isl29018: Assing ddata to NULL

[Andy Shevchenko]

When iio_get_acpi_device_name_and_data() fails, the ddata may be left
uninitialised. Initialise it to NULL.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/54fac4a7-b601-40ce-8c00-d94807f5e214@stanley.mountain
Fixes: 14686836fb69 ("iio: light: isl29018: Replace a variant of iio_get_acpi_device_name_and_data()")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/light/isl29018.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/isl29018.c b/drivers/iio/light/isl29018.c
index cbe34026bda6..201eae1c4589 100644
--- a/drivers/iio/light/isl29018.c
+++ b/drivers/iio/light/isl29018.c
@@ -703,7 +703,7 @@ static int isl29018_probe(struct i2c_client *client)
 	const struct i2c_device_id *id = i2c_client_get_device_id(client);
 	struct isl29018_chip *chip;
 	struct iio_dev *indio_dev;
-	const void *ddata;
+	const void *ddata = NULL;
 	const char *name;
 	int dev_id;
 	int err;
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v2 2/4] iio: light: isl29018: Assing ddata to NULL

[Markus Elfring]

> When iio_get_acpi_device_name_and_data() fails, the ddata may be left
> uninitialised. Initialise it to NULL.

Would a summary phrase like “Assign NULL to ddata in isl29018_probe()” be helpful?


How do you think about a null pointer check also with the local variable “name” instead?

Regards,
Markus

[PATCH v2 3/4] iio: accel: kxcjk-1013: Assing ddata to NULL instead of NULL check

[Andy Shevchenko]

When iio_get_acpi_device_name_and_data() fails, the ddata may be left
uninitialised. Initialise it to NULL and drop unneeded NULL check
in kxcjk1013_probe()

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/accel/kxcjk-1013.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index 28ed0e09d099..753ec2f71a9a 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -1462,7 +1462,7 @@ static int kxcjk1013_probe(struct i2c_client *client)
 	struct kxcjk1013_data *data;
 	struct iio_dev *indio_dev;
 	struct kxcjk_1013_platform_data *pdata;
-	const void *ddata;
+	const void *ddata = NULL;
 	const char *name;
 	int ret;
 
@@ -1507,8 +1507,7 @@ static int kxcjk1013_probe(struct i2c_client *client)
 		data->info = (const struct kx_chipset_info *)(id->driver_data);
 	} else {
 		name = iio_get_acpi_device_name_and_data(&client->dev, &ddata);
-		if (name)
-			data->info = ddata;
+		data->info = ddata;
 		if (data->info == &kxcj91008_kiox010a_info)
 			indio_dev->label = "accel-display";
 		else if (data->info == &kxcj91008_kiox020a_info)
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v2 3/4] iio: accel: kxcjk-1013: Assing ddata to NULL instead of NULL check

[Markus Elfring]

> When iio_get_acpi_device_name_and_data() fails, the ddata may be left
> uninitialised. Initialise it to NULL and drop unneeded NULL check
> in kxcjk1013_probe()

I propose to reduce the scope for this local variable.


How do you think about to use a summary phrase like “Replace a pointer check
by a variable initialisation in kxcjk1013_probe()” instead?

Regards,
Markus

[PATCH v2 4/4] iio: accel: kxcjk-1013: Deduplicate ODR startup time array

[Andy Shevchenko]

The content of kxcj91008_odr_start_up_times and kxcjk1013_odr_start_up_times
is identical, deduplicate it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/accel/kxcjk-1013.c | 25 ++++---------------------
 1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index 753ec2f71a9a..f65fde06f2c1 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -193,23 +193,6 @@ static const struct kx_odr_start_up_time kxcjk1013_odr_start_up_times[] = {
 	{ }
 };
 
-/* KXCJ9-1008 */
-static const struct kx_odr_start_up_time kxcj91008_odr_start_up_times[] = {
-	{ 0x08, 100000 },
-	{ 0x09, 100000 },
-	{ 0x0A, 100000 },
-	{ 0x0B, 100000 },
-	{ 0x00, 80000 },
-	{ 0x01, 41000 },
-	{ 0x02, 21000 },
-	{ 0x03, 11000 },
-	{ 0x04, 6400 },
-	{ 0x05, 3900 },
-	{ 0x06, 2700 },
-	{ 0x07, 2100 },
-	{ }
-};
-
 /* KXCTJ2-1009 */
 static const struct kx_odr_start_up_time kxtj21009_odr_start_up_times[] = {
 	{ 0x08, 1240000 },
@@ -325,24 +308,24 @@ static const struct kx_chipset_info kxcjk1013_info = {
 
 static const struct kx_chipset_info kxcj91008_info = {
 	.regs = &kxcjk1013_regs,
-	.times = pm_ptr(kxcj91008_odr_start_up_times),
+	.times = pm_ptr(kxcjk1013_odr_start_up_times),
 };
 
 static const struct kx_chipset_info kxcj91008_kiox010a_info = {
 	.regs = &kxcjk1013_regs,
-	.times = pm_ptr(kxcj91008_odr_start_up_times),
+	.times = pm_ptr(kxcjk1013_odr_start_up_times),
 	.acpi_type = ACPI_KIOX010A,
 };
 
 static const struct kx_chipset_info kxcj91008_kiox020a_info = {
 	.regs = &kxcjk1013_regs,
-	.times = pm_ptr(kxcj91008_odr_start_up_times),
+	.times = pm_ptr(kxcjk1013_odr_start_up_times),
 	.acpi_type = ACPI_GENERIC,
 };
 
 static const struct kx_chipset_info kxcj91008_smo8500_info = {
 	.regs = &kxcjk1013_regs,
-	.times = pm_ptr(kxcj91008_odr_start_up_times),
+	.times = pm_ptr(kxcjk1013_odr_start_up_times),
 	.acpi_type = ACPI_SMO8500,
 };
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v2 0/4] iio: initialise ddata for iio_get_acpi_device_name_and_data()

[Jonathan Cameron]

On Fri,  1 Nov 2024 10:08:25 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Dan reported that ddata might be used uninitialised in some cases.
> Let's initialise it to NULL (patches 1 - 3). With that, update one driver
> to drop an unneeded anymore check (included in patch 3).
> 
> While at it, one more cleanup to kxcjk-1013 (patch 4) is added.
> 
> Jonathan, dunno if you want to rebase at this stage (probably not),
> but if you do, feel free to fold the patches 1-3 to the initial code.
I had to rebase anyway for another reason so given that squashed those 3 in.

4 is on top of the tree.

Thanks,

Jonathan


> 
> In v2:
> - rewritten patches 1-3 to do the job on the caller side (Jonathan)
> 
> Andy Shevchenko (4):
>   iio: light: ltr501: Assing ddata to NULL
>   iio: light: isl29018: Assing ddata to NULL
>   iio: accel: kxcjk-1013: Assing ddata to NULL instead of NULL check
>   iio: accel: kxcjk-1013: Deduplicate ODR startup time array
> 
>  drivers/iio/accel/kxcjk-1013.c | 30 ++++++------------------------
>  drivers/iio/light/isl29018.c   |  2 +-
>  drivers/iio/light/ltr501.c     |  2 +-
>  3 files changed, 8 insertions(+), 26 deletions(-)
>