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 =]
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 <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
Closes: https://lore.kernel.org/r/[email protected]/
Link: https://lore.kernel.org/r/[email protected]/
Signed-off-by: Matti Vaittinen <[email protected]>
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 =]
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 <[email protected]>
Reviewed-by: Sakari Ailus <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Matti Vaittinen <[email protected]>
---
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 =]
On Fri, 12 May 2023 10:53:00 +0300
Matti Vaittinen <[email protected]> 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 <[email protected]>
> Reviewed-by: Sakari Ailus <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Matti Vaittinen <[email protected]>
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);
>
On Fri, 12 May 2023 10:53:41 +0300
Matti Vaittinen <[email protected]> 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 <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
> Closes: https://lore.kernel.org/r/[email protected]/
> Link: https://lore.kernel.org/r/[email protected]/
> Signed-off-by: Matti Vaittinen <[email protected]>
> 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;
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 <[email protected]> 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 <[email protected]>
>> Reviewed-by: Sakari Ailus <[email protected]>
>> Reviewed-by: Andy Shevchenko <[email protected]>
>> Signed-off-by: Matti Vaittinen <[email protected]>
>
> 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! ~~
On 5/13/23 21:44, Jonathan Cameron wrote:
> On Fri, 12 May 2023 10:53:41 +0300
> Matti Vaittinen <[email protected]> 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! ~~