| Message ID | 20230616092645.3384103-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: atlantic: fix ring buffer alignment | expand |
On Fri, Jun 16, 2023 at 11:26:32AM +0200, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > clang warns about conflicting packing annotations: > > drivers/net/ethernet/aquantia/atlantic/aq_ring.h:72:2: error: field within 'struct aq_ring_buff_s' is less aligned than 'union aq_ring_buff_s::(anonymous at drivers/net/ethernet/aquantia/atlantic/aq_ring.h:72:2)' and is usually due to 'struct aq_ring_buff_s' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access] FWIIW, I was able to reproduce this (warning) with clang-16 on ARM (32bit). And I agree with the approach you have taken here. > This was originally intended to ensure the structure fits exactly into > 32 bytes on 64-bit architectures, but apparently never did, and instead > just produced misaligned pointers as well as forcing byte-wise access > on hardware without unaligned load/store instructions. > > Update the comment to more closely reflect the layout and remove the > broken __packed annotation. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > .../net/ethernet/aquantia/atlantic/aq_ring.h | 26 +++++++++++-------- > 1 file changed, 15 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h > index 0a6c34438c1d0..a9cc5a1c4c479 100644 > --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h > +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h > @@ -26,19 +26,23 @@ struct aq_rxpage { > unsigned int pg_off; > }; > > -/* TxC SOP DX EOP > - * +----------+----------+----------+----------- > - * 8bytes|len l3,l4 | pa | pa | pa > - * +----------+----------+----------+----------- > - * 4/8bytes|len pkt |len pkt | | skb > - * +----------+----------+----------+----------- > - * 4/8bytes|is_gso |len,flags |len |len,is_eop > - * +----------+----------+----------+----------- > +/* TxC SOP DX EOP RX > + * +----------+----------+----------+----------+------- > + * 8bytes|len l3,l4 | pa | pa | pa | hash > + * +----------+----------+----------+----------+------- > + * 4/8bytes|len pkt |len pkt | | skb | page > + * +----------+----------+----------+----------+------- > + * 4/8bytes|is_gso |len,flags |len |len,is_eop| daddr > + * +----------+----------+----------+----------+------- > + * 4/8bytes| | | | | order,pgoff > + * +----------+----------+----------+----------+------- > + * 2bytes | | | | | vlan_rx_tag > + * +----------+----------+----------+----------+------- > + * 8bytes + flags > + * +----------+----------+----------+----------+------- Perhaps it just me. But I do have trouble reconciling the description above with the structure below. As such, my suggest would be to simply delete it. > * > - * This aq_ring_buff_s doesn't have endianness dependency. > - * It is __packed for cache line optimizations. > */ > -struct __packed aq_ring_buff_s { > +struct aq_ring_buff_s { > union { > /* RX/TX */ > dma_addr_t pa;
On Fri, 16 Jun 2023 15:10:03 +0200 Simon Horman wrote: > Perhaps it just me. But I do have trouble reconciling the description > above with the structure below. As such, my suggest would be to simply > delete it. Agreed that the comment is confusing seems to be incorrect post-change. Flags for instance are overlapped with len, is_gso etc. so they can't be a separate 8B at the end. Igor, please advise how you want to proceed.
Hi Jakub, Arnd, > On Fri, 16 Jun 2023 15:10:03 +0200 Simon Horman wrote: >> Perhaps it just me. But I do have trouble reconciling the description >> above with the structure below. As such, my suggest would be to simply >> delete it. > > Agreed that the comment is confusing seems to be incorrect post-change. > Flags for instance are overlapped with len, is_gso etc. so they can't be > a separate 8B at the end. > > Igor, please advise how you want to proceed. I do agree better to remove the comment at all - it explains almost nothing. Thats not a hardware related structure, so explicit pack is for sure not required. Acked-by: Igor Russkikh <irusskikh@marvell.com> Regards, Igor
On Thu, 22 Jun 2023 07:49:52 +0200 Igor Russkikh wrote: > > Agreed that the comment is confusing seems to be incorrect post-change. > > Flags for instance are overlapped with len, is_gso etc. so they can't be > > a separate 8B at the end. > > > > Igor, please advise how you want to proceed. > > I do agree better to remove the comment at all - it explains almost nothing. > > Thats not a hardware related structure, so explicit pack is for sure not required. > > Acked-by: Igor Russkikh <irusskikh@marvell.com> Thanks Igor! Arnd, could you respin with the comment removed?
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h index 0a6c34438c1d0..a9cc5a1c4c479 100644 --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h @@ -26,19 +26,23 @@ struct aq_rxpage { unsigned int pg_off; }; -/* TxC SOP DX EOP - * +----------+----------+----------+----------- - * 8bytes|len l3,l4 | pa | pa | pa - * +----------+----------+----------+----------- - * 4/8bytes|len pkt |len pkt | | skb - * +----------+----------+----------+----------- - * 4/8bytes|is_gso |len,flags |len |len,is_eop - * +----------+----------+----------+----------- +/* TxC SOP DX EOP RX + * +----------+----------+----------+----------+------- + * 8bytes|len l3,l4 | pa | pa | pa | hash + * +----------+----------+----------+----------+------- + * 4/8bytes|len pkt |len pkt | | skb | page + * +----------+----------+----------+----------+------- + * 4/8bytes|is_gso |len,flags |len |len,is_eop| daddr + * +----------+----------+----------+----------+------- + * 4/8bytes| | | | | order,pgoff + * +----------+----------+----------+----------+------- + * 2bytes | | | | | vlan_rx_tag + * +----------+----------+----------+----------+------- + * 8bytes + flags + * +----------+----------+----------+----------+------- * - * This aq_ring_buff_s doesn't have endianness dependency. - * It is __packed for cache line optimizations. */ -struct __packed aq_ring_buff_s { +struct aq_ring_buff_s { union { /* RX/TX */ dma_addr_t pa;