Make acpi_processor_idle use the common broadcast code, there's no
reason not to. This also removes some RCU usage after
rcu_idle_enter().
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
drivers/acpi/processor_idle.c | 49 +++++++++++++-----------------------------
1 file changed, 16 insertions(+), 33 deletions(-)
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -161,18 +161,10 @@ static void lapic_timer_propagate_broadc
}
/* Power(C) State timer broadcast control */
-static void lapic_timer_state_broadcast(struct acpi_processor *pr,
- struct acpi_processor_cx *cx,
- int broadcast)
-{
- int state = cx - pr->power.states;
-
- if (state >= pr->power.timer_broadcast_on_state) {
- if (broadcast)
- tick_broadcast_enter();
- else
- tick_broadcast_exit();
- }
+static bool lapic_timer_needs_broadcast(struct acpi_processor *pr,
+ struct acpi_processor_cx *cx)
+{
+ return cx - pr->power.states >= pr->power.timer_broadcast_on_state;
}
#else
@@ -180,9 +172,9 @@ static void lapic_timer_state_broadcast(
static void lapic_timer_check_state(int state, struct acpi_processor *pr,
struct acpi_processor_cx *cstate) { }
static void lapic_timer_propagate_broadcast(struct acpi_processor *pr) { }
-static void lapic_timer_state_broadcast(struct acpi_processor *pr,
- struct acpi_processor_cx *cx,
- int broadcast)
+
+static bool lapic_timer_needs_broadcast(struct acpi_processor *pr,
+ struct acpi_processor_cx *cx)
{
}
@@ -568,21 +560,13 @@ static DEFINE_RAW_SPINLOCK(c3_lock);
* acpi_idle_enter_bm - enters C3 with proper BM handling
* @pr: Target processor
* @cx: Target state context
- * @timer_bc: Whether or not to change timer mode to broadcast
*/
static void acpi_idle_enter_bm(struct acpi_processor *pr,
- struct acpi_processor_cx *cx, bool timer_bc)
+ struct acpi_processor_cx *cx)
{
acpi_unlazy_tlb(smp_processor_id());
/*
- * Must be done before busmaster disable as we might need to
- * access HPET !
- */
- if (timer_bc)
- lapic_timer_state_broadcast(pr, cx, 1);
-
- /*
* disable bus master
* bm_check implies we need ARB_DIS
* bm_control implies whether we can do ARB_DIS
@@ -609,9 +593,6 @@ static void acpi_idle_enter_bm(struct ac
c3_cpu_count--;
raw_spin_unlock(&c3_lock);
}
-
- if (timer_bc)
- lapic_timer_state_broadcast(pr, cx, 0);
}
static int acpi_idle_enter(struct cpuidle_device *dev,
@@ -630,7 +611,7 @@ static int acpi_idle_enter(struct cpuidl
cx = per_cpu(acpi_cstate[index], dev->cpu);
} else if (cx->type == ACPI_STATE_C3 && pr->flags.bm_check) {
if (cx->bm_sts_skip || !acpi_idle_bm_check()) {
- acpi_idle_enter_bm(pr, cx, true);
+ acpi_idle_enter_bm(pr, cx);
return index;
} else if (drv->safe_state_index >= 0) {
index = drv->safe_state_index;
@@ -642,15 +623,11 @@ static int acpi_idle_enter(struct cpuidl
}
}
- lapic_timer_state_broadcast(pr, cx, 1);
-
if (cx->type == ACPI_STATE_C3)
ACPI_FLUSH_CPU_CACHE();
acpi_idle_do_entry(cx);
- lapic_timer_state_broadcast(pr, cx, 0);
-
return index;
}
@@ -666,7 +643,7 @@ static int acpi_idle_enter_s2idle(struct
return 0;
if (pr->flags.bm_check) {
- acpi_idle_enter_bm(pr, cx, false);
+ acpi_idle_enter_bm(pr, cx);
return 0;
} else {
ACPI_FLUSH_CPU_CACHE();
@@ -682,6 +659,7 @@ static int acpi_processor_setup_cpuidle_
{
int i, count = ACPI_IDLE_STATE_START;
struct acpi_processor_cx *cx;
+ struct cpuidle_state *state;
if (max_cstate == 0)
max_cstate = 1;
@@ -694,6 +672,11 @@ static int acpi_processor_setup_cpuidle_
per_cpu(acpi_cstate[count], dev->cpu) = cx;
+ if (lapic_timer_needs_broadcast(pr, cx)) {
+ state = &acpi_idle_driver.states[count];
+ state->flags |= CPUIDLE_FLAG_TIMER_STOP;
+ }
+
count++;
if (count == CPUIDLE_STATE_MAX)
break;
On Tue, Sep 15, 2020 at 12:44 PM Peter Zijlstra <[email protected]> wrote:
>
> Make acpi_processor_idle use the common broadcast code, there's no
> reason not to. This also removes some RCU usage after
> rcu_idle_enter().
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
The whole series looks good to me, so please feel free to add
Acked-by: Rafael J. Wysocki <[email protected]>
to all of the four patches.
Alternatively, please let me know if you want me to take the patches.
> ---
> drivers/acpi/processor_idle.c | 49 +++++++++++++-----------------------------
> 1 file changed, 16 insertions(+), 33 deletions(-)
>
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -161,18 +161,10 @@ static void lapic_timer_propagate_broadc
> }
>
> /* Power(C) State timer broadcast control */
> -static void lapic_timer_state_broadcast(struct acpi_processor *pr,
> - struct acpi_processor_cx *cx,
> - int broadcast)
> -{
> - int state = cx - pr->power.states;
> -
> - if (state >= pr->power.timer_broadcast_on_state) {
> - if (broadcast)
> - tick_broadcast_enter();
> - else
> - tick_broadcast_exit();
> - }
> +static bool lapic_timer_needs_broadcast(struct acpi_processor *pr,
> + struct acpi_processor_cx *cx)
> +{
> + return cx - pr->power.states >= pr->power.timer_broadcast_on_state;
> }
>
> #else
> @@ -180,9 +172,9 @@ static void lapic_timer_state_broadcast(
> static void lapic_timer_check_state(int state, struct acpi_processor *pr,
> struct acpi_processor_cx *cstate) { }
> static void lapic_timer_propagate_broadcast(struct acpi_processor *pr) { }
> -static void lapic_timer_state_broadcast(struct acpi_processor *pr,
> - struct acpi_processor_cx *cx,
> - int broadcast)
> +
> +static bool lapic_timer_needs_broadcast(struct acpi_processor *pr,
> + struct acpi_processor_cx *cx)
> {
> }
>
> @@ -568,21 +560,13 @@ static DEFINE_RAW_SPINLOCK(c3_lock);
> * acpi_idle_enter_bm - enters C3 with proper BM handling
> * @pr: Target processor
> * @cx: Target state context
> - * @timer_bc: Whether or not to change timer mode to broadcast
> */
> static void acpi_idle_enter_bm(struct acpi_processor *pr,
> - struct acpi_processor_cx *cx, bool timer_bc)
> + struct acpi_processor_cx *cx)
> {
> acpi_unlazy_tlb(smp_processor_id());
>
> /*
> - * Must be done before busmaster disable as we might need to
> - * access HPET !
> - */
> - if (timer_bc)
> - lapic_timer_state_broadcast(pr, cx, 1);
> -
> - /*
> * disable bus master
> * bm_check implies we need ARB_DIS
> * bm_control implies whether we can do ARB_DIS
> @@ -609,9 +593,6 @@ static void acpi_idle_enter_bm(struct ac
> c3_cpu_count--;
> raw_spin_unlock(&c3_lock);
> }
> -
> - if (timer_bc)
> - lapic_timer_state_broadcast(pr, cx, 0);
> }
>
> static int acpi_idle_enter(struct cpuidle_device *dev,
> @@ -630,7 +611,7 @@ static int acpi_idle_enter(struct cpuidl
> cx = per_cpu(acpi_cstate[index], dev->cpu);
> } else if (cx->type == ACPI_STATE_C3 && pr->flags.bm_check) {
> if (cx->bm_sts_skip || !acpi_idle_bm_check()) {
> - acpi_idle_enter_bm(pr, cx, true);
> + acpi_idle_enter_bm(pr, cx);
> return index;
> } else if (drv->safe_state_index >= 0) {
> index = drv->safe_state_index;
> @@ -642,15 +623,11 @@ static int acpi_idle_enter(struct cpuidl
> }
> }
>
> - lapic_timer_state_broadcast(pr, cx, 1);
> -
> if (cx->type == ACPI_STATE_C3)
> ACPI_FLUSH_CPU_CACHE();
>
> acpi_idle_do_entry(cx);
>
> - lapic_timer_state_broadcast(pr, cx, 0);
> -
> return index;
> }
>
> @@ -666,7 +643,7 @@ static int acpi_idle_enter_s2idle(struct
> return 0;
>
> if (pr->flags.bm_check) {
> - acpi_idle_enter_bm(pr, cx, false);
> + acpi_idle_enter_bm(pr, cx);
> return 0;
> } else {
> ACPI_FLUSH_CPU_CACHE();
> @@ -682,6 +659,7 @@ static int acpi_processor_setup_cpuidle_
> {
> int i, count = ACPI_IDLE_STATE_START;
> struct acpi_processor_cx *cx;
> + struct cpuidle_state *state;
>
> if (max_cstate == 0)
> max_cstate = 1;
> @@ -694,6 +672,11 @@ static int acpi_processor_setup_cpuidle_
>
> per_cpu(acpi_cstate[count], dev->cpu) = cx;
>
> + if (lapic_timer_needs_broadcast(pr, cx)) {
> + state = &acpi_idle_driver.states[count];
> + state->flags |= CPUIDLE_FLAG_TIMER_STOP;
> + }
> +
> count++;
> if (count == CPUIDLE_STATE_MAX)
> break;
>
>
On Wed, Sep 16, 2020 at 6:01 PM Borislav Petkov <[email protected]> wrote:
>
> On Wed, Sep 16, 2020 at 05:42:12PM +0200, [email protected] wrote:
> > On Tue, Sep 15, 2020 at 06:26:52PM +0200, Rafael J. Wysocki wrote:
> > > On Tue, Sep 15, 2020 at 12:44 PM Peter Zijlstra <[email protected]> wrote:
> > > >
> > > > Make acpi_processor_idle use the common broadcast code, there's no
> > > > reason not to. This also removes some RCU usage after
> > > > rcu_idle_enter().
> > > >
> > > > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > >
> > > The whole series looks good to me, so please feel free to add
> > >
> > > Acked-by: Rafael J. Wysocki <[email protected]>
> > >
> > > to all of the four patches.
> > >
> > > Alternatively, please let me know if you want me to take the patches.
> >
> > Feel free to take them. All the prerequisite borkage is in linus' tree
> > already.
>
> You can add:
>
> Reported-by: Borislav Petkov <[email protected]>
>
> for this one and for this whole series:
>
> Tested-by: Borislav Petkov <[email protected]>
Done.
On Wed, Sep 16, 2020 at 5:42 PM <[email protected]> wrote:
>
> On Tue, Sep 15, 2020 at 06:26:52PM +0200, Rafael J. Wysocki wrote:
> > On Tue, Sep 15, 2020 at 12:44 PM Peter Zijlstra <[email protected]> wrote:
> > >
> > > Make acpi_processor_idle use the common broadcast code, there's no
> > > reason not to. This also removes some RCU usage after
> > > rcu_idle_enter().
> > >
> > > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> >
> > The whole series looks good to me, so please feel free to add
> >
> > Acked-by: Rafael J. Wysocki <[email protected]>
> >
> > to all of the four patches.
> >
> > Alternatively, please let me know if you want me to take the patches.
>
> Feel free to take them. All the prerequisite borkage is in linus' tree
> already.
OK
All applied (with some minor edits in the subjects) for 5.9-rc6.
Thanks!
On Wed, Sep 16, 2020 at 05:42:12PM +0200, [email protected] wrote:
> On Tue, Sep 15, 2020 at 06:26:52PM +0200, Rafael J. Wysocki wrote:
> > On Tue, Sep 15, 2020 at 12:44 PM Peter Zijlstra <[email protected]> wrote:
> > >
> > > Make acpi_processor_idle use the common broadcast code, there's no
> > > reason not to. This also removes some RCU usage after
> > > rcu_idle_enter().
> > >
> > > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> >
> > The whole series looks good to me, so please feel free to add
> >
> > Acked-by: Rafael J. Wysocki <[email protected]>
> >
> > to all of the four patches.
> >
> > Alternatively, please let me know if you want me to take the patches.
>
> Feel free to take them. All the prerequisite borkage is in linus' tree
> already.
You can add:
Reported-by: Borislav Petkov <[email protected]>
for this one and for this whole series:
Tested-by: Borislav Petkov <[email protected]>
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Sep 15, 2020 at 06:26:52PM +0200, Rafael J. Wysocki wrote:
> On Tue, Sep 15, 2020 at 12:44 PM Peter Zijlstra <[email protected]> wrote:
> >
> > Make acpi_processor_idle use the common broadcast code, there's no
> > reason not to. This also removes some RCU usage after
> > rcu_idle_enter().
> >
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
>
> The whole series looks good to me, so please feel free to add
>
> Acked-by: Rafael J. Wysocki <[email protected]>
>
> to all of the four patches.
>
> Alternatively, please let me know if you want me to take the patches.
Feel free to take them. All the prerequisite borkage is in linus' tree
already.
On Tue, Sep 15, 2020 at 12:31:58PM +0200, Peter Zijlstra wrote:
> Make acpi_processor_idle use the common broadcast code, there's no
> reason not to. This also removes some RCU usage after
> rcu_idle_enter().
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> Acked-by: Rafael J. Wysocki <[email protected]>
> Reported-by: Borislav Petkov <[email protected]>
> Tested-by: Borislav Petkov <[email protected]>
> ---
> drivers/acpi/processor_idle.c | 49 +++++++++++++-----------------------------
> 1 file changed, 16 insertions(+), 33 deletions(-)
>
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -161,18 +161,10 @@ static void lapic_timer_propagate_broadc
> }
>
> /* Power(C) State timer broadcast control */
> -static void lapic_timer_state_broadcast(struct acpi_processor *pr,
> - struct acpi_processor_cx *cx,
> - int broadcast)
> -{
> - int state = cx - pr->power.states;
> -
> - if (state >= pr->power.timer_broadcast_on_state) {
> - if (broadcast)
> - tick_broadcast_enter();
> - else
> - tick_broadcast_exit();
> - }
> +static bool lapic_timer_needs_broadcast(struct acpi_processor *pr,
> + struct acpi_processor_cx *cx)
> +{
> + return cx - pr->power.states >= pr->power.timer_broadcast_on_state;
> }
>
> #else
> @@ -180,9 +172,9 @@ static void lapic_timer_state_broadcast(
> static void lapic_timer_check_state(int state, struct acpi_processor *pr,
> struct acpi_processor_cx *cstate) { }
> static void lapic_timer_propagate_broadcast(struct acpi_processor *pr) { }
> -static void lapic_timer_state_broadcast(struct acpi_processor *pr,
> - struct acpi_processor_cx *cx,
> - int broadcast)
> +
> +static bool lapic_timer_needs_broadcast(struct acpi_processor *pr,
> + struct acpi_processor_cx *cx)
> {
> }
drivers/acpi/processor_idle.c: In function 'lapic_timer_needs_broadcast':
drivers/acpi/processor_idle.c:179:1: warning: no return statement in function returning non-void [-Wreturn-type]
Should this return true or false ?
Guenter
>
> @@ -568,21 +560,13 @@ static DEFINE_RAW_SPINLOCK(c3_lock);
> * acpi_idle_enter_bm - enters C3 with proper BM handling
> * @pr: Target processor
> * @cx: Target state context
> - * @timer_bc: Whether or not to change timer mode to broadcast
> */
> static void acpi_idle_enter_bm(struct acpi_processor *pr,
> - struct acpi_processor_cx *cx, bool timer_bc)
> + struct acpi_processor_cx *cx)
> {
> acpi_unlazy_tlb(smp_processor_id());
>
> /*
> - * Must be done before busmaster disable as we might need to
> - * access HPET !
> - */
> - if (timer_bc)
> - lapic_timer_state_broadcast(pr, cx, 1);
> -
> - /*
> * disable bus master
> * bm_check implies we need ARB_DIS
> * bm_control implies whether we can do ARB_DIS
> @@ -609,9 +593,6 @@ static void acpi_idle_enter_bm(struct ac
> c3_cpu_count--;
> raw_spin_unlock(&c3_lock);
> }
> -
> - if (timer_bc)
> - lapic_timer_state_broadcast(pr, cx, 0);
> }
>
> static int acpi_idle_enter(struct cpuidle_device *dev,
> @@ -630,7 +611,7 @@ static int acpi_idle_enter(struct cpuidl
> cx = per_cpu(acpi_cstate[index], dev->cpu);
> } else if (cx->type == ACPI_STATE_C3 && pr->flags.bm_check) {
> if (cx->bm_sts_skip || !acpi_idle_bm_check()) {
> - acpi_idle_enter_bm(pr, cx, true);
> + acpi_idle_enter_bm(pr, cx);
> return index;
> } else if (drv->safe_state_index >= 0) {
> index = drv->safe_state_index;
> @@ -642,15 +623,11 @@ static int acpi_idle_enter(struct cpuidl
> }
> }
>
> - lapic_timer_state_broadcast(pr, cx, 1);
> -
> if (cx->type == ACPI_STATE_C3)
> ACPI_FLUSH_CPU_CACHE();
>
> acpi_idle_do_entry(cx);
>
> - lapic_timer_state_broadcast(pr, cx, 0);
> -
> return index;
> }
>
> @@ -666,7 +643,7 @@ static int acpi_idle_enter_s2idle(struct
> return 0;
>
> if (pr->flags.bm_check) {
> - acpi_idle_enter_bm(pr, cx, false);
> + acpi_idle_enter_bm(pr, cx);
> return 0;
> } else {
> ACPI_FLUSH_CPU_CACHE();
> @@ -682,6 +659,7 @@ static int acpi_processor_setup_cpuidle_
> {
> int i, count = ACPI_IDLE_STATE_START;
> struct acpi_processor_cx *cx;
> + struct cpuidle_state *state;
>
> if (max_cstate == 0)
> max_cstate = 1;
> @@ -694,6 +672,11 @@ static int acpi_processor_setup_cpuidle_
>
> per_cpu(acpi_cstate[count], dev->cpu) = cx;
>
> + if (lapic_timer_needs_broadcast(pr, cx)) {
> + state = &acpi_idle_driver.states[count];
> + state->flags |= CPUIDLE_FLAG_TIMER_STOP;
> + }
> +
> count++;
> if (count == CPUIDLE_STATE_MAX)
> break;
On Tuesday, September 22, 2020 5:26:51 AM CEST Guenter Roeck wrote:
> On Tue, Sep 15, 2020 at 12:31:58PM +0200, Peter Zijlstra wrote:
> > Make acpi_processor_idle use the common broadcast code, there's no
> > reason not to. This also removes some RCU usage after
> > rcu_idle_enter().
> >
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > Acked-by: Rafael J. Wysocki <[email protected]>
> > Reported-by: Borislav Petkov <[email protected]>
> > Tested-by: Borislav Petkov <[email protected]>
> > ---
> > drivers/acpi/processor_idle.c | 49 +++++++++++++-----------------------------
> > 1 file changed, 16 insertions(+), 33 deletions(-)
> >
> > --- a/drivers/acpi/processor_idle.c
> > +++ b/drivers/acpi/processor_idle.c
> > @@ -161,18 +161,10 @@ static void lapic_timer_propagate_broadc
> > }
> >
> > /* Power(C) State timer broadcast control */
> > -static void lapic_timer_state_broadcast(struct acpi_processor *pr,
> > - struct acpi_processor_cx *cx,
> > - int broadcast)
> > -{
> > - int state = cx - pr->power.states;
> > -
> > - if (state >= pr->power.timer_broadcast_on_state) {
> > - if (broadcast)
> > - tick_broadcast_enter();
> > - else
> > - tick_broadcast_exit();
> > - }
> > +static bool lapic_timer_needs_broadcast(struct acpi_processor *pr,
> > + struct acpi_processor_cx *cx)
> > +{
> > + return cx - pr->power.states >= pr->power.timer_broadcast_on_state;
> > }
> >
> > #else
> > @@ -180,9 +172,9 @@ static void lapic_timer_state_broadcast(
> > static void lapic_timer_check_state(int state, struct acpi_processor *pr,
> > struct acpi_processor_cx *cstate) { }
> > static void lapic_timer_propagate_broadcast(struct acpi_processor *pr) { }
> > -static void lapic_timer_state_broadcast(struct acpi_processor *pr,
> > - struct acpi_processor_cx *cx,
> > - int broadcast)
> > +
> > +static bool lapic_timer_needs_broadcast(struct acpi_processor *pr,
> > + struct acpi_processor_cx *cx)
> > {
> > }
>
> drivers/acpi/processor_idle.c: In function 'lapic_timer_needs_broadcast':
> drivers/acpi/processor_idle.c:179:1: warning: no return statement in function returning non-void [-Wreturn-type]
>
> Should this return true or false ?
false - if the lapic timer doesn't stop, it doesn't need broadcast.
FWIW, patch appended.
Cheers!
---
From: Rafael J. Wysocki <[email protected]>
Subject: [PATCH] ACPI: processor: Fix build for ARCH_APICTIMER_STOPS_ON_C3 unset
Fix the lapic_timer_needs_broadcast() stub for
ARCH_APICTIMER_STOPS_ON_C3 unset to actually return
a value.
Fixes: aa6b43d57f99 ("ACPI: processor: Use CPUIDLE_FLAG_TIMER_STOP")
Reported-by: Guenter Roeck <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/processor_idle.c | 1 +
1 file changed, 1 insertion(+)
Index: linux-pm/drivers/acpi/processor_idle.c
===================================================================
--- linux-pm.orig/drivers/acpi/processor_idle.c
+++ linux-pm/drivers/acpi/processor_idle.c
@@ -176,6 +176,7 @@ static void lapic_timer_propagate_broadc
static bool lapic_timer_needs_broadcast(struct acpi_processor *pr,
struct acpi_processor_cx *cx)
{
+ return false;
}
#endif