Re: [PATCH v6 1/8] drivers: fwnode: fix fwnode_irq_get[_byname]()

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 26 May 2023 09:35:30 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> The fwnode_irq_get() and the fwnode_irq_get_byname() return 0 upon
> device-tree IRQ mapping failure. This is contradicting the
> fwnode_irq_get_byname() 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 make also the
> fwnode_irq_get() follow same common convention returning a negative errno
> upon failure.
> 
> Fixes: ca0acb511c21 ("device property: Add fwnode_irq_get_byname")
> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
This bothers me a little because there may be drivers that haven't been
caught yet that assume the zero value.   Still this is more consistent
with what I'd expect to happen, so fair enough

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> ---
> I dropped the existing reviewed-by tags because change to
> fwnode_irq_get() was added.
> 
> Revision history:
> v4 =>:
>  - No Changes
> v3 => v4:
>  - Change also the fwnode_irq_get()
> ---
>  drivers/base/property.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index f6117ec9805c..8c40abed7852 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -987,12 +987,18 @@ EXPORT_SYMBOL(fwnode_iomap);
>   * @fwnode:	Pointer to the firmware node
>   * @index:	Zero-based index of the IRQ
>   *
> - * Return: Linux IRQ number on success. Other values are determined
> - * according to acpi_irq_get() or of_irq_get() operation.
> + * Return: Linux IRQ number on success. Negative errno on failure.
>   */
>  int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
>  {
> -	return fwnode_call_int_op(fwnode, irq_get, index);
> +	int ret;
> +
> +	ret = fwnode_call_int_op(fwnode, irq_get, index);
> +	/* We treat mapping errors as invalid case */
> +	if (ret == 0)
> +		return -EINVAL;
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(fwnode_irq_get);
>