| Message ID | 20190306200206.60916-1-julius.n@gmx.net (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [RFC,v2] mac80211: debugfs option to force TX status frames | expand |
Julius Niedworok <julius.n@gmx.net> writes: > At Technical University of Munich we use MAC 802.11 TX status frames to > perform several measurements in MAC 802.11 setups. > > With ath based drivers this was possible until commit d94a461d7a7df6 > ("ath9k: use ieee80211_tx_status_noskb where possible") as the driver > ignored the IEEE80211_TX_CTL_REQ_TX_STATUS flag and always delivered > tx_status frames. Since that commit, this behavior was changed and the > driver now adheres to IEEE80211_TX_CTL_REQ_TX_STATUS. > > Due to performance reasons, IEEE80211_TX_CTL_REQ_TX_STATUS is not set for > data frames from interfaces in managed mode. Hence, frames that are sent > from a managed mode interface do never deliver tx_status frames. This > remains true even if a monitor mode interface (the measurement interface) > is added to the same ieee80211 physical device. Thus, there is no > possibility for receiving tx_status frames for frames sent on an interface > in managed mode, if the driver adheres to IEEE80211_TX_CTL_REQ_TX_STATUS. > > In order to force delivery of tx_status frames for research and debugging > purposes, implement a debugfs option force_tx_status for ieee80211 physical > devices. When this option is set for a physical device, > IEEE80211_TX_CTL_REQ_TX_STATUS is enabled in all packets sent from that > device. This option can be set via > /sys/kernel/debug/ieee80211/<dev>/force_tx_status. The default is disabled. > > Co-developed-by: Charlie Groh <ga58taw@mytum.de> > Signed-off-by: Charlie Groh <ga58taw@mytum.de> > Signed-off-by: Julius Niedworok <julius.n@gmx.net> [...] > + len = scnprintf(buf, sizeof(buf), "%d\n", (int)local->force_tx_status); I wonder about the cast, is it guaranteed that a bool is always of the same size as an int? > --- a/net/mac80211/ieee80211_i.h > +++ b/net/mac80211/ieee80211_i.h > @@ -1367,6 +1367,7 @@ struct ieee80211_local { > struct dentry *rcdir; > struct dentry *keys; > } debugfs; > + bool force_tx_status; [...] > #endif > > /* > diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c > index 928f13a..717fa71 100644 > --- a/net/mac80211/tx.c > +++ b/net/mac80211/tx.c > @@ -2463,6 +2463,11 @@ static struct sk_buff *ieee80211_build_hdr(struct ieee80211_sub_if_data *sdata, > if (IS_ERR(sta)) > sta = NULL; > > +#ifdef CONFIG_MAC80211_DEBUGFS > + if (local->force_tx_status) > + info_flags |= IEEE80211_TX_CTL_REQ_TX_STATUS; > +#endif > + > /* convert Ethernet header to proper 802.11 header (based on > * operation mode) */ > ethertype = (skb->data[12] << 8) | skb->data[13]; > @@ -3468,6 +3473,11 @@ static bool ieee80211_xmit_fast(struct ieee80211_sub_if_data *sdata, > (tid_tx ? IEEE80211_TX_CTL_AMPDU : 0); > info->control.flags = IEEE80211_TX_CTRL_FAST_XMIT; > > +#ifdef CONFIG_MAC80211_DEBUGFS > + if (local->force_tx_status) > + info->flags |= IEEE80211_TX_CTL_REQ_TX_STATUS; > +#endif IMHO the ifdefs look pointless just to save four bytes. I would move force_tx_status outside of ifdef in the struct so that the actual code doesn't have ugly ifdefs.
On Thu, Mar 07, 2019 at 05:42:04PM +0200, Kalle Valo wrote: > > + len = scnprintf(buf, sizeof(buf), "%d\n", (int)local->force_tx_status); > > I wonder about the cast, is it guaranteed that a bool is always of the > same size as an int? Why is this a problem? If a bool is smaller than an int, the compiler emits code that will prepend the value of force_tx_status with zeros. If it is larger than an int the compiler emits code that will truncate force_tx_status to sizeof(int) bytes. So it doesn't matter how large bool and/or int are, the code always prints '0' or '1' depending on the value of force_tx_status. > > --- a/net/mac80211/tx.c > > +++ b/net/mac80211/tx.c > > @@ -2463,6 +2463,11 @@ static struct sk_buff *ieee80211_build_hdr(struct ieee80211_sub_if_data *sdata, > > if (IS_ERR(sta)) > > sta = NULL; > > > > +#ifdef CONFIG_MAC80211_DEBUGFS > > + if (local->force_tx_status) > > + info_flags |= IEEE80211_TX_CTL_REQ_TX_STATUS; > > +#endif > > + > > /* convert Ethernet header to proper 802.11 header (based on > > * operation mode) */ > > ethertype = (skb->data[12] << 8) | skb->data[13]; > > @@ -3468,6 +3473,11 @@ static bool ieee80211_xmit_fast(struct ieee80211_sub_if_data *sdata, > > (tid_tx ? IEEE80211_TX_CTL_AMPDU : 0); > > info->control.flags = IEEE80211_TX_CTRL_FAST_XMIT; > > > > +#ifdef CONFIG_MAC80211_DEBUGFS > > + if (local->force_tx_status) > > + info->flags |= IEEE80211_TX_CTL_REQ_TX_STATUS; > > +#endif > > IMHO the ifdefs look pointless just to save four bytes. I would move > force_tx_status outside of ifdef in the struct so that the actual code > doesn't have ugly ifdefs. I don't think ifdefs are ugly in this case. They clearly indicate that the code belongs to the debugfs implementation of mac80211. In addition, when we remove the ifdefs, code that only belongs to the debugfs implementation of mac80211 is always included in the kernel (even when the config option is turned off!) which, I think, is much more ugly than the ifdefs. Thanks for your comments and ideas. Charlie
ga58taw@mytum.de writes: > On Thu, Mar 07, 2019 at 05:42:04PM +0200, Kalle Valo wrote: >> > + len = scnprintf(buf, sizeof(buf), "%d\n", (int)local->force_tx_status); >> >> I wonder about the cast, is it guaranteed that a bool is always of the >> same size as an int? > > Why is this a problem? If a bool is smaller than an int, the compiler > emits code that will prepend the value of force_tx_status with zeros. Let's say that a bool is a byte and int is four bytes. If you use "%d" I would guess that in that case scnprintf() writes 4 bytes, meaning that 3 bytes will be overwriting either padding or some other field in the struct. But I'm no compiler expert so I'm not going to argue about this anymore. I just wanted to point out that that the cast looks dangerous and I would not do it.
On 2019-03-11, at 16:03:38 +0200, Kalle Valo wrote: > ga58taw@mytum.de writes: > > On Thu, Mar 07, 2019 at 05:42:04PM +0200, Kalle Valo wrote: > > > > + len = scnprintf(buf, sizeof(buf), "%d\n", > > > > (int)local->force_tx_status); > > > > > > I wonder about the cast, is it guaranteed that a bool is always of > > > the same size as an int? > > > > Why is this a problem? If a bool is smaller than an int, the > > compiler emits code that will prepend the value of force_tx_status > > with zeros. > > Let's say that a bool is a byte and int is four bytes. If you use "%d" > I would guess that in that case scnprintf() writes 4 bytes, meaning > that 3 bytes will be overwriting either padding or some other field in > the struct. It's the value that matters, not the type. It will only be too big for the buffer if the result of casting local->force_tx_status to int is less than -9 or greeater than 99. scnprintf(buf, size(buf), "%lld\n", (long long)local->force_tx_status) would also be fine if the value were in range. Note also that scnprintf will not overrun the buffer: it will truncate the string. > But I'm no compiler expert so I'm not going to argue about this > anymore. I just wanted to point out that that the cast looks > dangerous and I would not do it. As it happens, arguments to variadic functions are subject to the "default argument promotions," so if local->force_tx_status is in fact a bool (I can't find the definition), it would be promoted to int and the cast is superfluous. J.
> On 11.03.2019 15:52, Jeremy Sowden wrote: > > It's the value that matters, not the type. It will only be too big for > the buffer if the result of casting local->force_tx_status to int is > less than -9 or greeater than 99. > > scnprintf(buf, size(buf), "%lld\n", (long long)local->force_tx_status) > > would also be fine if the value were in range. Note also that scnprintf > will not overrun the buffer: it will truncate the string. Thanks for the clarification :) > As it happens, arguments to variadic functions are subject to the > "default argument promotions," so if local->force_tx_status is in fact a > bool (I can't find the definition), it would be promoted to int and the > cast is superfluous. Yes - the cast is superfluous. We still think it might be useful to keep it there to make the point that the value will be casted. However, if you prefer to omit the cast, we are happy to take it out. Thank you, Julius and Charlie
On 2019-03-19, at 16:07:32 +0100, Julius Niedworok wrote: > On 11.03.2019 15:52, Jeremy Sowden wrote: > > It's the value that matters, not the type. It will only be too big > > for the buffer if the result of casting local->force_tx_status to > > int is less than -9 or greeater than 99. > > > > scnprintf(buf, size(buf), "%lld\n", > > (long long)local->force_tx_status) > > > > would also be fine if the value were in range. Note also that > > scnprintf will not overrun the buffer: it will truncate the string. > > Thanks for the clarification :) You're welcome. :) > > As it happens, arguments to variadic functions are subject to the > > "default argument promotions," so if local->force_tx_status is in > > fact a bool (I can't find the definition), it would be promoted to > > int and the cast is superfluous. > > Yes - the cast is superfluous. We still think it might be useful to > keep it there to make the point that the value will be casted. > However, if you prefer to omit the cast, we are happy to take it out. If I were maintaining this code, I'd probably take it out. However, I did have to check the documentation to verify that the cast wasn't required, and I know the more obscure corners of the C standard pretty well, so it may cause less confusion to keep it. J.
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c index 3fe541e..074b5d1 100644 --- a/net/mac80211/debugfs.c +++ b/net/mac80211/debugfs.c @@ -150,6 +150,58 @@ static const struct file_operations aqm_ops = { .llseek = default_llseek, }; +static ssize_t force_tx_status_read(struct file *file, + char __user *user_buf, + size_t count, + loff_t *ppos) +{ + struct ieee80211_local *local = file->private_data; + char buf[3]; + int len = 0; + + len = scnprintf(buf, sizeof(buf), "%d\n", (int)local->force_tx_status); + + return simple_read_from_buffer(user_buf, count, ppos, + buf, len); +} + +static ssize_t force_tx_status_write(struct file *file, + const char __user *user_buf, + size_t count, + loff_t *ppos) +{ + struct ieee80211_local *local = file->private_data; + char buf[3]; + size_t len; + + if (count > sizeof(buf)) + return -EINVAL; + + if (copy_from_user(buf, user_buf, count)) + return -EFAULT; + + buf[sizeof(buf) - 1] = '\0'; + len = strlen(buf); + if (len > 0 && buf[len - 1] == '\n') + buf[len - 1] = 0; + + if (buf[0] == '0' && buf[1] == '\0') + local->force_tx_status = 0; + else if (buf[0] == '1' && buf[1] == '\0') + local->force_tx_status = 1; + else + return -EINVAL; + + return count; +} + +static const struct file_operations force_tx_status_ops = { + .write = force_tx_status_write, + .read = force_tx_status_read, + .open = simple_open, + .llseek = default_llseek, +}; + #ifdef CONFIG_PM static ssize_t reset_write(struct file *file, const char __user *user_buf, size_t count, loff_t *ppos) @@ -379,6 +431,7 @@ void debugfs_hw_add(struct ieee80211_local *local) DEBUGFS_ADD(hwflags); DEBUGFS_ADD(user_power); DEBUGFS_ADD(power); + DEBUGFS_ADD_MODE(force_tx_status, 0600); if (local->ops->wake_tx_queue) DEBUGFS_ADD_MODE(aqm, 0600); diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h index 7dfb4e2..3339b5d 100644 --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -1367,6 +1367,7 @@ struct ieee80211_local { struct dentry *rcdir; struct dentry *keys; } debugfs; + bool force_tx_status; #endif /* diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 928f13a..717fa71 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -2463,6 +2463,11 @@ static struct sk_buff *ieee80211_build_hdr(struct ieee80211_sub_if_data *sdata, if (IS_ERR(sta)) sta = NULL; +#ifdef CONFIG_MAC80211_DEBUGFS + if (local->force_tx_status) + info_flags |= IEEE80211_TX_CTL_REQ_TX_STATUS; +#endif + /* convert Ethernet header to proper 802.11 header (based on * operation mode) */ ethertype = (skb->data[12] << 8) | skb->data[13]; @@ -3468,6 +3473,11 @@ static bool ieee80211_xmit_fast(struct ieee80211_sub_if_data *sdata, (tid_tx ? IEEE80211_TX_CTL_AMPDU : 0); info->control.flags = IEEE80211_TX_CTRL_FAST_XMIT; +#ifdef CONFIG_MAC80211_DEBUGFS + if (local->force_tx_status) + info->flags |= IEEE80211_TX_CTL_REQ_TX_STATUS; +#endif + if (hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_QOS_DATA)) { tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK; *ieee80211_get_qos_ctl(hdr) = tid;
At Technical University of Munich we use MAC 802.11 TX status frames to perform several measurements in MAC 802.11 setups. With ath based drivers this was possible until commit d94a461d7a7df6 ("ath9k: use ieee80211_tx_status_noskb where possible") as the driver ignored the IEEE80211_TX_CTL_REQ_TX_STATUS flag and always delivered tx_status frames. Since that commit, this behavior was changed and the driver now adheres to IEEE80211_TX_CTL_REQ_TX_STATUS. Due to performance reasons, IEEE80211_TX_CTL_REQ_TX_STATUS is not set for data frames from interfaces in managed mode. Hence, frames that are sent from a managed mode interface do never deliver tx_status frames. This remains true even if a monitor mode interface (the measurement interface) is added to the same ieee80211 physical device. Thus, there is no possibility for receiving tx_status frames for frames sent on an interface in managed mode, if the driver adheres to IEEE80211_TX_CTL_REQ_TX_STATUS. In order to force delivery of tx_status frames for research and debugging purposes, implement a debugfs option force_tx_status for ieee80211 physical devices. When this option is set for a physical device, IEEE80211_TX_CTL_REQ_TX_STATUS is enabled in all packets sent from that device. This option can be set via /sys/kernel/debug/ieee80211/<dev>/force_tx_status. The default is disabled. Co-developed-by: Charlie Groh <ga58taw@mytum.de> Signed-off-by: Charlie Groh <ga58taw@mytum.de> Signed-off-by: Julius Niedworok <julius.n@gmx.net> --- net/mac80211/debugfs.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++ net/mac80211/ieee80211_i.h | 1 + net/mac80211/tx.c | 10 +++++++++ 3 files changed, 64 insertions(+)