[PATCH] EDAC, sb_edac: remove redundant update of tad_base

[PATCH] EDAC, sb_edac: remove redundant update of tad_base

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

The variable tad_base is being set to a value that is never read
and is being over-written on the next iteration of a for-loop.
This assignment is therefore redundant and can be removed.

Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/edac/sb_edac.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c
index 9353c3fc7c05..6aa4b1b73a15 100644
--- a/drivers/edac/sb_edac.c
+++ b/drivers/edac/sb_edac.c
@@ -1513,7 +1513,6 @@ static int knl_get_dimm_capacity(struct sbridge_pvt *pvt, u64 *mc_sizes)
 						sad_actual_size[mc] += tad_size;
 					}
 				}
-				tad_base = tad_limit+1;
 			}
 		}
 
-- 
2.20.1

Re: [PATCH] EDAC, sb_edac: remove redundant update of tad_base

[Luck, Tony]

On Wed, May 08, 2019 at 11:42:01PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> The variable tad_base is being set to a value that is never read
> and is being over-written on the next iteration of a for-loop.
> This assignment is therefore redundant and can be removed.
> 
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/edac/sb_edac.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c
> index 9353c3fc7c05..6aa4b1b73a15 100644
> --- a/drivers/edac/sb_edac.c
> +++ b/drivers/edac/sb_edac.c
> @@ -1513,7 +1513,6 @@ static int knl_get_dimm_capacity(struct sbridge_pvt *pvt, u64 *mc_sizes)
>  						sad_actual_size[mc] += tad_size;
>  					}
>  				}
> -				tad_base = tad_limit+1;
>  			}
>  		}
>  

Looks good to me.

Acked-by: Tony Luck <tony.luck@intel.com>

Re: [PATCH] EDAC, sb_edac: remove redundant update of tad_base

[Borislav Petkov]

On Wed, May 08, 2019 at 11:42:01PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> The variable tad_base is being set to a value that is never read
> and is being over-written on the next iteration of a for-loop.
> This assignment is therefore redundant and can be removed.
> 
> Addresses-Coverity: ("Unused value")

What's that tag's function supposed to be?

I see a lot of those in commit messages but it is nowhere documented in
the tree.

$ git grep -i coverity

doesn't give anything relevant.

Hmm?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] EDAC, sb_edac: remove redundant update of tad_base

[Colin Ian King]

On 09/05/2019 15:13, Borislav Petkov wrote:
> On Wed, May 08, 2019 at 11:42:01PM +0100, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> The variable tad_base is being set to a value that is never read
>> and is being over-written on the next iteration of a for-loop.
>> This assignment is therefore redundant and can be removed.
>>
>> Addresses-Coverity: ("Unused value")
> 
> What's that tag's function supposed to be?

These are the Coverity static analysis warning/error message
classifications.  Tagging them should be useful for several reasons:

1. We can classify the types of issues being fixed
2. We can see how many issues are being found/fixed with the use of
static analysis tools like Coverity
3. It provides some context on how these bugs were being found.

I hope that helps.

> 
> I see a lot of those in commit messages but it is nowhere documented in
> the tree.
> 
> $ git grep -i coverity
> 
> doesn't give anything relevant.
> 
> Hmm?
> 

Re: [PATCH] EDAC, sb_edac: remove redundant update of tad_base

[Borislav Petkov]

On Thu, May 09, 2019 at 03:29:42PM +0100, Colin Ian King wrote:
> These are the Coverity static analysis warning/error message
> classifications.  Tagging them should be useful for several reasons:
> 
> 1. We can classify the types of issues being fixed
> 2. We can see how many issues are being found/fixed with the use of
> static analysis tools like Coverity

Who's "We"?

> 3. It provides some context on how these bugs were being found.

I figured as much but I have more questions:

* you say "tools like Coverity" but the name Coverity is in the tag.
So another tool would want to add its own tag. Which begs the second
question:

* has it ever been discussed and/or agreed upon all those "tools" tags?

Because we remove internal tags which have no bearing on the upstream
kernel. When I see that tag, how can I find out what it means? Can I run
coverity myself?

Lemme dig another one:

Addresses-Coverity-ID: 744899 ("Missing break in switch")

Where do I look up that ID?

And so on...

Bottom line of what I'm trying to say is, those tags better be useful to
the general kernel audience - that means, they should be documented so
that people can look them up - or better not be in commit messages at
all.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] EDAC, sb_edac: remove redundant update of tad_base

[Dan Carpenter]

On Thu, May 09, 2019 at 04:41:13PM +0200, Borislav Petkov wrote:
> Bottom line of what I'm trying to say is, those tags better be useful to
> the general kernel audience - that means, they should be documented so
> that people can look them up - or better not be in commit messages at
> all.

Other people will complain if you don't mention the tool name...

You can't win.  :(

regards,
dan carpenter

Re: [PATCH] EDAC, sb_edac: remove redundant update of tad_base

[Borislav Petkov]

On Thu, May 09, 2019 at 05:46:50PM +0300, Dan Carpenter wrote:
> On Thu, May 09, 2019 at 04:41:13PM +0200, Borislav Petkov wrote:
> > Bottom line of what I'm trying to say is, those tags better be useful to
> > the general kernel audience - that means, they should be documented so
> > that people can look them up - or better not be in commit messages at
> > all.
> 
> Other people will complain if you don't mention the tool name...

To quote again what I said: "... they should be documented so that
people can look them up... "

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] EDAC, sb_edac: remove redundant update of tad_base

[Colin Ian King]

On 09/05/2019 15:41, Borislav Petkov wrote:
> On Thu, May 09, 2019 at 03:29:42PM +0100, Colin Ian King wrote:
>> These are the Coverity static analysis warning/error message
>> classifications.  Tagging them should be useful for several reasons:
>>
>> 1. We can classify the types of issues being fixed
>> 2. We can see how many issues are being found/fixed with the use of
>> static analysis tools like Coverity
> 
> Who's "We"?

Well, I'm assuming folk who are using Coverity and folk who like
tracking bug stats.

> 
>> 3. It provides some context on how these bugs were being found.
> 
> I figured as much but I have more questions:
> 
> * you say "tools like Coverity" but the name Coverity is in the tag.
> So another tool would want to add its own tag. Which begs the second
> question:
> 
> * has it ever been discussed and/or agreed upon all those "tools" tags?
> 
> Because we remove internal tags which have no bearing on the upstream
> kernel. When I see that tag, how can I find out what it means? Can I run
> coverity myself?

Synopsis provide CoverityScan which can be used for free. There are
several instances of projects on the scan website that are analyzing the
kernel, for example:

https://scan.coverity.com/projects/linux
https://scan.coverity.com/projects/linux-next-weekly-scan

> 
> Lemme dig another one:
> 
> Addresses-Coverity-ID: 744899 ("Missing break in switch")
> 
> Where do I look up that ID?

https://scan.coverity.com/projects/linux

> 
> And so on...
> 
> Bottom line of what I'm trying to say is, those tags better be useful to
> the general kernel audience - that means, they should be documented so
> that people can look them up - or better not be in commit messages at
> all.

Yep, I agree, but explaining all the Coverity error types in a kernel
doc is going to take some effort, which I really don't have much time
for at the moment.

> 
> Thx.
> 

Colin

Re: [PATCH] EDAC, sb_edac: remove redundant update of tad_base

[Borislav Petkov]

On Thu, May 09, 2019 at 03:55:32PM +0100, Colin Ian King wrote:
> Yep, I agree, but explaining all the Coverity error types in a kernel
> doc is going to take some effort, which I really don't have much time
> for at the moment.

I'm not suggesting you should document them all or write a comprehensive
howto on how to run Coverity - all I'm suggesting is starting a small
doc somewhere in Documentation/ which contains some info on what all
those tools tags we use, mean and how people can find the information
they contain. Basically what you said above (which I've snipped).

But it would be a lot more helpful if it is written down so that people
can look it up. Also, that doc will serve as a documentation of all
those tags we're using in the kernel and what their format, etc would
be.

Otherwise we'll have the current head scratching when a tag like that is
encountered...

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.