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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Sun, May 17, 2020 at 03:52:44PM +0300, Serge Semin wrote:
> 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.

Serge, thanks for review, my answers below.

...

> > +	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;

Good catch! I'm puzzled how I missed it :-(
I usually compile with W=1.

Hmm... gcc (Debian 9.3.0-12) 9.3.0 building for i386, doesn't give me a
warning, but I see that there is potentially possible that scenario, so, I'm
going to fix it.

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

Will do.

> 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?

Makes sense.

...

> > +	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?

It's done in that way for a reason of the next clean ups, i.e. moving this to
the actual user of it. I hope you already read further patches to see the
intention behind this change. So, I prefer to leave it as is, however I agree
with you in general.

Btw, we may also leave the domain when even no IRQ is available as some other
drivers do, but I consider it less plausible than using memchr_inv().

-- 
With Best Regards,
Andy Shevchenko