[PATCH] Fix set apic mode from x2apic enabled bit patch

[PATCH] Fix set apic mode from x2apic enabled bit patch

[Mike Travis]

Do not set uv_system_type for hubless UV systems as it tricks the
is_uv_system function into thinking it's a UV hubbed system and includes
a UV HUB RTC.  This causes UV RTC init to panic on UV hubless systems.

Fixes: 41e2da9b5e67 ("x86/platform/uv: Use x2apic enabled bit as set by BIOS to indicate APIC mode")

[41e2da9b5e67 was accepted into tip x86/platform branch but not yet
pulled into the linus tree.]

Signed-off-by: Mike Travis <mike.travis@hpe.com>
Reviewed-by: Steve Wahl <steve.wahl@hpe.com>
Reviewed-by: Dimitri Sivanich <dimitri.sivanich@hpe.com>
---
 arch/x86/kernel/apic/x2apic_uv_x.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
index 2e99605f9a05..68ef9abc91f7 100644
--- a/arch/x86/kernel/apic/x2apic_uv_x.c
+++ b/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -413,9 +413,8 @@ static int __init uv_set_system_type(char *_oem_id, char *_oem_table_id)
 		else
 			uv_hubless_system |= 0x8;
 
-		/* Copy OEM Table ID and set APIC Mode */
+		/* Copy OEM Table ID */
 		uv_stringify(sizeof(oem_table_id), oem_table_id, _oem_table_id);
-		early_set_apic_mode();
 
 		pr_info("UV: OEM IDs %s/%s, SystemType %d, HUBLESS ID %x\n",
 			oem_id, oem_table_id, uv_system_type, uv_hubless_system);
-- 
2.21.0

Re: [PATCH] Fix set apic mode from x2apic enabled bit patch

[Thomas Gleixner]

Mike!

On Thu, Apr 15 2021 at 17:06, Mike Travis wrote:

I'm slowly getting tired of the fact that every patch coming from you
fails to comply with the minimal requirements of the documented
procedures.

$subject: [PATCH] Fix set apic mode from x2apic enabled bit patch

Documentation clearly states, that there has to be a subsystem
prefix. It's not rocket science to figure it out:

# git log arch/x86/kernel/apic/x2apic_uv_x.c

gives you a pretty good hint that the prefix should be:

 x86/platform/uv:

It's not that hard and it's not optional.

Also what the heck means:

     Fix set apic mode from x2apic enabled bit patch

Again documentation is very clear about what the subject line, aka git
shortlog of a patch should look like.

This sentence is just word salad and does not give any clue of what this
is about. Even you wont be able to decode this 3 month from now. Simply
because it's word salad.

I don't care what kind of standards you have @hpe, but I very much care
about the standards of the kernel.

> Do not set uv_system_type for hubless UV systems as it tricks the
> is_uv_system function into thinking it's a UV hubbed system and includes
> a UV HUB RTC.  This causes UV RTC init to panic on UV hubless systems.

And yet more word salad. Can you please describe the precise technical
problem and not use metaphors of functions which are thinking?

Aside of that this does not describe the change at all. The change is:

> -		early_set_apic_mode();

but your changelog blurbs about "thinking it's an UV hubbed system".

How the heck can this make sense for someone who is not part of the @hpe
universe mindset?

> Fixes: 41e2da9b5e67 ("x86/platform/uv: Use x2apic enabled bit as set by BIOS to indicate APIC mode")
>
> [41e2da9b5e67 was accepted into tip x86/platform branch but not yet
> pulled into the linus tree.]

Truly useful. The only value of that information is that the maintainer
has to manualy remove it because it's irrelevant for the final commit
message of the change which ends up on top of that commit in that
branch. You can stick this into the section below '---' if you think
it's helpful, but then maintainers and tools can just ignore it.

TBH, it's not helpful at all because tooling already tells us that
41e2da9b5e67 is not in Linus tree and only queued in tip x86/platform.

Commit SHAs are supposed to be unique for a reason...

> Signed-off-by: Mike Travis <mike.travis@hpe.com>
> Reviewed-by: Steve Wahl <steve.wahl@hpe.com>
> Reviewed-by: Dimitri Sivanich <dimitri.sivanich@hpe.com>

The value of these reviewed-by tags is close to zero because they are
just documenting that the change is ok at the @hpe universe level...

> ---
>  arch/x86/kernel/apic/x2apic_uv_x.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
> index 2e99605f9a05..68ef9abc91f7 100644
> --- a/arch/x86/kernel/apic/x2apic_uv_x.c
> +++ b/arch/x86/kernel/apic/x2apic_uv_x.c
> @@ -413,9 +413,8 @@ static int __init uv_set_system_type(char *_oem_id, char *_oem_table_id)
>  		else
>  			uv_hubless_system |= 0x8;
>  
> -		/* Copy OEM Table ID and set APIC Mode */
> +		/* Copy OEM Table ID */
>  		uv_stringify(sizeof(oem_table_id), oem_table_id, _oem_table_id);
> -		early_set_apic_mode();

So the patch itself tells me more about what's going on than the
changelog:

  Setting up the APIC mode at this place is wrong.

But it does not tell me WHY. Neither does the changelog because of
useless word salad...

If you can't come up with something sensible anytime soon before the
merge window opens then I'm simply going to revert 41e2da9b5e67 and you
can try again for the next cycle.

Thanks,

        tglx

Re: [PATCH] Fix set apic mode from x2apic enabled bit patch

[Thomas Gleixner]

Mike!

On Sun, Apr 18 2021 at 00:39, Thomas Gleixner wrote:
> If you can't come up with something sensible anytime soon before the
> merge window opens then I'm simply going to revert 41e2da9b5e67 and you
> can try again for the next cycle.

so I just figured out that Boris wasted his time once more to fix that
up and redo the commit in question.

Won't happen again.

Thanks,

        tglx

Re: [PATCH] Fix set apic mode from x2apic enabled bit patch

[Mike Travis]

Hi Thomas,

Thanks for pointing that out, though Boris did highlight the same 
problem.  I still do all the patches in quilt but I've created an 
automated conversion to take those patch(es) and use git send mail to 
send them upstream.  The platform information used to be included but 
it's not now and I hadn't noticed that.  I will look at the tool 
particularly the git format patch step.

Thanks again for accepting the patch as is.  And I will be more 
descriptive in the patch and in code comments.  I do get into the 
mindset that no one else really cares about how UV works but it sounds 
like I'm mistaken, at least in regards to how it affects the kernel.

Thanks,
Mike

On 4/17/2021 4:10 PM, Thomas Gleixner wrote:
> Mike!
> 
> On Sun, Apr 18 2021 at 00:39, Thomas Gleixner wrote:
>> If you can't come up with something sensible anytime soon before the
>> merge window opens then I'm simply going to revert 41e2da9b5e67 and you
>> can try again for the next cycle.
> 
> so I just figured out that Boris wasted his time once more to fix that
> up and redo the commit in question.
> 
> Won't happen again.
> 
> Thanks,
> 
>          tglx
>