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: [greybus-dev] [PATCH] staging: greybus: bootrom: fix
 uninitialized variables
To:     SAURAV GIREPUNJE <saurav.girepunje@gmail.com>,
        Johan Hovold <johan@kernel.org>
Cc:     devel@driverdev.osuosl.org, elder@kernel.org, vireshk@kernel.org,
        linux-kernel@vger.kernel.org, greybus-dev@lists.linaro.org
References: <20200125084403.GA3386@google.com>
 <20200125100011.GK8375@localhost> <20200125121459.GA2792@google.com>
From:   Alex Elder <elder@linaro.org>
Message-ID: <275d6509-ebd2-f00e-176d-abf97ae834da@linaro.org>
Date:   Mon, 27 Jan 2020 08:10:02 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.4.1
MIME-Version: 1.0
In-Reply-To: <20200125121459.GA2792@google.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 1/25/20 6:14 AM, SAURAV GIREPUNJE wrote:
> On 25/01/20 11:00 +0100, Johan Hovold wrote:
>> On Sat, Jan 25, 2020 at 02:14:03PM +0530, Saurav Girepunje wrote:
>>> fix uninitialized variables issue found using static code analysis tool
>>
>> Which tool is that?
>>
>>> (error) Uninitialized variable: offset
>>> (error) Uninitialized variable: size
>>>
>>> Signed-off-by: Saurav Girepunje <saurav.girepunje@gmail.com>
>>> ---
>>>   drivers/staging/greybus/bootrom.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c
>>> index a8efb86..9eabeb3 100644
>>> --- a/drivers/staging/greybus/bootrom.c
>>> +++ b/drivers/staging/greybus/bootrom.c
>>> @@ -245,7 +245,7 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
>>>       struct gb_bootrom_get_firmware_request *firmware_request;
>>>       struct gb_bootrom_get_firmware_response *firmware_response;
>>>       struct device *dev = &op->connection->bundle->dev;
>>> -    unsigned int offset, size;
>>> +    unsigned int offset = 0, size = 0;
>>>       enum next_request_type next_request;
>>>       int ret = 0;
>>
>> I think this has come up in the past, and while the code in question is
>> overly complicated and confuses static checkers as well as humans, it
>> looks correct to me.
>>
>> Please make sure to verify the output of any tools before posting
>> patches based on them.
>>
>> Johan
> I used cppcheck tool .

Implied in Johan's question is a suggestion.

When you propose a patch that addresses something flagged by a
tool of some kind, it is good practice to identify the tool in
the patch description, and even better, give an example of how
the tool was invoked when reported the problem you're fixing.
Sometimes people even include the output of the tool, though
I think that can sometimes be a bit much.

And as you have now heard several times, do not blindly trust
the output of these tools.  They're intended to call attention
to things for you to examine; they are no match for a human,
and things they tell you about are not guaranteed to be real
problems.

					-Alex

> _______________________________________________ 
> greybus-dev mailing list
> greybus-dev@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/greybus-dev