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;
Subject: Re: [PATCH] irqchip/gic: check return value of of_address_to_resource
From:   Bo Yan <byan@nvidia.com>
To:     Marc Zyngier <marc.zyngier@arm.com>
CC:     <tglx@linutronix.de>, <jason@lakedaemon.net>,
        <linux-kernel@vger.kernel.org>
References: <1530814859-11610-1-git-send-email-byan@nvidia.com>
 <20180705201327.4a4dc7dd@why.wild-wind.fr.eu.org>
 <5548bdf4-4e4f-a2e1-b194-da35da87e3d3@nvidia.com>
Message-ID: <c892593d-8824-e6c5-b435-0902fd0e7fde@nvidia.com>
Date:   Thu, 5 Jul 2018 12:32:22 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.8.0
MIME-Version: 1.0
In-Reply-To: <5548bdf4-4e4f-a2e1-b194-da35da87e3d3@nvidia.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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