Messages in this thread | | | Date | Tue, 25 Oct 2022 12:17:27 +0300 | Subject | Re: [PATCH 1/2] drivers: fwnode: fix fwnode_irq_get_byname() | From | Matti Vaittinen <> |
| |
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! ~~
| |