| Message ID | 20210511152538.148084-2-nchatrad@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] x86/MCE/AMD, EDAC/mce_amd: Add new SMCA bank types. | expand |
On Tue, May 11, 2021 at 08:55:37PM +0530, Naveen Krishna Chatradhi wrote: > Subject: Re: [PATCH 2/3] x86/MCE/AMD: Helper function to check UMC v2 The condensed patch description in the subject line should start with a uppercase letter and should be written in imperative tone: "x86/MCE/AMD: Add a helper function... " > Signed-off-by: Mukul Joshi <mukul.joshi@amd.com> > Reviewed-by: John Clements <John.Clements@amd.com> > Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com> This is all fine and dandy but it needs your SOB too when you send the patch. Please read Documentation/process/submitting-patches.rst sections Sign your work - the Developer's Certificate of Origin When to use Acked-by:, Cc:, and Co-developed-by: > +bool is_smca_umc_v2(int bank) > +{ > + return (smca_get_bank_type(bank) == SMCA_UMC_V2); > +} > +EXPORT_SYMBOL_GPL(is_smca_umc_v2); This addition looks useless when it doesn't have any users. Also, I'm pretty sceptical this even makes sense to have it exported - I'm guessing this is for mce_amd.c but I can't say without seeing it in use. The same remarks hold true for your patch 3. Thx.
[AMD Official Use Only - Internal Distribution Only] Hi Boris, Thanks for the review comments. I will update the Subject line and send out an updated patch. In the meantime, here is the link to where the changes in Patch 2 and 3 are being used: https://lists.freedesktop.org/archives/amd-gfx/2021-May/063423.html Thanks. Regards, Mukul -----Original Message----- From: Borislav Petkov <bp@alien8.de> Sent: Tuesday, May 11, 2021 1:35 PM To: Chatradhi, Naveen Krishna <NaveenKrishna.Chatradhi@amd.com> Cc: linux-edac@vger.kernel.org; x86@kernel.org; linux-kernel@vger.kernel.org; mingo@redhat.com; mchehab@kernel.org; Joshi, Mukul <Mukul.Joshi@amd.com> Subject: Re: [PATCH 2/3] x86/MCE/AMD: Helper function to check UMC v2 [CAUTION: External Email] On Tue, May 11, 2021 at 08:55:37PM +0530, Naveen Krishna Chatradhi wrote: > Subject: Re: [PATCH 2/3] x86/MCE/AMD: Helper function to check UMC v2 The condensed patch description in the subject line should start with a uppercase letter and should be written in imperative tone: "x86/MCE/AMD: Add a helper function... " > Signed-off-by: Mukul Joshi <mukul.joshi@amd.com> > Reviewed-by: John Clements <John.Clements@amd.com> > Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com> This is all fine and dandy but it needs your SOB too when you send the patch. Please read Documentation/process/submitting-patches.rst sections Sign your work - the Developer's Certificate of Origin When to use Acked-by:, Cc:, and Co-developed-by: > +bool is_smca_umc_v2(int bank) > +{ > + return (smca_get_bank_type(bank) == SMCA_UMC_V2); } > +EXPORT_SYMBOL_GPL(is_smca_umc_v2); This addition looks useless when it doesn't have any users. Also, I'm pretty sceptical this even makes sense to have it exported - I'm guessing this is for mce_amd.c but I can't say without seeing it in use. The same remarks hold true for your patch 3. Thx. -- Regards/Gruss, Boris. https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&data=04%7C01%7Cmukul.joshi%40amd.com%7C0a7f1e0a3bf04c9ed89408d914a3141c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563512928580623%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=z3xbuS8yX4Ea1kG9tWv0tGNQRPZP9N6QcBI2VNjn6W0%3D&reserved=0
Hi Mukul, On Wed, May 12, 2021 at 01:40:54AM +0000, Joshi, Mukul wrote: > [AMD Official Use Only - Internal Distribution Only] first of all, please do not top-post. > Thanks for the review comments. > I will update the Subject line and send out an updated patch. Ok. > In the meantime, here is the link to where the changes in Patch 2 and 3 are being used: > https://lists.freedesktop.org/archives/amd-gfx/2021-May/063423.html Yes, in the future, please Cc x86@kernel.org too when you're using/exporting facilities to be used by other subsystems. I'll try to reply to that patch through lore.kernel.org now. Thx.
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h index cf7f35fdf2c8..8cbe7221a253 100644 --- a/arch/x86/include/asm/mce.h +++ b/arch/x86/include/asm/mce.h @@ -357,6 +357,7 @@ extern int mce_threshold_remove_device(unsigned int cpu); void mce_amd_feature_init(struct cpuinfo_x86 *c); int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr); +bool is_smca_umc_v2(int bank); #else @@ -366,6 +367,7 @@ static inline bool amd_mce_is_memory_error(struct mce *m) { return false; }; static inline void mce_amd_feature_init(struct cpuinfo_x86 *c) { } static inline int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr) { return -EINVAL; }; +static inline bool is_smca_umc_v2(int bank) { return false; }; #endif static inline void mce_hygon_feature_init(struct cpuinfo_x86 *c) { return mce_amd_feature_init(c); } diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c index 055f3a0acf5e..41718e3111f2 100644 --- a/arch/x86/kernel/cpu/mce/amd.c +++ b/arch/x86/kernel/cpu/mce/amd.c @@ -1542,3 +1542,9 @@ int mce_threshold_create_device(unsigned int cpu) mce_threshold_remove_device(cpu); return err; } + +bool is_smca_umc_v2(int bank) +{ + return (smca_get_bank_type(bank) == SMCA_UMC_V2); +} +EXPORT_SYMBOL_GPL(is_smca_umc_v2);