From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933476AbdDESXL (ORCPT ); Wed, 5 Apr 2017 14:23:11 -0400 Received: from mail.skyhub.de ([5.9.137.197]:60618 "EHLO mail.skyhub.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755716AbdDESWc (ORCPT ); Wed, 5 Apr 2017 14:22:32 -0400 Date: Wed, 5 Apr 2017 20:22:23 +0200 From: Borislav Petkov To: "Ghannam, Yazen" Cc: "linux-edac@vger.kernel.org" , Tony Luck , "x86@kernel.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v2 1/2] x86/mce/AMD: Redo use of SMCA MCA_DE{STAT,ADDR} registers Message-ID: <20170405182223.27fzxe27jvinidk6@pd.tnic> References: <1491326672-48298-1-git-send-email-Yazen.Ghannam@amd.com> <20170405133850.ihfyvle3sc5p4yer@pd.tnic> <20170405164433.jo2wghk7jvtsgdq7@pd.tnic> <20170405172203.stnpfpvtm2afzkvk@pd.tnic> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Apr 05, 2017 at 05:53:57PM +0000, Ghannam, Yazen wrote: > Correct, but only on SMCA systems. On !SMCA systems you log only once anyway - you don't have DE* MSRs. > This works so I don't know why it's not okay. So I should take the code just because you have found one way that it works? Screw readability or future proof design - people should dump the code on maintainers and maintainers should deal with the stinking pile. Who cares, it works for you. > Your suggestion also does an SMCA check. Why TF does that even matter? Enough with the dumb checks argument already. > So code that does a check-and-return is preferable to code using > if/else-if statements? If that's the case then I can try to rework it. No, readable code matters. Your suggestion is confusing. I see if (is_deferred_error(status)) { __log_error } else if (mce_flags.smca) { ... if (is_deferred_error(status)) But then here ---> we still deal with deferred errors. Even though we did the deferred check in the if-clause. And that is confusing and makes the code hard to follow. My suggestion does *exactly* what the commit message says: if (__log_error(..)) goto out; when we go to the out label, we know we've succeeded logging the deferred error and we're done. if (!mce_flags.smca) return; When we return here, we know, we've taken care of the !SMCA systems. Now here starts the second attempt to read the DE* registers and we *know* we're on SMCA systems. > How does log_error() know if we can't use the normal MSRs? MCI_STATUS_VAL. > We check for MCI_STATUS_VAL in log_error(). Yes. > We also need to check for MCI_STATUS_DEFERRED but only if we're coming > from the deferred error handler. Why? We *are* coming from the #DF handler so are you expecting a different type of error in the MSRs? -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.