[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

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: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 <andriy.shevchenko@linux.intel.com>

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