Messages in this thread | | | Date | Mon, 19 Jun 2023 15:04:10 +0200 | From | "Arnd Bergmann" <> | Subject | Re: [PATCH 3/3] sfc: selftest: fix struct packing |
| |
On Mon, Jun 19, 2023, at 12:25, Edward Cree wrote: > On 19/06/2023 10:12, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> >> Three of the sfc drivers define a packed loopback_payload structure with an >> ethernet header followed by an IP header. However, the kernel definition >> of iphdr specifies that this is 4-byte aligned, causing a W=1 warning: >> >> net/ethernet/sfc/siena/selftest.c:46:15: error: field ip within 'struct efx_loopback_payload' is less aligned than 'struct iphdr' and is usually due to 'struct efx_loopback_payload' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access] >> struct iphdr ip; >> >> As the iphdr packing is not easily changed without breaking other code, >> change the three structures to use a local definition instead. >> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Duplicating the definition isn't the prettiest thing in the world; it'd > do for a quick fix if needed but I assume W=1 warnings aren't blocking > anyone, so maybe defer this one for now and I'll follow up soon with a > rewrite that fixes this more cleanly? My idea is to drop the __packed > from the containing struct, make efx_begin_loopback() copy the layers > separately, and efx_loopback_rx_packet() similarly do something less > direct than casting the packet data to the struct. > > But I don't insist on it; if you want this fix in immediately then I'm > okay with that too. > >> --- >> drivers/net/ethernet/sfc/falcon/selftest.c | 21 ++++++++++++++++++++- >> drivers/net/ethernet/sfc/selftest.c | 21 ++++++++++++++++++++- >> drivers/net/ethernet/sfc/siena/selftest.c | 21 ++++++++++++++++++++- >> 3 files changed, 60 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/ethernet/sfc/falcon/selftest.c b/drivers/net/ethernet/sfc/falcon/selftest.c >> index 6a454ac6f8763..fb7fcd27a33a5 100644 >> --- a/drivers/net/ethernet/sfc/falcon/selftest.c >> +++ b/drivers/net/ethernet/sfc/falcon/selftest.c >> @@ -40,7 +40,26 @@ >> */ >> struct ef4_loopback_payload { >> struct ethhdr header; >> - struct iphdr ip; >> + struct { >> +#if defined(__LITTLE_ENDIAN_BITFIELD) >> + __u8 ihl:4, >> + version:4; >> +#elif defined (__BIG_ENDIAN_BITFIELD) >> + __u8 version:4, >> + ihl:4; >> +#else >> +#error "Please fix <asm/byteorder.h>" >> +#endif >> + __u8 tos; >> + __be16 tot_len; >> + __be16 id; >> + __be16 frag_off; >> + __u8 ttl; >> + __u8 protocol; >> + __sum16 check; >> + __be32 saddr; >> + __be32 daddr; >> + } __packed ip; /* unaligned struct iphdr */ >> struct udphdr udp; >> __be16 iteration; >> char msg[64]; >> diff --git a/drivers/net/ethernet/sfc/selftest.c b/drivers/net/ethernet/sfc/selftest.c >> index 3c5227afd4977..440a57953779c 100644 >> --- a/drivers/net/ethernet/sfc/selftest.c >> +++ b/drivers/net/ethernet/sfc/selftest.c >> @@ -43,7 +43,26 @@ >> */ >> struct efx_loopback_payload { >> struct ethhdr header; >> - struct iphdr ip; >> + struct { >> +#if defined(__LITTLE_ENDIAN_BITFIELD) >> + __u8 ihl:4, >> + version:4; >> +#elif defined (__BIG_ENDIAN_BITFIELD) >> + __u8 version:4, >> + ihl:4; >> +#else >> +#error "Please fix <asm/byteorder.h>" >> +#endif >> + __u8 tos; >> + __be16 tot_len; >> + __be16 id; >> + __be16 frag_off; >> + __u8 ttl; >> + __u8 protocol; >> + __sum16 check; >> + __be32 saddr; >> + __be32 daddr; >> + } __packed ip; /* unaligned struct iphdr */ >> struct udphdr udp; >> __be16 iteration; >> char msg[64]; >> diff --git a/drivers/net/ethernet/sfc/siena/selftest.c b/drivers/net/ethernet/sfc/siena/selftest.c >> index 07715a3d6beab..b8a8b0495f661 100644 >> --- a/drivers/net/ethernet/sfc/siena/selftest.c >> +++ b/drivers/net/ethernet/sfc/siena/selftest.c >> @@ -43,7 +43,26 @@ >> */ >> struct efx_loopback_payload { >> struct ethhdr header; >> - struct iphdr ip; >> + struct { >> +#if defined(__LITTLE_ENDIAN_BITFIELD) >> + __u8 ihl:4, >> + version:4; >> +#elif defined (__BIG_ENDIAN_BITFIELD) >> + __u8 version:4, >> + ihl:4; >> +#else >> +#error "Please fix <asm/byteorder.h>" >> +#endif >> + __u8 tos; >> + __be16 tot_len; >> + __be16 id; >> + __be16 frag_off; >> + __u8 ttl; >> + __u8 protocol; >> + __sum16 check; >> + __be32 saddr; >> + __be32 daddr; >> + } __packed ip; /* unaligned struct iphdr */ >> struct udphdr udp; >> __be16 iteration; >> char msg[64]; >>
| |