Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 5 Jul 2018 20:13:27 +0100
From:   Marc Zyngier <marc.zyngier@arm.com>
To:     Bo Yan <byan@nvidia.com>
Cc:     <tglx@linutronix.de>, <jason@lakedaemon.net>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] irqchip/gic: check return value of
 of_address_to_resource
Message-ID: <20180705201327.4a4dc7dd@why.wild-wind.fr.eu.org>
In-Reply-To: <1530814859-11610-1-git-send-email-byan@nvidia.com>
References: <1530814859-11610-1-git-send-email-byan@nvidia.com>
Organization: ARM Ltd
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Bo,

On Thu, 5 Jul 2018 11:20:59 -0700
Bo Yan <byan@nvidia.com> wrote:

> The of_address_to_resource returns 0 if successful. gic_check_eoimode
> calls it without checking the return value. This induces Coverity
> warning: "Unchecked return value".
> 
> Return false from gic_check_eoimode if of_address_to_resource returns
> non-0 value.
> 
> Signed-off-by: Bo Yan <byan@nvidia.com>
> ---
>  drivers/irqchip/irq-gic.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index ced10c4..0bceb10 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -1284,7 +1284,8 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
>  {
>  	struct resource cpuif_res;
>  
> -	of_address_to_resource(node, 1, &cpuif_res);
> +	if (of_address_to_resource(node, 1, &cpuif_res))
> +		return false;

We've just done an of_iomap() on this resource, which succeeded. How
can the same thing now fail? It would mean that the device tree has
been pulled from under our feet...

And if it could happen, why is returning false the right thing to do?
Why would we say we want EOImode==0 instead of 1?

>  
>  	if (!is_hyp_mode_available())
>  		return false;

As it stands, I'm not taking such a patch. It either papers over a
bigger problem, or just keeps a warning quiet for the sake of it.

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.