[0/2] fix fwnode_irq_get_byname() returnvalue

Message

Matti Vaittinen Oct. 25, 2022, 8:50 a.m. UTC

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.

---

Matti Vaittinen (2):
  drivers: fwnode: fix fwnode_irq_get_byname()
  i2c: i2c-smbus: fwnode_irq_get_byname() return value fix

 drivers/base/property.c | 9 +++++++--
 drivers/i2c/i2c-smbus.c | 2 +-
 2 files changed, 8 insertions(+), 3 deletions(-)


base-commit: 247f34f7b80357943234f93f247a1ae6b6c3a740

Comments

Andy Shevchenko Oct. 25, 2022, 9:20 a.m. UTC | #1

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 <[e-mail]>