Received-SPF: pass (google.com: domain of linux-kernel+bounces-204559-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <4569763.LvFx2qVVIh@kreacher> <5f93f034-f781-47e0-b8ce-3c8407a709f7@linaro.org>
 <CAJZ5v0hkOvYL66D+tCRJxbp=XqV59yeZ0dA1Kxoczkcpe5X9sA@mail.gmail.com>
 <CAJZ5v0joTyOcRU0FyaHEEYEbiPbbNSFzW3P7eJJ4MM5QdhQR2w@mail.gmail.com> <90a9df3e-153f-4972-8086-13c21a574763@linaro.org>
In-Reply-To: <90a9df3e-153f-4972-8086-13c21a574763@linaro.org>
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: Thu, 6 Jun 2024 17:15:24 +0200
Message-ID: <CAJZ5v0jurx=TfT2GHCeMt7xhTFRQwpS_H6-fDss=bt+HodvBoQ@mail.gmail.com>
Subject: Re: [PATCH v1] thermal: core: Do not fail cdev registration because
 of invalid initial state
To: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>, "Rafael J. Wysocki" <rjw@rjwysocki.net>, 
	Linux PM <linux-pm@vger.kernel.org>, Linux ACPI <linux-acpi@vger.kernel.org>, 
	LKML <linux-kernel@vger.kernel.org>, Lukasz Luba <lukasz.luba@arm.com>, 
	Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>, Zhang Rui <rui.zhang@intel.com>, 
	Laura Nao <laura.nao@collabora.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Jun 6, 2024 at 4:50=E2=80=AFPM Daniel Lezcano <daniel.lezcano@linar=
o.org> wrote:
>
> On 06/06/2024 16:18, Rafael J. Wysocki wrote:
> > On Thu, Jun 6, 2024 at 3:42=E2=80=AFPM Rafael J. Wysocki <rafael@kernel=
.org> wrote:
> >>
> >> On Thu, Jun 6, 2024 at 3:07=E2=80=AFPM Daniel Lezcano <daniel.lezcano@=
linaro.org> wrote:
> >>>
> >>> On 05/06/2024 21:17, Rafael J. Wysocki wrote:
> >>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>>
> >>>> It is reported that commit 31a0fa0019b0 ("thermal/debugfs: Pass cool=
ing
> >>>> device state to thermal_debug_cdev_add()") causes the ACPI fan drive=
r
> >>>> to fail probing on some systems which turns out to be due to the _FS=
T
> >>>> control method returning an invalid value until _FSL is first evalua=
ted
> >>>> for the given fan.  If this happens, the .get_cur_state() cooling de=
vice
> >>>> callback returns an error and __thermal_cooling_device_register() fa=
ils
> >>>> as uses that callback after commit 31a0fa0019b0.
> >>>>
> >>>> Arguably, _FST should not return an inavlid value even if it is
> >>>> evaluated before _FSL, so this may be regarded as a platform firmwar=
e
> >>>> issue, but at the same time it is not a good enough reason for faili=
ng
> >>>> the cooling device registration where the initial cooling device sta=
te
> >>>> is only needed to initialize a thermal debug facility.
> >>>>
> >>>> Accordingly, modify __thermal_cooling_device_register() to pass a
> >>>> negative state value to thermal_debug_cdev_add() instead of failing
> >>>> if the initial .get_cur_state() callback invocation fails and adjust
> >>>> the thermal debug code to ignore negative cooling device state value=
s.
> >>>>
> >>>> Fixes: 31a0fa0019b0 ("thermal/debugfs: Pass cooling device state to =
thermal_debug_cdev_add()")
> >>>> Closes: https://lore.kernel.org/linux-acpi/20240530153727.843378-1-l=
aura.nao@collabora.com
> >>>> Reported-by: Laura Nao <laura.nao@collabora.com>
> >>>> Tested-by: Laura Nao <laura.nao@collabora.com>
> >>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>
> >>> As it is a driver issue, it should be fixed in the driver, not in the
> >>> core code. The resulting code logic in the core is trying to deal wit=
h
> >>> bad driver behavior, it does not really seem appropriate.
> >
> > Besides, I don't quite agree with dismissing it as a driver issue.  If
> > a driver cannot determine the cooling device state, it should not be
> > required to make it up.
> >
> > Because .get_cur_state() is specifically designed to be able to return
> > an error, the core should be prepared to deal with errors returned by
> > it and propagating the error is not always the best choice, like in
> > this particular case.
> >
> >>> The core code has been clean up from the high friction it had with th=
e
> >>> legacy ACPI code. It would be nice to continue it this direction.
> >
> > This isn't really ACPI specific.  Any driver can return an error from
> > .get_cur_state() if it has a good enough reason.
>
> We are talking about registration time, right? If the driver is
> registering too soon, eg. the firmware is not ready, should it fix the
> moment it is registering the cooling device when it is sure the firmware
> completed its initialization ?

OK, so arguably the driver could set the initial state of the cooling
device to 0.  That may or may not be the right thing to do depending
on the thermal state of the system at the moment.  Then it would need
to wait for the governor to pick up a more suitable state for it or
leave it at 0.  This could address the particular case at hand.

However, should the core fail the cooling device registration if it
gets an error from .get_cur_state() to start with?  It didn't do that
before.  Indeed, it didn't even call .get_cur_state() then in the
first place.  Moreover, the current state of the cooling device is not
even needed to register it except for the initialization of the debug
code for that cooling device, so why fail the registration of it?