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: <20230324070807.6342-1-rui.zhang@intel.com> <20230324070807.6342-2-rui.zhang@intel.com>
 <CAJZ5v0gze1wBEMcuEu4Aa9343rh-3=Bu+pdSYuY3stMd8QGf0A@mail.gmail.com>
 <528f7e58507df4b6137856828e371bb8913b8b59.camel@intel.com>
 <CAJZ5v0gL_7VPdcgkmuTz=afYSbGsWZi5wW9vUvyAgverjxsNdw@mail.gmail.com> <ba48eb3b3f4b82d33faa9233dc151f65d6380ed7.camel@intel.com>
In-Reply-To: <ba48eb3b3f4b82d33faa9233dc151f65d6380ed7.camel@intel.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 28 Mar 2023 19:54:53 +0200
Message-ID: <CAJZ5v0h6PX8EmPtVKbPPG+e0FZn53CGG=RvpOH-vuO0fNF6-=g@mail.gmail.com>
Subject: Re: [PATCH 2/5] thermal/core: Reset cooling state during cooling
 device unregistration
To:     "Zhang, Rui" <rui.zhang@intel.com>
Cc:     "rafael@kernel.org" <rafael@kernel.org>,
        "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
        "Wysocki, Rafael J" <rafael.j.wysocki@intel.com>,
        "daniel.lezcano@linaro.org" <daniel.lezcano@linaro.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Tue, Mar 28, 2023 at 4:46 AM Zhang, Rui <rui.zhang@intel.com> wrote:
>
> On Mon, 2023-03-27 at 17:13 +0200, Rafael J. Wysocki wrote:
> > On Mon, Mar 27, 2023 at 4:50 PM Zhang, Rui <rui.zhang@intel.com>
> > wrote:
> > > On Fri, 2023-03-24 at 14:19 +0100, Rafael J. Wysocki wrote:
> > > > On Fri, Mar 24, 2023 at 8:08 AM Zhang Rui <rui.zhang@intel.com>
> > > > wrote:
> > > > > When unregistering a cooling device, it is possible that the
> > > > > cooling
> > > > > device has been activated. And once the cooling device is
> > > > > unregistered,
> > > > > no one will deactivate it anymore.
> > > > >
> > > > > Reset cooling state during cooling device unregistration.
> > > > >
> > > > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > > > ---
> > > > > In theory, this problem that this patch fixes can be triggered
> > > > > on a
> > > > > platform with ACPI Active cooling, by
> > > > > 1. overheat the system to trigger ACPI active cooling
> > > > > 2. unload ACPI fan driver
> > > > > 3. check if the fan is still spinning
> > > > > But I don't have such a system so I didn't trigger then problem
> > > > > and
> > > > > I
> > > > > only did build & boot test.
> > > >
> > > > So I'm not sure if this change is actually safe.
> > > >
> > > > In the example above, the system will still need the fan to spin
> > > > after
> > > > the ACPI fan driver is unloaded in order to cool down, won't it?
> > >
> > > Then we can argue that the ACPI fan driver should not be unloaded
> > > in
> > > this case.
> >
> > I don't think that whether or not the driver is expected to be
> > unloaded at a given time has any bearing on how it should behave when
> > actually unloaded.
> >
> > Leaving the cooling device in its current state is "safe" from the
> > thermal control perspective, but it may affect the general user
> > experience (which may include performance too) going forward, so
> > there
> > is a tradeoff.
>
> Right.
> If we don't have a third choice, then the question is simple.
> "thermal safety" vs. "user experience"?
>
> I'd vote for "thermal safety" and drop this patch series.

Works for me.

> > What do the other cooling device drivers do in general when they get
> > removed?
>
> No cooling device driver has extra handling after cdev unregistration.

However, the question regarding what to do when the driver of a
cooling device in use is being removed is a valid one.

One possible approach that comes to mind could be to defer the driver
removal until the overheat condition goes away, but anyway it would be
better to do that in the core IMV.