Re: [PATCH v1 2/4] gpio: dwapb: Don't use 0 as valid Linux interrupt number

[Serge Semin <fancer.lancer@xxxxxxxxx>]

On Tue, May 12, 2020 at 09:45:11PM +0300, Andy Shevchenko wrote:
> 0 is not valid IRQ number in Linux interrupt number space.
> Refactor the code with this kept in mind.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Cc: Serge Semin <fancer.lancer@xxxxxxxxx>
> ---
>  drivers/gpio/gpio-dwapb.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 5bc5057f071f37..78662d6d73634e 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -417,7 +417,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>  		int i;
>  
>  		for (i = 0; i < pp->ngpio; i++) {
> -			if (pp->irq[i] >= 0)
> +			if (pp->irq[i])
>  				irq_set_chained_handler_and_data(pp->irq[i],
>  						dwapb_irq_handler, gpio);
>  		}
> @@ -531,23 +531,23 @@ static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode,
>  			  struct dwapb_port_property *pp)
>  {
>  	struct device_node *np = NULL;
> -	int j;
> +	int err, j;

Err might be used uninitialized here. The compiler also says so:

drivers/gpio/gpio-dwapb.c:560:14: warning: ‘err’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   pp->irq[j] = err;

drivers/gpio/gpio-dwapb.c:547:6: note: ‘err’ was declared here
  int err, j;

Could you please make it initialized with error number like -ENXIO by default?

Also if it was just a single issue in my mind, I wouldn't have probably payed
much attention to this. But since you need to send v2 anyway, I'd prefer to have
a positive naming here since normally both of_irq_get() and
platform_get_irq_optional() return IRQ number, and error is returned only on
failure. So checking an erroneous value of a positively named variable seems
more natural, rather than copying an error-named variable to a normal variable.
To cut it short could you please rename err to something like irq?

>  
>  	if (fwnode_property_read_bool(fwnode, "interrupt-controller"))
>  		np = to_of_node(fwnode);
>  
>  	for (j = 0; j < pp->ngpio; j++) {
> -		pp->irq[j] = -ENXIO;
> -
>  		if (np)
> -			pp->irq[j] = of_irq_get(np, j);
> +			err = of_irq_get(np, j);
>  		else if (has_acpi_companion(dev))
> -			pp->irq[j] = platform_get_irq_optional(to_platform_device(dev), j);
> +			err = platform_get_irq_optional(to_platform_device(dev), j);
> +		if (err < 0)
> +			continue;
>  
> -		if (pp->irq[j] >= 0)
> -			pp->has_irq = true;
> +		pp->irq[j] = err;
>  	}
>  

> +	pp->has_irq = memchr_inv(pp->irq, 0, sizeof(pp->irq)) != NULL;

Sorry, but I don't see why is setting the has_irq flag in the loop above worse than
using memchr_inv()? As I see it since we need to perform the loop above anyway, it
would be normal to update the flag synchronously there instead of traversing the
irq's array byte-by-byte again in the memchr_inv() method. Moreover
(memchr_inv() != NULL) seems harder to read. A kernel hacker needs to keep in
mind the method semantics, that it returns non-null if unmatched character found
in the array, to get the line logic. Setting "has_irq = true" is straightforward -
if IRQ's found then set the flag to true. So if you don't have a strong opinion
against my reasoning could you please get the setting the has_irq flag back in to
the loop above?

-Sergey

>  	if (!pp->has_irq)
>  		dev_warn(dev, "no irq for port%d\n", pp->idx);
>  }
> -- 
> 2.26.2
>