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

From: "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Matti Vaittinen <mazziesaccount@gmail.com>
Cc: Daniel Scally <djrscally@gmail.com>,
	Heikki Krogerus <heikki.krogerus@linux.intel.com>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Wolfram Sang <wsa@kernel.org>, Akhil R <akhilrajeev@nvidia.com>,
	"linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>
Subject: Re: [PATCH 1/2] drivers: fwnode: fix fwnode_irq_get_byname()
Date: Tue, 25 Oct 2022 10:00:07 +0000	[thread overview]
Message-ID: <ec8cc1bd-3150-dc5d-8b8a-870f21b52433@fi.rohmeurope.com> (raw)
In-Reply-To: <Y1ep5zN+wifkP7v+@smile.fi.intel.com>

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! ~~