Mike, On Mon, Apr 04 2022 at 12:41, Mike Travis wrote: > Update to not check TSC sync state for uv5+ as it is not available. > It is assumed that TSC will always be in sync for multiple chassis and > will pass the tests for the kernel to accept it as the clocksource. > To disable this check use the kernel start options tsc=reliable > clocksource=tsc. ... > --- > v2: Update patch description to be more explanatory. after reading the above word salad, I have no urge to go back to V1 to figure out how bad that changelog was. Let me walk through it: > Update to not check TSC sync state for uv5+ as it is not available. That's not a sentence and it does not provide any information about the background and the why. See: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog for further explanation. > It is assumed that TSC will always be in sync for multiple chassis and > will pass the tests for the kernel to accept it as the clocksource. "It is assumed" is a real convincing technical argument - NOT! Either the hardware guarantees something or not. If the hardware cannot guarantee it but does not provide a mechanism to verify it then spell it out: "UV5 gave up on providing an interface which allows to query the TSC synchronization state. The hardware/firmware specification defines that TSC is synchronized accross multiple chassis, but there is neither a guarantee nor a validation mechanism. This leaves the kernel with the only option to assume that TSC is synchronized and TSC_ADJUST might be different between sockets due to UV5+ firmware synchronization." > To disable this check use the kernel start options tsc=reliable > clocksource=tsc. So now it get's really confusing. Which check? The one in your explanatory description above: "Update to not check TSC sync state for uv5+ as it is not available." or what? I can assume what are you trying to tell me here, but that's not a qualification for 'explanatory' > Signed-off-by: Mike Travis <mike.travis@xxxxxxx> > Reviewed-by: Dimitri Sivanich <dimitri.sivanich@xxxxxxx> > Reviewed-by: Steve Wahl <steve.wahl@xxxxxxx> Impressive list of Reviewed-by tags. Just for clarification: Reviewed-by tags include the changelog part. > --- > arch/x86/kernel/apic/x2apic_uv_x.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c > index f5a48e66e4f5..387d6533549a 100644 > --- a/arch/x86/kernel/apic/x2apic_uv_x.c > +++ b/arch/x86/kernel/apic/x2apic_uv_x.c > @@ -199,10 +199,16 @@ static void __init uv_tsc_check_sync(void) > int mmr_shift; > char *state; > > - /* Different returns from different UV BIOS versions */ > + /* UV5+, sync state from bios not available, assumed valid */ > + if (!is_uv(UV2|UV3|UV4)) { > + pr_debug("UV: TSC sync state for UV5+ assumed valid\n"); UV advertises its version all over the place and the call here: > + mark_tsc_async_resets("UV5+"); emits the reason at info level. So you surely need the pr_debug() above to figure out that your code works as expected. You just failed to add the obviuos counterpart: pr_debug("After calling hello_world()!\n"); Seriously. Neither the changelog nor the comment above the condition qualify for explanatory or useful. Please try again. > + > + /* UV2,3,4, UV BIOS TSC sync state available */ > mmr = uv_early_read_mmr(UVH_TSC_SYNC_MMR); > - mmr_shift = > - is_uv2_hub() ? UVH_TSC_SYNC_SHIFT_UV2K : UVH_TSC_SYNC_SHIFT; > + mmr_shift = is_uv2_hub() ? UVH_TSC_SYNC_SHIFT_UV2K : UVH_TSC_SYNC_SHIFT; How is this change related to the problem which this patch tries to solve? Documentation/process/* applies to UV too. You definitely should know that by now. Thanks, tglx
