| Message ID | cc853a7e4b3533585e3641620bf4972663f22edc.1666687086.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fix fwnode_irq_get_byname() returnvalue | expand |
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);
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
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; > }
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
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).
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);
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(-)