ath5k: fix uninitialized value use in ath5k_eeprom_read_turbo_modes()

PATCH In review Pavel Roskin 8 participants 21 messages Aug 27, 2009 – Aug 31, 2009 Atheros ATH5K wireless driver

Share & tools

Pavel Roskin

author

2009-08-27 02:30 UTC #0 lore raw

PATCH 1/2 ath5k: fix uninitialized value use in ath5k_eeprom_read_turbo_modes()

The `val' variable in ath5k_eeprom_read_turbo_modes() is used uninitialized. gcc 4.4.1 with -fno-inline-functions-called-once reports it:

eeprom.c: In function 'ath5k_eeprom_read_turbo_modes':
eeprom.c:441: warning: 'val' may be used uninitialized in this function

Comparing the code to the Atheros HAL, it's clear that the split between ath5k_eeprom_read_modes() and ath5k_eeprom_read_turbo_modes() was incorrect.

The Atheros HAL reads both turbo and non-turbo data from EEPROM in one function. Some turbo mode parameters are derived from the same EEPROM values as non-turbo parameters, just from different bits.

Merge ath5k_eeprom_read_turbo_modes() into ath5k_eeprom_read_modes() to fix the warning. The actual values and offsets have been cross-checked against Atheros HAL.

Signed-off-by Pavel Roskin <[email protected]>

---
 drivers/net/wireless/ath/ath5k/eeprom.c |   29 +++++------------------------
 1 files changed, 5 insertions(+), 24 deletions(-)

drivers/net/wireless/ath/ath5k/eeprom.c +5 ~~-24~~

index 8af477d..644962a 100644
@@ -414,27 +414,11 @@ static int ath5k_eeprom_read_modes(struct ath5k_hw *ah, u32 *offset,
 		break;
 	}
 
-done:
-	/* return new offset */
-	*offset = o;
-
-	return 0;
-}
-
-/*
- * Read turbo mode information on newer EEPROM versions
- */
-static int
-ath5k_eeprom_read_turbo_modes(struct ath5k_hw *ah,
-			      u32 *offset, unsigned int mode)
-{
-	struct ath5k_eeprom_info *ee = &ah->ah_capabilities.cap_eeprom;
-	u32 o = *offset;
-	u16 val;
-	int ret;
-
+	/*
+	 * Read turbo mode information on newer EEPROM versions
+	 */
 	if (ee->ee_version < AR5K_EEPROM_VERSION_5_0)
-		return 0;
+		goto done;
 
 	switch (mode){
 	case AR5K_EEPROM_MODE_11A:

@@ -468,6 +452,7 @@ ath5k_eeprom_read_turbo_modes(struct ath5k_hw *ah,
 		break;
 	}
 
+done:
 	/* return new offset */
 	*offset = o;

@@ -504,10 +489,6 @@ ath5k_eeprom_init_modes(struct ath5k_hw *ah)
 		ret = ath5k_eeprom_read_modes(ah, &offset, mode);
 		if (ret)
 			return ret;
-
-		ret = ath5k_eeprom_read_turbo_modes(ah, &offset, mode);
-		if (ret)
-			return ret;
 	}
 
 	/* override for older eeprom versions for better performance */

Pavel Roskin

author

2009-08-27 02:30 UTC #1 lore raw

PATCH 2/2 ath5k: don't use PCI ID to find the chip revision

AR5K_SREV is available even if the chip has been put to sleep. Relying on the chip register allows binding non-standard PCI IDs by

echo VENDOR_ID PRODUCT_ID >/sys/bus/pci/drivers/ath5k/new_id

without having to specify the driver data as well.

Signed-off-by Pavel Roskin <[email protected]>

---
 drivers/net/wireless/ath/ath5k/ath5k.h  |    2 +-
 drivers/net/wireless/ath/ath5k/attach.c |   14 +++++++----
 drivers/net/wireless/ath/ath5k/base.c   |   38 ++++++++++++++++---------------
 3 files changed, 29 insertions(+), 25 deletions(-)

drivers/net/wireless/ath/ath5k/ath5k.h +1 -1

index cdc79cd..c2d4c6a 100644
@@ -1159,7 +1159,7 @@ struct ath5k_hw {
  */
 
 /* Attach/Detach Functions */
-extern struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc, u8 mac_version);
+extern struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc);
 extern void ath5k_hw_detach(struct ath5k_hw *ah);
 
 /* LED functions */

drivers/net/wireless/ath/ath5k/attach.c +9 -5

index 109ab7b..0d789ef 100644
@@ -95,14 +95,13 @@ static int ath5k_hw_post(struct ath5k_hw *ah)
  * ath5k_hw_attach - Check if hw is supported and init the needed structs
  *
  * @sc: The &struct ath5k_softc we got from the driver's attach function
- * @mac_version: The mac version id (check out ath5k.h) based on pci id
  *
  * Check if the device is supported, perform a POST and initialize the needed
  * structs. Returns -ENOMEM if we don't have memory for the needed structs,
  * -ENODEV if the device is not supported or prints an error msg if something
  * else went wrong.
  */
-struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc, u8 mac_version)
+struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc)
 {
 	struct ath5k_hw *ah;
 	struct pci_dev *pdev = sc->pdev;

@@ -136,9 +135,15 @@ struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc, u8 mac_version)
 	ah->ah_software_retry = false;
 
 	/*
-	 * Set the mac version based on the pci id
+	 * Find the mac version
 	 */
-	ah->ah_version = mac_version;
+	srev = ath5k_hw_reg_read(ah, AR5K_SREV);
+	if (srev < AR5K_SREV_AR5311)
+		ah->ah_version = AR5K_AR5210;
+	else if (srev < AR5K_SREV_AR5212)
+		ah->ah_version = AR5K_AR5211;
+	else
+		ah->ah_version = AR5K_AR5212;
 
 	/*Fill the ath5k_hw struct with the needed functions*/
 	ret = ath5k_hw_init_desc_functions(ah);

@@ -151,7 +156,6 @@ struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc, u8 mac_version)
 		goto err_free;
 
 	/* Get MAC, PHY and RADIO revisions */
-	srev = ath5k_hw_reg_read(ah, AR5K_SREV);
 	ah->ah_mac_srev = srev;
 	ah->ah_mac_version = AR5K_REG_MS(srev, AR5K_SREV_VER);
 	ah->ah_mac_revision = AR5K_REG_MS(srev, AR5K_SREV_REV);

drivers/net/wireless/ath/ath5k/base.c +19 ~~-19~~

index 94d46fd..9c6ab53 100644
@@ -84,24 +84,24 @@ MODULE_VERSION("0.6.0 (EXPERIMENTAL)");
 
 /* Known PCI ids */
 static const struct pci_device_id ath5k_pci_id_table[] = {
-	{ PCI_VDEVICE(ATHEROS, 0x0207), .driver_data = AR5K_AR5210 }, /* 5210 early */
-	{ PCI_VDEVICE(ATHEROS, 0x0007), .driver_data = AR5K_AR5210 }, /* 5210 */
-	{ PCI_VDEVICE(ATHEROS, 0x0011), .driver_data = AR5K_AR5211 }, /* 5311 - this is on AHB bus !*/
-	{ PCI_VDEVICE(ATHEROS, 0x0012), .driver_data = AR5K_AR5211 }, /* 5211 */
-	{ PCI_VDEVICE(ATHEROS, 0x0013), .driver_data = AR5K_AR5212 }, /* 5212 */
-	{ PCI_VDEVICE(3COM_2,  0x0013), .driver_data = AR5K_AR5212 }, /* 3com 5212 */
-	{ PCI_VDEVICE(3COM,    0x0013), .driver_data = AR5K_AR5212 }, /* 3com 3CRDAG675 5212 */
-	{ PCI_VDEVICE(ATHEROS, 0x1014), .driver_data = AR5K_AR5212 }, /* IBM minipci 5212 */
-	{ PCI_VDEVICE(ATHEROS, 0x0014), .driver_data = AR5K_AR5212 }, /* 5212 combatible */
-	{ PCI_VDEVICE(ATHEROS, 0x0015), .driver_data = AR5K_AR5212 }, /* 5212 combatible */
-	{ PCI_VDEVICE(ATHEROS, 0x0016), .driver_data = AR5K_AR5212 }, /* 5212 combatible */
-	{ PCI_VDEVICE(ATHEROS, 0x0017), .driver_data = AR5K_AR5212 }, /* 5212 combatible */
-	{ PCI_VDEVICE(ATHEROS, 0x0018), .driver_data = AR5K_AR5212 }, /* 5212 combatible */
-	{ PCI_VDEVICE(ATHEROS, 0x0019), .driver_data = AR5K_AR5212 }, /* 5212 combatible */
-	{ PCI_VDEVICE(ATHEROS, 0x001a), .driver_data = AR5K_AR5212 }, /* 2413 Griffin-lite */
-	{ PCI_VDEVICE(ATHEROS, 0x001b), .driver_data = AR5K_AR5212 }, /* 5413 Eagle */
-	{ PCI_VDEVICE(ATHEROS, 0x001c), .driver_data = AR5K_AR5212 }, /* PCI-E cards */
-	{ PCI_VDEVICE(ATHEROS, 0x001d), .driver_data = AR5K_AR5212 }, /* 2417 Nala */
+	{ PCI_VDEVICE(ATHEROS, 0x0207) }, /* 5210 early */
+	{ PCI_VDEVICE(ATHEROS, 0x0007) }, /* 5210 */
+	{ PCI_VDEVICE(ATHEROS, 0x0011) }, /* 5311 - this is on AHB bus !*/
+	{ PCI_VDEVICE(ATHEROS, 0x0012) }, /* 5211 */
+	{ PCI_VDEVICE(ATHEROS, 0x0013) }, /* 5212 */
+	{ PCI_VDEVICE(3COM_2,  0x0013) }, /* 3com 5212 */
+	{ PCI_VDEVICE(3COM,    0x0013) }, /* 3com 3CRDAG675 5212 */
+	{ PCI_VDEVICE(ATHEROS, 0x1014) }, /* IBM minipci 5212 */
+	{ PCI_VDEVICE(ATHEROS, 0x0014) }, /* 5212 combatible */
+	{ PCI_VDEVICE(ATHEROS, 0x0015) }, /* 5212 combatible */
+	{ PCI_VDEVICE(ATHEROS, 0x0016) }, /* 5212 combatible */
+	{ PCI_VDEVICE(ATHEROS, 0x0017) }, /* 5212 combatible */
+	{ PCI_VDEVICE(ATHEROS, 0x0018) }, /* 5212 combatible */
+	{ PCI_VDEVICE(ATHEROS, 0x0019) }, /* 5212 combatible */
+	{ PCI_VDEVICE(ATHEROS, 0x001a) }, /* 2413 Griffin-lite */
+	{ PCI_VDEVICE(ATHEROS, 0x001b) }, /* 5413 Eagle */
+	{ PCI_VDEVICE(ATHEROS, 0x001c) }, /* PCI-E cards */
+	{ PCI_VDEVICE(ATHEROS, 0x001d) }, /* 2417 Nala */
 	{ 0 }
 };
 MODULE_DEVICE_TABLE(pci, ath5k_pci_id_table);

@@ -566,7 +566,7 @@ ath5k_pci_probe(struct pci_dev *pdev,
 	}
 
 	/* Initialize device */
-	sc->ah = ath5k_hw_attach(sc, id->driver_data);
+	sc->ah = ath5k_hw_attach(sc);
 	if (IS_ERR(sc->ah)) {
 		ret = PTR_ERR(sc->ah);
 		goto err_irq;

Bob Copeland

Acked-by on v1 2/2

2009-08-27 03:45 UTC #2 lore raw

Re: [PATCH 2/2] ath5k: don't use PCI ID to find the chip revision

Quoted 8 lines · Pavel Roskin

AR5K_SREV is available even if the chip has been put to sleep. Relying on the chip register allows binding non-standard PCI IDs by
echo VENDOR_ID PRODUCT_ID >/sys/bus/pci/drivers/ath5k/new_id
without having to specify the driver data as well.
Signed-off-by Pavel Roskin <[email protected]>

Looks good, thanks!

Acked-by Bob Copeland <[email protected]>

Signature · 1 line

Bob Copeland %% www.bobcopeland.com

Nick Kossifidis

maintainer Acked-by on v1 2/2

2009-08-27 13:10 UTC #3 lore raw

Re: [ath5k-devel] [PATCH 2/2] ath5k: don't use PCI ID to find the chip revision

In reply to Pavel Roskin

2009/8/27 Pavel Roskin <[email protected]>:

Quoted 121 lines

AR5K_SREV is available even if the chip has been put to sleep. Relying on the chip register allows binding non-standard PCI IDs by

echo VENDOR_ID PRODUCT_ID >/sys/bus/pci/drivers/ath5k/new_id

without having to specify the driver data as well.

Signed-off-by Pavel Roskin <[email protected]>

---
drivers/net/wireless/ath/ath5k/ath5k.h | 2 +- drivers/net/wireless/ath/ath5k/attach.c | 14 +++++++---- drivers/net/wireless/ath/ath5k/base.c | 38 ++++++++++++++++---------------

 3 files changed, 29 insertions(+), 25 deletions(-)

drivers/net/wireless/ath/ath5k/ath5k.h +1 -1

index cdc79cd..c2d4c6a 100644
@@ -1159,7 +1159,7 @@ struct ath5k_hw {
  */
 
  /* Attach/Detach Functions */
-extern struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc, u8 mac_version);
+extern struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc);
  extern void ath5k_hw_detach(struct ath5k_hw *ah);
 
  /* LED functions */

drivers/net/wireless/ath/ath5k/attach.c +9 -5

index 109ab7b..0d789ef 100644
@@ -95,14 +95,13 @@ static int ath5k_hw_post(struct ath5k_hw *ah)
  * ath5k_hw_attach - Check if hw is supported and init the needed structs
  *
  * @sc: The &struct ath5k_softc we got from the driver's attach function
- * @mac_version: The mac version id (check out ath5k.h) based on pci id
  *
  * Check if the device is supported, perform a POST and initialize the needed
  * structs. Returns -ENOMEM if we don't have memory for the needed structs,
  * -ENODEV if the device is not supported or prints an error msg if something
  * else went wrong.
  */
-struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc, u8 mac_version)
+struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc)
  {
        struct ath5k_hw *ah;
        struct pci_dev *pdev = sc->pdev;

@@ -136,9 +135,15 @@ struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc, u8 mac_version)
        ah->ah_software_retry = false;
 
        /*
-        * Set the mac version based on the pci id
+        * Find the mac version
         */
-       ah->ah_version = mac_version;
+       srev = ath5k_hw_reg_read(ah, AR5K_SREV);
+       if (srev < AR5K_SREV_AR5311)
+               ah->ah_version = AR5K_AR5210;
+       else if (srev < AR5K_SREV_AR5212)
+               ah->ah_version = AR5K_AR5211;
+       else
+               ah->ah_version = AR5K_AR5212;
 
        /*Fill the ath5k_hw struct with the needed functions*/
        ret = ath5k_hw_init_desc_functions(ah);

@@ -151,7 +156,6 @@ struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc, u8 mac_version)
                goto err_free;
 
        /* Get MAC, PHY and RADIO revisions */
-       srev = ath5k_hw_reg_read(ah, AR5K_SREV);
        ah->ah_mac_srev = srev;
        ah->ah_mac_version = AR5K_REG_MS(srev, AR5K_SREV_VER);
        ah->ah_mac_revision = AR5K_REG_MS(srev, AR5K_SREV_REV);

drivers/net/wireless/ath/ath5k/base.c +18 ~~-18~~

index 94d46fd..9c6ab53 100644
@@ -84,24 +84,24 @@ MODULE_VERSION("0.6.0 (EXPERIMENTAL)");
 
  /* Known PCI ids */
  static const struct pci_device_id ath5k_pci_id_table[] = {
-       { PCI_VDEVICE(ATHEROS, 0x0207), .driver_data = AR5K_AR5210 }, /* 5210 early */
-       { PCI_VDEVICE(ATHEROS, 0x0007), .driver_data = AR5K_AR5210 }, /* 5210 */
-       { PCI_VDEVICE(ATHEROS, 0x0011), .driver_data = AR5K_AR5211 }, /* 5311 - this is on AHB bus !*/
-       { PCI_VDEVICE(ATHEROS, 0x0012), .driver_data = AR5K_AR5211 }, /* 5211 */
-       { PCI_VDEVICE(ATHEROS, 0x0013), .driver_data = AR5K_AR5212 }, /* 5212 */
-       { PCI_VDEVICE(3COM_2,  0x0013), .driver_data = AR5K_AR5212 }, /* 3com 5212 */
-       { PCI_VDEVICE(3COM,    0x0013), .driver_data = AR5K_AR5212 }, /* 3com 3CRDAG675 5212 */
-       { PCI_VDEVICE(ATHEROS, 0x1014), .driver_data = AR5K_AR5212 }, /* IBM minipci 5212 */
-       { PCI_VDEVICE(ATHEROS, 0x0014), .driver_data = AR5K_AR5212 }, /* 5212 combatible */
-       { PCI_VDEVICE(ATHEROS, 0x0015), .driver_data = AR5K_AR5212 }, /* 5212 combatible */
-       { PCI_VDEVICE(ATHEROS, 0x0016), .driver_data = AR5K_AR5212 }, /* 5212 combatible */
-       { PCI_VDEVICE(ATHEROS, 0x0017), .driver_data = AR5K_AR5212 }, /* 5212 combatible */
-       { PCI_VDEVICE(ATHEROS, 0x0018), .driver_data = AR5K_AR5212 }, /* 5212 combatible */
-       { PCI_VDEVICE(ATHEROS, 0x0019), .driver_data = AR5K_AR5212 }, /* 5212 combatible */
-       { PCI_VDEVICE(ATHEROS, 0x001a), .driver_data = AR5K_AR5212 }, /* 2413 Griffin-lite */
-       { PCI_VDEVICE(ATHEROS, 0x001b), .driver_data = AR5K_AR5212 }, /* 5413 Eagle */
-       { PCI_VDEVICE(ATHEROS, 0x001c), .driver_data = AR5K_AR5212 }, /* PCI-E cards */
-       { PCI_VDEVICE(ATHEROS, 0x001d), .driver_data = AR5K_AR5212 }, /* 2417 Nala */
+       { PCI_VDEVICE(ATHEROS, 0x0207) }, /* 5210 early */
+       { PCI_VDEVICE(ATHEROS, 0x0007) }, /* 5210 */
+       { PCI_VDEVICE(ATHEROS, 0x0011) }, /* 5311 - this is on AHB bus !*/
+       { PCI_VDEVICE(ATHEROS, 0x0012) }, /* 5211 */
+       { PCI_VDEVICE(ATHEROS, 0x0013) }, /* 5212 */
+       { PCI_VDEVICE(3COM_2,  0x0013) }, /* 3com 5212 */
+       { PCI_VDEVICE(3COM,    0x0013) }, /* 3com 3CRDAG675 5212 */
+       { PCI_VDEVICE(ATHEROS, 0x1014) }, /* IBM minipci 5212 */
+       { PCI_VDEVICE(ATHEROS, 0x0014) }, /* 5212 combatible */
+       { PCI_VDEVICE(ATHEROS, 0x0015) }, /* 5212 combatible */
+       { PCI_VDEVICE(ATHEROS, 0x0016) }, /* 5212 combatible */
+       { PCI_VDEVICE(ATHEROS, 0x0017) }, /* 5212 combatible */
+       { PCI_VDEVICE(ATHEROS, 0x0018) }, /* 5212 combatible */
+       { PCI_VDEVICE(ATHEROS, 0x0019) }, /* 5212 combatible */
+       { PCI_VDEVICE(ATHEROS, 0x001a) }, /* 2413 Griffin-lite */
+       { PCI_VDEVICE(ATHEROS, 0x001b) }, /* 5413 Eagle */
+       { PCI_VDEVICE(ATHEROS, 0x001c) }, /* PCI-E cards */
+       { PCI_VDEVICE(ATHEROS, 0x001d) }, /* 2417 Nala */
        { 0 }
  };
  MODULE_DEVICE_TABLE(pci, ath5k_pci_id_table);

drivers/net/wireless/ath/ath5k/base.c ath5k_pci_probe(struct pci_dev *pdev, · line 572

Quoted 8 lines

drivers/net/wireless/ath/ath5k/base.c +1 -1

@@ -566,7 +566,7 @@ ath5k_pci_probe(struct pci_dev *pdev,
        }
 
        /* Initialize device */
-       sc->ah = ath5k_hw_attach(sc, id->driver_data);
+       sc->ah = ath5k_hw_attach(sc);
        if (IS_ERR(sc->ah)) {
                ret = PTR_ERR(sc->ah);
                goto err_irq;

Acked-by Nick Kossifidis <[email protected]>

Signature · 3 lines

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

Bob Copeland

Acked-by on v1 1/2

2009-08-27 03:43 UTC #4 lore raw

In reply to Pavel Roskin

Quoted 6 lines · Pavel Roskin

The `val' variable in ath5k_eeprom_read_turbo_modes() is used uninitialized. gcc 4.4.1 with -fno-inline-functions-called-once reports it:
eeprom.c: In function 'ath5k_eeprom_read_turbo_modes':
eeprom.c:441: warning: 'val' may be used uninitialized in this function

Thanks!

Acked-by Bob Copeland <[email protected]>

Signature · 1 line

Bob Copeland %% www.bobcopeland.com

Nick Kossifidis

maintainer

2009-08-27 12:58 UTC #5 lore raw

In reply to Pavel Roskin

2009/8/27 Pavel Roskin <[email protected]>:

Quoted 15 lines

The `val' variable in ath5k_eeprom_read_turbo_modes() is used uninitialized. gcc 4.4.1 with -fno-inline-functions-called-once reports it:
eeprom.c: In function 'ath5k_eeprom_read_turbo_modes':
eeprom.c:441: warning: 'val' may be used uninitialized in this function
Comparing the code to the Atheros HAL, it's clear that the split between ath5k_eeprom_read_modes() and ath5k_eeprom_read_turbo_modes() was incorrect.
The Atheros HAL reads both turbo and non-turbo data from EEPROM in one function. Some turbo mode parameters are derived from the same EEPROM values as non-turbo parameters, just from different bits.

Quoted 5 lines

Merge ath5k_eeprom_read_turbo_modes() into ath5k_eeprom_read_modes() to fix the warning. The actual values and offsets have been cross-checked against Atheros HAL.
Signed-off-by Pavel Roskin <[email protected]>

Current code works fine (i 've checked it against various cards), there is nothing wrong
with having another function for reading turbo modes, i find it's cleaner that way.

Just change

u16 val;

u16 val = 0;

and it should be fine.

Signature · 3 lines

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

Bob Copeland

2009-08-27 18:17 UTC #6 lore raw

Quoted 6 lines · Nick Kossifidis

2009/8/27 Pavel Roskin <[email protected]>:
Current code works fine (i 've checked it against various cards), there is nothing wrong
with having another function for reading turbo modes, i find it's cleaner that way.

Well, we also don't use the turbo modes at all and that's where the error is (IIRC) so it shouldn't have any impact. :)

Signature · 1 line

Bob Copeland %% www.bobcopeland.com

Luis R. Rodriguez

2009-08-27 18:25 UTC #7 lore raw

Quoted 10 lines · Bob Copeland

Quoted 6 lines · Nick Kossifidis
2009/8/27 Pavel Roskin <[email protected]>:
Current code works fine (i 've checked it against various cards), there is nothing wrong
with having another function for reading turbo modes, i find it's cleaner that way.
Well, we also don't use the turbo modes at all and that's where the error is (IIRC) so it shouldn't have any impact. :)

Again, why don't we just remove all that fucking turbo cruft?

Luis

Bob Copeland

2009-08-27 21:40 UTC #8 lore raw

Quoted 4 lines · Luis R. Rodriguez

Quoted 2 lines
Well, we also don't use the turbo modes at all and that's where the error is (IIRC) so it shouldn't have any impact. :)
Again, why don't we just remove all that fucking turbo cruft?

OK with me since no one seems to care enough to implement mac80211 support. I left some of the #defines in place as they are useful documentation. Approx. 80 of the lines removed are comments.

text data bss dec hex filename

 136746	    480	     56	 137282	  21842	ath5k_old.ko
 134913	    480	     56	 135449	  21119	ath5k_new.ko

Disclaimer: only barely tested.

---
 drivers/net/wireless/ath/ath5k/ath5k.h    |   84 +-------
 drivers/net/wireless/ath/ath5k/attach.c   |    1 -
 drivers/net/wireless/ath/ath5k/base.c     |    7 -
 drivers/net/wireless/ath/ath5k/caps.c     |    6 -
 drivers/net/wireless/ath/ath5k/eeprom.c   |   68 +------
 drivers/net/wireless/ath/ath5k/eeprom.h   |    7 -
 drivers/net/wireless/ath/ath5k/initvals.c |  335 ++++++++++++++---------------
 drivers/net/wireless/ath/ath5k/pcu.c      |   16 +-
 drivers/net/wireless/ath/ath5k/phy.c      |   26 +---
 drivers/net/wireless/ath/ath5k/qcu.c      |   57 ++----
 drivers/net/wireless/ath/ath5k/reset.c    |  115 ++--------
 11 files changed, 223 insertions(+), 499 deletions(-)

drivers/net/wireless/ath/ath5k/ath5k.h +7 ~~-77~~

index 1275ba0..cf432ce 100644
@@ -349,73 +349,11 @@ struct ath5k_srev_name {
 /* IEEE defs */
 #define IEEE80211_MAX_LEN       2500
 
-/* TODO add support to mac80211 for vendor-specific rates and modes */
-
-/*
- * Some of this information is based on Documentation from:
- *
- * http://madwifi.org/wiki/ChipsetFeatures/SuperAG
- *
- * Modulation for Atheros' eXtended Range - range enhancing extension that is
- * supposed to double the distance an Atheros client device can keep a
- * connection with an Atheros access point. This is achieved by increasing
- * the receiver sensitivity up to, -105dBm, which is about 20dB above what
- * the 802.11 specifications demand. In addition, new (proprietary) data rates
- * are introduced: 3, 2, 1, 0.5 and 0.25 MBit/s.
- *
- * Please note that can you either use XR or TURBO but you cannot use both,
- * they are exclusive.
- *
- */
-#define MODULATION_XR 		0x00000200
-/*
- * Modulation for Atheros' Turbo G and Turbo A, its supposed to provide a
- * throughput transmission speed up to 40Mbit/s-60Mbit/s at a 108Mbit/s
- * signaling rate achieved through the bonding of two 54Mbit/s 802.11g
- * channels. To use this feature your Access Point must also suport it.
- * There is also a distinction between "static" and "dynamic" turbo modes:
- *
- * - Static: is the dumb version: devices set to this mode stick to it until
- *     the mode is turned off.
- * - Dynamic: is the intelligent version, the network decides itself if it
- *     is ok to use turbo. As soon as traffic is detected on adjacent channels
- *     (which would get used in turbo mode), or when a non-turbo station joins
- *     the network, turbo mode won't be used until the situation changes again.
- *     Dynamic mode is achieved by Atheros' Adaptive Radio (AR) feature which
- *     monitors the used radio band in order to decide whether turbo mode may
- *     be used or not.
- *
- * This article claims Super G sticks to bonding of channels 5 and 6 for
- * USA:
- *
- * http://www.pcworld.com/article/id,113428-page,1/article.html
- *
- * The channel bonding seems to be driver specific though. In addition to
- * deciding what channels will be used, these "Turbo" modes are accomplished
- * by also enabling the following features:
- *
- * - Bursting: allows multiple frames to be sent at once, rather than pausing
- *     after each frame. Bursting is a standards-compliant feature that can be
- *     used with any Access Point.
- * - Fast frames: increases the amount of information that can be sent per
- *     frame, also resulting in a reduction of transmission overhead. It is a
- *     proprietary feature that needs to be supported by the Access Point.
- * - Compression: data frames are compressed in real time using a Lempel Ziv
- *     algorithm. This is done transparently. Once this feature is enabled,
- *     compression and decompression takes place inside the chipset, without
- *     putting additional load on the host CPU.
- *
- */
-#define MODULATION_TURBO	0x00000080
-
 enum ath5k_driver_mode {
 	AR5K_MODE_11A		=	0,
-	AR5K_MODE_11A_TURBO	=	1,
-	AR5K_MODE_11B		=	2,
-	AR5K_MODE_11G		=	3,
-	AR5K_MODE_11G_TURBO	=	4,
-	AR5K_MODE_XR		=	0,
-	AR5K_MODE_MAX		=	5
+	AR5K_MODE_11B		=	1,
+	AR5K_MODE_11G		=	2,
+	AR5K_MODE_MAX		=	3
 };
 
 enum ath5k_ant_mode {

@@ -506,7 +444,6 @@ enum ath5k_tx_queue_id {
 	AR5K_TX_QUEUE_ID_CAB		= 6, /*IEEE80211_TX_QUEUE_AFTER_BEACON*/
 	AR5K_TX_QUEUE_ID_BEACON		= 7, /*IEEE80211_TX_QUEUE_BEACON*/
 	AR5K_TX_QUEUE_ID_UAPSD		= 8,
-	AR5K_TX_QUEUE_ID_XR_DATA	= 9,
 };
 
 /*

@@ -696,11 +633,6 @@ struct ath5k_gain {
 #define	CHANNEL_A	(CHANNEL_5GHZ|CHANNEL_OFDM)
 #define	CHANNEL_B	(CHANNEL_2GHZ|CHANNEL_CCK)
 #define	CHANNEL_G	(CHANNEL_2GHZ|CHANNEL_OFDM)
-#define	CHANNEL_T	(CHANNEL_5GHZ|CHANNEL_OFDM|CHANNEL_TURBO)
-#define	CHANNEL_TG	(CHANNEL_2GHZ|CHANNEL_OFDM|CHANNEL_TURBO)
-#define	CHANNEL_108A	CHANNEL_T
-#define	CHANNEL_108G	CHANNEL_TG
-#define	CHANNEL_X	(CHANNEL_5GHZ|CHANNEL_OFDM|CHANNEL_XR)
 
 #define	CHANNEL_ALL 	(CHANNEL_OFDM|CHANNEL_CCK|CHANNEL_2GHZ|CHANNEL_5GHZ| \
 		CHANNEL_TURBO)

@@ -712,7 +644,6 @@ struct ath5k_gain {
  * Used internaly for reset_tx_queue).
  * Also see struct struct ieee80211_channel.
  */
-#define IS_CHAN_XR(_c)	((_c->hw_value & CHANNEL_XR) != 0)
 #define IS_CHAN_B(_c)	((_c->hw_value & CHANNEL_B) != 0)
 
 /*

@@ -1035,7 +966,6 @@ struct ath5k_hw {
 
 	enum nl80211_iftype	ah_op_mode;
 	struct ieee80211_channel *ah_current_channel;
-	bool			ah_turbo;
 	bool			ah_calibration;
 	bool			ah_single_chip;
 	bool			ah_aes_support;

@@ -1315,18 +1245,18 @@ extern int ath5k_hw_set_txpower_limit(struct ath5k_hw *ah, u8 txpower);
  * Translate usec to hw clock units
  * TODO: Half/quarter rate
  */
-static inline unsigned int ath5k_hw_htoclock(unsigned int usec, bool turbo)
+static inline unsigned int ath5k_hw_htoclock(unsigned int usec)
 {
-	return turbo ? (usec * 80) : (usec * 40);
+	return (usec * 40);
 }
 
 /*
  * Translate hw clock units to usec
  * TODO: Half/quarter rate
  */
-static inline unsigned int ath5k_hw_clocktoh(unsigned int clock, bool turbo)
+static inline unsigned int ath5k_hw_clocktoh(unsigned int clock)
 {
-	return turbo ? (clock / 80) : (clock / 40);
+	return (clock / 40);
 }
 
 /*

drivers/net/wireless/ath/ath5k/attach.c +0 -1

index 4819f39..49d973b 100644
@@ -126,7 +126,6 @@ struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc, u8 mac_version)
 	 */
 	ah->ah_op_mode = NL80211_IFTYPE_STATION;
 	ah->ah_radar.r_enabled = AR5K_TUNE_RADAR_ALERT;
-	ah->ah_turbo = false;
 	ah->ah_txpower.txp_tpc = AR5K_TUNE_TPC_TXPOWER;
 	ah->ah_imr = 0;
 	ah->ah_atim_window = 0;

drivers/net/wireless/ath/ath5k/base.c +0 -7

index 94d46fd..b1804e5 100644
@@ -924,14 +924,12 @@ ath5k_copy_channels(struct ath5k_hw *ah,
 
 	switch (mode) {
 	case AR5K_MODE_11A:
-	case AR5K_MODE_11A_TURBO:
 		/* 1..220, but 2GHz frequencies are filtered by check_channel */
 		size = 220 ;
 		chfreq = CHANNEL_5GHZ;
 		break;
 	case AR5K_MODE_11B:
 	case AR5K_MODE_11G:
-	case AR5K_MODE_11G_TURBO:
 		size = 26;
 		chfreq = CHANNEL_2GHZ;
 		break;

@@ -960,11 +958,6 @@ ath5k_copy_channels(struct ath5k_hw *ah,
 		case AR5K_MODE_11G:
 			channels[count].hw_value = chfreq | CHANNEL_OFDM;
 			break;
-		case AR5K_MODE_11A_TURBO:
-		case AR5K_MODE_11G_TURBO:
-			channels[count].hw_value = chfreq |
-				CHANNEL_OFDM | CHANNEL_TURBO;
-			break;
 		case AR5K_MODE_11B:
 			channels[count].hw_value = CHANNEL_B;
 		}

drivers/net/wireless/ath/ath5k/caps.c +0 -6

index 367a6c7..2b9ffbb 100644
@@ -50,7 +50,6 @@ int ath5k_hw_set_capabilities(struct ath5k_hw *ah)
 
 		/* Set supported modes */
 		__set_bit(AR5K_MODE_11A, ah->ah_capabilities.cap_mode);
-		__set_bit(AR5K_MODE_11A_TURBO, ah->ah_capabilities.cap_mode);
 	} else {
 		/*
 		 * XXX The tranceiver supports frequencies from 4920 to 6100GHz

@@ -75,11 +74,6 @@ int ath5k_hw_set_capabilities(struct ath5k_hw *ah)
 			/* Set supported modes */
 			__set_bit(AR5K_MODE_11A,
 					ah->ah_capabilities.cap_mode);
-			__set_bit(AR5K_MODE_11A_TURBO,
-					ah->ah_capabilities.cap_mode);
-			if (ah->ah_version == AR5K_AR5212)
-				__set_bit(AR5K_MODE_11G_TURBO,
-						ah->ah_capabilities.cap_mode);
 		}
 
 		/* Enable  802.11b if a 2GHz capable radio (2111/5112) is

drivers/net/wireless/ath/ath5k/eeprom.c +2 ~~-66~~

105 lines not on this page · raw message

drivers/net/wireless/ath/ath5k/eeprom.h +0 -7

index 0123f35..0ca1758 100644
@@ -410,8 +410,6 @@ struct ath5k_eeprom_info {
 	u16	ee_i_cal[AR5K_EEPROM_N_MODES];
 	u16	ee_q_cal[AR5K_EEPROM_N_MODES];
 	u16	ee_fixed_bias[AR5K_EEPROM_N_MODES];
-	u16	ee_turbo_max_power[AR5K_EEPROM_N_MODES];
-	u16	ee_xr_power[AR5K_EEPROM_N_MODES];
 	u16	ee_switch_settling[AR5K_EEPROM_N_MODES];
 	u16	ee_atn_tx_rx[AR5K_EEPROM_N_MODES];
 	u16	ee_ant_control[AR5K_EEPROM_N_MODES][AR5K_EEPROM_N_PCDAC];

@@ -426,9 +424,6 @@ struct ath5k_eeprom_info {
 	u16	ee_x_gain[AR5K_EEPROM_N_MODES];
 	u16	ee_i_gain[AR5K_EEPROM_N_MODES];
 	u16	ee_margin_tx_rx[AR5K_EEPROM_N_MODES];
-	u16	ee_switch_settling_turbo[AR5K_EEPROM_N_MODES];
-	u16	ee_margin_tx_rx_turbo[AR5K_EEPROM_N_MODES];
-	u16	ee_atn_tx_rx_turbo[AR5K_EEPROM_N_MODES];
 
 	/* Power calibration data */
 	u16	ee_false_detect[AR5K_EEPROM_N_MODES];

@@ -458,8 +453,6 @@ struct ath5k_eeprom_info {
 	s16	ee_noise_floor_thr[AR5K_EEPROM_N_MODES];
 	s8	ee_adc_desired_size[AR5K_EEPROM_N_MODES];
 	s8	ee_pga_desired_size[AR5K_EEPROM_N_MODES];
-	s8	ee_adc_desired_size_turbo[AR5K_EEPROM_N_MODES];
-	s8	ee_pga_desired_size_turbo[AR5K_EEPROM_N_MODES];
 	s8	ee_pd_gain_overlap;
 
 	/* Spur mitigation data (fbin values for spur channels) */

drivers/net/wireless/ath/ath5k/initvals.c +167 ~~-168~~

534 lines not on this page · raw message

drivers/net/wireless/ath/ath5k/pcu.c +8 -8

index 2942f13..ab1065f 100644
@@ -185,7 +185,7 @@ unsigned int ath5k_hw_get_ack_timeout(struct ath5k_hw *ah)
 	ATH5K_TRACE(ah->ah_sc);
 
 	return ath5k_hw_clocktoh(AR5K_REG_MS(ath5k_hw_reg_read(ah,
-			AR5K_TIME_OUT), AR5K_TIME_OUT_ACK), ah->ah_turbo);
+			AR5K_TIME_OUT), AR5K_TIME_OUT_ACK));
 }
 
 /**

@@ -197,12 +197,12 @@ unsigned int ath5k_hw_get_ack_timeout(struct ath5k_hw *ah)
 int ath5k_hw_set_ack_timeout(struct ath5k_hw *ah, unsigned int timeout)
 {
 	ATH5K_TRACE(ah->ah_sc);
-	if (ath5k_hw_clocktoh(AR5K_REG_MS(0xffffffff, AR5K_TIME_OUT_ACK),
-			ah->ah_turbo) <= timeout)
+	if (ath5k_hw_clocktoh(AR5K_REG_MS(0xffffffff, AR5K_TIME_OUT_ACK))
+			<= timeout)
 		return -EINVAL;
 
 	AR5K_REG_WRITE_BITS(ah, AR5K_TIME_OUT, AR5K_TIME_OUT_ACK,
-		ath5k_hw_htoclock(timeout, ah->ah_turbo));
+		ath5k_hw_htoclock(timeout));
 
 	return 0;
 }

@@ -216,7 +216,7 @@ unsigned int ath5k_hw_get_cts_timeout(struct ath5k_hw *ah)
 {
 	ATH5K_TRACE(ah->ah_sc);
 	return ath5k_hw_clocktoh(AR5K_REG_MS(ath5k_hw_reg_read(ah,
-			AR5K_TIME_OUT), AR5K_TIME_OUT_CTS), ah->ah_turbo);
+			AR5K_TIME_OUT), AR5K_TIME_OUT_CTS));
 }
 
 /**

@@ -228,12 +228,12 @@ unsigned int ath5k_hw_get_cts_timeout(struct ath5k_hw *ah)
 int ath5k_hw_set_cts_timeout(struct ath5k_hw *ah, unsigned int timeout)
 {
 	ATH5K_TRACE(ah->ah_sc);
-	if (ath5k_hw_clocktoh(AR5K_REG_MS(0xffffffff, AR5K_TIME_OUT_CTS),
-			ah->ah_turbo) <= timeout)
+	if (ath5k_hw_clocktoh(AR5K_REG_MS(0xffffffff, AR5K_TIME_OUT_CTS))
+			<= timeout)
 		return -EINVAL;
 
 	AR5K_REG_WRITE_BITS(ah, AR5K_TIME_OUT, AR5K_TIME_OUT_CTS,
-			ath5k_hw_htoclock(timeout, ah->ah_turbo));
+			ath5k_hw_htoclock(timeout));
 
 	return 0;
 }

drivers/net/wireless/ath/ath5k/phy.c +1 ~~-25~~

index 1a039f2..e2a5606 100644
@@ -635,7 +635,7 @@ int ath5k_hw_rfregs_init(struct ath5k_hw *ah, struct ieee80211_channel *channel,
 	} else if ((channel->hw_value & CHANNEL_5GHZ) ||
 			(ah->ah_radio == AR5K_RF5111)) {
 
-		/* For 11a, Turbo and XR we need to choose
+		/* For 11a, we need to choose
 		 * OB/DB based on frequency range */
 		ee_mode = AR5K_EEPROM_MODE_11A;
 		obdb =	 channel->center_freq >= 5725 ? 3 :

@@ -1095,7 +1095,6 @@ int ath5k_hw_channel(struct ath5k_hw *ah, struct ieee80211_channel *channel)
 	}
 
 	ah->ah_current_channel = channel;
-	ah->ah_turbo = channel->hw_value == CHANNEL_T ? true : false;
 
 	return 0;
 }

@@ -1437,8 +1436,6 @@ ath5k_hw_set_spur_mitigation_filter(struct ath5k_hw *ah,
 	spur_chan_fbin = AR5K_EEPROM_NO_SPUR;
 	spur_detection_window = AR5K_SPUR_CHAN_WIDTH;
 	/* XXX: Half/Quarter channels ?*/
-	if (channel->hw_value & CHANNEL_TURBO)
-		spur_detection_window *= 2;
 
 	for (i = 0; i < AR5K_EEPROM_N_SPUR_CHANS; i++) {
 		spur_chan_fbin = ee->ee_spur_chans[i][freq_band];

@@ -1483,13 +1480,6 @@ ath5k_hw_set_spur_mitigation_filter(struct ath5k_hw *ah,
 			spur_delta_phase = (spur_offset << 17) / 25;
 			symbol_width = AR5K_SPUR_SYMBOL_WIDTH_BASE_100Hz;
 			break;
-		case CHANNEL_T:
-		case CHANNEL_TG:
-			/* Both sample_freq and chip_freq are 80MHz */
-			spur_delta_phase = (spur_offset << 16) / 25;
-			spur_freq_sigma_delta = (spur_delta_phase >> 10);
-			symbol_width = AR5K_SPUR_SYMBOL_WIDTH_TURBO_100Hz;
-			break;
 		default:
 			return;
 		}

@@ -1774,12 +1764,9 @@ ath5k_hw_set_antenna_mode(struct ath5k_hw *ah, u8 ant_mode)
 
 	switch (channel->hw_value & CHANNEL_MODES) {
 	case CHANNEL_A:
-	case CHANNEL_T:
-	case CHANNEL_XR:
 		ee_mode = AR5K_EEPROM_MODE_11A;
 		break;
 	case CHANNEL_G:
-	case CHANNEL_TG:
 		ee_mode = AR5K_EEPROM_MODE_11G;
 		break;
 	case CHANNEL_B:

@@ -2221,14 +2208,6 @@ ath5k_get_max_ctl_power(struct ath5k_hw *ah,
 	case CHANNEL_B:
 		ctl_mode |= AR5K_CTL_11B;
 		break;
-	case CHANNEL_T:
-		ctl_mode |= AR5K_CTL_TURBO;
-		break;
-	case CHANNEL_TG:
-		ctl_mode |= AR5K_CTL_TURBOG;
-		break;
-	case CHANNEL_XR:
-		/* Fall through */
 	default:
 		return;
 	}

@@ -3051,12 +3030,9 @@ int ath5k_hw_set_txpower_limit(struct ath5k_hw *ah, u8 txpower)
 
 	switch (channel->hw_value & CHANNEL_MODES) {
 	case CHANNEL_A:
-	case CHANNEL_T:
-	case CHANNEL_XR:
 		ee_mode = AR5K_EEPROM_MODE_11A;
 		break;
 	case CHANNEL_G:
-	case CHANNEL_TG:
 		ee_mode = AR5K_EEPROM_MODE_11G;
 		break;
 	case CHANNEL_B:

drivers/net/wireless/ath/ath5k/qcu.c +14 ~~-43~~

103 lines not on this page · raw message

drivers/net/wireless/ath/ath5k/reset.c +24 ~~-91~~

171 lines not on this page · raw message

Signature · 5 lines

1.6.2.5


-- 
Bob Copeland %% www.bobcopeland.com

Luis R. Rodriguez

Acked-by

2009-08-27 23:15 UTC #9 lore raw

Quoted 10 lines · Bob Copeland

Quoted 4 lines · Luis R. Rodriguez
Quoted 2 lines
Well, we also don't use the turbo modes at all and that's where the error is (IIRC) so it shouldn't have any impact. :)
Again, why don't we just remove all that fucking turbo cruft?
OK with me since no one seems to care enough to implement mac80211 support. I left some of the #defines in place as they are useful documentation. Approx. 80 of the lines removed are comments.

Quoted 5 lines

text data bss dec hex filename 136746 480 56 137282 21842 ath5k_old.ko 134913 480 56 135449 21119 ath5k_new.ko
Disclaimer: only barely tested.

Sexy, thanks!

Acked-by Luis R. Rodriguez <[email protected]>

Quoted 3 lines

{ AR5K_PHY(14),
- { 0x00000007, 0x00000007, 0x0000000b, 0x0000000b } },
+ { 0x00000007, 0x0000000b, 0x0000000b } },

Apart from this one initval all other ar5211_ini_mode[] values between 802.11a and 802.11g are the same! Which means we could potentially save (35 lines here * 4 bytes) + (3 entries for AR5K_PHY(14) * 4) = 152 bytes here if we just converge 11a and 11g as just OFDM data for this ini array and remove AR5K_PHY(14) entry and deal with it as a final write. I wonder if AR5K_PHY(14) (0x9838) was a typo for 11a or 11g. I'll try to dig to see what this is, perhaps its not a good idea to move this out and set this later, not sure if this is inline for the rf buffer trigger stuff, I'll poke.

All other ini arrays differ in more than 1 line so probably not worth mucking.

Luis

Luis R. Rodriguez

2009-08-28 00:09 UTC #10 lore raw

Quoted 35 lines · Luis R. Rodriguez

Quoted 10 lines · Bob Copeland
Quoted 4 lines · Luis R. Rodriguez
Quoted 2 lines
Well, we also don't use the turbo modes at all and that's where the error is (IIRC) so it shouldn't have any impact. :)
Again, why don't we just remove all that fucking turbo cruft?
OK with me since no one seems to care enough to implement mac80211 support. I left some of the #defines in place as they are useful documentation. Approx. 80 of the lines removed are comments.
Quoted 5 lines
text data bss dec hex filename 136746 480 56 137282 21842 ath5k_old.ko 134913 480 56 135449 21119 ath5k_new.ko
Disclaimer: only barely tested.
Sexy, thanks!
Acked-by Luis R. Rodriguez <[email protected]>
Quoted 3 lines
{ AR5K_PHY(14),
- { 0x00000007, 0x00000007, 0x0000000b, 0x0000000b } },
+ { 0x00000007, 0x0000000b, 0x0000000b } },
Apart from this one initval all other ar5211_ini_mode[] values between 802.11a and 802.11g are the same! Which means we could potentially save (35 lines here * 4 bytes) + (3 entries for AR5K_PHY(14) * 4) = 152 bytes here if we just converge 11a and 11g as just OFDM data for this ini array and remove AR5K_PHY(14) entry and deal with it as a final write. I wonder if AR5K_PHY(14) (0x9838) was a typo for 11a or 11g. I'll try to dig to see what this is, perhaps its not a good idea to move this out and set this later, not sure if this is inline for the rf buffer trigger stuff, I'll poke.

Quoted 1 line

All other ini arrays differ in more than 1 line so probably not worth mucking.

OK so upon review here is what I can determine from the above:

AR5211 supports 802.11a and 802.11b, so I suspect the 11g stuff was kept there for testing purposes, all of that is actually unused. I say we remove 11g stuff for AR5211 as well and save ourselves those bytes.

And for the record -- reordering the mode stuff seems fine except care must be taken to ensure all those writes are done prior to AR5K_PHY_ACT aka 0x981c aka AR5K_PHY(7).

The rf buffer stuff I mentioned earlier would not apply here as that is related to other stuff.

Luis

Nick Kossifidis

maintainer

2009-08-28 03:06 UTC #11 lore raw

2009/8/28 Luis R. Rodriguez <[email protected]>:

Quoted 36 lines · Luis R. Rodriguez

Quoted 35 lines · Bob Copeland
Quoted 10 lines · Luis R. Rodriguez
Quoted 4 lines
Quoted 2 lines
Well, we also don't use the turbo modes at all and that's where the error is (IIRC) so it shouldn't have any impact. :)
Again, why don't we just remove all that fucking turbo cruft?
OK with me since no one seems to care enough to implement mac80211 support. I left some of the #defines in place as they are useful documentation. Approx. 80 of the lines removed are comments.
Quoted 5 lines
text data bss dec hex filename 136746 480 56 137282 21842 ath5k_old.ko 134913 480 56 135449 21119 ath5k_new.ko
Disclaimer: only barely tested.
Sexy, thanks!
Acked-by Luis R. Rodriguez <[email protected]>
Quoted 3 lines
{ AR5K_PHY(14),
- { 0x00000007, 0x00000007, 0x0000000b, 0x0000000b } },
+ { 0x00000007, 0x0000000b, 0x0000000b } },
Apart from this one initval all other ar5211_ini_mode[] values between 802.11a and 802.11g are the same! Which means we could potentially save (35 lines here * 4 bytes) + (3 entries for AR5K_PHY(14) * 4) = 152 bytes here if we just converge 11a and 11g as just OFDM data for this ini array and remove AR5K_PHY(14) entry and deal with it as a final write. I wonder if AR5K_PHY(14) (0x9838) was a typo for 11a or 11g. I'll try to dig to see what this is, perhaps its not a good idea to move this out and set this later, not sure if this is inline for the rf buffer trigger stuff, I'll poke.

Quoted 7 lines

Quoted 1 line
All other ini arrays differ in more than 1 line so probably not worth mucking.
OK so upon review here is what I can determine from the above:
AR5211 supports 802.11a and 802.11b, so I suspect the 11g stuff was kept there for testing purposes, all of that is actually unused. I say we remove 11g stuff for AR5211 as well and save ourselves those bytes.

AR5211 supports draft g (ofdm only) so these values are correct, we currently don't support draft g on mac80211 but again i don't see why we should remove all this data, Atheros hasn't removed any of this from their HAL neither Sam (and MadWiFi/ath both support draft g -pureg- operation).

Signature · 3 lines

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

John W. Linville

2009-08-28 15:45 UTC #12 lore raw

In reply to Luis R. Rodriguez

Quoted 15 lines · Luis R. Rodriguez

Quoted 14 lines · Bob Copeland
Quoted 4 lines · Luis R. Rodriguez
Quoted 2 lines
Well, we also don't use the turbo modes at all and that's where the error is (IIRC) so it shouldn't have any impact. :)
Again, why don't we just remove all that fucking turbo cruft?
OK with me since no one seems to care enough to implement mac80211 support. I left some of the #defines in place as they are useful documentation. Approx. 80 of the lines removed are comments.
text data bss dec hex filename 136746 480 56 137282 21842 ath5k_old.ko 134913 480 56 135449 21119 ath5k_new.ko

Quoted 5 lines

Quoted 1 line
Disclaimer: only barely tested.
Sexy, thanks!
Acked-by Luis R. Rodriguez <[email protected]>

I think I am on the side of removing the non-standard extensions. FWIW, I'll need a patch posted with a proper Subject and Signed-off-by...

I think Nick has some valid points. His position sounds similar to that expressed to me by Felix and some others that it would be nice if we could more easily accomodate some of the features of madwifi that have made it attractive to researchers and experimenters. Perhaps NL80211_CMD_TESTMODE relates to this desire?

Just a thought...

John

Signature · 2 lines

John W. Linville		Someday the world will need a hero, and you
[email protected]			might be all we have.  Be ready.

Felix Fietkau

2009-08-29 01:35 UTC #13 lore raw

Quoted 9 lines · John W. Linville

I think I am on the side of removing the non-standard extensions. FWIW, I'll need a patch posted with a proper Subject and Signed-off-by...
I think Nick has some valid points. His position sounds similar to that expressed to me by Felix and some others that it would be nice if we could more easily accomodate some of the features of madwifi that have made it attractive to researchers and experimenters. Perhaps NL80211_CMD_TESTMODE relates to this desire?

Yes, that would be a good solution. I know a lot of people who are going continue to stick with madwifi if they won't be able to use turbo mode with ath5k. Since quite a few of those people are using OpenWrt, it would also mean that I would have to continue to maintain my madwifi semi-fork in OpenWrt even longer, and I definitely want to avoid that.

- Felix

Nick Kossifidis

maintainer

2009-08-28 03:01 UTC #14 lore raw

In reply to Luis R. Rodriguez

2009/8/27 Luis R. Rodriguez <[email protected]>:

Quoted 9 lines · Bob Copeland

Quoted 8 lines · Nick Kossifidis
Quoted 6 lines
2009/8/27 Pavel Roskin <[email protected]>:
Current code works fine (i 've checked it against various cards), there is nothing wrong
with having another function for reading turbo modes, i find it's cleaner that way.

Quoted 7 lines

Quoted 2 lines
Well, we also don't use the turbo modes at all and that's where the error is (IIRC) so it shouldn't have any impact. :)
Again, why don't we just remove all that fucking turbo cruft?
Luis

Why should we remove it, we are discussing on implementing channel width setting for 5 and 10 MHz channels already so where is the problem supporting turbo mode (40MHz) ?

Also EEPROM code should read the eeprom and fill the structs, since these infos are there we should read them, i don't see any reason to skip them, i thought our goal was to support this hw as much as possible, if we want to get rid of MadWiFi we 'll have to at least support 5, 10 and 40MHz (turbo) channels. I understand that there is no support yet on mac80211/cfg80211 but i don't think removing all this stuff and bring it back is the right thing to do.

Signature · 3 lines

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

Luis R. Rodriguez

2009-08-28 03:57 UTC #15 lore raw

Quoted 13 lines · Nick Kossifidis

2009/8/27 Luis R. Rodriguez <[email protected]>:
Quoted 12 lines · Bob Copeland
Quoted 10 lines · Nick Kossifidis
Quoted 6 lines
2009/8/27 Pavel Roskin <[email protected]>:
Current code works fine (i 've checked it against various cards), there is nothing wrong
with having another function for reading turbo modes, i find it's cleaner that way.
Well, we also don't use the turbo modes at all and that's where the error is (IIRC) so it shouldn't have any impact. :)

Quoted 8 lines

Quoted 4 lines
Again, why don't we just remove all that fucking turbo cruft?
Luis
Why should we remove it, we are discussing on implementing channel width setting for 5 and 10 MHz channels already so where is the problem supporting turbo mode (40MHz) ?

Supporting 5 MHz and 10 MHz channels is very different than supporting Turbo (40 MHz). 5 MHz and 10 MHz channels seems to be something you can use as per 802.11, 40 MHz "Turbo" stuff is just a vendor extension and at least by my part I don't want to move a finger to either support it nor do I think its a good idea to support it. Other people have objected to vendor extensions before on mac80211 so I don't think you'll find much support for this from a lot of people.

The way I see is if you want vendor extensions like Atheros Turbo or XR use MadWifi.

Quoted 3 lines

Also EEPROM code should read the eeprom and fill the structs, since these infos are there we should read them, i don't see any reason to skip them

I didn't see Bob's patch remove that stuff. Its pointless to use it though.

Quoted 2 lines

i thought our goal was to support this hw as much as possible,

We should support users as best as possible, whether or not you support vendor extensions is an entirely different story.

Quoted 2 lines

if we want to get rid of MadWiFi we 'll have to at least support 5, 10 and 40MHz (turbo) channels.

I don't want to get rid of MadWifi, what we have now is a proper upstream replacement. MadWifi is still a hack put together, and people who want hacks can use that.

Quoted 3 lines

I understand that there is
no support yet on mac80211/cfg80211 but i don't think removing all this stuff and bring it back is the right thing to do.

I don't expect it will come back.

Luis

Nick Kossifidis

maintainer

2009-08-28 04:17 UTC #16 lore raw

2009/8/28 Luis R. Rodriguez <[email protected]>:

Quoted 23 lines · Nick Kossifidis

Quoted 13 lines
2009/8/27 Luis R. Rodriguez <[email protected]>:
Quoted 12 lines · Bob Copeland
Quoted 10 lines · Nick Kossifidis
Quoted 6 lines
2009/8/27 Pavel Roskin <[email protected]>:
Current code works fine (i 've checked it against various cards), there is nothing wrong
with having another function for reading turbo modes, i find it's cleaner that way.
Well, we also don't use the turbo modes at all and that's where the error is (IIRC) so it shouldn't have any impact. :)
Quoted 8 lines
Quoted 4 lines
Again, why don't we just remove all that fucking turbo cruft?
Luis
Why should we remove it, we are discussing on implementing channel width setting for 5 and 10 MHz channels already so where is the problem supporting turbo mode (40MHz) ?

Quoted 8 lines

Supporting 5 MHz and 10 MHz channels is very different than supporting Turbo (40 MHz). 5 MHz and 10 MHz channels seems to be something you can use as per 802.11, 40 MHz "Turbo" stuff is just a vendor extension and at least by my part I don't want to move a finger to either support it nor do I think its a good idea to support it. Other people have objected to vendor extensions before on mac80211 so I don't think you'll find much support for this from a lot of people.

Many people use turbo mode and it's not an ugly proprietary extension, static turbo mode is close to just having 40MHz channels, we can use the same way to switch to it as with 5 and 10MHz channels. The difficult part is having dynamic turbo
(supporting 20MHz and 40MHz stations at the same time) but we don't deal with it anyway (and it's only useful on ap mode).

Most code is there, we are ready to support 5/10/40MHz channels on the driver part
as soon as we are done with cfg80211/mac80211 compatibility so why drop it ?

Quoted 3 lines

The way I see is if you want vendor extensions like Atheros Turbo or XR use MadWifi.

XR is not supported on MadWiFi, i remember only some parts were supported + we don't have much to work with anyway (XR code is missing from Legacy and Sam's HAL) + not many people use it anyway so i agree we can drop it but it's nothing like turbo mode, as i said many people use that.

Quoted 6 lines

Quoted 3 lines
Also EEPROM code should read the eeprom and fill the structs, since these infos are there we should read them, i don't see any reason to skip them
I didn't see Bob's patch remove that stuff. Its pointless to use it though.

It can be confusing (offsets missing etc) + we might want to put eeprom infos on debugfs (i had an ugly patch just for that) for debuging.

Quoted 6 lines

Quoted 2 lines
i thought our goal was to support this hw as much as possible,
We should support users as best as possible, whether or not you support vendor extensions is an entirely different story.

Quoted 7 lines

Quoted 2 lines
if we want to get rid of MadWiFi we 'll have to at least support 5, 10 and 40MHz (turbo) channels.
I don't want to get rid of MadWifi, what we have now is a proper upstream replacement. MadWifi is still a hack put together, and people who want hacks can use that.

Having multiple drivers won't help users, i thought that MadWiFi was "dead" and we were working on a complete alternative.

Signature · 3 lines

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

Luis R. Rodriguez

2009-08-28 05:21 UTC #17 lore raw

Quoted 2 lines · Nick Kossifidis

Many people use turbo mode and it's not an ugly proprietary extension, static turbo mode is close to just having 40MHz channels,

Its not following any spec and I suspect it will create pretty noise on existing wireless networks. 11n has at least some precautions to try to be friendly, such as trying using primary and extension channels to match nearby APs. I don't believe Atheros Turbo has such things.

Quoted 2 lines

we can use the same way to
switch to it as with 5 and 10MHz channels.

5 and 10 MHz seem to be defined and used as per 802.11 and those seem reasonable to support.

Quoted 2 lines

Most code is there, we are ready to support 5/10/40MHz channels on the driver part

Great.

Quoted 1 line

as soon as we are done with cfg80211/mac80211 compatibility so why drop it ?

We can drop Turbo, but it seems reasonable to keep 5 MHz and 10 MHz support.

Quoted 2 lines

Having multiple drivers won't help users, i thought that MadWiFi was "dead" and we were working on a complete alternative.

MadWifi is dead.

Luis

Nick Kossifidis

maintainer

2009-08-28 13:06 UTC #18 lore raw

2009/8/28 Luis R. Rodriguez <[email protected]>:

Quoted 11 lines · Nick Kossifidis

Quoted 3 lines
Many people use turbo mode and it's not an ugly proprietary extension, static turbo mode is close to just having 40MHz channels,
Its not following any spec and I suspect it will create pretty noise on existing wireless networks. 11n has at least some precautions to try to be friendly, such as trying using primary and extension channels to match nearby APs. I don't believe Atheros Turbo has such things.

With 16dB attenuation it's gone (check out the spectrum analyzer graph), it's no big deal + it only operates on channels we want (on channels with no "edges" eg as the whitepaper mentions)
http://www.super-g.com/collateral/atheros_superg_whitepaper.pdf

Trust me, many people are using turbo mode and it's no big deal to support it, code is there
and everyone else supports it (MadWiFi, ath, atheros windows driver etc). Dynamic turbo
is even better since it can operate on 40MHz on-demand only. If hw supports it then why shouldn't
we add support on ath5k ?

Quoted 3 lines

MadWifi is dead.

Well if people want turbo and other fancy stuff then they 'll keep it alive...

Signature · 3 lines

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

Jon Loeliger

2009-08-31 15:50 UTC #19 lore raw

In reply to Luis R. Rodriguez

Quoted 6 lines · Luis R. Rodriguez

Quoted 2 lines
Having multiple drivers won't help users, i thought that MadWiFi was "dead" and we were working on a complete alternative.
MadWifi is dead.
Luis

An interesting pronouncement. Do you mean "Dead to Linux"? Or "Dead to the world, including FreeBSD"?

jdl

Nick Kossifidis

maintainer

2009-08-28 03:17 UTC #20 lore raw

In reply to Nick Kossifidis

2009/8/27 Nick Kossifidis <[email protected]>:

Quoted 29 lines

2009/8/27 Pavel Roskin <[email protected]>:
Quoted 15 lines
The `val' variable in ath5k_eeprom_read_turbo_modes() is used uninitialized. gcc 4.4.1 with -fno-inline-functions-called-once reports it:
eeprom.c: In function 'ath5k_eeprom_read_turbo_modes':
eeprom.c:441: warning: 'val' may be used uninitialized in this function
Comparing the code to the Atheros HAL, it's clear that the split between ath5k_eeprom_read_modes() and ath5k_eeprom_read_turbo_modes() was incorrect.
The Atheros HAL reads both turbo and non-turbo data from EEPROM in one function. Some turbo mode parameters are derived from the same EEPROM values as non-turbo parameters, just from different bits.
Quoted 5 lines
Merge ath5k_eeprom_read_turbo_modes() into ath5k_eeprom_read_modes() to fix the warning. The actual values and offsets have been cross-checked against Atheros HAL.
Signed-off-by Pavel Roskin <[email protected]>
Current code works fine (i 've checked it against various cards), there is nothing wrong
with having another function for reading turbo modes, i find it's cleaner that way.
Just change

Quoted 8 lines

u16 val;
to
u16 val = 0;
and it should be fine.

Ouch seems my local tree and current wireless-testing are out of sync or something, yup compiler is right
you' ll need to put a

AR5K_EEPROM_READ(o, val);

before the switch.

Signature · 3 lines

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