| Message ID | 20200304114952.1827-1-markus.theil@tu-ilmenau.de (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | nl80211: fix tx_control_port trace point | expand |
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
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 >
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
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 >
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))
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
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)) );
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(-)