diff mbox series

[v2,1/2] drivers: fwnode: fix fwnode_irq_get_byname()

Message ID a3bf7094a9f9ebf114736dc7944553dcc701fe73.1666710197.git.mazziesaccount@gmail.com
State New
Headers
Series fix fwnode_irq_get_byname() returnvalue |

Commit Message

Matti Vaittinen Oct. 25, 2022, 3:11 p.m. UTC 
  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>
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

---
v1 => v2:
- change check if (!ret) => if (ret == 0), add comment.
- fix drop redundant empty line.

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(-)

Comments

Andy Shevchenko Oct. 25, 2022, 4:21 p.m. UTC | #1 
On Tue, Oct 25, 2022 at 06:11:49PM +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.

and abd ?

...

> +	ret = fwnode_irq_get(fwnode, index);
> +	/* We treat mapping errors as invalid case */
> +	if (ret == 0)
> +		return -EINVAL;
> +
> +	return ret;

This looks good.
diff mbox series

Patch

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 4d6278a84868..b4ccc64f7bd2 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);
+	/* We treat mapping errors as invalid case */
+	if (ret == 0)
+		return -EINVAL;
+
+	return ret;
 }
 EXPORT_SYMBOL(fwnode_irq_get_byname);