[0/2] fix fwnode_irq_get_byname() returnvalue

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

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

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