Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 2 Aug 2023 09:00:03 +0300
From:   Dan Carpenter <dan.carpenter@linaro.org>
To:     Jakub Kicinski <kuba@kernel.org>
Cc:     Ruan Jinjie <ruanjinjie@huawei.com>, yisen.zhuang@huawei.com,
        salil.mehta@huawei.com, davem@davemloft.net, edumazet@google.com,
        pabeni@redhat.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH -next] net: hisilicon: fix the return value handle and
 remove redundant netdev_err() for platform_get_irq()
Message-ID: <079063f5-e8e1-4a5f-8124-34d79d2fc9bd@kadam.mountain>
References: <20230731073858.3633193-1-ruanjinjie@huawei.com>
 <20230801144347.140cc06f@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230801144347.140cc06f@kernel.org>
Precedence: bulk

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@huawei.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@linaro.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