[PATCH v3 0/3] fix fwnode_irq_get_byname() returnvalue

[PATCH v3 0/3] fix fwnode_irq_get_byname() returnvalue

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2161 bytes --]

The fix fwnode_irq_get_byname() may have returned zero if mapping the
IRQ fails. This contradicts the documentation. Furthermore, returning
zero or errno on error is unepected and can easily lead to problems
like:

int probe(foo)
{
...
	ret = fwnode_irq_get_byname(...);
	if (ret < 0)
		return ret;
...
}

or

int probe(foo)
{
...
	ret = fwnode_irq_get_byname(...);
	if (ret <= 0)
		return ret;
...
}

which are both likely to be wrong. First treats zero as successful call and
misses the IRQ mapping failure. Second returns zero from probe even though
it detects the IRQ mapping failure correvtly.

Here we change the fwnode_irq_get_byname() to always return a negative
errno upon failure. I have also audited following callers (v6.4-rc1):

drivers/i2c/i2c-smbus.c
drivers/iio/accel/adxl355_core.c
drivers/iio/accel/kionix-kx022a.c
drivers/iio/adc/ad4130.c
drivers/iio/adc/max11410.c
drivers/iio/addac/ad74115.c
drivers/iio/gyro/fxas21002c_core.c
drivers/iio/imu/adis16480.c
drivers/iio/imu/bmi160/bmi160_core.c
drivers/iio/imu/bmi160/bmi160_core.c

and it seems to me these calls will be Ok after the change. The
i2c-smbus.c and kionix-kx022a.c will gain a functional change (bugfix?) as
after this patch the probe will return -EINVAL should the IRQ mapping fail.
The series will also adjust the return value check for zero to be omitted.

---

Matti Vaittinen (3):
  drivers: fwnode: fix fwnode_irq_get_byname()
  i2c: i2c-smbus: fwnode_irq_get_byname() return value fix
  iio: kx022a fix irq getting

 drivers/base/property.c           | 9 +++++++--
 drivers/i2c/i2c-smbus.c           | 2 +-
 drivers/iio/accel/kionix-kx022a.c | 2 +-
 3 files changed, 9 insertions(+), 4 deletions(-)


base-commit: ac9a78681b921877518763ba0e89202254349d1b
-- 
2.40.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 1/3] drivers: fwnode: fix fwnode_irq_get_byname()

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2125 bytes --]

The fwnode_irq_get_byname() does return 0 upon device-tree IRQ mapping
failure. This is contradicting the function documentation and can
potentially be a source of errors like:

int probe(...) {
	...

	irq = fwnode_irq_get_byname();
	if (irq <= 0)
		return irq;

	...
}

Here we do correctly check the return value from fwnode_irq_get_byname()
but the driver probe will now return success. (There was already one
such user in-tree).

Change the fwnode_irq_get_byname() to work as documented and according to
the common convention and abd always return a negative errno upon failure.

Fixes: ca0acb511c21 ("device property: Add fwnode_irq_get_byname")
Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/base/property.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index f6117ec9805c..a3b95d2d781f 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1011,7 +1011,7 @@ EXPORT_SYMBOL(fwnode_irq_get);
  */
 int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name)
 {
-	int index;
+	int index, ret;
 
 	if (!name)
 		return -EINVAL;
@@ -1020,7 +1020,12 @@ int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name)
 	if (index < 0)
 		return index;
 
-	return fwnode_irq_get(fwnode, index);
+	ret = fwnode_irq_get(fwnode, index);
+	/* We treat mapping errors as invalid case */
+	if (ret == 0)
+		return -EINVAL;
+
+	return ret;
 }
 EXPORT_SYMBOL(fwnode_irq_get_byname);
 
-- 
2.40.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 2/3] i2c: i2c-smbus: fwnode_irq_get_byname() return value fix

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1846 bytes --]

The fwnode_irq_get_byname() was changed to not return 0 upon failure so
return value check can be adjusted to reflect the change.

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---

Depends on the mentioned return value change which is in patch 1/2. The
return value change does also cause a functional change here. Eg. when
IRQ mapping fails, the fwnode_irq_get_byname() no longer returns zero.
This will cause also the probe here to return nonzero failure. I guess
this is desired behaviour - but I would appreciate any confirmation.

Please, see also previous discussion here:
https://lore.kernel.org/all/fbd52f5f5253b382b8d7b3e8046134de29f965b8.1666710197.git.mazziesaccount@gmail.com/

Another suggestion has been to drop the check altogether. I am slightly
reluctant on doing that unless it gets confirmed that is the "right
thing to do".
---
 drivers/i2c/i2c-smbus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index 138c3f5e0093..893fe7cd3e41 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -129,7 +129,7 @@ static int smbalert_probe(struct i2c_client *ara)
 	} else {
 		irq = fwnode_irq_get_byname(dev_fwnode(adapter->dev.parent),
 					    "smbus_alert");
-		if (irq <= 0)
+		if (irq < 0)
 			return irq;
 	}
 
-- 
2.40.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 3/3] iio: kx022a fix irq getting

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2135 bytes --]

The fwnode_irq_get_byname() was returning 0 at device-tree mapping
error. If this occurred, the KX022A driver did abort the probe but
errorneously directly returned the return value from
fwnode_irq_get_byname() from probe. In case of a device-tree mapping
error this indicated success.

The fwnode_irq_get_byname() has since been fixed to not return zero on
error so the check for fwnode_irq_get_byname() can be relaxed to only
treat negative values as errors. This will also do decent fix even when
backported to branches where fwnode_irq_get_byname() can still return
zero on error because KX022A probe should later fail at IRQ requesting
and a prober error handling should follow.

Relax the return value check for fwnode_irq_get_byname() to only treat
negative values as errors.

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <error27@gmail.com>
Closes: https://lore.kernel.org/r/202305110245.MFxC9bUj-lkp@intel.com/
Link: https://lore.kernel.org/r/202305110245.MFxC9bUj-lkp@intel.com/
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Fixes: 7c1d1677b322 ("iio: accel: Support Kionix/ROHM KX022A accelerometer")
---
 drivers/iio/accel/kionix-kx022a.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
index f98393d74666..b8636fa8eaeb 100644
--- a/drivers/iio/accel/kionix-kx022a.c
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -1048,7 +1048,7 @@ int kx022a_probe_internal(struct device *dev)
 		data->ien_reg = KX022A_REG_INC4;
 	} else {
 		irq = fwnode_irq_get_byname(fwnode, "INT2");
-		if (irq <= 0)
+		if (irq < 0)
 			return dev_err_probe(dev, irq, "No suitable IRQ\n");
 
 		data->inc_reg = KX022A_REG_INC5;
-- 
2.40.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/3] drivers: fwnode: fix fwnode_irq_get_byname()

[Jonathan Cameron]

On Fri, 12 May 2023 10:53:00 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> The fwnode_irq_get_byname() does return 0 upon device-tree IRQ mapping
> failure. This is contradicting the function documentation and can
> potentially be a source of errors like:
> 
> int probe(...) {
> 	...
> 
> 	irq = fwnode_irq_get_byname();
> 	if (irq <= 0)
> 		return irq;
> 
> 	...
> }
> 
> Here we do correctly check the return value from fwnode_irq_get_byname()
> but the driver probe will now return success. (There was already one
> such user in-tree).
> 
> Change the fwnode_irq_get_byname() to work as documented and according to
> the common convention and abd always return a negative errno upon failure.
> 
> Fixes: ca0acb511c21 ("device property: Add fwnode_irq_get_byname")
> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

Whilst the docs don't contradict behaviour for fwnode_irq_get()
unlike the byname() variant, it does seem odd to fix it only in this
version rather than modifying them both not to return 0.

Is there clear logic why they should be different?

> ---
>  drivers/base/property.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index f6117ec9805c..a3b95d2d781f 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -1011,7 +1011,7 @@ EXPORT_SYMBOL(fwnode_irq_get);
>   */
>  int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name)
>  {
> -	int index;
> +	int index, ret;
>  
>  	if (!name)
>  		return -EINVAL;
> @@ -1020,7 +1020,12 @@ int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name)
>  	if (index < 0)
>  		return index;
>  
> -	return fwnode_irq_get(fwnode, index);
> +	ret = fwnode_irq_get(fwnode, index);
> +	/* We treat mapping errors as invalid case */
> +	if (ret == 0)
> +		return -EINVAL;
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(fwnode_irq_get_byname);
>  

Re: [PATCH v3 3/3] iio: kx022a fix irq getting

[Jonathan Cameron]

On Fri, 12 May 2023 10:53:41 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> The fwnode_irq_get_byname() was returning 0 at device-tree mapping
> error. If this occurred, the KX022A driver did abort the probe but
> errorneously directly returned the return value from
> fwnode_irq_get_byname() from probe. In case of a device-tree mapping
> error this indicated success.
> 
> The fwnode_irq_get_byname() has since been fixed to not return zero on
> error so the check for fwnode_irq_get_byname() can be relaxed to only
> treat negative values as errors. This will also do decent fix even when
> backported to branches where fwnode_irq_get_byname() can still return
> zero on error because KX022A probe should later fail at IRQ requesting
> and a prober error handling should follow.
On that basis I've picked this one up directly for the fixes-togreg branch of
iio.git and marked it for stable.

Thanks,

Jonathan

> 
> Relax the return value check for fwnode_irq_get_byname() to only treat
> negative values as errors.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <error27@gmail.com>
> Closes: https://lore.kernel.org/r/202305110245.MFxC9bUj-lkp@intel.com/
> Link: https://lore.kernel.org/r/202305110245.MFxC9bUj-lkp@intel.com/
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> Fixes: 7c1d1677b322 ("iio: accel: Support Kionix/ROHM KX022A accelerometer")
> ---
>  drivers/iio/accel/kionix-kx022a.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
> index f98393d74666..b8636fa8eaeb 100644
> --- a/drivers/iio/accel/kionix-kx022a.c
> +++ b/drivers/iio/accel/kionix-kx022a.c
> @@ -1048,7 +1048,7 @@ int kx022a_probe_internal(struct device *dev)
>  		data->ien_reg = KX022A_REG_INC4;
>  	} else {
>  		irq = fwnode_irq_get_byname(fwnode, "INT2");
> -		if (irq <= 0)
> +		if (irq < 0)
>  			return dev_err_probe(dev, irq, "No suitable IRQ\n");
>  
>  		data->inc_reg = KX022A_REG_INC5;

Re: [PATCH v3 1/3] drivers: fwnode: fix fwnode_irq_get_byname()

[Matti Vaittinen]

Hi Jonathan,

It was somewhat busy "Mother's day" weekend for me but now I'm back in 
the business :)

On 5/13/23 21:40, Jonathan Cameron wrote:
> On Fri, 12 May 2023 10:53:00 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> The fwnode_irq_get_byname() does return 0 upon device-tree IRQ mapping
>> failure. This is contradicting the function documentation and can
>> potentially be a source of errors like:
>>
>> int probe(...) {
>> 	...
>>
>> 	irq = fwnode_irq_get_byname();
>> 	if (irq <= 0)
>> 		return irq;
>>
>> 	...
>> }
>>
>> Here we do correctly check the return value from fwnode_irq_get_byname()
>> but the driver probe will now return success. (There was already one
>> such user in-tree).
>>
>> Change the fwnode_irq_get_byname() to work as documented and according to
>> the common convention and abd always return a negative errno upon failure.
>>
>> Fixes: ca0acb511c21 ("device property: Add fwnode_irq_get_byname")
>> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> Whilst the docs don't contradict behaviour for fwnode_irq_get()
> unlike the byname() variant, it does seem odd to fix it only in this
> version rather than modifying them both not to return 0.

I think you're right. I think I overlooked this because the whole thing 
started as a documentation fix :)

> Is there clear logic why they should be different?

Not that I know of. I'll re-spin this with fwnode_irq_get() modified if 
no-one objects. Thanks for pointing this out!

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v3 3/3] iio: kx022a fix irq getting

[Matti Vaittinen]

On 5/13/23 21:44, Jonathan Cameron wrote:
> On Fri, 12 May 2023 10:53:41 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> The fwnode_irq_get_byname() was returning 0 at device-tree mapping
>> error. If this occurred, the KX022A driver did abort the probe but
>> errorneously directly returned the return value from
>> fwnode_irq_get_byname() from probe. In case of a device-tree mapping
>> error this indicated success.
>>
>> The fwnode_irq_get_byname() has since been fixed to not return zero on
>> error so the check for fwnode_irq_get_byname() can be relaxed to only
>> treat negative values as errors. This will also do decent fix even when
>> backported to branches where fwnode_irq_get_byname() can still return
>> zero on error because KX022A probe should later fail at IRQ requesting
>> and a prober error handling should follow.
> On that basis I've picked this one up directly for the fixes-togreg branch of
> iio.git and marked it for stable.

Thanks for picking this up Jonathan. Although, the commit message is 
slightly misleading w/o the previous patches in this series because the 
fwnode_irq_get_byname() is fixed in the first patch.

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~