Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 16 Jun 2023 15:10:03 +0200
From:   Simon Horman <simon.horman@corigine.com>
To:     Arnd Bergmann <arnd@kernel.org>
Cc:     Igor Russkikh <irusskikh@marvell.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, Arnd Bergmann <arnd@arndb.de>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Tom Rix <trix@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, llvm@lists.linux.dev
Subject: Re: [PATCH] net: atlantic: fix ring buffer alignment
Message-ID: <ZIxfK1MVRL+1wDvq@corigine.com>
References: <20230616092645.3384103-1-arnd@kernel.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230616092645.3384103-1-arnd@kernel.org>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?2iccjbmPr0Qyt78gFY6BmlfTUMQlkNXaGzfas93YPDFy+ByW64UyJDt6A5d1?=
 =?us-ascii?Q?3c3wNIB+5iIzosZyNVFiuPKNS90kc4ovRLa7dlnbskFQ6W9fIaTQEtgV0X6l?=
 =?us-ascii?Q?Pkz/3+XMaq/JbWT9JVzLTFSpoU9yeWPXln7cajfbHcjJtNOD3nkLwjI0ATDA?=
 =?us-ascii?Q?HR809sZk07JPGrf7PRx7NM1tUTnJdxm8Nust+jcaBkNuZSsBbP2BQ1/wefH0?=
 =?us-ascii?Q?LQWhGOZP+EiuCcTbImX/zPhnyq6tMwOUjFf38+DGZYyyztMroN8MlHCy00qN?=
 =?us-ascii?Q?utg6KH2Vsj2G4RSfeG18cTE6j2Q46tfYgMOto+L0J8+w7PEr0AqPNDC3MR7a?=
 =?us-ascii?Q?3NhmICyhI2kDqGpmFNeFHIftHirmw/eInRxe4ikCfpC+ZERr917e66z+z10x?=
 =?us-ascii?Q?juWszvwVOf//QQkooagnGPdR29AFtWsmr43gz8oXkluxfkQ26e1a+yLKThn2?=
 =?us-ascii?Q?vB3VUZmO453J19yYwUpEiP1fVn8E6HyjeZdIhxLmuSiy01EkYLwnmdAFs+J5?=
 =?us-ascii?Q?puORD+vgpxSGmCk7h7IxF5bMtPHxHezb3YurvooMBluwe4uHdw12WCYRffDA?=
 =?us-ascii?Q?euliyblvXnfJ3UNljO3vzEVtNALTbApCWPKO1k8JvS79MB5DDcD5qHVnUIvR?=
 =?us-ascii?Q?yIAA/9Hd3GDPMez2trKi4WAVaGMRu1aqwGw6rtFcVSV7nT01LlYDlrwwRC05?=
 =?us-ascii?Q?5Y8bsO3UqNVmxuleE7qtkEYkZ9EXt3TjkXatIvN3Rpey8Vjj9UYCZarnt2O4?=
 =?us-ascii?Q?xI8/dZ05dTwC9XFdYjl8b82DdgkMZQMCR/MTVdregaeZEEEKLvElNQSTZy+Y?=
 =?us-ascii?Q?wOglDX617n42NIsMzlCLfPYvttrIRxqGcRH7ARx0a/FGm9dchVclELecjVbq?=
 =?us-ascii?Q?/BoBPBoEQlaOty0tBai9TI5JzBqX9NVcKUyoobwa+bvInjhsHMVpoK2os7+b?=
 =?us-ascii?Q?WBVTZbW0XnlZXKqgyqelKP/I7SEsifuWr6zmj0Htq/7nTy/xnOzNesccrg77?=
 =?us-ascii?Q?8NprQGNHXOonRECk4FRtz0/Ewj7L/ikGktyEePUe12WD/BiRkjNZ/+Xopm3d?=
 =?us-ascii?Q?1SNXpxhSFigtCmpdVCxCwnjSsTiJjYOHUJJT3fcjCmlyzB4yIqO7r5dKoIWt?=
 =?us-ascii?Q?BiY7qJiY6t18kQ8TA1TbH63jFTyHJCMDweAsMIy1ito2KMuTvpt1Dj3ZF5lX?=
 =?us-ascii?Q?HrNpxlmVmFhOg6tFjbPYW5FscY+Fe5b0uUydV1+HEXjrLWpid0753v8tCSQd?=
 =?us-ascii?Q?rMOe0GtFhmjeejQRlkJlG5voDyfG16VSfuovzZTg6CVclYgyZit/IHYrhpII?=
 =?us-ascii?Q?EbchQWiimvOjkzIiJb3FfplD8H05PMaM4/F9oW18xz1SLueLca/IFuBqb4DB?=
 =?us-ascii?Q?IxiKz5QNYKZDNOa4620KGd0MqbOkki6S3V1JaUbv/8YEA4XI8uM1ZylAzqAj?=
 =?us-ascii?Q?QLTVvIiNIHxAU4EYy+R2bymk3eWew7ZtlXhVTsiHlp0nrWEQMHQI0/L1pfIk?=
 =?us-ascii?Q?zf7vG312fVcOmkwTQmKqot2O4JfjzG3O/tgVcoMqIpWE59yZbzZuEXIOxCK7?=
 =?us-ascii?Q?mKwtCqUzULCpxpmnhiUF8+NBn7vyzu/10qHr2TepS057NYLVtHPQyxtJa49y?=
 =?us-ascii?Q?5HZACQV9l94sCyak3tC7WzQrKcsfQCcMm0SHAxM+RXToBJIklx0UEvf0ZYOq?=
 =?us-ascii?Q?xdDK1w=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f036685f-fa13-4bd8-d1a5-08db6e6b0421
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Jun 2023 13:10:11.4430
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: qoK6BZlI/pp6a4KkkXxhW8I9iTJ3/2xfrPUpGJdPcNYrLVzFH0PGdmZgYcYZ0D2FgsRrOR16dI/FAvt9J1Vw5ktnp2T6m0iGyqOGoxl+vcc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR13MB5713
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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;