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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <079063f5-e8e1-4a5f-8124-34d79d2fc9bd@kadam.mountain>
Date: Wed, 2 Aug 2023 09:00:03 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Ruan Jinjie <ruanjinjie@...wei.com>, yisen.zhuang@...wei.com,
	salil.mehta@...wei.com, davem@...emloft.net, edumazet@...gle.com,
	pabeni@...hat.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH -next] net: hisilicon: fix the return value handle and
 remove redundant netdev_err() for platform_get_irq()

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@...wei.com>
> 
> 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@...aro.org>

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