Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <5633735.DvuYhMxLoT@kreacher> <Y0gOI9hiP9NXsuJP@chenyu5-mobl1>
In-Reply-To: <Y0gOI9hiP9NXsuJP@chenyu5-mobl1>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Thu, 13 Oct 2022 15:27:30 +0200
Message-ID: <CAJZ5v0jyYB7PVQm-q3UQAaCu15LbMpfZ+8bCckZWS93w3y2XRA@mail.gmail.com>
Subject: Re: [PATCH] thermal: intel_powerclamp: Use first online CPU as control_cpu
To:     Chen Yu <yu.c.chen@intel.com>
Cc:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Linux PM <linux-pm@vger.kernel.org>,
        Bjorn Helgaas <helgaas@kernel.org>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Jacob Pan <jacob.jun.pan@linux.intel.com>,
        Pavel Machek <pavel@ucw.cz>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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);
> >
> >
> >