[PATCH] gpio: gpio-xilinx: Check return value of of_property_read_u32

[PATCH] gpio: gpio-xilinx: Check return value of of_property_read_u32

[Srinivas Neeli]

In five different instances the return value of "of_property_read_u32"
API was neither captured nor checked.

Fixed it by capturing the return value and then checking for any error.

Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
Addresses-Coverity: "check_return"
---
 drivers/gpio/gpio-xilinx.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
index b6d3a57e27ed..268c7b0e481d 100644
--- a/drivers/gpio/gpio-xilinx.c
+++ b/drivers/gpio/gpio-xilinx.c
@@ -570,7 +570,8 @@ static int xgpio_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, chip);
 
 	/* First, check if the device is dual-channel */
-	of_property_read_u32(np, "xlnx,is-dual", &is_dual);
+	if (of_property_read_u32(np, "xlnx,is-dual", &is_dual))
+		is_dual = 0;
 
 	/* Setup defaults */
 	memset32(width, 0, ARRAY_SIZE(width));
@@ -578,14 +579,18 @@ static int xgpio_probe(struct platform_device *pdev)
 	memset32(dir, 0xFFFFFFFF, ARRAY_SIZE(dir));
 
 	/* Update GPIO state shadow register with default value */
-	of_property_read_u32(np, "xlnx,dout-default", &state[0]);
-	of_property_read_u32(np, "xlnx,dout-default-2", &state[1]);
+	if (of_property_read_u32(np, "xlnx,dout-default", &state[0]))
+		state[0] = 0x0;
+	if (of_property_read_u32(np, "xlnx,dout-default-2", &state[1]))
+		state[1] = 0x0;
 
 	bitmap_from_arr32(chip->state, state, 64);
 
 	/* Update GPIO direction shadow register with default value */
-	of_property_read_u32(np, "xlnx,tri-default", &dir[0]);
-	of_property_read_u32(np, "xlnx,tri-default-2", &dir[1]);
+	if (of_property_read_u32(np, "xlnx,tri-default", &dir[0]))
+		dir[0] = 0xFFFFFFFF;
+	if (of_property_read_u32(np, "xlnx,tri-default-2", &dir[1]))
+		dir[1] = 0xFFFFFFFF;
 
 	bitmap_from_arr32(chip->dir, dir, 64);
 
-- 
2.25.1

Re: [PATCH] gpio: gpio-xilinx: Check return value of of_property_read_u32

[Michal Simek]

On 6/17/22 07:19, Srinivas Neeli wrote:
> In five different instances the return value of "of_property_read_u32"
> API was neither captured nor checked.
> 
> Fixed it by capturing the return value and then checking for any error.
> 
> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> Addresses-Coverity: "check_return"
> ---
>   drivers/gpio/gpio-xilinx.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
> index b6d3a57e27ed..268c7b0e481d 100644
> --- a/drivers/gpio/gpio-xilinx.c
> +++ b/drivers/gpio/gpio-xilinx.c
> @@ -570,7 +570,8 @@ static int xgpio_probe(struct platform_device *pdev)
>   	platform_set_drvdata(pdev, chip);
>   
>   	/* First, check if the device is dual-channel */
> -	of_property_read_u32(np, "xlnx,is-dual", &is_dual);
> +	if (of_property_read_u32(np, "xlnx,is-dual", &is_dual))
> +		is_dual = 0;

All these values are initialized already.
Isn't it enough to just ignore return value like this to make coverity happy?

(void)of_property_read_u32(np, "xlnx,is-dual", &is_dual)

Thanks,
Michal

Re: [PATCH] gpio: gpio-xilinx: Check return value of of_property_read_u32

[Andy Shevchenko]

On Fri, Jun 17, 2022 at 7:20 AM Srinivas Neeli
<srinivas.neeli@xilinx.com> wrote:
>
> In five different instances the return value of "of_property_read_u32"
> API was neither captured nor checked.
>
> Fixed it by capturing the return value and then checking for any error.
>
> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> Addresses-Coverity: "check_return"

I think the best course of action here is to go and fix Coverity while
marking these as false positives.

To the idea of castings -- this is not good style and (many?)
maintainers in kernel do not accept such "workaround" for fixing
broken tool.

That said, NAK to the patch in any of its forms.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpio: gpio-xilinx: Check return value of of_property_read_u32

[Michal Simek]

Hi Andy,

On 6/17/22 18:02, Andy Shevchenko wrote:
> 
> 
> On Fri, Jun 17, 2022 at 7:20 AM Srinivas Neeli
> <srinivas.neeli@xilinx.com> wrote:
>>
>> In five different instances the return value of "of_property_read_u32"
>> API was neither captured nor checked.
>>
>> Fixed it by capturing the return value and then checking for any error.
>>
>> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
>> Addresses-Coverity: "check_return"
> 
> I think the best course of action here is to go and fix Coverity while
> marking these as false positives.
> 
> To the idea of castings -- this is not good style and (many?)
> maintainers in kernel do not accept such "workaround" for fixing
> broken tool.

Let's wait for Linus what he will say about it.
I can't see nothing wrong about declaring that I am intentionally ignoring 
return code.

Thanks,
Michal

Re: [PATCH] gpio: gpio-xilinx: Check return value of of_property_read_u32

[Linus Walleij]

On Mon, Jun 20, 2022 at 8:26 AM Michal Simek <michal.simek@amd.com> wrote:
> On 6/17/22 18:02, Andy Shevchenko wrote:
> > On Fri, Jun 17, 2022 at 7:20 AM Srinivas Neeli
> > <srinivas.neeli@xilinx.com> wrote:
> >>
> >> In five different instances the return value of "of_property_read_u32"
> >> API was neither captured nor checked.
> >>
> >> Fixed it by capturing the return value and then checking for any error.
> >>
> >> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> >> Addresses-Coverity: "check_return"
> >
> > I think the best course of action here is to go and fix Coverity while
> > marking these as false positives.
> >
> > To the idea of castings -- this is not good style and (many?)
> > maintainers in kernel do not accept such "workaround" for fixing
> > broken tool.
>
> Let's wait for Linus what he will say about it.
> I can't see nothing wrong about declaring that I am intentionally ignoring
> return code.

I don't think this patch should be applied.

The problem with static analysis is that such tools have no feeling
for context at all, and in this case the context makes it pretty
clear why it is safe to ignore these return values.

But we need to adopt the tool to the code not adopt the code to
the tool.

Yours,
Linus Walleij

Re: [PATCH] gpio: gpio-xilinx: Check return value of of_property_read_u32

[Michal Simek]

On 6/28/22 14:27, Linus Walleij wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
> 
> 
> On Mon, Jun 20, 2022 at 8:26 AM Michal Simek <michal.simek@amd.com> wrote:
>> On 6/17/22 18:02, Andy Shevchenko wrote:
>>> On Fri, Jun 17, 2022 at 7:20 AM Srinivas Neeli
>>> <srinivas.neeli@xilinx.com> wrote:
>>>>
>>>> In five different instances the return value of "of_property_read_u32"
>>>> API was neither captured nor checked.
>>>>
>>>> Fixed it by capturing the return value and then checking for any error.
>>>>
>>>> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
>>>> Addresses-Coverity: "check_return"
>>>
>>> I think the best course of action here is to go and fix Coverity while
>>> marking these as false positives.
>>>
>>> To the idea of castings -- this is not good style and (many?)
>>> maintainers in kernel do not accept such "workaround" for fixing
>>> broken tool.
>>
>> Let's wait for Linus what he will say about it.
>> I can't see nothing wrong about declaring that I am intentionally ignoring
>> return code.
> 
> I don't think this patch should be applied.
> 
> The problem with static analysis is that such tools have no feeling
> for context at all, and in this case the context makes it pretty
> clear why it is safe to ignore these return values.
> 
> But we need to adopt the tool to the code not adopt the code to
> the tool.

ok. No problem. Thanks for discussion.

Thanks,
Michal