Re: [PATCH -next] net: hisilicon: fix the return value handle and remove redundant netdev_err() for platform_get_irq()

[Dan Carpenter]

On Tue, Aug 01, 2023 at 02:43:47PM -0700, Jakub Kicinski wrote:
> On Mon, 31 Jul 2023 15:38:58 +0800 Ruan Jinjie wrote:
> > There is no possible for platform_get_irq() to return 0
> > and the return value of platform_get_irq() is more sensible
> > to show the error reason.
> > 
> > And there is no need to call the netdev_err() function directly to print
> > a custom message when handling an error from platform_get_irq() function as
> > it is going to display an appropriate error message in case of a failure.
> > 
> > Signed-off-by: Ruan Jinjie <ruanjinjie@xxxxxxxxxx>
> 
> Dan, with the sample of one patch from you I just applied I induce
> that treating 0 as error and returning a -EINVAL in that case may
> be preferable here?

This patch is correct.

Reviewed-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>

The comments for platform_get_irq() say it returns negatives on error
and that's also how the code is implemented.

Is zero an error code?  Historically, a lot of IRQ functions returned
0 on error and some of those haven't been replaced with new functions
that return negative error codes.  irq_of_parse_and_map() is an example
of this.  I've been meaning to make a complete list but apparently
that's the only one Smatch checks for.

Is zero a valid IRQ?  In upstream code the answer is no and it never
will be.  In this code the platform_get_irq_optional() will trigger a
warning for that.
	if (WARN(!ret, "0 is an invalid IRQ number\n"))
However there are some old out of tree arches where zero is a valid IRQ.

regards,
dan carpenter