[v4] x86/mce: Clean up some x86/mce code

[PATCH v4 8/8] x86/mce: Fix typos

Posted by Qiuxu Zhuo 1 week, 5 days ago

Fix typos in comments.

Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v4:
  - No changes.

Changes in v3:
  - Collect "Reviewed-by:" from Nikolay & Sohil.
  - Remove the detail typos from the commit message (Sohil).

Changes in v2:
  - Collect "Reviewed-by:" from Tony.

 arch/x86/kernel/cpu/mce/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 0f0c6e9d9183..6e194ccffc7c 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1144,7 +1144,7 @@ static noinstr int mce_start(int *no_way_out)
 	} else {
 		/*
 		 * Subject: Now start the scanning loop one by one in
-		 * the original callin order.
+		 * the original calling order.
 		 * This way when there are any shared banks it will be
 		 * only seen by one CPU before cleared, avoiding duplicates.
 		 */
@@ -1917,7 +1917,7 @@ static void apply_quirks_amd(struct cpuinfo_x86 *c)
 	/* This should be disabled by the BIOS, but isn't always */
 	if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
 		/*
-		 * disable GART TBL walk error reporting, which
+		 * disable GART TLB walk error reporting, which
 		 * trips off incorrectly with the IOMMU & 3ware
 		 * & Cerberus:
 		 */
-- 
2.17.1

Re: [PATCH v4 8/8] x86/mce: Fix typos

Posted by Yazen Ghannam 1 week, 4 days ago

On Mon, Nov 11, 2024 at 02:04:28PM +0800, Qiuxu Zhuo wrote:
> Fix typos in comments.
> 
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
> Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> ---
> Changes in v4:
>   - No changes.
> 
> Changes in v3:
>   - Collect "Reviewed-by:" from Nikolay & Sohil.
>   - Remove the detail typos from the commit message (Sohil).
> 
> Changes in v2:
>   - Collect "Reviewed-by:" from Tony.
> 
>  arch/x86/kernel/cpu/mce/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 0f0c6e9d9183..6e194ccffc7c 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -1144,7 +1144,7 @@ static noinstr int mce_start(int *no_way_out)
>  	} else {
>  		/*
>  		 * Subject: Now start the scanning loop one by one in
> -		 * the original callin order.
> +		 * the original calling order.

I don't think this is a typo. It seems to refer to the mce_callin
variable/idea.

For example, each CPU "calls in" when ready. This is independent of when
each CPU is "called" to do something.

	CPUs are called in this order 0, 1, 2.
	CPUs "call in" in this order 1, 0, 2.

When a CPU is called can be different from when it responds.

Maybe I'm reading too much into this. :/

>  		 * This way when there are any shared banks it will be
>  		 * only seen by one CPU before cleared, avoiding duplicates.
>  		 */
> @@ -1917,7 +1917,7 @@ static void apply_quirks_amd(struct cpuinfo_x86 *c)
>  	/* This should be disabled by the BIOS, but isn't always */
>  	if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
>  		/*
> -		 * disable GART TBL walk error reporting, which
> +		 * disable GART TLB walk error reporting, which

This also is not a typo. TBL -> table

From old AMD K8 BKDG document:
	10 GartTblWkEn GART Table Walk Error Reporting Enable R/W 0

Thanks,
Yazen

RE: [PATCH v4 8/8] x86/mce: Fix typos

Posted by Zhuo, Qiuxu 1 week, 3 days ago

Hi Yazen,

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> [...]
> > @@ -1144,7 +1144,7 @@ static noinstr int mce_start(int *no_way_out)
> >  	} else {
> >  		/*
> >  		 * Subject: Now start the scanning loop one by one in
> > -		 * the original callin order.
> > +		 * the original calling order.
> 
> I don't think this is a typo. It seems to refer to the mce_callin variable/idea.
> 
> For example, each CPU "calls in" when ready. This is independent of when
> each CPU is "called" to do something.
> 
> 	CPUs are called in this order 0, 1, 2.
> 	CPUs "call in" in this order 1, 0, 2.
> 
> When a CPU is called can be different from when it responds.
> 
> Maybe I'm reading too much into this. :/

Too finicky to me :(
But I appreciate you sharing your reading and thoughts. 😊

-Qiuxu

Re: [PATCH v4 8/8] x86/mce: Fix typos

Posted by Sohil Mehta 1 week, 3 days ago

On 11/12/2024 7:38 AM, Yazen Ghannam wrote:
> 
>>  		 * This way when there are any shared banks it will be
>>  		 * only seen by one CPU before cleared, avoiding duplicates.
>>  		 */
>> @@ -1917,7 +1917,7 @@ static void apply_quirks_amd(struct cpuinfo_x86 *c)
>>  	/* This should be disabled by the BIOS, but isn't always */
>>  	if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
>>  		/*
>> -		 * disable GART TBL walk error reporting, which
>> +		 * disable GART TLB walk error reporting, which
> 
> This also is not a typo. TBL -> table
> 
> From old AMD K8 BKDG document:
> 	10 GartTblWkEn GART Table Walk Error Reporting Enable R/W 0
> 

There is another comment in init_amd_gh() that seems to be related to
the same thing and similarly worded. That seems to refer to TLB instead
of TBL(table).

/*
 * Disable GART TLB Walk Errors on Fam10h. We do this here because this
 * is always needed when GART is enabled, even in a kernel which has no
 * MCE support built in. BIOS should disable GartTlbWlk Errors already.
 * If it doesn't, we do it here as suggested by the BKDG.
 *
 * Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=33012
 */
msr_set_bit(MSR_AMD64_MCx_MASK(4), 10);

Re: [PATCH v4 8/8] x86/mce: Fix typos

Posted by Yazen Ghannam 1 week, 3 days ago

On Tue, Nov 12, 2024 at 02:35:32PM -0800, Sohil Mehta wrote:
> On 11/12/2024 7:38 AM, Yazen Ghannam wrote:
> > 
> >>  		 * This way when there are any shared banks it will be
> >>  		 * only seen by one CPU before cleared, avoiding duplicates.
> >>  		 */
> >> @@ -1917,7 +1917,7 @@ static void apply_quirks_amd(struct cpuinfo_x86 *c)
> >>  	/* This should be disabled by the BIOS, but isn't always */
> >>  	if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
> >>  		/*
> >> -		 * disable GART TBL walk error reporting, which
> >> +		 * disable GART TLB walk error reporting, which
> > 
> > This also is not a typo. TBL -> table
> > 
> > From old AMD K8 BKDG document:
> > 	10 GartTblWkEn GART Table Walk Error Reporting Enable R/W 0
> > 
> 
> There is another comment in init_amd_gh() that seems to be related to
> the same thing and similarly worded. That seems to refer to TLB instead
> of TBL(table).
> 
> /*
>  * Disable GART TLB Walk Errors on Fam10h. We do this here because this
>  * is always needed when GART is enabled, even in a kernel which has no
>  * MCE support built in. BIOS should disable GartTlbWlk Errors already.
>  * If it doesn't, we do it here as suggested by the BKDG.
>  *
>  * Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=33012
>  */
> msr_set_bit(MSR_AMD64_MCx_MASK(4), 10);
> 
>

Now I think *that* is a typo, since it doesn't match the documentation.
:)

Thanks,
Yazen

RE: [PATCH v4 8/8] x86/mce: Fix typos

Posted by Zhuo, Qiuxu 1 week, 2 days ago

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> [...]
> > >> -		 * disable GART TBL walk error reporting, which
> > >> +		 * disable GART TLB walk error reporting, which
> > >
> > > This also is not a typo. TBL -> table
> > >
> > > From old AMD K8 BKDG document:
> > > 	10 GartTblWkEn GART Table Walk Error Reporting Enable R/W 0
> > >
> >
> > There is another comment in init_amd_gh() that seems to be related to
> > the same thing and similarly worded. That seems to refer to TLB
> > instead of TBL(table).
> >
> > /*
> >  * Disable GART TLB Walk Errors on Fam10h. We do this here because
> > this
> >  * is always needed when GART is enabled, even in a kernel which has
> > no
> >  * MCE support built in. BIOS should disable GartTlbWlk Errors already.
> >  * If it doesn't, we do it here as suggested by the BKDG.
> >  *
> >  * Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=33012
> >  */

Thanks Sohil for this reference. 

> > msr_set_bit(MSR_AMD64_MCx_MASK(4), 10);
> >
> >
> 
> Now I think *that* is a typo, since it doesn't match the documentation.
> :)

As Yazen has the documentation that says TBL (table) is the intended meaning. 
I'll drop this patch in next version. 

-Qiuxu

Re: [PATCH v4 8/8] x86/mce: Fix typos

Posted by Sohil Mehta 1 week, 4 days ago

On 11/10/2024 10:04 PM, Qiuxu Zhuo wrote:
> Fix typos in comments.
> 
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
> Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> ---

It might have been fine to merge this with patch 6/8. They both touch
the same file and the changes are mostly superficial. Probably consider
it if you do another revision but not needed otherwise.

>  arch/x86/kernel/cpu/mce/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 0f0c6e9d9183..6e194ccffc7c 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -1144,7 +1144,7 @@ static noinstr int mce_start(int *no_way_out)
>  	} else {
>  		/*
>  		 * Subject: Now start the scanning loop one by one in
> -		 * the original callin order.
> +		 * the original calling order.
>  		 * This way when there are any shared banks it will be
>  		 * only seen by one CPU before cleared, avoiding duplicates.
>  		 */
> @@ -1917,7 +1917,7 @@ static void apply_quirks_amd(struct cpuinfo_x86 *c)
>  	/* This should be disabled by the BIOS, but isn't always */
>  	if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
>  		/*
> -		 * disable GART TBL walk error reporting, which
> +		 * disable GART TLB walk error reporting, which
>  		 * trips off incorrectly with the IOMMU & 3ware
>  		 * & Cerberus:
>  		 */