| Message ID | 20090827023000.21926.90867.stgit@mj.roinet.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, Aug 26, 2009 at 10:30 PM, Pavel Roskin<proski@gnu.org> wrote: > 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 <me@bobcopeland.com>
2009/8/27 Pavel Roskin <proski@gnu.org>: > 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 <proski@gnu.org> 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; to u16 val = 0; and it should be fine.
On Thu, Aug 27, 2009 at 8:58 AM, Nick Kossifidis<mickflemm@gmail.com> wrote: > 2009/8/27 Pavel Roskin <proski@gnu.org>: > 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. :)
On Thu, Aug 27, 2009 at 11:17 AM, Bob Copeland<bcopeland@gmail.com> wrote: > On Thu, Aug 27, 2009 at 8:58 AM, Nick Kossifidis<mickflemm@gmail.com> wrote: >> 2009/8/27 Pavel Roskin <proski@gnu.org>: > >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2009/8/27 Luis R. Rodriguez <mcgrof@gmail.com>: > On Thu, Aug 27, 2009 at 11:17 AM, Bob Copeland<bcopeland@gmail.com> wrote: >> On Thu, Aug 27, 2009 at 8:58 AM, Nick Kossifidis<mickflemm@gmail.com> wrote: >>> 2009/8/27 Pavel Roskin <proski@gnu.org>: >> >>> 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 > 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.
2009/8/27 Nick Kossifidis <mickflemm@gmail.com>: > 2009/8/27 Pavel Roskin <proski@gnu.org>: >> 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 <proski@gnu.org> > > 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; > > 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.
On Thu, Aug 27, 2009 at 8:01 PM, Nick Kossifidis<mickflemm@gmail.com> wrote: > 2009/8/27 Luis R. Rodriguez <mcgrof@gmail.com>: >> On Thu, Aug 27, 2009 at 11:17 AM, Bob Copeland<bcopeland@gmail.com> wrote: >>> On Thu, Aug 27, 2009 at 8:58 AM, Nick Kossifidis<mickflemm@gmail.com> wrote: >>>> 2009/8/27 Pavel Roskin <proski@gnu.org>: >>> >>>> 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 >> > > 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. > 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. > 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. > 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. > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2009/8/28 Luis R. Rodriguez <mcgrof@gmail.com>: > On Thu, Aug 27, 2009 at 8:01 PM, Nick Kossifidis<mickflemm@gmail.com> wrote: >> 2009/8/27 Luis R. Rodriguez <mcgrof@gmail.com>: >>> On Thu, Aug 27, 2009 at 11:17 AM, Bob Copeland<bcopeland@gmail.com> wrote: >>>> On Thu, Aug 27, 2009 at 8:58 AM, Nick Kossifidis<mickflemm@gmail.com> wrote: >>>>> 2009/8/27 Pavel Roskin <proski@gnu.org>: >>>> >>>>> 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 >>> >> >> 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. > 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 ? > 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. >> 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. >> 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. > >> 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.
On Thu, Aug 27, 2009 at 9:17 PM, Nick Kossifidis<mickflemm@gmail.com> wrote: > 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. > 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. > Most code is there, we are ready to support 5/10/40MHz channels on the > driver part Great. > 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. > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2009/8/28 Luis R. Rodriguez <mcgrof@gmail.com>: > On Thu, Aug 27, 2009 at 9:17 PM, Nick Kossifidis<mickflemm@gmail.com> wrote: > >> 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 ? > > MadWifi is dead. > Well if people want turbo and other fancy stuff then they 'll keep it alive...
On Thu, 2009-08-27 at 22:21 -0700, Luis R. Rodriguez wrote: > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wireless/ath/ath5k/eeprom.c b/drivers/net/wireless/ath/ath5k/eeprom.c index 8af477d..644962a 100644 --- a/drivers/net/wireless/ath/ath5k/eeprom.c +++ b/drivers/net/wireless/ath/ath5k/eeprom.c @@ -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 */
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 <proski@gnu.org> --- drivers/net/wireless/ath/ath5k/eeprom.c | 29 +++++------------------------ 1 files changed, 5 insertions(+), 24 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html