diff mbox series

thermal: intel_powerclamp: Use first online CPU as control_cpu

Message ID 5633735.DvuYhMxLoT@kreacher
State New
Headers
Series thermal: intel_powerclamp: Use first online CPU as control_cpu |

Commit Message

Rafael J. Wysocki Oct. 13, 2022, 12:50 p.m. UTC 
  From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Commit 68b99e94a4a2 ("thermal: intel_powerclamp: Use get_cpu() instead
of smp_processor_id() to avoid crash") fixed an issue related to using
smp_processor_id() in preemptible context by replacing it with a pair
of get_cpu()/put_cpu(), but what is needed there really is any online
CPU and not necessarily the one currently running the code.  Arguably,
getting the one that's running the code in there is confusing.

For this reason, simply give the control CPU role to the first online
one which automatically will be CPU0 if it is online, so one check
can be dropped from the code for an added benefit.

Link: https://lore.kernel.org/linux-pm/20221011113646.GA12080@duo.ucw.cz/
Fixes: 68b99e94a4a2 ("thermal: intel_powerclamp: Use get_cpu() instead of smp_processor_id() to avoid crash")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/intel/intel_powerclamp.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Comments

Chen Yu Oct. 13, 2022, 1:09 p.m. UTC | #1 
On 2022-10-13 at 14:50:28 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Commit 68b99e94a4a2 ("thermal: intel_powerclamp: Use get_cpu() instead
> of smp_processor_id() to avoid crash") fixed an issue related to using
> smp_processor_id() in preemptible context by replacing it with a pair
> of get_cpu()/put_cpu(), but what is needed there really is any online
> CPU and not necessarily the one currently running the code.  Arguably,
> getting the one that's running the code in there is confusing.
> 
> For this reason, simply give the control CPU role to the first online
> one which automatically will be CPU0 if it is online, so one check
> can be dropped from the code for an added benefit.
> 
> Link: https://lore.kernel.org/linux-pm/20221011113646.GA12080@duo.ucw.cz/
> Fixes: 68b99e94a4a2 ("thermal: intel_powerclamp: Use get_cpu() instead of smp_processor_id() to avoid crash")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/thermal/intel/intel_powerclamp.c |    6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> Index: linux-pm/drivers/thermal/intel/intel_powerclamp.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/intel/intel_powerclamp.c
> +++ linux-pm/drivers/thermal/intel/intel_powerclamp.c
> @@ -516,11 +516,7 @@ static int start_power_clamp(void)
>  	cpus_read_lock();
>  
>  	/* prefer BSP */
Above comment line is not true any more, might delete it as well?

Reviewed-by: Chen Yu <yu.c.chen@intel.com>

thanks,
Chenyu
> -	control_cpu = 0;
> -	if (!cpu_online(control_cpu)) {
> -		control_cpu = get_cpu();
> -		put_cpu();
> -	}
> +	control_cpu = cpumask_first(cpu_online_mask);
>  
>  	clamping = true;
>  	schedule_delayed_work(&poll_pkg_cstate_work, 0);
> 
> 
>
Rafael J. Wysocki Oct. 13, 2022, 1:27 p.m. UTC | #2 
On Thu, Oct 13, 2022 at 3:10 PM Chen Yu <yu.c.chen@intel.com> wrote:
>
> On 2022-10-13 at 14:50:28 +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Commit 68b99e94a4a2 ("thermal: intel_powerclamp: Use get_cpu() instead
> > of smp_processor_id() to avoid crash") fixed an issue related to using
> > smp_processor_id() in preemptible context by replacing it with a pair
> > of get_cpu()/put_cpu(), but what is needed there really is any online
> > CPU and not necessarily the one currently running the code.  Arguably,
> > getting the one that's running the code in there is confusing.
> >
> > For this reason, simply give the control CPU role to the first online
> > one which automatically will be CPU0 if it is online, so one check
> > can be dropped from the code for an added benefit.
> >
> > Link: https://lore.kernel.org/linux-pm/20221011113646.GA12080@duo.ucw.cz/
> > Fixes: 68b99e94a4a2 ("thermal: intel_powerclamp: Use get_cpu() instead of smp_processor_id() to avoid crash")
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/thermal/intel/intel_powerclamp.c |    6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > Index: linux-pm/drivers/thermal/intel/intel_powerclamp.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/intel/intel_powerclamp.c
> > +++ linux-pm/drivers/thermal/intel/intel_powerclamp.c
> > @@ -516,11 +516,7 @@ static int start_power_clamp(void)
> >       cpus_read_lock();
> >
> >       /* prefer BSP */
> Above comment line is not true any more, might delete it as well?

Well, why not?  If CPU0 is the BSP, it is still preferred as before.

> Reviewed-by: Chen Yu <yu.c.chen@intel.com>

Thanks!

> > -     control_cpu = 0;
> > -     if (!cpu_online(control_cpu)) {
> > -             control_cpu = get_cpu();
> > -             put_cpu();
> > -     }
> > +     control_cpu = cpumask_first(cpu_online_mask);
> >
> >       clamping = true;
> >       schedule_delayed_work(&poll_pkg_cstate_work, 0);
> >
> >
> >
Chen Yu Oct. 13, 2022, 1:50 p.m. UTC | #3 
On 2022-10-13 at 15:27:30 +0200, Rafael J. Wysocki wrote:
> On Thu, Oct 13, 2022 at 3:10 PM Chen Yu <yu.c.chen@intel.com> wrote:
> >
> > On 2022-10-13 at 14:50:28 +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > Commit 68b99e94a4a2 ("thermal: intel_powerclamp: Use get_cpu() instead
> > > of smp_processor_id() to avoid crash") fixed an issue related to using
> > > smp_processor_id() in preemptible context by replacing it with a pair
> > > of get_cpu()/put_cpu(), but what is needed there really is any online
> > > CPU and not necessarily the one currently running the code.  Arguably,
> > > getting the one that's running the code in there is confusing.
> > >
> > > For this reason, simply give the control CPU role to the first online
> > > one which automatically will be CPU0 if it is online, so one check
> > > can be dropped from the code for an added benefit.
> > >
> > > Link: https://lore.kernel.org/linux-pm/20221011113646.GA12080@duo.ucw.cz/
> > > Fixes: 68b99e94a4a2 ("thermal: intel_powerclamp: Use get_cpu() instead of smp_processor_id() to avoid crash")
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  drivers/thermal/intel/intel_powerclamp.c |    6 +-----
> > >  1 file changed, 1 insertion(+), 5 deletions(-)
> > >
> > > Index: linux-pm/drivers/thermal/intel/intel_powerclamp.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/thermal/intel/intel_powerclamp.c
> > > +++ linux-pm/drivers/thermal/intel/intel_powerclamp.c
> > > @@ -516,11 +516,7 @@ static int start_power_clamp(void)
> > >       cpus_read_lock();
> > >
> > >       /* prefer BSP */
> > Above comment line is not true any more, might delete it as well?
> 
> Well, why not?  If CPU0 is the BSP, it is still preferred as before.
>
I see. Got it.

thanks,
Chenyu 
> > Reviewed-by: Chen Yu <yu.c.chen@intel.com>
> 
> Thanks!
> 
> > > -     control_cpu = 0;
> > > -     if (!cpu_online(control_cpu)) {
> > > -             control_cpu = get_cpu();
> > > -             put_cpu();
> > > -     }
> > > +     control_cpu = cpumask_first(cpu_online_mask);
> > >
> > >       clamping = true;
> > >       schedule_delayed_work(&poll_pkg_cstate_work, 0);
> > >
> > >
> > >
diff mbox series

Patch

Index: linux-pm/drivers/thermal/intel/intel_powerclamp.c
===================================================================
--- linux-pm.orig/drivers/thermal/intel/intel_powerclamp.c
+++ linux-pm/drivers/thermal/intel/intel_powerclamp.c
@@ -516,11 +516,7 @@  static int start_power_clamp(void)
 	cpus_read_lock();
 
 	/* prefer BSP */
-	control_cpu = 0;
-	if (!cpu_online(control_cpu)) {
-		control_cpu = get_cpu();
-		put_cpu();
-	}
+	control_cpu = cpumask_first(cpu_online_mask);
 
 	clamping = true;
 	schedule_delayed_work(&poll_pkg_cstate_work, 0);