[PATCH 0/2] staging: slicoss: cleanup checksum computation

[PATCH 0/2] staging: slicoss: cleanup checksum computation

[David Matlack]

This patchset fixes a broken checksum function and removes a struct
that was being used to ignore bad checksum values.

David Matlack (2):
  staging: slicoss: rewrite eeprom checksum code
  staging: slicoss: remove slic_reg_params struct

 drivers/staging/slicoss/slic.h    |   7 --
 drivers/staging/slicoss/slicoss.c | 140 +++++++++++++-------------------------
 2 files changed, 47 insertions(+), 100 deletions(-)

-- 
1.9.2

[PATCH 1/2] staging: slicoss: rewrite eeprom checksum code

[David Matlack]

Rewrite slic_eeprom_cksum() to fix bugs and make readable. The original
implementation had the following issues:

  1. 2 of the 3 unrolled loops had the following bug:

       while ((len -= 32) >= 0) {
               [...]
               sum += w[15];
               w = (u16 *)((ulong) w + 16);    /* verify */
       }

     This processes 32-bytes of data but only increments the word
     pointer by 16 bytes. Fixing both of these bugs seems to fix
     slic_eeprom_cksum().

  2. Non-descriptive variable names, use of unions, and macros that
     change local state make the code difficult to read.

  3. The checksum loop is unrolled which makes the code harder to
     reason about while providing small performance improvement:
      - max eeprom length is 0x80 bytes (MAX_EECODE_SIZE), that's
        only 0x40 iterations
      - checksum is only computed during pci probe(), so not very
        often

Tested on Mojave card. Also tested against the old implementation
(with the above bug fix applied) with test data.

Signed-off-by: David Matlack <matlackdavid@gmail.com>
---
 drivers/staging/slicoss/slicoss.c | 137 +++++++++++++-------------------------
 1 file changed, 46 insertions(+), 91 deletions(-)

diff --git a/drivers/staging/slicoss/slicoss.c b/drivers/staging/slicoss/slicoss.c
index dba6a00..d3fe8a7 100644
--- a/drivers/staging/slicoss/slicoss.c
+++ b/drivers/staging/slicoss/slicoss.c
@@ -1137,106 +1137,61 @@ static int slic_config_get(struct adapter *adapter, u32 config, u32 config_h)
 				0, 0);
 }
 
+static inline u16 __reduce(u32 checksum)
+{
+	u16 lower_16 = checksum & 0xFFFF;
+	u16 upper_16 = (checksum >> 16) & 0xFFFF;
+
+	checksum = lower_16 + upper_16;
+
+	if (checksum > 65535)
+		checksum -= 65535;
+
+	return checksum;
+}
+
 /*
- *  this is here to checksum the eeprom, there is some ucode bug
- *  which prevens us from using the ucode result.
- *  remove this once ucode is fixed.
+ * Compute a checksum of the eeprom. There is a firmware bug that prevents
+ * us from using the firmware result. Remove this once firmware is fixed.
+ *
+ * This function does not assume the eeprom begins at a word aligned address
+ * or that the length of the eeprom is a multiple of word size. These cases
+ * are handled by treating the leading and trailing byte specially and
+ * conditionally adding them to the checksum at the end.
  */
-static ushort slic_eeprom_cksum(char *m, int len)
+static u16 slic_eeprom_cksum(void *eeprom, unsigned len)
 {
-#define ADDCARRY(x)  (x > 65535 ? x -= 65535 : x)
-#define REDUCE {l_util.l = sum; sum = l_util.s[0] + l_util.s[1]; ADDCARRY(sum);\
-		}
-
-	u16 *w;
-	u32 sum = 0;
-	u32 byte_swapped = 0;
-	u32 w_int;
-
-	union {
-		char c[2];
-		ushort s;
-	} s_util;
-
-	union {
-		ushort s[2];
-		int l;
-	} l_util;
-
-	l_util.l = 0;
-	s_util.s = 0;
-
-	w = (u16 *)m;
-#if BITS_PER_LONG == 64
-	w_int = (u32) ((ulong) w & 0x00000000FFFFFFFF);
-#else
-	w_int = (u32) (w);
-#endif
-	if ((1 & w_int) && (len > 0)) {
-		REDUCE;
-		sum <<= 8;
-		s_util.c[0] = *(unsigned char *)w;
-		w = (u16 *)((char *)w + 1);
-		len--;
-		byte_swapped = 1;
+	u8 *bp = eeprom;
+	u8 *leading_byte = NULL;
+	u8 *trailing_byte = NULL;
+	u16 final_word = 0;
+	u32 checksum = 0;
+
+	if ((unsigned long) eeprom & 1) {
+		leading_byte = bp;
+		bp++;
 	}
 
-	/* Unroll the loop to make overhead from branches &c small. */
-	while ((len -= 32) >= 0) {
-		sum += w[0];
-		sum += w[1];
-		sum += w[2];
-		sum += w[3];
-		sum += w[4];
-		sum += w[5];
-		sum += w[6];
-		sum += w[7];
-		sum += w[8];
-		sum += w[9];
-		sum += w[10];
-		sum += w[11];
-		sum += w[12];
-		sum += w[13];
-		sum += w[14];
-		sum += w[15];
-		w = (u16 *)((ulong) w + 16);	/* verify */
-	}
-	len += 32;
-	while ((len -= 8) >= 0) {
-		sum += w[0];
-		sum += w[1];
-		sum += w[2];
-		sum += w[3];
-		w = (u16 *)((ulong) w + 4);	/* verify */
+	while ((void *) (bp + 1) < eeprom + len) {
+		checksum += *((u16 *) bp);
+		bp += 2;
 	}
-	len += 8;
-	if (len != 0 || byte_swapped != 0) {
-		REDUCE;
-		while ((len -= 2) >= 0)
-			sum += *w++;	/* verify */
-		if (byte_swapped) {
-			REDUCE;
-			sum <<= 8;
-			byte_swapped = 0;
-			if (len == -1) {
-				s_util.c[1] = *(char *) w;
-				sum += s_util.s;
-				len = 0;
-			} else {
-				len = -1;
-			}
 
-		} else if (len == -1) {
-			s_util.c[0] = *(char *) w;
-		}
+	if ((void *) bp < eeprom + len)
+		trailing_byte = bp;
 
-		if (len == -1) {
-			s_util.c[1] = 0;
-			sum += s_util.s;
-		}
+	if (leading_byte) {
+		checksum = __reduce(checksum);
+		checksum <<= 8;
+
+		final_word = *leading_byte;
+		if (trailing_byte)
+			final_word |= *trailing_byte << 8;
+	} else if (trailing_byte) {
+		final_word = *trailing_byte;
 	}
-	REDUCE;
-	return (ushort) sum;
+
+	return __reduce(checksum + final_word);
 }
 
 static void slic_rspqueue_free(struct adapter *adapter)
-- 
1.9.2

Re: [PATCH 1/2] staging: slicoss: rewrite eeprom checksum code

[Joe Perches]

On Sat, 2014-05-17 at 21:00 -0700, David Matlack wrote:
[]
> diff --git a/drivers/staging/slicoss/slicoss.c b/drivers/staging/slicoss/slicoss.c
[]
> +static inline u16 __reduce(u32 checksum)
> +{
> +	u16 lower_16 = checksum & 0xFFFF;
> +	u16 upper_16 = (checksum >> 16) & 0xFFFF;
> +
> +	checksum = lower_16 + upper_16;
> +
> +	if (checksum > 65535)
> +		checksum -= 65535;
> +
> +	return checksum;
> +}

The if seems unnecessary.

Perhaps declare a u16 return var or use

	return lower_16 + upper_16;

Re: [PATCH 1/2] staging: slicoss: rewrite eeprom checksum code

[David Matlack]

On Sat, May 17, 2014 at 9:12 PM, Joe Perches <joe@perches.com> wrote:
> On Sat, 2014-05-17 at 21:00 -0700, David Matlack wrote:
> []
>> diff --git a/drivers/staging/slicoss/slicoss.c b/drivers/staging/slicoss/slicoss.c
> []
>> +static inline u16 __reduce(u32 checksum)
>> +{
>> +     u16 lower_16 = checksum & 0xFFFF;
>> +     u16 upper_16 = (checksum >> 16) & 0xFFFF;
>> +
>> +     checksum = lower_16 + upper_16;
>> +
>> +     if (checksum > 65535)
>> +             checksum -= 65535;
>> +
>> +     return checksum;
>> +}
>
> The if seems unnecessary.
>
> Perhaps declare a u16 return var or use
>
>         return lower_16 + upper_16;

I agree it's fishy... but using overflow doesn't produce the same result:

         (u16) 65536   == 0
         65536 - 65535 == 1

Now which is the correct result, I have no idea. The eeprom on this device is
small (0x80 bytes max, not enough to trigger overflow) and I have no
documentation, so I don't know how to test :(

Re: [PATCH 1/2] staging: slicoss: rewrite eeprom checksum code

[David Matlack]

On Sat, May 17, 2014 at 9:51 PM, David Matlack <matlackdavid@gmail.com> wrote:
> On Sat, May 17, 2014 at 9:12 PM, Joe Perches <joe@perches.com> wrote:
>> The if seems unnecessary.
>>
>> Perhaps declare a u16 return var or use
>>
>>         return lower_16 + upper_16;
>
> I agree it's fishy... but using overflow doesn't produce the same result:
>
>          (u16) 65536   == 0
>          65536 - 65535 == 1
>
> Now which is the correct result, I have no idea.

I think the checksum algorithm being used here is RFC 1071 [1]. Which means the
if is correct and just accounting for double overflow.

> The eeprom on this device is
> small (0x80 bytes max, not enough to trigger overflow) and I have no

Sorry, I was wrong about this. I was thinking in terms of summing bytes,
but the checksum is summing words. Overflow _does_ get triggered.


I think I'll go over this patch again while looking at the RFC to make sure
everything is ok. Thanks!

[1] http://tools.ietf.org/html/rfc1071

Re: [PATCH 1/2] staging: slicoss: rewrite eeprom checksum code

[Dan Carpenter]

This patch is great, thanks, don't resend.  But I wish the subject said
"fix" instead of "rewrite".  I was expecting it to just be a cleanup.

It helps a lot if `git log --oneline` says which patches should be
backported.

regards,
dan carpenter

Re: [PATCH 1/2] staging: slicoss: rewrite eeprom checksum code

[David Matlack]

On Mon, May 19, 2014 at 2:16 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> This patch is great, thanks, don't resend.  But I wish the subject said
> "fix" instead of "rewrite".  I was expecting it to just be a cleanup.
>
> It helps a lot if `git log --oneline` says which patches should be
> backported.
>
> regards,
> dan carpenter
>

Thanks for the review Dan. I know you said not resend but I'd like to clean up
the patch a bit if you don't mind:

* Add a comment making it clear this is RFC 1071

* Remove the byte swapping code. Specifically,

+       if ((unsigned long) eeprom & 1) {
+               leading_byte = bp;
+               bp++;
        }

and

+       if (leading_byte) {
+               checksum = __reduce(checksum);
+               checksum <<= 8;
+
+               final_word = *leading_byte;
+               if (trailing_byte)
+                       final_word |= *trailing_byte << 8;
+       }

  After reading the RFC [1], I don't think the it's correct in either
  implementation (the bytes in the final result are in the wrong order). And
  since this function is only used to checksum the eeprom, and not checksum a
  fragment of a IP packet, it's not needed. So we can just remove it.

* s/rewrite/fix/ in the subject

[1] http://tools.ietf.org/html/rfc1071 (see Section 2.(A) and 2.(B))

Re: [PATCH 1/2] staging: slicoss: rewrite eeprom checksum code

[Dan Carpenter]

Sounds great.

regards,
dan carpenter

Re: [PATCH 1/2] staging: slicoss: rewrite eeprom checksum code

[Dan Carpenter]

When you resend, please put in the changelog what the affect of this fix
is for the user.  Even if it's something like "It doesn't affect the
user normally." that is useful information.

regards,
dan carpenter

[PATCH 2/2] staging: slicoss: remove slic_reg_params struct

[David Matlack]

Remove uninitialized struct that is only used to regulate whether
incorrect eeprom checksums are ignored.

Signed-off-by: David Matlack <matlackdavid@gmail.com>
---
 drivers/staging/slicoss/slic.h    | 7 -------
 drivers/staging/slicoss/slicoss.c | 3 +--
 2 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/staging/slicoss/slic.h b/drivers/staging/slicoss/slic.h
index 0dc73d5..3a5aa88 100644
--- a/drivers/staging/slicoss/slic.h
+++ b/drivers/staging/slicoss/slic.h
@@ -362,12 +362,6 @@ struct slic_shmem {
 	volatile struct slic_stats     inicstats;
 };
 
-struct slic_reg_params {
-	u32       linkspeed;
-	u32       linkduplex;
-	u32       fail_on_bad_eeprom;
-};
-
 struct slic_upr {
 	uint               adapter;
 	u32            upr_request;
@@ -492,7 +486,6 @@ struct adapter {
 	u32             intagg_period;
 	struct inicpm_state    *inicpm_info;
 	void *pinicpm_info;
-	struct slic_reg_params   reg_params;
 	struct slic_ifevents  if_events;
 	struct slic_stats        inicstats_prev;
 	struct slicnet_stats     slic_stats;
diff --git a/drivers/staging/slicoss/slicoss.c b/drivers/staging/slicoss/slicoss.c
index d3fe8a7..1dae641 100644
--- a/drivers/staging/slicoss/slicoss.c
+++ b/drivers/staging/slicoss/slicoss.c
@@ -2876,8 +2876,7 @@ static int slic_card_init(struct sliccard *card, struct adapter *adapter)
 				    sizeof(struct slic_eeprom),
 				    peeprom, phys_config);
 
-		if ((!card->config.EepromValid) &&
-		    (adapter->reg_params.fail_on_bad_eeprom)) {
+		if (!card->config.EepromValid) {
 			slic_reg64_write(adapter, &slic_regs->slic_isp, 0,
 					 &slic_regs->slic_addr_upper,
 					 0, FLUSH);
-- 
1.9.2

Re: [PATCH 2/2] staging: slicoss: remove slic_reg_params struct

[Dan Carpenter]

On Sat, May 17, 2014 at 09:00:04PM -0700, David Matlack wrote:
> Remove uninitialized struct that is only used to regulate whether
> incorrect eeprom checksums are ignored.
> 

Looking at the patch, this looks like a bugfix but the changelog doesn't
give any clues.

regards,
dan carpenter

Re: [PATCH 2/2] staging: slicoss: remove slic_reg_params struct

[David Matlack]

On Mon, May 19, 2014 at 2:21 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Looking at the patch, this looks like a bugfix but the changelog doesn't
> give any clues.

Yeah this isn't a bug fix.

The only member of struct slic_reg_params that was in use was fail_on_bad_eeprom
(implicitly set to zero by alloc_etherdev -> kzalloc). Since the previous patch
in this series fixes the eeprom checksum, we can remove this struct entirely.

If we do want the feature of ignoring a corrupt/bad eeprom, a module param
would work better anyway.

Re: [PATCH 2/2] staging: slicoss: remove slic_reg_params struct

[Dan Carpenter]

On Mon, May 19, 2014 at 08:57:49AM -0700, David Matlack wrote:
> On Mon, May 19, 2014 at 2:21 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > Looking at the patch, this looks like a bugfix but the changelog doesn't
> > give any clues.
> 
> Yeah this isn't a bug fix.
> 
> The only member of struct slic_reg_params that was in use was fail_on_bad_eeprom
> (implicitly set to zero by alloc_etherdev -> kzalloc). Since the previous patch
> in this series fixes the eeprom checksum, we can remove this struct entirely.
> 
> If we do want the feature of ignoring a corrupt/bad eeprom, a module param
> would work better anyway.

I think we are talking at cross purposes.

-               if ((!card->config.EepromValid) &&
-                   (adapter->reg_params.fail_on_bad_eeprom)) {
+               if (!card->config.EepromValid) {
                        slic_reg64_write(adapter, &slic_regs->slic_isp, 0,

In the original code then this if condition is never true because
->fail_on_bad_eeprom is zero.

You are saying that this condition is still not true because
->EepromValid is true now you fixed the checksum code.  I am saying
that *sometimes* it *could* be true if the eeprom is corrupt.

You have to understand that I review a lot of staging patches every day.
Most patches try to remove struct members but the code should still work
exactly as it did before (a clean up).  Some patches remove struct
members and the behavior changes.  Hopefully it's deliberate and the
changelog mentions that it is a bug fix.  If it changes the run time,
and not deliberately then that's a bug.

This code, your code, is a bugfix.  Thanks.  :)  Next time mention then
that you fixing stuff in changelog.

regards,
dan carpenter

Re: [PATCH 2/2] staging: slicoss: remove slic_reg_params struct

[David Matlack]

On Mon, May 19, 2014 at 12:52 PM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
>
> You have to understand that I review a lot of staging patches every day.
> Most patches try to remove struct members but the code should still work
> exactly as it did before (a clean up).  Some patches remove struct
> members and the behavior changes.  Hopefully it's deliberate and the
> changelog mentions that it is a bug fix.  If it changes the run time,
> and not deliberately then that's a bug.
>

Ah I see. This patch does indeed change the behavior. Since I plan to resend
the other patch in this series, I'll fix the changelog for this patch as well.

Thanks for the review.