| Message ID | 20221129161359.75792-2-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [resend,net-next,v1,1/2] net: thunderbolt: Switch from __maybe_unused to pm_sleep_ptr() etc | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 18 this patch: 1 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 18 this patch: 1 |
| netdev/checkpatch | warning | WARNING: line length of 82 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Tue, Nov 29, 2022 at 06:13:59PM +0200, Andy Shevchenko wrote: > The same data type structure is used for bitwise operations and > regular ones. It makes sparse unhappy, for example: > > .../thunderbolt.c:718:23: warning: cast to restricted __le32 > > .../thunderbolt.c:953:23: warning: incorrect type in initializer (different base types) > .../thunderbolt.c:953:23: expected restricted __wsum [usertype] wsum > .../thunderbolt.c:953:23: got restricted __be32 [usertype] > > Split the header to bitwise one and specific for Rx to make sparse > happy. Assure the layout by involving static_assert() against size > and offsets of the member of the structures. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/net/thunderbolt.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) I would much rather keep the humans reading this happy than add 20+ lines just to silence a tool. Unless this of course is some kind of a real bug.
On Wed, Nov 30, 2022 at 09:46:16AM +0200, Mika Westerberg wrote: > On Tue, Nov 29, 2022 at 06:13:59PM +0200, Andy Shevchenko wrote: > > The same data type structure is used for bitwise operations and > > regular ones. It makes sparse unhappy, for example: > > > > .../thunderbolt.c:718:23: warning: cast to restricted __le32 > > > > .../thunderbolt.c:953:23: warning: incorrect type in initializer (different base types) > > .../thunderbolt.c:953:23: expected restricted __wsum [usertype] wsum > > .../thunderbolt.c:953:23: got restricted __be32 [usertype] > > > > Split the header to bitwise one and specific for Rx to make sparse > > happy. Assure the layout by involving static_assert() against size > > and offsets of the member of the structures. > I would much rather keep the humans reading this happy than add 20+ > lines just to silence a tool. Unless this of course is some kind of a > real bug. Actually, changing types to bitwise ones reduces the sparse noise (I will double check this) without reducing readability. Would it be accepted?
On Wed, Nov 30, 2022 at 12:51:06PM +0200, Andy Shevchenko wrote: > On Wed, Nov 30, 2022 at 09:46:16AM +0200, Mika Westerberg wrote: > > On Tue, Nov 29, 2022 at 06:13:59PM +0200, Andy Shevchenko wrote: > > > The same data type structure is used for bitwise operations and > > > regular ones. It makes sparse unhappy, for example: > > > > > > .../thunderbolt.c:718:23: warning: cast to restricted __le32 > > > > > > .../thunderbolt.c:953:23: warning: incorrect type in initializer (different base types) > > > .../thunderbolt.c:953:23: expected restricted __wsum [usertype] wsum > > > .../thunderbolt.c:953:23: got restricted __be32 [usertype] > > > > > > Split the header to bitwise one and specific for Rx to make sparse > > > happy. Assure the layout by involving static_assert() against size > > > and offsets of the member of the structures. > > > I would much rather keep the humans reading this happy than add 20+ > > lines just to silence a tool. Unless this of course is some kind of a > > real bug. > > Actually, changing types to bitwise ones reduces the sparse noise > (I will double check this) without reducing readability. > Would it be accepted? Sure if it makes it more readable and does not add too many lines :)
On Wed, Nov 30, 2022 at 01:09:59PM +0200, Mika Westerberg wrote: > On Wed, Nov 30, 2022 at 12:51:06PM +0200, Andy Shevchenko wrote: > > On Wed, Nov 30, 2022 at 09:46:16AM +0200, Mika Westerberg wrote: > > > On Tue, Nov 29, 2022 at 06:13:59PM +0200, Andy Shevchenko wrote: > > > > The same data type structure is used for bitwise operations and > > > > regular ones. It makes sparse unhappy, for example: > > > > > > > > .../thunderbolt.c:718:23: warning: cast to restricted __le32 > > > > > > > > .../thunderbolt.c:953:23: warning: incorrect type in initializer (different base types) > > > > .../thunderbolt.c:953:23: expected restricted __wsum [usertype] wsum > > > > .../thunderbolt.c:953:23: got restricted __be32 [usertype] > > > > > > > > Split the header to bitwise one and specific for Rx to make sparse > > > > happy. Assure the layout by involving static_assert() against size > > > > and offsets of the member of the structures. > > > > > I would much rather keep the humans reading this happy than add 20+ > > > lines just to silence a tool. Unless this of course is some kind of a > > > real bug. > > > > Actually, changing types to bitwise ones reduces the sparse noise > > (I will double check this) without reducing readability. > > Would it be accepted? > > Sure if it makes it more readable and does not add too many lines :) It replaces types u* by __le*, that's it: -4 +4 LoCs.
diff --git a/drivers/net/thunderbolt.c b/drivers/net/thunderbolt.c index 4dbc6c7f2e10..f7b3d0d4646c 100644 --- a/drivers/net/thunderbolt.c +++ b/drivers/net/thunderbolt.c @@ -58,12 +58,32 @@ * supported then @frame_id is filled, otherwise it stays %0. */ struct thunderbolt_ip_frame_header { + __le32 frame_size; + __le16 frame_index; + __le16 frame_id; + __le32 frame_count; +}; + +/* Same as &struct thunderbolt_ip_frame_header for Rx */ +struct thunderbolt_ip_frame_rx_hdr { u32 frame_size; u16 frame_index; u16 frame_id; u32 frame_count; }; +static_assert(sizeof(struct thunderbolt_ip_frame_header) == + sizeof(struct thunderbolt_ip_frame_rx_hdr)); + +#define TBIP_FRAME_HDR_MATCH(x) \ + static_assert(offsetof(struct thunderbolt_ip_frame_header, frame_##x) == \ + offsetof(struct thunderbolt_ip_frame_rx_hdr, frame_##x)) +TBIP_FRAME_HDR_MATCH(size); +TBIP_FRAME_HDR_MATCH(index); +TBIP_FRAME_HDR_MATCH(id); +TBIP_FRAME_HDR_MATCH(count); +#undef TBIP_FRAME_HDR_MATCH + enum thunderbolt_ip_frame_pdf { TBIP_PDF_FRAME_START = 1, TBIP_PDF_FRAME_END, @@ -193,7 +213,7 @@ struct tbnet { struct delayed_work login_work; struct work_struct connected_work; struct work_struct disconnect_work; - struct thunderbolt_ip_frame_header rx_hdr; + struct thunderbolt_ip_frame_rx_hdr rx_hdr; struct tbnet_ring rx_ring; atomic_t frame_id; struct tbnet_ring tx_ring;
The same data type structure is used for bitwise operations and regular ones. It makes sparse unhappy, for example: .../thunderbolt.c:718:23: warning: cast to restricted __le32 .../thunderbolt.c:953:23: warning: incorrect type in initializer (different base types) .../thunderbolt.c:953:23: expected restricted __wsum [usertype] wsum .../thunderbolt.c:953:23: got restricted __be32 [usertype] Split the header to bitwise one and specific for Rx to make sparse happy. Assure the layout by involving static_assert() against size and offsets of the member of the structures. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/net/thunderbolt.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)