2020-05-08 22:55:22

by Colin King

Subject: [PATCH] net: tg3: tidy up loop, remove need to compute off with a multiply

From: Colin Ian King <[email protected]>

Currently the value for 'off' is computed using a multiplication and
a couple of statements later off is being incremented by len and
this value is never read. Clean up the code by removing the
multiplication and just increment off by len on each iteration. Also
use len instead of TG3_OCIR_LEN.

Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King <[email protected]>
---
drivers/net/ethernet/broadcom/tg3.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index ff98a82b7bc4..9dd9bd506bcc 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -10798,16 +10798,14 @@ static int tg3_init_hw(struct tg3 *tp, bool reset_phy)
static void tg3_sd_scan_scratchpad(struct tg3 *tp, struct tg3_ocir *ocir)
{
int i;
+ u32 off, len = TG3_OCIR_LEN;

- for (i = 0; i < TG3_SD_NUM_RECS; i++, ocir++) {
- u32 off = i * TG3_OCIR_LEN, len = TG3_OCIR_LEN;
-
+ for (i = 0, off = 0; i < TG3_SD_NUM_RECS; i++, ocir++, off += len) {
tg3_ape_scratchpad_read(tp, (u32 *) ocir, off, len);
- off += len;

if (ocir->signature != TG3_OCIR_SIG_MAGIC ||
!(ocir->version_flags & TG3_OCIR_FLAG_ACTIVE))
- memset(ocir, 0, TG3_OCIR_LEN);
+ memset(ocir, 0, len);
}
}

--
2.25.1

2020-05-08 23:06:55

by Michael Chan

Subject: Re: [PATCH] net: tg3: tidy up loop, remove need to compute off with a multiply

On Fri, May 8, 2020 at 3:53 PM Colin King <[email protected]> wrote:
>
> From: Colin Ian King <[email protected]>
>
> Currently the value for 'off' is computed using a multiplication and
> a couple of statements later off is being incremented by len and
> this value is never read. Clean up the code by removing the
> multiplication and just increment off by len on each iteration. Also
> use len instead of TG3_OCIR_LEN.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King <[email protected]>
> ---
> drivers/net/ethernet/broadcom/tg3.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index ff98a82b7bc4..9dd9bd506bcc 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -10798,16 +10798,14 @@ static int tg3_init_hw(struct tg3 *tp, bool reset_phy)
> static void tg3_sd_scan_scratchpad(struct tg3 *tp, struct tg3_ocir *ocir)
> {
> int i;
> + u32 off, len = TG3_OCIR_LEN;

Please use reverse X-mas tree style variable declarations. Other than
that, it looks good.

Thanks.

>
> - for (i = 0; i < TG3_SD_NUM_RECS; i++, ocir++) {
> - u32 off = i * TG3_OCIR_LEN, len = TG3_OCIR_LEN;
> -
> + for (i = 0, off = 0; i < TG3_SD_NUM_RECS; i++, ocir++, off += len) {
> tg3_ape_scratchpad_read(tp, (u32 *) ocir, off, len);
> - off += len;
>
> if (ocir->signature != TG3_OCIR_SIG_MAGIC ||
> !(ocir->version_flags & TG3_OCIR_FLAG_ACTIVE))
> - memset(ocir, 0, TG3_OCIR_LEN);
> + memset(ocir, 0, len);
> }
> }
>
> --
> 2.25.1
>

2020-05-08 23:25:52

by Joe Perches

Subject: Re: [PATCH] net: tg3: tidy up loop, remove need to compute off with a multiply

On Fri, 2020-05-08 at 23:53 +0100, Colin King wrote:
> From: Colin Ian King <[email protected]>
>
> Currently the value for 'off' is computed using a multiplication and
> a couple of statements later off is being incremented by len and
> this value is never read. Clean up the code by removing the
> multiplication and just increment off by len on each iteration. Also
> use len instead of TG3_OCIR_LEN.

I think this is a lot harder to read.

> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
[]
> @@ -10798,16 +10798,14 @@ static int tg3_init_hw(struct tg3 *tp, bool reset_phy)
> static void tg3_sd_scan_scratchpad(struct tg3 *tp, struct tg3_ocir *ocir)
> {
> int i;
> + u32 off, len = TG3_OCIR_LEN;
>
> - for (i = 0; i < TG3_SD_NUM_RECS; i++, ocir++) {
> - u32 off = i * TG3_OCIR_LEN, len = TG3_OCIR_LEN;
> -
> + for (i = 0, off = 0; i < TG3_SD_NUM_RECS; i++, ocir++, off += len) {
> tg3_ape_scratchpad_read(tp, (u32 *) ocir, off, len);
> - off += len;
>
> if (ocir->signature != TG3_OCIR_SIG_MAGIC ||
> !(ocir->version_flags & TG3_OCIR_FLAG_ACTIVE))
> - memset(ocir, 0, TG3_OCIR_LEN);
> + memset(ocir, 0, len);
> }
> }

My preference would be for

{
int i;
u32 off = 0;

for (i = 0; i < TG3_SD_NUM_RECS; i++) {
tg3_ape_scratchpad_read(tp, (u32 *)ocir, off, TC3_OCIR_LEN);

if (ocir->signature != TG3_OCIR_SIG_MAGIC ||
!(ocir->version_flags & TG3_OCIR_FLAG_ACTIVE))
memset(ocir, 0, TG3_OCIR_LEN);

off += TG3_OCIR_LEN;
ocir++;
}

2020-05-08 23:34:51

by Colin King

Subject: Re: [PATCH] net: tg3: tidy up loop, remove need to compute off with a multiply

On 09/05/2020 00:21, Joe Perches wrote:
> On Fri, 2020-05-08 at 23:53 +0100, Colin King wrote:
>> From: Colin Ian King <[email protected]>
>>
>> Currently the value for 'off' is computed using a multiplication and
>> a couple of statements later off is being incremented by len and
>> this value is never read. Clean up the code by removing the
>> multiplication and just increment off by len on each iteration. Also
>> use len instead of TG3_OCIR_LEN.
>
> I think this is a lot harder to read.
>
>> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> []
>> @@ -10798,16 +10798,14 @@ static int tg3_init_hw(struct tg3 *tp, bool reset_phy)
>> static void tg3_sd_scan_scratchpad(struct tg3 *tp, struct tg3_ocir *ocir)
>> {
>> int i;
>> + u32 off, len = TG3_OCIR_LEN;
>>
>> - for (i = 0; i < TG3_SD_NUM_RECS; i++, ocir++) {
>> - u32 off = i * TG3_OCIR_LEN, len = TG3_OCIR_LEN;
>> -
>> + for (i = 0, off = 0; i < TG3_SD_NUM_RECS; i++, ocir++, off += len) {
>> tg3_ape_scratchpad_read(tp, (u32 *) ocir, off, len);
>> - off += len;
>>
>> if (ocir->signature != TG3_OCIR_SIG_MAGIC ||
>> !(ocir->version_flags & TG3_OCIR_FLAG_ACTIVE))
>> - memset(ocir, 0, TG3_OCIR_LEN);
>> + memset(ocir, 0, len);
>> }
>> }
>
> My preference would be for
>
> {
> int i;
> u32 off = 0;
>
> for (i = 0; i < TG3_SD_NUM_RECS; i++) {
> tg3_ape_scratchpad_read(tp, (u32 *)ocir, off, TC3_OCIR_LEN);
>
> if (ocir->signature != TG3_OCIR_SIG_MAGIC ||
> !(ocir->version_flags & TG3_OCIR_FLAG_ACTIVE))
> memset(ocir, 0, TG3_OCIR_LEN);
>
> off += TG3_OCIR_LEN;
> ocir++;
> }
>
OK, I'll send a V3 tomorrow.

2020-05-09 01:49:55

by Jakub Kicinski

Subject: Re: [PATCH] net: tg3: tidy up loop, remove need to compute off with a multiply

On Sat, 9 May 2020 00:31:03 +0100 Colin Ian King wrote:
> > My preference would be for
> >
> > {
> > int i;
> > u32 off = 0;
> >
> > for (i = 0; i < TG3_SD_NUM_RECS; i++) {
> > tg3_ape_scratchpad_read(tp, (u32 *)ocir, off, TC3_OCIR_LEN);
> >
> > if (ocir->signature != TG3_OCIR_SIG_MAGIC ||
> > !(ocir->version_flags & TG3_OCIR_FLAG_ACTIVE))
> > memset(ocir, 0, TG3_OCIR_LEN);
> >
> > off += TG3_OCIR_LEN;
> > ocir++;
> > }
> >
> OK, I'll send a V3 tomorrow.

I already reviewed and applied v2, just waiting for builds to finish,
let's leave it.

2020-05-09 02:35:33

by Joe Perches

Subject: Re: [PATCH] net: tg3: tidy up loop, remove need to compute off with a multiply

On Fri, 2020-05-08 at 18:48 -0700, Jakub Kicinski wrote:
> On Sat, 9 May 2020 00:31:03 +0100 Colin Ian King wrote:
> > > My preference would be for
> > >
> > > {
> > > int i;
> > > u32 off = 0;
> > >
> > > for (i = 0; i < TG3_SD_NUM_RECS; i++) {
> > > tg3_ape_scratchpad_read(tp, (u32 *)ocir, off, TC3_OCIR_LEN);
> > >
> > > if (ocir->signature != TG3_OCIR_SIG_MAGIC ||
> > > !(ocir->version_flags & TG3_OCIR_FLAG_ACTIVE))
> > > memset(ocir, 0, TG3_OCIR_LEN);
> > >
> > > off += TG3_OCIR_LEN;
> > > ocir++;
> > > }
> > >
> > OK, I'll send a V3 tomorrow.
>
> I already reviewed and applied v2, just waiting for builds to finish,
> let's leave it.


I think clarity should be preferred.
Are you a maintainer of this file?

$ ./scripts/get_maintainer.pl -f drivers/net/ethernet/broadcom/tg3.c
Siva Reddy Kallam <[email protected]> (supporter:BROADCOM TG3 GIGABIT ETHERNET DRIVER)
Prashant Sreedharan <[email protected]> (supporter:BROADCOM TG3 GIGABIT ETHERNET DRIVER)
Michael Chan <[email protected]> (supporter:BROADCOM TG3 GIGABIT ETHERNET DRIVER)
"David S. Miller" <[email protected]> (odd fixer:NETWORKING DRIVERS)
[email protected] (open list:BROADCOM TG3 GIGABIT ETHERNET DRIVER)
[email protected] (open list)

2020-05-09 04:50:49

by Michael Chan

Subject: Re: [PATCH] net: tg3: tidy up loop, remove need to compute off with a multiply

On Fri, May 8, 2020 at 7:31 PM Joe Perches <[email protected]> wrote:
>
> On Fri, 2020-05-08 at 18:48 -0700, Jakub Kicinski wrote:
> > On Sat, 9 May 2020 00:31:03 +0100 Colin Ian King wrote:
> > > > My preference would be for
> > > >
> > > > {
> > > > int i;
> > > > u32 off = 0;
> > > >
> > > > for (i = 0; i < TG3_SD_NUM_RECS; i++) {
> > > > tg3_ape_scratchpad_read(tp, (u32 *)ocir, off, TC3_OCIR_LEN);
> > > >
> > > > if (ocir->signature != TG3_OCIR_SIG_MAGIC ||
> > > > !(ocir->version_flags & TG3_OCIR_FLAG_ACTIVE))
> > > > memset(ocir, 0, TG3_OCIR_LEN);
> > > >
> > > > off += TG3_OCIR_LEN;
> > > > ocir++;
> > > > }
> > > >
> > > OK, I'll send a V3 tomorrow.
> >
> > I already reviewed and applied v2, just waiting for builds to finish,
> > let's leave it.
>
>
> I think clarity should be preferred.

Either way is fine with me. I'm fine with v2 since it's already applied.