Re: [PATCH RFC v2] mac80211: debugfs option to force TX status frames

[ga58taw]

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