* [PATCH 1/2] drivers: fwnode: fix fwnode_irq_get_byname()
2022-10-25 8:50 [PATCH 0/2] fix fwnode_irq_get_byname() returnvalue Matti Vaittinen
@ 2022-10-25 8:50 ` Matti Vaittinen
2022-10-25 9:08 ` Sakari Ailus
2022-10-25 9:18 ` Andy Shevchenko
2022-10-25 8:51 ` [PATCH 2/2] i2c: i2c-smbus: fwnode_irq_get_byname() return value fix Matti Vaittinen
2022-10-25 9:20 ` [PATCH 0/2] fix fwnode_irq_get_byname() returnvalue Andy Shevchenko
2 siblings, 2 replies; 9+ messages in thread
From: Matti Vaittinen @ 2022-10-25 8:50 UTC (permalink / raw)
To: Matti Vaittinen, Matti Vaittinen
Cc: Andy Shevchenko, Daniel Scally, Heikki Krogerus, Sakari Ailus,
Greg Kroah-Hartman, Rafael J. Wysocki, Wolfram Sang, Akhil R,
linux-acpi, linux-kernel, linux-i2c
[-- Attachment #1: Type: text/plain, Size: 2465 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>
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
I did a quick audit for the callers at v6.1-rc2:
drivers/i2c/i2c-smbus.c
drivers/iio/accel/adxl355_core.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
I did not spot any errors to be caused by this change. There will be a
functional change in i2c-smbus.c as the probe will now return -EINVAL
should the IRQ dt-mapping fail. It'd be nice if this was checked to be
Ok by the peeps knowing the i2c-smbus :)
---
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 4d6278a84868..bfc6c7286db2 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -964,7 +964,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;
@@ -973,7 +973,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);
+
+ if (!ret)
+ return -EINVAL;
+
+ return ret;
}
EXPORT_SYMBOL(fwnode_irq_get_byname);
--
2.37.3
--
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 --]
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drivers: fwnode: fix fwnode_irq_get_byname()
2022-10-25 8:50 ` [PATCH 1/2] drivers: fwnode: fix fwnode_irq_get_byname() Matti Vaittinen
@ 2022-10-25 9:08 ` Sakari Ailus
2022-10-25 9:17 ` Matti Vaittinen
2022-10-25 9:18 ` Andy Shevchenko
1 sibling, 1 reply; 9+ messages in thread
From: Sakari Ailus @ 2022-10-25 9:08 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Matti Vaittinen, Andy Shevchenko, Daniel Scally, Heikki Krogerus,
Greg Kroah-Hartman, Rafael J. Wysocki, Wolfram Sang, Akhil R,
linux-acpi, linux-kernel, linux-i2c
Moi,
On Tue, Oct 25, 2022 at 11:50:59AM +0300, Matti Vaittinen 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>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>
> ---
>
> I did a quick audit for the callers at v6.1-rc2:
> drivers/i2c/i2c-smbus.c
> drivers/iio/accel/adxl355_core.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
>
> I did not spot any errors to be caused by this change. There will be a
It won't as you're decreasing the possible values the function may
return...
> functional change in i2c-smbus.c as the probe will now return -EINVAL
> should the IRQ dt-mapping fail. It'd be nice if this was checked to be
> Ok by the peeps knowing the i2c-smbus :)
FWIW, for both patches (but see below):
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.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 4d6278a84868..bfc6c7286db2 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -964,7 +964,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;
> @@ -973,7 +973,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);
> +
This newline is extra.
Or:
return ret ?: -EINVAL;
Or even:
return fwnode_irq_get(fwnode, index) ?: -EINVAL;
Up to you.
> + if (!ret)
> + return -EINVAL;
> +
> + return ret;
> }
> EXPORT_SYMBOL(fwnode_irq_get_byname);
--
Terveisin,
Sakari Ailus
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drivers: fwnode: fix fwnode_irq_get_byname()
2022-10-25 9:08 ` Sakari Ailus
@ 2022-10-25 9:17 ` Matti Vaittinen
0 siblings, 0 replies; 9+ messages in thread
From: Matti Vaittinen @ 2022-10-25 9:17 UTC (permalink / raw)
To: Sakari Ailus
Cc: Matti Vaittinen, Andy Shevchenko, Daniel Scally, Heikki Krogerus,
Greg Kroah-Hartman, Rafael J. Wysocki, Wolfram Sang, Akhil R,
linux-acpi, linux-kernel, linux-i2c
Moro Sakari,
Thanks for the review (and the suggestion)!
On 10/25/22 12:08, Sakari Ailus wrote:
> Moi,
>
> On Tue, Oct 25, 2022 at 11:50:59AM +0300, Matti Vaittinen 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>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>
>> ---
>>
>> I did a quick audit for the callers at v6.1-rc2:
>> drivers/i2c/i2c-smbus.c
>> drivers/iio/accel/adxl355_core.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
>>
>> I did not spot any errors to be caused by this change. There will be a
>
> It won't as you're decreasing the possible values the function may
> return...
>
Unless someone had implemented special handling for the IRQ mapping
failure...
>> functional change in i2c-smbus.c as the probe will now return -EINVAL
>> should the IRQ dt-mapping fail. It'd be nice if this was checked to be
>> Ok by the peeps knowing the i2c-smbus :)
>
> FWIW, for both patches (but see below):
>
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.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 4d6278a84868..bfc6c7286db2 100644
>> --- a/drivers/base/property.c
>> +++ b/drivers/base/property.c
>> @@ -964,7 +964,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;
>> @@ -973,7 +973,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);
>> +
>
> This newline is extra.
>
> Or:
>
> return ret ?: -EINVAL;
>
> Or even:
>
> return fwnode_irq_get(fwnode, index) ?: -EINVAL;
>
> Up to you.
>
My personal preference is to not use the ternary. I think the plain
clarity of if() just in many places justifies burning couple of lines
more :)
Yours
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drivers: fwnode: fix fwnode_irq_get_byname()
2022-10-25 8:50 ` [PATCH 1/2] drivers: fwnode: fix fwnode_irq_get_byname() Matti Vaittinen
2022-10-25 9:08 ` Sakari Ailus
@ 2022-10-25 9:18 ` Andy Shevchenko
2022-10-25 10:00 ` Vaittinen, Matti
1 sibling, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2022-10-25 9:18 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Matti Vaittinen, Daniel Scally, Heikki Krogerus, Sakari Ailus,
Greg Kroah-Hartman, Rafael J. Wysocki, Wolfram Sang, Akhil R,
linux-acpi, linux-kernel, linux-i2c
On Tue, Oct 25, 2022 at 11:50:59AM +0300, Matti Vaittinen 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.
...
> + ret = fwnode_irq_get(fwnode, index);
> +
Redundant blank line and better to use traditional pattern:
> + if (!ret)
> + return -EINVAL;
> +
> + return ret;
if (ret)
return ret;
/* We treat mapping errors as invalid case */
return -EINVAL;
> }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drivers: fwnode: fix fwnode_irq_get_byname()
2022-10-25 9:18 ` Andy Shevchenko
@ 2022-10-25 10:00 ` Vaittinen, Matti
2022-10-25 11:15 ` Andy Shevchenko
0 siblings, 1 reply; 9+ messages in thread
From: Vaittinen, Matti @ 2022-10-25 10:00 UTC (permalink / raw)
To: Andy Shevchenko, Matti Vaittinen
Cc: Daniel Scally, Heikki Krogerus, Sakari Ailus, Greg Kroah-Hartman,
Rafael J. Wysocki, Wolfram Sang, Akhil R, linux-acpi,
linux-kernel, linux-i2c
Hi Andy,
Thanks for the review.
On 10/25/22 12:18, Andy Shevchenko wrote:
> On Tue, Oct 25, 2022 at 11:50:59AM +0300, Matti Vaittinen 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.
>
> ...
>
>> + ret = fwnode_irq_get(fwnode, index);
>
>> +
>
> Redundant blank line and better to use traditional pattern: >
>> + if (!ret)
>> + return -EINVAL;
>> +
>> + return ret;
>
> if (ret)
> return ret;
>
> /* We treat mapping errors as invalid case */
> return -EINVAL;
>
>> }
I like the added comment - but in this case I don't prefer the
"traditional pattern" you suggest. We do check for a very special error
case indicated by ret 0.
if (!ret)
return -EINVAL;
makes it obvious the zero is special error.
if (ret)
return ret;
the traditional pattern makes this look like traditional error return -
which it is not. The comment you added is nice though. It could be just
before the check for
if (!ret).
I can cook v2 later when I have finished my current task - which may or
may not take a while though....
Yours
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drivers: fwnode: fix fwnode_irq_get_byname()
2022-10-25 10:00 ` Vaittinen, Matti
@ 2022-10-25 11:15 ` Andy Shevchenko
0 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2022-10-25 11:15 UTC (permalink / raw)
To: Vaittinen, Matti
Cc: Matti Vaittinen, Daniel Scally, Heikki Krogerus, Sakari Ailus,
Greg Kroah-Hartman, Rafael J. Wysocki, Wolfram Sang, Akhil R,
linux-acpi, linux-kernel, linux-i2c
On Tue, Oct 25, 2022 at 10:00:07AM +0000, Vaittinen, Matti wrote:
> On 10/25/22 12:18, Andy Shevchenko wrote:
> > On Tue, Oct 25, 2022 at 11:50:59AM +0300, Matti Vaittinen wrote:
...
> >> + ret = fwnode_irq_get(fwnode, index);
> >
> >> +
> >
> > Redundant blank line and better to use traditional pattern: >
> >> + if (!ret)
> >> + return -EINVAL;
> >> +
> >> + return ret;
> >
> > if (ret)
> > return ret;
> >
> > /* We treat mapping errors as invalid case */
> > return -EINVAL;
> >
> >> }
>
> I like the added comment - but in this case I don't prefer the
> "traditional pattern" you suggest. We do check for a very special error
> case indicated by ret 0.
>
> if (!ret)
> return -EINVAL;
>
> makes it obvious the zero is special error.
I don't think so. It makes ! easily to went through the cracks. If you want an
explicit, use ' == 0' and add a comment.
> if (ret)
> return ret;
>
> the traditional pattern makes this look like traditional error return -
> which it is not. The comment you added is nice though. It could be just
> before the check for
>
> if (!ret).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] i2c: i2c-smbus: fwnode_irq_get_byname() return value fix
2022-10-25 8:50 [PATCH 0/2] fix fwnode_irq_get_byname() returnvalue Matti Vaittinen
2022-10-25 8:50 ` [PATCH 1/2] drivers: fwnode: fix fwnode_irq_get_byname() Matti Vaittinen
@ 2022-10-25 8:51 ` Matti Vaittinen
2022-10-25 9:20 ` [PATCH 0/2] fix fwnode_irq_get_byname() returnvalue Andy Shevchenko
2 siblings, 0 replies; 9+ messages in thread
From: Matti Vaittinen @ 2022-10-25 8:51 UTC (permalink / raw)
To: Matti Vaittinen, Matti Vaittinen
Cc: Andy Shevchenko, Daniel Scally, Heikki Krogerus, Sakari Ailus,
Greg Kroah-Hartman, Rafael J. Wysocki, Wolfram Sang, Akhil R,
linux-acpi, linux-kernel, linux-i2c
[-- Attachment #1: Type: text/plain, Size: 1364 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.
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.
---
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 07c92c8495a3..d0cc4b7903ed 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -130,7 +130,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.37.3
--
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 --]
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] fix fwnode_irq_get_byname() returnvalue
2022-10-25 8:50 [PATCH 0/2] fix fwnode_irq_get_byname() returnvalue Matti Vaittinen
2022-10-25 8:50 ` [PATCH 1/2] drivers: fwnode: fix fwnode_irq_get_byname() Matti Vaittinen
2022-10-25 8:51 ` [PATCH 2/2] i2c: i2c-smbus: fwnode_irq_get_byname() return value fix Matti Vaittinen
@ 2022-10-25 9:20 ` Andy Shevchenko
2 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2022-10-25 9:20 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Matti Vaittinen, Daniel Scally, Heikki Krogerus, Sakari Ailus,
Greg Kroah-Hartman, Rafael J. Wysocki, Wolfram Sang, Akhil R,
linux-acpi, linux-kernel, linux-i2c
On Tue, Oct 25, 2022 at 11:50:24AM +0300, Matti Vaittinen wrote:
> 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:
>
> drivers/i2c/i2c-smbus.c
> drivers/iio/accel/adxl355_core.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 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.
Thanks for doing this, no major comments except worrying about fwnode_irq_get()
which is left untouched an hence different error domain for the same
family of API.
For these patches:
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 9+ messages in thread