Re: [PATCH 1/2] drivers: fwnode: fix fwnode_irq_get_byname()

[Vaittinen, Matti]

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
-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~