Re: [PATCH] irqchip/gic: check return value of of_address_to_resource

[Bo Yan]

Marc,

I'm also wondering if of_address_to_resource can really fail in this 
particular case?

What if we just explicitly discard the return value like this:

(void)of_address_to_resource(node, 1, &cpuif_res);

This suppresses Coverity warning by explicitly stating we are 100% sure 
the function call will always return success.

On 07/05/2018 12:13 PM, Marc Zyngier wrote:
> Hi Bo,
>
> On Thu, 5 Jul 2018 11:20:59 -0700
> Bo Yan <byan@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >   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.