[PATCH v1 0/4] iio: acpi: always initialise data in iio_get_acpi_device_name_and_data()

[PATCH v1 0/4] iio: acpi: always initialise data in iio_get_acpi_device_name_and_data()

[Andy Shevchenko]

Dan reported that data might be used uninitialised in some cases.
Let's initialise it to NULL (patch 1). With that, update one driver
to drop an unneeded anymore check (patch 2). Another driver (patch 4)
gain almost a dead code — feel free to not apply it.

While at it, one more cleanup to kxcjk-1013 (patch 3) 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-2 to the initial code.
The rest seems to me like the independent patches.

Andy Shevchenko (4):
  iio: acpi: Fill data with NULL when
    iio_get_acpi_device_name_and_data() fails
  iio: accel: kxcjk-1013: Drop duplicate NULL check in kxcjk1013_probe()
  iio: accel: kxcjk-1013: Deduplicate ODR startup time array
  iio: light: isl29018: Check if name is valid in isl29018_probe()

 drivers/iio/accel/kxcjk-1013.c  | 28 +++++-----------------------
 drivers/iio/industrialio-acpi.c |  6 +++++-
 drivers/iio/light/isl29018.c    |  2 ++
 3 files changed, 12 insertions(+), 24 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 1/4] iio: acpi: Fill data with NULL when iio_get_acpi_device_name_and_data() fails

[Andy Shevchenko]

Fill data with NULL, if provided, when returning NULL from
iio_get_acpi_device_name_and_data(). Note, the current users check
for name to be valid, except one case which was initially doing
like that and has to be fixed separately.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/54fac4a7-b601-40ce-8c00-d94807f5e214@stanley.mountain
Fixes: dc60de4eb0a4 ("iio: acpi: Add iio_get_acpi_device_name_and_data() helper function")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/industrialio-acpi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-acpi.c b/drivers/iio/industrialio-acpi.c
index d67a43843799..33c737c7a2f8 100644
--- a/drivers/iio/industrialio-acpi.c
+++ b/drivers/iio/industrialio-acpi.c
@@ -102,13 +102,17 @@ EXPORT_SYMBOL_GPL(iio_read_acpi_mount_matrix);
  * NOTE: This helper function exists only for backward compatibility,
  * do not use in a new code!
  *
- * Returns: ACPI device instance name or %NULL.
+ * Returns: ACPI device instance name or %NULL When returning %NULL, the @data,
+ * if provided, will be also initialised to %NULL.
  */
 const char *iio_get_acpi_device_name_and_data(struct device *dev, const void **data)
 {
 	const struct acpi_device_id *id;
 	acpi_handle handle;
 
+	if (data)
+		*data = NULL;
+
 	handle = ACPI_HANDLE(dev);
 	if (!handle)
 		return NULL;
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 2/4] iio: accel: kxcjk-1013: Drop duplicate NULL check in kxcjk1013_probe()

[Andy Shevchenko]

Since iio_get_acpi_device_name_and_data() always initialises data,
if provided, there is no need to have this check in the caller.
Drop it.

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

diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index 28ed0e09d099..3d24d4fb6621 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -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

[PATCH v1 3/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 3d24d4fb6621..27c83c17931a 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

[PATCH v1 4/4] iio: light: isl29018: Check if name is valid in isl29018_probe()

[Andy Shevchenko]

Theoretically the name can be invalid if device has an ACPI handle
but hadn't been matched via ACPI ID table. This should never happen,
but let's make code very slightly more robust as some other drivers do.

Fixes: dc4ecaf21c4a ("staging: iio: light: isl29018: add ACPI support")
Depends-on: 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, 2 insertions(+)

diff --git a/drivers/iio/light/isl29018.c b/drivers/iio/light/isl29018.c
index cbe34026bda6..938fc19cfe59 100644
--- a/drivers/iio/light/isl29018.c
+++ b/drivers/iio/light/isl29018.c
@@ -723,6 +723,8 @@ static int isl29018_probe(struct i2c_client *client)
 		name = iio_get_acpi_device_name_and_data(&client->dev, &ddata);
 		dev_id = (intptr_t)ddata;
 	}
+	if (!name)
+		return -ENODEV;
 
 	mutex_init(&chip->lock);
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 1/4] iio: acpi: Fill data with NULL when iio_get_acpi_device_name_and_data() fails

[Jonathan Cameron]

On Wed, 30 Oct 2024 18:02:17 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Fill data with NULL, if provided, when returning NULL from
> iio_get_acpi_device_name_and_data(). Note, the current users check
> for name to be valid, except one case which was initially doing
> like that and has to be fixed separately.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/54fac4a7-b601-40ce-8c00-d94807f5e214@stanley.mountain
> Fixes: dc60de4eb0a4 ("iio: acpi: Add iio_get_acpi_device_name_and_data() helper function")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

This is counter intuitive as usual expectation would be no side effects on an
error return.  How hard to fix all the users to initialize to NULL if they
care about that?

There is still a chance we set it to NULL in here anyway, but that should only happen
if we know the return is good in the sense of no error (missing ACPI etc)
but not necessarily that dev_name() won't return NULL.

Don't think dev_name() can currently return NULL but 'maybe' it could...



> ---
>  drivers/iio/industrialio-acpi.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/industrialio-acpi.c b/drivers/iio/industrialio-acpi.c
> index d67a43843799..33c737c7a2f8 100644
> --- a/drivers/iio/industrialio-acpi.c
> +++ b/drivers/iio/industrialio-acpi.c
> @@ -102,13 +102,17 @@ EXPORT_SYMBOL_GPL(iio_read_acpi_mount_matrix);
>   * NOTE: This helper function exists only for backward compatibility,
>   * do not use in a new code!
>   *
> - * Returns: ACPI device instance name or %NULL.
> + * Returns: ACPI device instance name or %NULL When returning %NULL, the @data,
> + * if provided, will be also initialised to %NULL.
>   */
>  const char *iio_get_acpi_device_name_and_data(struct device *dev, const void **data)
>  {
>  	const struct acpi_device_id *id;
>  	acpi_handle handle;
>  
> +	if (data)
> +		*data = NULL;
> +
>  	handle = ACPI_HANDLE(dev);
>  	if (!handle)
>  		return NULL;

Re: [PATCH v1 1/4] iio: acpi: Fill data with NULL when iio_get_acpi_device_name_and_data() fails

[Andy Shevchenko]

On Thu, Oct 31, 2024 at 07:17:17PM +0000, Jonathan Cameron wrote:
> On Wed, 30 Oct 2024 18:02:17 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > Fill data with NULL, if provided, when returning NULL from
> > iio_get_acpi_device_name_and_data(). Note, the current users check
> > for name to be valid, except one case which was initially doing
> > like that and has to be fixed separately.
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/r/54fac4a7-b601-40ce-8c00-d94807f5e214@stanley.mountain
> > Fixes: dc60de4eb0a4 ("iio: acpi: Add iio_get_acpi_device_name_and_data() helper function")
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> This is counter intuitive as usual expectation would be no side effects on an
> error return.  How hard to fix all the users to initialize to NULL if they
> care about that?

v2 just has been sent, indeed the result looks much better, thanks for the review!

> There is still a chance we set it to NULL in here anyway, but that should only happen
> if we know the return is good in the sense of no error (missing ACPI etc)
> but not necessarily that dev_name() won't return NULL.
> 
> Don't think dev_name() can currently return NULL but 'maybe' it could...

dev_name() can't return NULL on the properly initialised device (either with
device_add(), or via dev_set_name() call). I do not think we can ever get to
the ->probe() without the above. Tell me, if I'm wrong.

-- 
With Best Regards,
Andy Shevchenko