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 8353DD1D887 for ; Tue, 15 Oct 2024 16:15:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=+npNlJ0RDGZqVX53oFq9mVoS50bQaKiInLVdoQQ1tdA=; b=jAw9wlk/wj/fcnMm9wX1VSEQ5V CDKsicNzSPr8R0RTgsz2urriHYksADk4UNwQPTT+QjG/PyBTRflKQldnnrmQNCYzHZ9j2VrJXZwEt jXjzc2ewY/v1ABCLYOLr0aLeGgKmXJuLHAA9en0SuTyr03T1hyTVOuaSnuN9yqnzwFgDLaH2PpKgS lUKf4Kembc4cdF/8JdNCieQ20qOPiiM+dFfVAqRGbQGmy8Dn+D+3FWjpT8A18QyxsXaA5eTqIpaOa AsqvAc96POxE2q9k6MsJWwIU6cuH3l0ZIKGYP8CXuh/bnNjRiYb2HpU8SIyqaNYvUYvWHi3IYmYho dV2Owiaw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1t0kD1-00000008qlf-2NqC; Tue, 15 Oct 2024 16:15:43 +0000 Received: from mail.alien8.de ([65.109.113.108]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1t0k9Z-00000008puQ-3IqF for linux-arm-kernel@lists.infradead.org; Tue, 15 Oct 2024 16:12:11 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by mail.alien8.de (SuperMail on ZX Spectrum 128k) with ESMTP id 8B5CD40E0184; Tue, 15 Oct 2024 16:12:07 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at mail.alien8.de Authentication-Results: mail.alien8.de (amavisd-new); dkim=pass (4096-bit key) header.d=alien8.de Received: from mail.alien8.de ([127.0.0.1]) by localhost (mail.alien8.de [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id nh40SCebYiZC; Tue, 15 Oct 2024 16:12:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=alien8.de; s=alien8; t=1729008722; bh=+npNlJ0RDGZqVX53oFq9mVoS50bQaKiInLVdoQQ1tdA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=kSXC3tImw65OzUTFwjKquU2RUgWwy6hy7Dcnoh2Lsl2C+h5gDxPu5r3tjT4p/j7GQ ojOJ6xA77m+nR25NMYmWtTtob7o8h1zcDOqpgxSgMa6v3dnw9lPOJDvxAhcgdN1yfW nNbaNSW0tw4enoFu7L6nUUA64vFD8V7wNrlM+vz8dGcjGDKyB4ItSA8jksdyTGNFQI jfNIQ9OGyNglPmx0fm8p8IWOaJ9N0nXHNcdjiINyiaH+t05Rl19iTMc6ZBqKeZufMc yF10mv95uX8V0qPYDJ9bQMAWOs7wb/lu8TgmD6zTPnJDcU58cdzAqAj1QThlHBxGSu /+nRprGHvgBjAMJqe6hVQDGox9F0U477M74jwvRsAJLCXeJw/4mon/EVaOgK/WqLnW BA2QnpVlZta6yT0RUgSieyytIg9YL61MiMwuRNC1XdXcJwxlUSUM5cDb3WV5b9EPTO QzR3iPvUJpHyYQMOnxMTaBqP8jmB1vk99e6cD1CA1fDM9q7Q34gNBkBAHw7DXDTFeG qj2BZkqJZawf1fWtMOuBdtSd4MAhFErDvO10VmnMaxA65oqHQWd7Mzwxj9BmoeWgMu FcKqKhkYWYfHUvocfplk+f4Nj8NtAdptmkXfDl66Ia4en6YPC+jT4CvmUufAVW6oMz BeoetI6sJ2Gh3Jeju4ykTflY= Received: from zn.tnic (p5de8e8eb.dip0.t-ipconnect.de [93.232.232.235]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature ECDSA (P-256) server-digest SHA256) (No client certificate requested) by mail.alien8.de (SuperMail on ZX Spectrum 128k) with ESMTPSA id 6A1B940E0263; Tue, 15 Oct 2024 16:11:40 +0000 (UTC) Date: Tue, 15 Oct 2024 18:11:39 +0200 From: Borislav Petkov To: Frank Li Cc: York Sun , Tony Luck , James Morse , Mauro Carvalho Chehab , Robert Richter , Krzysztof Kozlowski , Rob Herring , Conor Dooley , Krzysztof Kozlowski , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org, Priyanka Singh , Sherry Sun , Li Yang Subject: Re: [PATCH v2 3/6] EDAC/fsl_ddr: Fix bad bit shift operations Message-ID: <20241015161139.GEZw6UO2txZVBXffKc@fat_crate.local> References: <20241011-imx95_edac-v2-0-011b68290951@nxp.com> <20241011-imx95_edac-v2-3-011b68290951@nxp.com> <20241014181647.GQZw1gDwIhBdnFnleH@fat_crate.local> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20241015_091210_002237_984B2BF7 X-CRM114-Status: GOOD ( 20.79 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Oct 15, 2024 at 11:31:41AM -0400, Frank Li wrote: > I don't think it is urgent. In most system the return value is 0. I am not > sure who caught it because patch already exist at downstream tree for a > whole. That current patch looks like it needs rethinking and making it sane - see below. > > > diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c > > > index 7a9fb1202f1a0..ccc13c2adfd6f 100644 > > > --- a/drivers/edac/fsl_ddr_edac.c > > > +++ b/drivers/edac/fsl_ddr_edac.c > > > @@ -338,11 +338,18 @@ static void fsl_mc_check(struct mem_ctl_info *mci) > > > fsl_mc_printk(mci, KERN_ERR, > > > "Faulty ECC bit: %d\n", bad_ecc_bit); > > > > > > - fsl_mc_printk(mci, KERN_ERR, > > > - "Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n", > > > - cap_high ^ (1 << (bad_data_bit - 32)), > > > - cap_low ^ (1 << bad_data_bit), > > > - syndrome ^ (1 << bad_ecc_bit)); > > > + if ((bad_data_bit > 0 && bad_data_bit < 32) && bad_ecc_bit > 0) { > > > + fsl_mc_printk(mci, KERN_ERR, > > > + "Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n", > > > + cap_high, cap_low ^ (1 << bad_data_bit), > > > + syndrome ^ (1 << bad_ecc_bit)); > > > + } > > > + if (bad_data_bit >= 32 && bad_ecc_bit > 0) { > > > + fsl_mc_printk(mci, KERN_ERR, > > > + "Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n", > > > + cap_high ^ (1 << (bad_data_bit - 32)), > > > + cap_low, syndrome ^ (1 << bad_ecc_bit)); > > > + } > > > > This is getting unnecessarily clumsy than it should be. Please do the > > following: > > > > if (bad_data_bit != 1 && bad_ecc_bit != -1) { > > > > // prep the values you need to print > > > > // do an exactly one fsl_mc_printk() with the prepared values. > > > > } > > > > Not have 4 fsl_mc_printks with a bunch of silly if-checks in front. You missed the most important feedback. See above. ^^^^^^^ -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette