[PATCH] ath5k: Fix eeprom checksum check for custom sized eeproms

[PATCH] ath5k: Fix eeprom checksum check for custom sized eeproms

[Luis R. Rodriguez]

Commit 8bf3d79bc401ca417ccf9fc076d3295d1a71dbf5 enabled EEPROM
checksum checks to avoid bogus bug reports but failed to address
updating the code to consider devices with custom EEPROM sizes.
Devices with custom sized EEPROMs have the upper limit size stuffed
in the EEPROM. Use this as the upper limit instead of the static
default size. In case of a checksum error also provide back the
max size and whether or not this was the default size or a custom
one. If the EEPROM is busted we add a failsafe check to ensure
we don't loop forever or try to read bogus areas of hardware.

This closes bug 14874

http://bugzilla.kernel.org/show_bug.cgi?id=14874

Cc: stable@kernel.org
Cc: David Quan <david.quan@atheros.com>
Cc: Stephen Beahm <stephenbeahm@comcast.net>
Reported-by: Joshua Covington <joshuacov@googlemail.com>
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---

This should apply on >= 2.6.32.2 cleanly. This is not applicable to
older kernels.

 drivers/net/wireless/ath/ath5k/eeprom.c |   32 ++++++++++++++++++++++++++++--
 drivers/net/wireless/ath/ath5k/eeprom.h |    8 +++++++
 2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/eeprom.c b/drivers/net/wireless/ath/ath5k/eeprom.c
index 5d1c867..6a3f4da 100644
--- a/drivers/net/wireless/ath/ath5k/eeprom.c
+++ b/drivers/net/wireless/ath/ath5k/eeprom.c
@@ -97,7 +97,7 @@ ath5k_eeprom_init_header(struct ath5k_hw *ah)
 	struct ath5k_eeprom_info *ee = &ah->ah_capabilities.cap_eeprom;
 	int ret;
 	u16 val;
-	u32 cksum, offset;
+	u32 cksum, offset, eep_max = AR5K_EEPROM_INFO_MAX;
 
 	/*
 	 * Read values from EEPROM and store them in the capability structure
@@ -116,12 +116,38 @@ ath5k_eeprom_init_header(struct ath5k_hw *ah)
 	 * Validate the checksum of the EEPROM date. There are some
 	 * devices with invalid EEPROMs.
 	 */
-	for (cksum = 0, offset = 0; offset < AR5K_EEPROM_INFO_MAX; offset++) {
+	AR5K_EEPROM_READ(AR5K_EEPROM_SIZE_UPPER, val);
+	if (val) {
+		eep_max = (val & AR5K_EEPROM_SIZE_UPPER_MASK) <<
+			   AR5K_EEPROM_SIZE_ENDLOC_SHIFT;
+		AR5K_EEPROM_READ(AR5K_EEPROM_SIZE_LOWER, val);
+		eep_max = (eep_max | val) - AR5K_EEPROM_INFO_BASE;
+
+		/*
+		 * Fail safe check to prevent stupid loops due
+		 * to busted EEPROMs. XXX: This value is likely too
+		 * big still, waiting on a better value.
+		 */
+		if (eep_max > (3 * AR5K_EEPROM_INFO_MAX)) {
+			ATH5K_ERR(ah->ah_sc, "Invalid max custom EEPROM size: "
+				  "%d (0x%04x) max expected: %d (0x%04x)\n",
+				  eep_max, eep_max,
+				  3 * AR5K_EEPROM_INFO_MAX,
+				  3 * AR5K_EEPROM_INFO_MAX);
+			return -EIO;
+		}
+	}
+
+	for (cksum = 0, offset = 0; offset < eep_max; offset++) {
 		AR5K_EEPROM_READ(AR5K_EEPROM_INFO(offset), val);
 		cksum ^= val;
 	}
 	if (cksum != AR5K_EEPROM_INFO_CKSUM) {
-		ATH5K_ERR(ah->ah_sc, "Invalid EEPROM checksum 0x%04x\n", cksum);
+		ATH5K_ERR(ah->ah_sc, "Invalid EEPROM "
+			  "checksum: 0x%04x eep_max: 0x%04x (%s)\n",
+			  cksum, eep_max,
+			  eep_max == AR5K_EEPROM_INFO_MAX ?
+				"default size" : "custom size");
 		return -EIO;
 	}
 
diff --git a/drivers/net/wireless/ath/ath5k/eeprom.h b/drivers/net/wireless/ath/ath5k/eeprom.h
index 0123f35..473a483 100644
--- a/drivers/net/wireless/ath/ath5k/eeprom.h
+++ b/drivers/net/wireless/ath/ath5k/eeprom.h
@@ -37,6 +37,14 @@
 #define AR5K_EEPROM_RFKILL_POLARITY_S	1
 
 #define AR5K_EEPROM_REG_DOMAIN		0x00bf	/* EEPROM regdom */
+
+/* FLASH(EEPROM) Defines for AR531X chips */
+#define AR5K_EEPROM_SIZE_LOWER		0x1b /* size info -- lower */
+#define AR5K_EEPROM_SIZE_UPPER		0x1c /* size info -- upper */
+#define AR5K_EEPROM_SIZE_UPPER_MASK	0xfff0
+#define AR5K_EEPROM_SIZE_UPPER_SHIFT	4
+#define AR5K_EEPROM_SIZE_ENDLOC_SHIFT	12
+
 #define AR5K_EEPROM_CHECKSUM		0x00c0	/* EEPROM checksum */
 #define AR5K_EEPROM_INFO_BASE		0x00c0	/* EEPROM header */
 #define AR5K_EEPROM_INFO_MAX		(0x400 - AR5K_EEPROM_INFO_BASE)
-- 
1.6.3.3

Re: [PATCH] ath5k: Fix eeprom checksum check for custom sized eeproms

[Nick Kossifidis]

2010/1/4 Luis R. Rodriguez <lrodriguez@atheros.com>:
>
> +
> +/* FLASH(EEPROM) Defines for AR531X chips */
> +#define AR5K_EEPROM_SIZE_LOWER         0x1b /* size info -- lower */
> +#define AR5K_EEPROM_SIZE_UPPER         0x1c /* size info -- upper */
> +#define AR5K_EEPROM_SIZE_UPPER_MASK    0xfff0
> +#define AR5K_EEPROM_SIZE_UPPER_SHIFT   4
> +#define AR5K_EEPROM_SIZE_ENDLOC_SHIFT  12
> +

AR531X chips are SoCs, are you sure this comment is correct ?

In the docs this marks the end of EAR section (and the end of
checksum) for EEPROMs larger than 16k
valid values for checksum end are 0x00000C0 to 0x0080000

Also to calculate EEPROM size (stored on the first 4 bits of 0x1c) you
do this according to the docs
2 ^ (EEPROM size + 9) and valid values are from 1 to 11 (11 = 1MB)

There is also an EEPROM size indicator on PCICFG but it seems it's
only for older chips.

A value of 0 on 0x1c means we have a 2k EEPROM and an end location 0x0000400.


-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [PATCH] ath5k: Fix eeprom checksum check for custom sized eeproms

[Luis R. Rodriguez]

On Mon, Jan 4, 2010 at 10:09 AM, Nick Kossifidis <mickflemm@gmail.com> wrote:
> 2010/1/4 Luis R. Rodriguez <lrodriguez@atheros.com>:
>>
>> +
>> +/* FLASH(EEPROM) Defines for AR531X chips */
>> +#define AR5K_EEPROM_SIZE_LOWER         0x1b /* size info -- lower */
>> +#define AR5K_EEPROM_SIZE_UPPER         0x1c /* size info -- upper */
>> +#define AR5K_EEPROM_SIZE_UPPER_MASK    0xfff0
>> +#define AR5K_EEPROM_SIZE_UPPER_SHIFT   4
>> +#define AR5K_EEPROM_SIZE_ENDLOC_SHIFT  12
>> +
>
> AR531X chips are SoCs, are you sure this comment is correct ?

I got that from the legacy HAL, so just using that as a source of
documentation for this.

> In the docs this marks the end of EAR section (and the end of
> checksum) for EEPROMs larger than 16k
> valid values for checksum end are 0x00000C0 to 0x0080000

I didn't see this used, just replicating what I saw in the legacy HAL
to match the code on Linux with what we have on other operating
systems. Not sure about the valid values for the checksum as per
documentation Vs what is implemented, I am just following what I see
implemented to ensure consistency.

> Also to calculate EEPROM size (stored on the first 4 bits of 0x1c) you
> do this according to the docs
> 2 ^ (EEPROM size + 9) and valid values are from 1 to 11 (11 = 1MB)

OK thanks so the eep_max should not be > 11 ?

> There is also an EEPROM size indicator on PCICFG but it seems it's
> only for older chips.

Yeah not sure.

> A value of 0 on 0x1c means we have a 2k EEPROM and an end location 0x0000400.

I don't see this being implemented in the legacy HAL so although it
may be documented I'd avoid adding the code unless we run into issue
with the existing code + this patch.

  Luis

Re: [PATCH] ath5k: Fix eeprom checksum check for custom sized eeproms

[Nick Kossifidis]

2010/1/4 Luis R. Rodriguez <lrodriguez@atheros.com>:
> On Mon, Jan 4, 2010 at 10:09 AM, Nick Kossifidis <mickflemm@gmail.com> wrote:
>> 2010/1/4 Luis R. Rodriguez <lrodriguez@atheros.com>:
>>>
>>> +
>>> +/* FLASH(EEPROM) Defines for AR531X chips */
>>> +#define AR5K_EEPROM_SIZE_LOWER         0x1b /* size info -- lower */
>>> +#define AR5K_EEPROM_SIZE_UPPER         0x1c /* size info -- upper */
>>> +#define AR5K_EEPROM_SIZE_UPPER_MASK    0xfff0
>>> +#define AR5K_EEPROM_SIZE_UPPER_SHIFT   4
>>> +#define AR5K_EEPROM_SIZE_ENDLOC_SHIFT  12
>>> +
>>
>> AR531X chips are SoCs, are you sure this comment is correct ?
>
> I got that from the legacy HAL, so just using that as a source of
> documentation for this.
>
>> In the docs this marks the end of EAR section (and the end of
>> checksum) for EEPROMs larger than 16k
>> valid values for checksum end are 0x00000C0 to 0x0080000
>
> I didn't see this used, just replicating what I saw in the legacy HAL
> to match the code on Linux with what we have on other operating
> systems. Not sure about the valid values for the checksum as per
> documentation Vs what is implemented, I am just following what I see
> implemented to ensure consistency.
>
>> Also to calculate EEPROM size (stored on the first 4 bits of 0x1c) you
>> do this according to the docs
>> 2 ^ (EEPROM size + 9) and valid values are from 1 to 11 (11 = 1MB)
>
> OK thanks so the eep_max should not be > 11 ?
>

I guess

eep_max > (3 * AR5K_EEPROM_INFO_MAX)

should be

eep_max > 1024



-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [PATCH] ath5k: Fix eeprom checksum check for custom sized eeproms

[Luis R. Rodriguez]

On Tue, Jan 05, 2010 at 03:06:11PM -0800, Nick Kossifidis wrote:
> 2010/1/4 Luis R. Rodriguez <lrodriguez@atheros.com>:
> > On Mon, Jan 4, 2010 at 10:09 AM, Nick Kossifidis <mickflemm@gmail.com> wrote:
> >> 2010/1/4 Luis R. Rodriguez <lrodriguez@atheros.com>:
> >>>
> >>> +
> >>> +/* FLASH(EEPROM) Defines for AR531X chips */
> >>> +#define AR5K_EEPROM_SIZE_LOWER         0x1b /* size info -- lower */
> >>> +#define AR5K_EEPROM_SIZE_UPPER         0x1c /* size info -- upper */
> >>> +#define AR5K_EEPROM_SIZE_UPPER_MASK    0xfff0
> >>> +#define AR5K_EEPROM_SIZE_UPPER_SHIFT   4
> >>> +#define AR5K_EEPROM_SIZE_ENDLOC_SHIFT  12
> >>> +
> >>
> >> AR531X chips are SoCs, are you sure this comment is correct ?
> >
> > I got that from the legacy HAL, so just using that as a source of
> > documentation for this.
> >
> >> In the docs this marks the end of EAR section (and the end of
> >> checksum) for EEPROMs larger than 16k
> >> valid values for checksum end are 0x00000C0 to 0x0080000
> >
> > I didn't see this used, just replicating what I saw in the legacy HAL
> > to match the code on Linux with what we have on other operating
> > systems. Not sure about the valid values for the checksum as per
> > documentation Vs what is implemented, I am just following what I see
> > implemented to ensure consistency.
> >
> >> Also to calculate EEPROM size (stored on the first 4 bits of 0x1c) you
> >> do this according to the docs
> >> 2 ^ (EEPROM size + 9) and valid values are from 1 to 11 (11 = 1MB)
> >
> > OK thanks so the eep_max should not be > 11 ?
> >
> 
> I guess
> 
> eep_max > (3 * AR5K_EEPROM_INFO_MAX)
> 
> should be
> 
> eep_max > 1024

; 0x400 - 0x00c0
	0x340
0x340 = 832

Whatever that means not sure. Have been lazy to read the EEPROM docs.
Anyway once we are certain of a value it can be updated. For now this
fixed some user issues, the main goal of the limit check was to prevent
out of range access to some bogus registers or doing an infinite loop
when a busted EEPROM was detected. This was already merged, once we
get the right value we can update this.

  Luis