[ath9k-devel] [PATCH 2/6] ath9k: Only process fft samples when ATH9K_DEBUGFS is enabled
The code can only be used when ATH9k_DEBUGFS is enabled an not when ATH_DEBUG
is activated. Still enabling it would cause build failures.
Signed-off-by: Sven Eckelmann s...@narfation.org
---
drivers/net/wireless/ath/ath9k/recv.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath9k/recv.c
b/drivers/net/wireless/ath/ath9k/recv.c
index 2af6f19..d372e4a 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -1027,7 +1027,7 @@ static s8 fix_rssi_inv_only(u8 rssi_val)
static int ath_process_fft(struct ath_softc *sc, struct ieee80211_hdr *hdr,
struct ath_rx_status *rs, u64 tsf)
{
-#ifdef CONFIG_ATH_DEBUG
+#ifdef CONFIG_ATH9K_DEBUGFS
struct ath_hw *ah = sc-sc_ah;
u8 bins[SPECTRAL_HT20_NUM_BINS];
u8 *vdata = (u8 *)hdr;
--
1.7.10.4
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel
[ath9k-devel] [PATCH 5/6] ath9k: Fix sparse __CHECK_ENDIAN__ for spectral code
Signed-off-by: Sven Eckelmann s...@narfation.org
---
drivers/net/wireless/ath/ath9k/ath9k.h |8
drivers/net/wireless/ath/ath9k/recv.c |9 +
2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h
b/drivers/net/wireless/ath/ath9k/ath9k.h
index 67df864..5fb99d6 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -869,7 +869,7 @@ enum ath_fft_sample_type {
struct fft_sample_tlv {
u8 type;/* see ath_fft_sample */
- u16 length;
+ __be16 length;
/* type dependent data follows */
} __packed;
@@ -878,15 +878,15 @@ struct fft_sample_ht20 {
u8 max_exp;
- u16 freq;
+ __be16 freq;
s8 rssi;
s8 noise;
- u16 max_magnitude;
+ __be16 max_magnitude;
u8 max_index;
u8 bitmap_weight;
- u64 tsf;
+ __be64 tsf;
u8 data[SPECTRAL_HT20_NUM_BINS];
} __packed;
diff --git a/drivers/net/wireless/ath/ath9k/recv.c
b/drivers/net/wireless/ath/ath9k/recv.c
index e349bff..0910146 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -1038,6 +1038,7 @@ static int ath_process_fft(struct ath_softc *sc, struct
ieee80211_hdr *hdr,
struct ath_ht20_mag_info *mag_info;
int len = rs-rs_datalen;
int dc_pos;
+ u16 length, max_magnitude;
/* AR9280 and before report via ATH9K_PHYERR_RADAR, AR93xx and newer
* via ATH9K_PHYERR_SPECTRAL. Haven't seen ATH9K_PHYERR_FALSE_RADAR_EXT
@@ -1065,8 +1066,8 @@ static int ath_process_fft(struct ath_softc *sc, struct
ieee80211_hdr *hdr,
return 1;
fft_sample.tlv.type = ATH_FFT_SAMPLE_HT20;
- fft_sample.tlv.length = sizeof(fft_sample) - sizeof(fft_sample.tlv);
- fft_sample.tlv.length = __cpu_to_be16(fft_sample.tlv.length);
+ length = sizeof(fft_sample) - sizeof(fft_sample.tlv);
+ fft_sample.tlv.length = __cpu_to_be16(length);
fft_sample.freq = __cpu_to_be16(ah-curchan-chan-center_freq);
fft_sample.rssi = fix_rssi_inv_only(rs-rs_rssi_ctl0);
@@ -1112,8 +1113,8 @@ static int ath_process_fft(struct ath_softc *sc, struct
ieee80211_hdr *hdr,
memcpy(fft_sample.data, bins, SPECTRAL_HT20_NUM_BINS);
fft_sample.max_exp = mag_info-max_exp 0xf;
- fft_sample.max_magnitude = spectral_max_magnitude(mag_info-all_bins);
- fft_sample.max_magnitude = __cpu_to_be16(fft_sample.max_magnitude);
+ max_magnitude = spectral_max_magnitude(mag_info-all_bins);
+ fft_sample.max_magnitude = __cpu_to_be16(max_magnitude);
fft_sample.max_index = spectral_max_index(mag_info-all_bins);
fft_sample.bitmap_weight = spectral_bitmap_weight(mag_info-all_bins);
fft_sample.tsf = __cpu_to_be64(tsf);
--
1.7.10.4
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel
[ath9k-devel] [PATCH 3/6] ath9k: Only add fix_rssi_inv_only when spectral code is used
The code is only used when ATH9K_DEBUGFS is activated and causes build warnings
when it is still compiled without user.
Signed-off-by: Sven Eckelmann s...@narfation.org
---
drivers/net/wireless/ath/ath9k/recv.c |2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/wireless/ath/ath9k/recv.c
b/drivers/net/wireless/ath/ath9k/recv.c
index d372e4a..e349bff 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -1016,12 +1016,14 @@ static void ath9k_rx_skb_postprocess(struct ath_common
*common,
rxs-flag = ~RX_FLAG_DECRYPTED;
}
+#ifdef CONFIG_ATH9K_DEBUGFS
static s8 fix_rssi_inv_only(u8 rssi_val)
{
if (rssi_val == 128)
rssi_val = 0;
return (s8) rssi_val;
}
+#endif
/* returns 1 if this was a spectral frame, even if not handled. */
static int ath_process_fft(struct ath_softc *sc, struct ieee80211_hdr *hdr,
--
1.7.10.4
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel
[ath9k-devel] [PATCH 1/6] ath9k: Select RELAY for ATH9K_DEBUGFS
The spectral scan support activated through ATH9K_DEBUGFS depends on RELAY for the kernel-userspace communication. Not activating RELAY causes build failures. The RELAY is added as select instead of depend to do it similar like the only other user of RELAY: BLK_DEV_IO_TRACE Signed-off-by: Sven Eckelmann s...@narfation.org --- drivers/net/wireless/ath/ath9k/Kconfig |1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/wireless/ath/ath9k/Kconfig b/drivers/net/wireless/ath/ath9k/Kconfig index 7647ed6..17507dc 100644 --- a/drivers/net/wireless/ath/ath9k/Kconfig +++ b/drivers/net/wireless/ath/ath9k/Kconfig @@ -58,6 +58,7 @@ config ATH9K_DEBUGFS bool Atheros ath9k debugging depends on ATH9K select MAC80211_DEBUGFS + select RELAY ---help--- Say Y, if you need access to ath9k's statistics for interrupts, rate control, etc. -- 1.7.10.4 ___ ath9k-devel mailing list ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel
[ath9k-devel] [PATCH 4/6] ath9k: Only spectral scan relay file when it was created
The relay file depends on relayfs. Trying to close this file without having
ATH9K_DEBUGFS (and therefore RELAY) activated causes build failures.
Signed-off-by: Sven Eckelmann s...@narfation.org
---
drivers/net/wireless/ath/ath9k/init.c |2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/wireless/ath/ath9k/init.c
b/drivers/net/wireless/ath/ath9k/init.c
index c7d116c..93a6dba 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -922,10 +922,12 @@ static void ath9k_deinit_softc(struct ath_softc *sc)
ath9k_eeprom_release(sc);
+#ifdef CONFIG_ATH9K_DEBUGFS
if (sc-rfs_chan_spec_scan) {
relay_close(sc-rfs_chan_spec_scan);
sc-rfs_chan_spec_scan = NULL;
}
+#endif
}
void ath9k_deinit_device(struct ath_softc *sc)
--
1.7.10.4
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] [RFC 2/3] mac80211: mesh power save doze scheduling
On Wed, 2013-01-23 at 11:19 +0100, Marco Porsch wrote:
Expose a callback ieee80211_mps_init for drivers to register
mesh powersave ops:
- hw_doze - put the radio to sleep now
- hw_wakeup - wake the radio up for frame RX
These ops may be extended in the future to allow drivers/HW to
implement mesh PS themselves. (The current design goal was to
concentrate mesh PS routines in mac80211 to keep driver
modifications minimal.
Track the beacon timing information of peers we are in PS mode
towards. Set a per-STA hrtimer which will trigger a wakeup right
before the peer's next TBTT.
Also use the same hrtimer to go to sleep mode after not
receiving a beacon in a defined time margin. In this case
calculate the next TBTT and increase the margin.
For mesh Awake Windows wakeup on SWBA (beacon_get_tim) and start
a timer which triggers a hw_doze call on expiry.
Hmm. I'm not completely happy with this hrtimer stuff in mac80211.
There's a lot of (USB) hardware that could never implement such a thing,
but could, conceivably, implement this differently?
@@ -1146,6 +1148,11 @@ struct ieee80211_local {
int user_power_level; /* in dBm, for all interfaces */
+ /* mesh power save */
+ bool mps_enabled;
+ bool mps_hw_doze;
Generally, this also seems wrong, you're making the assumption that mesh
will be the only interface. That might actually be true for many use
cases, but is it really an assumption we should still put into the stack
today, with multi-channel etc? I'm not convinced of that.
So I think you should (at least attempt to) make an implementation that
is less tied to the exact timing implementation. Maybe program the
wakeup TBTT in advance. Maybe with a small library the driver can
connect to this that uses hrtimers to implement it? That could then also
assume that callbacks need not sleep, thus allowing to reduce
TBTT_MARGIN.
In theory I think with ath9k you could even use hardware timers
interrupts to wake the hardware, thus probably being able to reduce
TBTT_MARGIN significantly.
[haven't really looked at the rest of the implementation]
johannes
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] [RFC 1/3] mac80211: move mesh sync beacon handler into neighbour_update
On Wed, 2013-01-23 at 11:19 +0100, Marco Porsch wrote:
Move the beacon handler into mesh_neighbour_update where the STA
pointer is already available. This avoids additional overhead
and simplifies the handler.
The repositioning will also benefit mesh PS which uses the
T_offset value right after it has been updated.
Rename the handler to better reflect its purpose.
Signed-off-by: Marco Porsch ma...@cozybit.com
---
net/mac80211/ieee80211_i.h | 10 +-
net/mac80211/mesh.c|8 ++--
net/mac80211/mesh.h|5 +++--
net/mac80211/mesh_plink.c | 16 ---
net/mac80211/mesh_sync.c | 47
+++-
5 files changed, 39 insertions(+), 47 deletions(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index d77d3f7..e08b4c0 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -560,11 +560,11 @@ struct ieee80211_if_ibss {
*/
struct ieee802_11_elems;
struct ieee80211_mesh_sync_ops {
- void (*rx_bcn_presp)(struct ieee80211_sub_if_data *sdata,
- u16 stype,
- struct ieee80211_mgmt *mgmt,
- struct ieee802_11_elems *elems,
- struct ieee80211_rx_status *rx_status);
+ void (*rx_bcn)(struct sta_info *sta,
+struct ieee80211_mgmt *mgmt,
+struct ieee802_11_elems *elems,
+struct ieee80211_rx_status *rx_status,
+u64 tsf);
Is anyone actually planning to add more sync ops? I'm tempted to just
remove the entire abstraction here, since there's only a single concrete
implementation.
johannes
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] [PATCH 4/6] ath9k: Only spectral scan relay file when it was created
Sven Eckelmann s...@narfation.org writes:
The relay file depends on relayfs. Trying to close this file without having
ATH9K_DEBUGFS (and therefore RELAY) activated causes build failures.
Signed-off-by: Sven Eckelmann s...@narfation.org
[...]
+#ifdef CONFIG_ATH9K_DEBUGFS
if (sc-rfs_chan_spec_scan) {
relay_close(sc-rfs_chan_spec_scan);
sc-rfs_chan_spec_scan = NULL;
}
+#endif
Instead of ugly #ifdef you could do something like this:
if (config_enabled(CONFIG_ATH6KL_REGDOMAIN)
I think you could use that elsehwere in your patchset as well, but I
didn't check that carefully.
--
Kalle Valo
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel
[ath9k-devel] ath9k - frame aggregation
Hi all, I am trying to set a minimum threshold for the number of frames (MPDU's) to be aggregated in an AMPDU. Can anyone please help ? Ram ___ ath9k-devel mailing list ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel
[ath9k-devel] [PATCHv2 4/6] ath9k: Only remove spectral scan relay file when it was created
The relay file depends on relayfs. Trying to close this file without having
ATH9K_DEBUGFS (and therefore RELAY) activated causes build failures.
Signed-off-by: Sven Eckelmann s...@narfation.org
---
drivers/net/wireless/ath/ath9k/init.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
v2:
* Use config_enabled instead of #ifdef as requested by
Kalle Valo kv...@adurom.com
diff --git a/drivers/net/wireless/ath/ath9k/init.c
b/drivers/net/wireless/ath/ath9k/init.c
index c7d116c..af932c9 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -922,7 +922,7 @@ static void ath9k_deinit_softc(struct ath_softc *sc)
ath9k_eeprom_release(sc);
- if (sc-rfs_chan_spec_scan) {
+ if (config_enabled(CONFIG_ATH9K_DEBUGFS) sc-rfs_chan_spec_scan) {
relay_close(sc-rfs_chan_spec_scan);
sc-rfs_chan_spec_scan = NULL;
}
--
1.7.10.4
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] [RFC 1/3] mac80211: move mesh sync beacon handler into neighbour_update
On 01/31/2013 02:43 PM, Johannes Berg wrote:
On Wed, 2013-01-23 at 11:19 +0100, Marco Porsch wrote:
Move the beacon handler into mesh_neighbour_update where the STA
pointer is already available. This avoids additional overhead
and simplifies the handler.
The repositioning will also benefit mesh PS which uses the
T_offset value right after it has been updated.
Rename the handler to better reflect its purpose.
Signed-off-by: Marco Porsch ma...@cozybit.com
---
net/mac80211/ieee80211_i.h | 10 +-
net/mac80211/mesh.c|8 ++--
net/mac80211/mesh.h|5 +++--
net/mac80211/mesh_plink.c | 16 ---
net/mac80211/mesh_sync.c | 47
+++-
5 files changed, 39 insertions(+), 47 deletions(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index d77d3f7..e08b4c0 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -560,11 +560,11 @@ struct ieee80211_if_ibss {
*/
struct ieee802_11_elems;
struct ieee80211_mesh_sync_ops {
-void (*rx_bcn_presp)(struct ieee80211_sub_if_data *sdata,
- u16 stype,
- struct ieee80211_mgmt *mgmt,
- struct ieee802_11_elems *elems,
- struct ieee80211_rx_status *rx_status);
+void (*rx_bcn)(struct sta_info *sta,
+ struct ieee80211_mgmt *mgmt,
+ struct ieee802_11_elems *elems,
+ struct ieee80211_rx_status *rx_status,
+ u64 tsf);
Is anyone actually planning to add more sync ops? I'm tempted to just
remove the entire abstraction here, since there's only a single concrete
implementation.
No plans to do so as of now. But since mesh networks are still a
research topic, there may already be researchers somewhere, happy to
have that extensible interface.
Also the standard explicitly defines an Extensible synchronization
framework in 13.13.2.
--Marco
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] [RFC 2/3] mac80211: mesh power save doze scheduling
On Thu, 2013-01-31 at 16:23 +0100, Marco Porsch wrote: On 01/31/2013 02:51 PM, Johannes Berg wrote: On Wed, 2013-01-23 at 11:19 +0100, Marco Porsch wrote: Expose a callback ieee80211_mps_init for drivers to register mesh powersave ops: - hw_doze - put the radio to sleep now - hw_wakeup - wake the radio up for frame RX These ops may be extended in the future to allow drivers/HW to implement mesh PS themselves. (The current design goal was to concentrate mesh PS routines in mac80211 to keep driver modifications minimal. Track the beacon timing information of peers we are in PS mode towards. Set a per-STA hrtimer which will trigger a wakeup right before the peer's next TBTT. Also use the same hrtimer to go to sleep mode after not receiving a beacon in a defined time margin. In this case calculate the next TBTT and increase the margin. For mesh Awake Windows wakeup on SWBA (beacon_get_tim) and start a timer which triggers a hw_doze call on expiry. Hmm. I'm not completely happy with this hrtimer stuff in mac80211. There's a lot of (USB) hardware that could never implement such a thing, but could, conceivably, implement this differently? The use of hrtimers for MPS is debatable currently. The approach of calculating the peer's TSF should be accurate to the usec. Good timing here directly affects the energy savings. On the other handside the margin of 5ms used shows that something is not working as expected yet. If this cannot be fixed, I may as well use regular timers here. How strongly do you oppose hrtimers? :) Oh it's not the user of hrtimers vs. timers (although first using an hrtimer and then kicking off to a work struct seems ... pointless and questionable), it's more the fact that you're starting to put real-time behaviour into mac80211. I'm not entirely convinced that is the right approach for core mac80211. So I think you should (at least attempt to) make an implementation that is less tied to the exact timing implementation. Maybe program the wakeup TBTT in advance. Maybe with a small library the driver can connect to this that uses hrtimers to implement it? That could then also assume that callbacks need not sleep, thus allowing to reduce TBTT_MARGIN. In theory I think with ath9k you could even use hardware timers interrupts to wake the hardware, thus probably being able to reduce TBTT_MARGIN significantly. Earlier, I had successfully implemented wakeups using ath9k HW before we at cozybit decided to concentrate all code in mac80211. Concerning ath9k it worked fine but required more callbacks to mac80211 and will eventually add redundant code that has to be maintained to multiple drivers. The question isn't so much about concentrating the code, I think you can still do that for most devices, and use hrtimers more effectively, if you somewhat put it off to the side in a less integrated way. I'm thinking that, for example, if I ever wanted to implement this with our current device (not likely, but just for perspective) we'd definitely want to program the firmware to do all the real-time behaviour. In fact, I suspect we probably could with the new MVM firmware API. Therefore, I think it would be better to have a more generic time-oriented interface in mac80211, and put the actual implementation of the events a bit separately in a way that it is optional for drivers and not used in the API. johannes ___ ath9k-devel mailing list ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] [PATCH 5/7] mac80211: Expand powersave configuration flag to be two bits
On Thu, Jan 31, 2013 at 05:53:49PM +0100, Johannes Berg wrote: On Thu, 2013-01-31 at 10:33 -0600, Seth Forshee wrote: On Thu, Jan 31, 2013 at 04:20:48PM +0100, Johannes Berg wrote: On Tue, 2013-01-29 at 17:47 -0600, Seth Forshee wrote: +static inline bool ieee80211_is_ps_disabled(struct ieee80211_conf *conf) +static inline bool ieee80211_is_ps_enabled(struct ieee80211_conf *conf) Huh, is that worth the confusion? It seems !enabled should be the same as disabled, but it's not quite the same, which might be confusing. In this patch there's no distinction, but after adding the off-channel powersave state there is -- disabled == !enabled !offchannel. I thought it was something like that, yeah. Actually one of the last bugs I fixed before sending these was a place where I had used disabled instead of !enabled, and the frames ended up with PM set when it shouldn't have been. I agree though that the distinction is confusing. Maybe some better state names are needed. Perhaps awake, offchannel, and doze? I think what you really want is to distinguish between HW can go to powersave and PM bit should be set? That's pretty much what your CONF_PS_ENABLED and CONF_PS_OFFCHANNEL means, respectively, but maybe Correct, with the understanding that HW can go to powersave also implies PM bit should be set. Another approach would be to keep the CONF_PS flag the same and add a CONF_PM flag or similar. I didn't go with this approach because CONF_PS !CONF_PM really doesn't make any sense, which really doesn't help with reducing confusion. The advantage is that it separates setting PM from PS for those driver that don't support PS but need to configure the hardware to set PM for off-channel. putting it in different terms would make it less confusing? Yes. I think the reason enabled/disabled is confusing because it would usually imply a binary state. Seth ___ ath9k-devel mailing list ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] [PATCH 5/7] mac80211: Expand powersave configuration flag to be two bits
On Tue, 2013-01-29 at 17:47 -0600, Seth Forshee wrote:
+static inline bool ieee80211_is_ps_disabled(struct ieee80211_conf *conf)
+static inline bool ieee80211_is_ps_enabled(struct ieee80211_conf *conf)
Huh, is that worth the confusion? It seems !enabled should be the same
as disabled, but it's not quite the same, which might be confusing.
+/**
+ * ieee80211_set_ps_state - set device powersave state
+ *
+ * Sets the powersave state in the supplied device configuration to the
+ * specified state.
+ *
+ * @conf: device configuration
+ * @state: new powersave state. Must be one of the IEEE80211_CONF_PS_*
+ * flags from enum ieee80211_conf_flags.
+ */
+static inline void ieee80211_set_ps_state(struct ieee80211_conf *conf,
+ u32 state)
+{
+ conf-flags = (conf-flags ~IEEE80211_CONF_PS_MASK) |
+ (state IEEE80211_CONF_PS_MASK);
+}
I don't think the driver should do this, so the inline shouldn't be
here?
johannes
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] [PATCH 5/7] mac80211: Expand powersave configuration flag to be two bits
On Thu, 2013-01-31 at 10:33 -0600, Seth Forshee wrote:
On Thu, Jan 31, 2013 at 04:20:48PM +0100, Johannes Berg wrote:
On Tue, 2013-01-29 at 17:47 -0600, Seth Forshee wrote:
+static inline bool ieee80211_is_ps_disabled(struct ieee80211_conf *conf)
+static inline bool ieee80211_is_ps_enabled(struct ieee80211_conf *conf)
Huh, is that worth the confusion? It seems !enabled should be the same
as disabled, but it's not quite the same, which might be confusing.
In this patch there's no distinction, but after adding the off-channel
powersave state there is -- disabled == !enabled !offchannel.
I thought it was something like that, yeah.
Actually one of the last bugs I fixed before sending these was a place
where I had used disabled instead of !enabled, and the frames ended up
with PM set when it shouldn't have been.
I agree though that the distinction is confusing. Maybe some better
state names are needed. Perhaps awake, offchannel, and doze?
I think what you really want is to distinguish between HW can go to
powersave and PM bit should be set? That's pretty much what your
CONF_PS_ENABLED and CONF_PS_OFFCHANNEL means, respectively, but maybe
putting it in different terms would make it less confusing?
+/**
+ * ieee80211_set_ps_state - set device powersave state
+ *
+ * Sets the powersave state in the supplied device configuration to the
+ * specified state.
+ *
+ * @conf: device configuration
+ * @state: new powersave state. Must be one of the IEEE80211_CONF_PS_*
+ * flags from enum ieee80211_conf_flags.
+ */
+static inline void ieee80211_set_ps_state(struct ieee80211_conf *conf,
+ u32 state)
+{
+ conf-flags = (conf-flags ~IEEE80211_CONF_PS_MASK) |
+ (state IEEE80211_CONF_PS_MASK);
+}
I don't think the driver should do this, so the inline shouldn't be
here?
That's true. Would moving it to ieee80211_i.h be appropriate, or is
there somewhere better?
ieee80211_i.h is good
johannes
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] [PATCH 5/7] mac80211: Expand powersave configuration flag to be two bits
On Thu, 2013-01-31 at 11:18 -0600, Seth Forshee wrote: Actually one of the last bugs I fixed before sending these was a place where I had used disabled instead of !enabled, and the frames ended up with PM set when it shouldn't have been. I agree though that the distinction is confusing. Maybe some better state names are needed. Perhaps awake, offchannel, and doze? I think what you really want is to distinguish between HW can go to powersave and PM bit should be set? That's pretty much what your CONF_PS_ENABLED and CONF_PS_OFFCHANNEL means, respectively, but maybe Correct, with the understanding that HW can go to powersave also implies PM bit should be set. Another approach would be to keep the CONF_PS flag the same and add a CONF_PM flag or similar. I didn't go with this approach because CONF_PS !CONF_PM really doesn't make any sense, which really doesn't help with reducing confusion. The advantage is that it separates setting PM from PS for those driver that don't support PS but need to configure the hardware to set PM for off-channel. Good point, that'd work too. PS !PM would never be used, I guess? It'd also have the advantage of not having to touch all the drivers? It doesn't really matter all that much to me though, I just think what you have right now is (too) confusing. johannes ___ ath9k-devel mailing list ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel
