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 <[email protected]>
---
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;
if (!is_hyp_mode_available())
return false;
--
2.7.4
Hi Bo,
On Thu, 5 Jul 2018 11:20:59 -0700
Bo Yan <[email protected]> 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 <[email protected]>
> ---
> 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.
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 <[email protected]> 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 <[email protected]>
>> ---
>> 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.
>
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 <[email protected]> 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 <[email protected]>
>>> ---
>>> 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.
>>
On Thu, 5 Jul 2018 12:32:22 -0700
Bo Yan <[email protected]> wrote:
> 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.
In all honesty. I don't see the point of patching the kernel to silence
a warning when we know that this is a false positive. I'm sure you can
flag that one as "false positive" in Coverity.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.