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

[Bo Yan]

Marc,

Sorry for the previous reply. My email settings were not correct, so it 
inserted those confidentiality text, which was not what I intended.

This is what I think:

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index ced10c4..0b60bb0 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1284,7 +1284,7 @@ static bool gic_check_eoimode(struct device_node 
*node, void __iomem **base)
 {
        struct resource cpuif_res;

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

        if (!is_hyp_mode_available())
                return false;

We are 100% sure of_address_to_resource will succeed in this particular 
case, so "(void)" will help suppress Coverity warning.

On 07/05/2018 12:18 PM, Bo Yan wrote:
> 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.