From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id AF046C433EF for ; Thu, 2 Jun 2022 07:40:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Ua2mLF7YNn2rABGDdD8mbZtiBVQRELnOwSC+JNtfEJk=; b=DeiRq7YZJj/wgV AANdaGzAY+6m2X7ZnRkA37Dl2tzJZRCPSbPoWGl5xCyf95e3+CA5HQQ1smUmZKGBPSQ8L/y+3GrGZ tdkXTxlOrV7V8/JOp7g3/4VqCH4i2Ar6lpQC1HEnDACRx7ALFq9TxVY2ohLLlDjwako7+MUJHjtXT sjXC1Mq5rvSJ3iRfWxS1ZwM8ELM94UpwBADFV4ipWVYZpZKJxB91k8LCF8mCdNg63ydVj9afVoGC3 RkH8KzOjazTMi0SLVs7qyb3ZAokME27DB/KN3YzzqOcrukNTRuqfB3D9LveZpbRqqNSILIN4ojGk2 tTnEWwf3nxqvWwIb6u9A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nwfQT-0022fr-Ta; Thu, 02 Jun 2022 07:39:26 +0000 Received: from mout.kundenserver.de ([212.227.126.133]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nwfQP-0022e3-W3 for linux-arm-kernel@lists.infradead.org; Thu, 02 Jun 2022 07:39:23 +0000 Received: from mail-yb1-f170.google.com ([209.85.219.170]) by mrelayeu.kundenserver.de (mreue011 [213.165.67.97]) with ESMTPSA (Nemesis) id 1N9dbx-1nktlO0C7J-015ZHZ for ; Thu, 02 Jun 2022 09:39:15 +0200 Received: by mail-yb1-f170.google.com with SMTP id r82so6871863ybc.13 for ; Thu, 02 Jun 2022 00:39:14 -0700 (PDT) X-Gm-Message-State: AOAM530viDDZxeYUfKfJCucKDsRwZFECYJtA7bKinrFCss8PyMBRWorK KkNEprqapl0UDFJ8Jq5axhlZEMcVvk2F1/401Ws= X-Google-Smtp-Source: ABdhPJxVfhxEh2sV1U0/oHXedOW7Ij9SWgaqXnw+k1hmmKvq9A8gu2wmdTH5eNlILVHHm3z97S0Z2Co5/rXDXyJX6t0= X-Received: by 2002:a25:1209:0:b0:65d:63f9:e10a with SMTP id 9-20020a251209000000b0065d63f9e10amr3777324ybs.480.1654155553742; Thu, 02 Jun 2022 00:39:13 -0700 (PDT) MIME-Version: 1.0 References: <91E67F46-A3C7-4159-9E0C-C6C6306F3669@inria.fr> In-Reply-To: From: Arnd Bergmann Date: Thu, 2 Jun 2022 09:38:56 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: mainline build failure due to f1e4c916f97f ("drm/edid: add EDID block count and size helpers") To: Linus Torvalds Cc: Keisuke Nishimura , Kentaro Takeda , Tetsuo Handa , Ayush Sawal , Vinay Kumar Yadav , Rohit Maheshwari , Julia Lawall , Arnd Bergmann , Jani Nikula , Sudip Mukherjee , Russell King , Viresh Kumar , Shiraz Hashim , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Daniel Vetter , dri-devel , Linux Kernel Mailing List , Linux ARM , SoC Team X-Provags-ID: V03:K1:y8cuu2fWo4/2/Bhg/Rkw3mOUM3kv1gUzQed26Bg+t+/oyw+SCir MfU6Fodcz9SbfKVtPMtIj3WqMjLf22JVuQVX3JesjhJoVu8Ar0HMfG3Abv1egdfuKhaIADw 7a880YZze0J2Be//GgmzZmtA/+5+MTvu0B816kqp+VGiRfUwUpJxKeyhWhF1NPTXcgB2ezv wFKDP9CtT09NbhvQajZyQ== X-UI-Out-Filterresults: notjunk:1;V03:K0:vP3sAS+EiBY=:1D8/2sbd6lBwhf+XHN3S48 2T1IFm3FmpVoZdf9iaou4/wHOfiyvmP96DT9Ro8qkLkddRangGTAzUDfTXcLFmhaZ7BNbc/Xv 6aD9w8/0AM4k80m2mPsIYRZUqCjRC9t91goU5Qvx6ObbokLTkj9Lc/adzJIDp2IgtCuPbifz7 9FF2yHO5BBAwWHMNzNiiaUK7TTltKBwXf7Ui2/iPPtrvIBCKgP5KvjXIj2ijOQIPl7RVGZ32M nbPzfWjWj7e54rj/67QPLCPtrMNrSPjJD/1/zePyrLI73l04Ve6D+7Mjk/vR/18HBREYdadz2 GDBzMdWpGsITXZQWi+N1+Na6rU2wdAHSBui501RgmjWbSy08+7OaxDkm03KWa5NJHNeR/+rB/ tfzTFmIXzr5vTa8aHSvTD5YGuCmLRgbSKFd2WTN1leqxkIdA+/C/SlDSYiHgrFB5/Y25ZuuF2 t9+6vj3P2TUPGUxydNykyNXjkqK0s/NXmST347BbWCVVedlnhMEd7rZ6ObZFDgLKdCHvMpBG2 TXdK9FH+nD0gboY/Ph0CxKOg5QOW65WhHZqQszC1YGiL5Y9zNOWXs5FbQWGX3bIwUVOL8bGGv iQ8Mb5czA8jtEWIgSk8IqXpWe3zOBnubXfvMKXjV7oOFAlfh3+tTqLEClAot5bykU/kXsz6i1 KMT6Jlg+j38Z8DAbfQCjiHZX/ZshVl2UfA0CUlnyFbq7NlTjcr+saLUdgnSRkqmqiKw4ygN4X bhNYB79puOznsxPNdWr9Dg3L0xSYYzYABBBL+unzy0AjLE1QiMj0sQRu5vIIq9vfndta7pa6f S+h4RbmhhORiSmaFW0SJ7/sdY0BJtc2fNMz2mo9Pse0v/ZhXi5PPwO9NzNqNSCRG9XjNzFIQg cXsoqergSzqifiFhTUQlhDbz0Bz2jzNYUN765cRLA= X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220602_003922_373812_B27D19A9 X-CRM114-Status: GOOD ( 38.09 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Jun 2, 2022 at 3:08 AM Linus Torvalds wrote: > > On Wed, Jun 1, 2022 at 3:28 PM Keisuke Nishimura > wrote: > > > > > > I found 13 definitions of packed structure that contains: > > > - spinlock_t > > > - atomic_t > > > - dma_addr_t > > > - phys_addr_t > > > - size_t > > > - struct mutex > > > - struct device > > > > - raw_spinlock_t > > Ok, so I don't think dma_addr_t/phys_addr_t/size_t are problematic, > they are just regular integers. > > And 'struct device' is problematic only as it then contains any of the > atomic types (which it does) is I suggested this list because they are problematic for different reasons: - any atomics are clearly broken here - dma_addr_t/phys_addr_t are sometimes put into hardware data structures in coherent DMA allocations. This is broken because these types are variably-sized depending on the architecture, and annotating structures in uncached memory as __packed is also broken on architectures that have neither coherent DMA nor unaligned access (most 32-bit mips and armv5), where this will result in a series of expensive one-byte uncached load/store instructions. - having any complex kernel data structure embedded in a __packed struct is a red flag, because there should not be a need to mark it packed for compatibility with either hardware or user space. If the structure is actually misaligned, passing a pointer for the embedded struct into an interface that expects an aligned pointer is undefined behavior in C, and gcc may decide to do something bad here even on architectures that can access unaligned pointers. > > security/tomoyo/common.h: atomic_t in tomoyo_shared_acl_head > > drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls.h: spinlock_t in key_map > > arch/s390/include/asm/kvm_host.h: atomic_t in kvm_s390_sie_block > > So these do look problematic. > > I'm not actually clear on why tomoyo_shared_acl_head would be packed. > That makes no sense to me. > > Same goes for key_map, it's not clear what the reason for that > __packed is, and it's clearly bogus. It might work, almost by mistake, > but it's wrong to try to pack that spinlock_t. > > The s390 kvm use actually looks fine: the structure is packed, but > it's also aligned, and the spin-lock is at the beginning, so the > "packing" part is about the other members, not the first one. Right, I think the coccinelle script should nor report structures that are both packed and aligned. > The two that look wrong look like they will probably work anyway > (they'll presumably be effectively word-aligned, and that's sufficient > for spinlocks in practice). > > But let's cc the tomoyo and chelsio people. I think both of them work because the structures are always embedded inside of larger structures that have at least word alignment. This is the thing I was looking for, and the __packed attribute was added in error, most likely copied from somewhere else. Looking at the other ones: include/linux/ti-emif-sram.h: phys_addr_t in ti_emif_pm_data No misalignment because of the __aligned(8), but this might go wrong if the emif firmware relies on the structure layout to have a 32-bit phys_addr_t. drivers/scsi/wd719x.h: dma_addr_t in wd719x_scb This one is correct, as the structure has 64 bytes of hardware data and a few members that are only accessed by the kernel. There should still be an __aligned(8) for efficiency. drivers/net/wireless/intel/ipw2x00/ipw2200.h: dma_addr_t in clx2_queue Al marked the incorrect __packed annotations in 2008, see 83f7d57c37e8 ("ipw2200 annotations and fixes"). Mostly harmless but the __packed should just get removed here. > drivers/infiniband/hw/irdma/osdep.h: dma_addr_t in irdma_dma_mem > drivers/infiniband/core/mad_priv.h: size_t in ib_mad_private Same here: harmless but __packed should be removed, possibly while reordering members by size. > drivers/crypto/qat/qat_common/qat_asym_algs.c: > - dma_addr_t in qat_rsa_ctx > - dma_addr_t in qat_dh_ctx Probably harmless because the structure is __aligned(64), but I'm completely puzzled by what the author was actually trying to achieve here. There are also 'bool' members in the packed struct, which is probably something we want to look for as well. > drivers/atm/idt77252.h: dma_addr_t in idt77252_skb_prv This is a bug on architectures with 64-bit dma_addr_t, it should be an __le32, and the structure should be __aligned() as a DMA descriptor. > drivers/net/wireless/ath/ath10k/core.h: dma_addr_t in ath10k_skb_cb > drivers/net/wireless/ath/ath11k/core.h: dma_addr_t in ath10k_skb_cb Should almost certainly not be __packed, fixing these will likely improve performance on mips32 routers using ath10k > drivers/crypto/ccp/ccp-dev.h: dma_addr_t in ccp_dma_info This looks ok, the "__packed __aligned(4)" here can save a bit of stack space as intended. I think that SmPL script worked great, almost every instance is something that ought to be changed, as long as it stops reporting those structures that are also __aligned(). I would extend it to also report structures with 'bool', 'enum', or any pointer, but that could give more false-positives. Maybe have a separate script for those instances embedding atomics or spinlocks (very broken) vs the other members (causes more harm than good or might need alignment). Arnd _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel