| Message ID | 20210121204037.61390-2-rasmus.villemoes@prevas.dk (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | fix and move definitions of MRP data structures | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | fail | Series targets non-next tree, but doesn't contain any Fixes tags |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: nikolay@nvidia.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 16 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
The 01/21/2021 21:40, Rasmus Villemoes wrote: It seems that is missing a Fixes tag, other than that it looks fine. Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com> > > Wireshark says that the MRP test packets cannot be decoded - and the > reason for that is that there's a two-byte hole filled with garbage > between the "transitions" and "timestamp" members. > > So Wireshark decodes the two garbage bytes and the top two bytes of > the timestamp written by the kernel as the timestamp value (which thus > fluctuates wildly), and interprets the lower two bytes of the > timestamp as a new (type, length) pair, which is of course broken. > > Even though this makes the timestamp field in the struct unaligned, it > actually makes it end up on a 32 bit boundary in the frame as mandated > by the standard, since it is preceded by a two byte TLV header. > > The struct definitions live under include/uapi/, but they are not > really part of any kernel<->userspace API/ABI, so fixing the > definitions by adding the packed attribute should not cause any > compatibility issues. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- > include/uapi/linux/mrp_bridge.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/uapi/linux/mrp_bridge.h b/include/uapi/linux/mrp_bridge.h > index 6aeb13ef0b1e..d1d0cf65916d 100644 > --- a/include/uapi/linux/mrp_bridge.h > +++ b/include/uapi/linux/mrp_bridge.h > @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr { > __be16 state; > __be16 transitions; > __be32 timestamp; > -}; > +} __attribute__((__packed__)); > > struct br_mrp_ring_topo_hdr { > __be16 prio; > @@ -141,7 +141,7 @@ struct br_mrp_in_test_hdr { > __be16 state; > __be16 transitions; > __be32 timestamp; > -}; > +} __attribute__((__packed__)); > > struct br_mrp_in_topo_hdr { > __u8 sa[ETH_ALEN]; > -- > 2.23.0 >
On 21/01/2021 22.23, Horatiu Vultur wrote: > The 01/21/2021 21:40, Rasmus Villemoes wrote: > > It seems that is missing a Fixes tag, other than that it looks fine. That would be these two I guess: Fixes: 2801758391ba ("bridge: uapi: mrp: Extend MRP attributes for MRP interconnect") Fixes: 4714d13791f8 ("bridge: uapi: mrp: Add mrp attributes.") > Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com> Thanks, Rasmus
diff --git a/include/uapi/linux/mrp_bridge.h b/include/uapi/linux/mrp_bridge.h index 6aeb13ef0b1e..d1d0cf65916d 100644 --- a/include/uapi/linux/mrp_bridge.h +++ b/include/uapi/linux/mrp_bridge.h @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr { __be16 state; __be16 transitions; __be32 timestamp; -}; +} __attribute__((__packed__)); struct br_mrp_ring_topo_hdr { __be16 prio; @@ -141,7 +141,7 @@ struct br_mrp_in_test_hdr { __be16 state; __be16 transitions; __be32 timestamp; -}; +} __attribute__((__packed__)); struct br_mrp_in_topo_hdr { __u8 sa[ETH_ALEN];
Wireshark says that the MRP test packets cannot be decoded - and the reason for that is that there's a two-byte hole filled with garbage between the "transitions" and "timestamp" members. So Wireshark decodes the two garbage bytes and the top two bytes of the timestamp written by the kernel as the timestamp value (which thus fluctuates wildly), and interprets the lower two bytes of the timestamp as a new (type, length) pair, which is of course broken. Even though this makes the timestamp field in the struct unaligned, it actually makes it end up on a 32 bit boundary in the frame as mandated by the standard, since it is preceded by a two byte TLV header. The struct definitions live under include/uapi/, but they are not really part of any kernel<->userspace API/ABI, so fixing the definitions by adding the packed attribute should not cause any compatibility issues. Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- include/uapi/linux/mrp_bridge.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)