[PATCH] nl80211: fix tx_control_port trace point

[PATCH] nl80211: fix tx_control_port trace point

[Markus Theil]

Endianess conversion should not happen when writing out the string,
instead convert proto endiannes when creating the trace point data,
like its already done for control port rx.

Without this patch, trace looks like:
<...>-53819 [000] 12957.654875: rdev_tx_control_port: [FAILED TO PARSE]
  wiphy_name=phy0 name=wlan0 ifindex=3 dest=ARRAY[dc, 7b, 94, a5, bb, 3e]
  proto=36488 unencrypted=1

After applying this patch:
wpa_supplicant-553   [001]   121.211264: rdev_tx_control_port: phy0,
  netdev:wlan0(3), dc:7b:94:a5:bb:3e, proto: 0x888e, unencrypted: true

Fixes: 2576a9ace47e ("nl80211: Implement TX of control port frames")
Signed-off-by: Markus Theil <markus.theil@tu-ilmenau.de>
---
 net/wireless/trace.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index 839df54cee21..8c974245c8e1 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -1959,13 +1959,13 @@ TRACE_EVENT(rdev_tx_control_port,
 		WIPHY_ASSIGN;
 		NETDEV_ASSIGN;
 		MAC_ASSIGN(dest, dest);
-		__entry->proto = proto;
+		__entry->proto = be16_to_cpu(proto);
 		__entry->unencrypted = unencrypted;
 	),
 	TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", " MAC_PR_FMT ","
 		  " proto: 0x%x, unencrypted: %s",
 		  WIPHY_PR_ARG, NETDEV_PR_ARG, MAC_PR_ARG(dest),
-		  be16_to_cpu(__entry->proto),
+		  __entry->proto,
 		  BOOL_TO_STR(__entry->unencrypted))
 );
 
-- 
2.25.1

Re: [PATCH] nl80211: fix tx_control_port trace point

[Johannes Berg]

On Wed, 2020-03-04 at 12:49 +0100, Markus Theil wrote:
> Endianess conversion should not happen when writing out the string,
> instead convert proto endiannes when creating the trace point data,
> like its already done for control port rx.
> 
> Without this patch, trace looks like:
> <...>-53819 [000] 12957.654875: rdev_tx_control_port: [FAILED TO PARSE]
>   wiphy_name=phy0 name=wlan0 ifindex=3 dest=ARRAY[dc, 7b, 94, a5, bb, 3e]
>   proto=36488 unencrypted=1
> 
> After applying this patch:
> wpa_supplicant-553   [001]   121.211264: rdev_tx_control_port: phy0,
>   netdev:wlan0(3), dc:7b:94:a5:bb:3e, proto: 0x888e, unencrypted: true

This is from trace-cmd? That just means it doesn't know about the endian
conversion, but I don't really see any reason to change this - big
endian is a perfectly valid format for trace points?

Maybe teach trace-cmd to understand it? We already did that for
__le16_to_cpu().

johannes

Re: [PATCH] nl80211: fix tx_control_port trace point

[Markus Theil]

On 3/4/20 1:16 PM, Johannes Berg wrote:
> On Wed, 2020-03-04 at 12:49 +0100, Markus Theil wrote:
>> Endianess conversion should not happen when writing out the string,
>> instead convert proto endiannes when creating the trace point data,
>> like its already done for control port rx.
>>
>> Without this patch, trace looks like:
>> <...>-53819 [000] 12957.654875: rdev_tx_control_port: [FAILED TO PARSE]
>>   wiphy_name=phy0 name=wlan0 ifindex=3 dest=ARRAY[dc, 7b, 94, a5, bb, 3e]
>>   proto=36488 unencrypted=1
>>
>> After applying this patch:
>> wpa_supplicant-553   [001]   121.211264: rdev_tx_control_port: phy0,
>>   netdev:wlan0(3), dc:7b:94:a5:bb:3e, proto: 0x888e, unencrypted: true
> This is from trace-cmd? That just means it doesn't know about the endian
> conversion, but I don't really see any reason to change this - big
> endian is a perfectly valid format for trace points?
Yes, its trace-cmd output. Without this patch, the print fmt in the
trace data file looks like this:
print fmt: "%s, netdev:%s(%d), %pM, proto: 0x%x, unencrypted: %s",
REC->wiphy_name, REC->name, REC->ifindex, (REC->dest),
(__u16)__builtin_bswap16((__u16)(( __u16)(__be16)(REC->proto))),
(REC->unencrypted) ? "true" : "false"

With the patch, the builtin_bswap16 does not get placed there:
print fmt: "%s, netdev:%s(%d), %pM, proto: 0x%x, unencrypted: %s",
REC->wiphy_name, REC->name, REC->ifindex, (REC->dest), REC->proto,
(REC->unencrypted) ? "true" : "false"

Markus
> Maybe teach trace-cmd to understand it? We already did that for
> __le16_to_cpu().
>
> johannes
>

Re: [PATCH] nl80211: fix tx_control_port trace point

[Johannes Berg]

On Wed, 2020-03-04 at 13:32 +0100, Markus Theil wrote:
> 
> Yes, its trace-cmd output. Without this patch, the print fmt in the
> trace data file looks like this:
> print fmt: "%s, netdev:%s(%d), %pM, proto: 0x%x, unencrypted: %s",
> REC->wiphy_name, REC->name, REC->ifindex, (REC->dest),
> (__u16)__builtin_bswap16((__u16)(( __u16)(__be16)(REC->proto))),
> (REC->unencrypted) ? "true" : "false"
> 
> With the patch, the builtin_bswap16 does not get placed there:

Sure.

But trace-cmd has infrastructure to handle such "function calls" in the
output format, so it should be able to handle this pretty easily.

So really it's mostly a presentation issue, and having the data in big
endian when it's that way over the air etc. IMHO does make sense.

johannes

Re: [PATCH] nl80211: fix tx_control_port trace point

[Markus Theil]

On 3/4/20 1:16 PM, Johannes Berg wrote:
> On Wed, 2020-03-04 at 12:49 +0100, Markus Theil wrote:
>> Endianess conversion should not happen when writing out the string,
>> instead convert proto endiannes when creating the trace point data,
>> like its already done for control port rx.
>>
>> Without this patch, trace looks like:
>> <...>-53819 [000] 12957.654875: rdev_tx_control_port: [FAILED TO PARSE]
>>   wiphy_name=phy0 name=wlan0 ifindex=3 dest=ARRAY[dc, 7b, 94, a5, bb, 3e]
>>   proto=36488 unencrypted=1
>>
>> After applying this patch:
>> wpa_supplicant-553   [001]   121.211264: rdev_tx_control_port: phy0,
>>   netdev:wlan0(3), dc:7b:94:a5:bb:3e, proto: 0x888e, unencrypted: true
> This is from trace-cmd? That just means it doesn't know about the endian
> conversion, but I don't really see any reason to change this - big
> endian is a perfectly valid format for trace points?
>
> Maybe teach trace-cmd to understand it? We already did that for
> __le16_to_cpu().
Maybe, but then this is also inconsistent in comparison to the same kind of
conversion in control port rx. I think, both calls should have the same
conversion
(be that introducing the fn to trace-cmd or converting the input data
for tx control port).
> johannes
>

Re: [PATCH] nl80211: fix tx_control_port trace point

[Markus Theil]

On 3/4/20 1:36 PM, Johannes Berg wrote:
> On Wed, 2020-03-04 at 13:32 +0100, Markus Theil wrote:
>> Yes, its trace-cmd output. Without this patch, the print fmt in the
>> trace data file looks like this:
>> print fmt: "%s, netdev:%s(%d), %pM, proto: 0x%x, unencrypted: %s",
>> REC->wiphy_name, REC->name, REC->ifindex, (REC->dest),
>> (__u16)__builtin_bswap16((__u16)(( __u16)(__be16)(REC->proto))),
>> (REC->unencrypted) ? "true" : "false"
>>
>> With the patch, the builtin_bswap16 does not get placed there:
> Sure.
>
> But trace-cmd has infrastructure to handle such "function calls" in the
> output format, so it should be able to handle this pretty easily.
>
> So really it's mostly a presentation issue, and having the data in big
> endian when it's that way over the air etc. IMHO does make sense.

Sure.

Should cfg80211_rx_control_port then also adopt this behavior?
It currently does the conversion in the way I changed
cfg80211_tx_control port to.
IMHO, as long as the trace events are just printed, it is cleaner to
do the endiannes conversion already in the kernel.

TRACE_EVENT(cfg80211_rx_control_port,
...
    __entry->proto = be16_to_cpu(skb->protocol);
        __entry->unencrypted = unencrypted;
    ),
    TP_printk(NETDEV_PR_FMT ", len=%d, " MAC_PR_FMT ", proto: 0x%x,
unencrypted: %s",
          NETDEV_PR_ARG, __entry->len, MAC_PR_ARG(from),
          __entry->proto, BOOL_TO_STR(__entry->unencrypted))

Re: [PATCH] nl80211: fix tx_control_port trace point

[Johannes Berg]

On Wed, 2020-03-04 at 17:17 +0100, Markus Theil wrote:

> Should cfg80211_rx_control_port then also adopt this behavior?
> It currently does the conversion in the way I changed
> cfg80211_tx_control port to.
[...]
> TRACE_EVENT(cfg80211_rx_control_port,
> ...
>     __entry->proto = be16_to_cpu(skb->protocol);
>         __entry->unencrypted = unencrypted;
>     ),
>     TP_printk(NETDEV_PR_FMT ", len=%d, " MAC_PR_FMT ", proto: 0x%x,
> unencrypted: %s",
>           NETDEV_PR_ARG, __entry->len, MAC_PR_ARG(from),
>           __entry->proto, BOOL_TO_STR(__entry->unencrypted))

Great ...

I dunno. I guess I don't like touching these so much, because it may be
that somebody is relying on them for some kind of tooling?

But I guess that's actually unlikely and we can just try, but then I'd
rather change this one to BE than the other way around.

> IMHO, as long as the trace events are just printed, it is cleaner to
> do the endiannes conversion already in the kernel.

But they aren't just printed, trace-cmd can record them in binary form,
and you can have python plugin code to consume that, for example.

johannes