Commit 9c77dcf6 authored by Sebastian Andrzej Siewior's avatar Sebastian Andrzej Siewior Committed by Alex Deucher

drm/amd/display: Remove migrate_en/dis from dc_fpu_begin().

This is a revert of the commit mentioned below while it is not wrong, as
in the kernel will explode, having migrate_disable() here it is
complete waste of resources.

Additionally commit message is plain wrong the review tag does not make
it any better. The migrate_disable() interface has a fat comment
describing it and it includes the word "undesired" in the headline which
should tickle people to read it before using it.
Initially I assumed it is worded too harsh but now I beg to differ.

The reviewer of the original commit, even not understanding what
migrate_disable() does should ask the following:

- migrate_disable() is added only to the CONFIG_X86 block and it claims
  to protect fpu_recursion_depth. Why are the other the architectures
  excluded?

- migrate_disable() is added after fpu_recursion_depth was modified.
  Shouldn't it be added before the modification or referencing takes
  place?

Moving on.
Disabling preemption DOES prevent CPU migration. A task, that can not be
pushed away from the CPU by the scheduler (due to disabled preemption)
can not be pushed or migrated to another CPU.

Disabling migration DOES NOT ensure consistency of per-CPU variables. It
only ensures that the task acts always on the same per-CPU variable. The
task remains preemptible meaning multiple tasks can access the same
per-CPU variable. This in turn leads to inconsistency for the statement

                  *pcpu -= 1;

with two tasks on one CPU and a preemption point during the RMW
operation:

     Task A                           Task B
     read pcpu to reg  # 0
     inc reg           # 0 -> 1
                                      read pcpu to reg  # 0
                                      inc reg           # 0 -> 1
                                      write reg to pcpu # 1
     write reg to pcpu # 1

At the end pcpu reads 1 but should read 2 instead. Boom.

get_cpu_ptr() already contains a preempt_disable() statement. That means
that the per-CPU variable can only be referenced by a single task which
is currently running. The only inconsistency that can occur if the
variable is additionally accessed from an interrupt.

Remove migrate_disable/enable() from dc_fpu_begin/end().

Cc: Tianci Yin <tianci.yin@amd.com>
Cc: Aurabindo Pillai <aurabindo.pillai@amd.com>
Fixes: 0c316556 ("drm/amd/display: Disable migration to ensure consistency of per-CPU variable")
Acked-by: default avatarHarry Wentland <harry.wentland@amd.com>
Reviewed-by: default avatarRodrigo Siqueira <Rodrigo.Siqueira@amd.com>
Signed-off-by: default avatarSebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: default avatarHamza Mahfooz <hamza.mahfooz@amd.com>
Signed-off-by: default avatarAlex Deucher <alexander.deucher@amd.com>
parent 0029e4d4
...@@ -91,7 +91,6 @@ void dc_fpu_begin(const char *function_name, const int line) ...@@ -91,7 +91,6 @@ void dc_fpu_begin(const char *function_name, const int line)
if (*pcpu == 1) { if (*pcpu == 1) {
#if defined(CONFIG_X86) || defined(CONFIG_LOONGARCH) #if defined(CONFIG_X86) || defined(CONFIG_LOONGARCH)
migrate_disable();
kernel_fpu_begin(); kernel_fpu_begin();
#elif defined(CONFIG_PPC64) #elif defined(CONFIG_PPC64)
if (cpu_has_feature(CPU_FTR_VSX_COMP)) { if (cpu_has_feature(CPU_FTR_VSX_COMP)) {
...@@ -132,7 +131,6 @@ void dc_fpu_end(const char *function_name, const int line) ...@@ -132,7 +131,6 @@ void dc_fpu_end(const char *function_name, const int line)
if (*pcpu <= 0) { if (*pcpu <= 0) {
#if defined(CONFIG_X86) || defined(CONFIG_LOONGARCH) #if defined(CONFIG_X86) || defined(CONFIG_LOONGARCH)
kernel_fpu_end(); kernel_fpu_end();
migrate_enable();
#elif defined(CONFIG_PPC64) #elif defined(CONFIG_PPC64)
if (cpu_has_feature(CPU_FTR_VSX_COMP)) { if (cpu_has_feature(CPU_FTR_VSX_COMP)) {
disable_kernel_vsx(); disable_kernel_vsx();
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment