On Wed, Jul 17, 2024 at 11:24 PM Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
>
> On Wed, Jul 17, 2024 at 09:45:02PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >
> > The iwlwifi wireless driver registers a thermal zone that is only needed
> > when the network interface handled by it is up and it wants that thermal
> > zone to be effectively ignored by the core otherwise.
> >
> > Before commit a8a261774466 ("thermal: core: Call monitor_thermal_zone()
> > if zone temperature is invalid") that could be achieved by returning
> > an error code from the thermal zone's .get_temp() callback because the
> > core did not really handle errors returned by it almost at all.
> > However, commit a8a261774466 made the core attempt to recover from the
> > situation in which the temperature of a thermal zone cannot be
> > determined due to errors returned by its .get_temp() and is always
> > invalid from the core's perspective.
> >
> > That was done because there are thermal zones in which .get_temp()
> > returns errors to start with due to some difficulties related to the
> > initialization ordering, but then it will start to produce valid
> > temperature values at one point.
> >
> > Unfortunately, the simple approach taken by commit a8a261774466,
> > which is to poll the thermal zone periodically until its .get_temp()
> > callback starts to return valid temperature values, is at odds with
> > the special thermal zone in iwlwifi in which .get_temp() may always
> > return an error because its network interface may always be down. If
> > that happens, every attempt to invoke the thermal zone's .get_temp()
> > callback resulting in an error causes the thermal core to print a
> > dev_warn() message to the kernel log which is super-noisy.
> >
> > To address this problem, make the core handle the case in which
> > .get_temp() returns 0, but the temperature value returned by it
> > is not actually valid, in a special way. Namely, make the core
> > completely ignore the invalid temperature value coming from
> > .get_temp() in that case, which requires folding in
> > update_temperature() into its caller and a few related changes.
> >
> > On the iwlwifi side, modify iwl_mvm_tzone_get_temp() to return 0
> > and put THERMAL_TEMP_INVALID into the temperature return memory
> > location instead of returning an error when the firmware is not
> > running or it is not of the right type.
> >
> > Also, to clearly separate the handling of invalid temperature
> > values from the thermal zone initialization, introduce a special
> > THERMAL_TEMP_INIT value specifically for the latter purpose.
> >
> > Fixes: a8a261774466 ("thermal: core: Call monitor_thermal_zone() if zone temperature is invalid")
> > Closes: https://lore.kernel.org/linux-pm/20240715044527.GA1544@sol.localdomain/
> > Reported-by: Eric Biggers <ebiggers@xxxxxxxxxx>
> > Reported-by: Stefan Lippers-Hollmann <s.l-h@xxxxxx>
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=201761
> > Tested-by: Oleksandr Natalenko <oleksandr@xxxxxxxxxxxxxx>
> > Tested-by: Stefan Lippers-Hollmann <s.l-h@xxxxxx>
> > Cc: 6.10+ <stable@xxxxxxxxxxxxxxx> # 6.10+
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > ---
> >
> > v1 -> v2:
> > * It is safer to retain the old behavior in thermal_zone_get_temp(),
> > which is the second place where the .get_temp() zone callback is
> > used, so make it return -ENODATA if the temperature value coming
> > from that callback is invalid.
> > * Add Tested-by: for Stefan.
> >
> > I have retained the previous Tested-by because the part of the patch that has
> > been tested remains unchanged.
> >
> > ---
> > drivers/net/wireless/intel/iwlwifi/mvm/tt.c | 7 +++
> > drivers/thermal/thermal_core.c | 51 +++++++++++++---------------
> > drivers/thermal/thermal_core.h | 3 +
> > drivers/thermal/thermal_helpers.c | 2 +
> > 4 files changed, 35 insertions(+), 28 deletions(-)
>
> This makes the log messages go away for me. However I had to resolve a conflict
> in drivers/net/wireless/intel/iwlwifi/mvm/tt.c to apply this patch to the latest
> upstream.
Thanks for the heads-up, I've rebased it on top of the current
mainline while applying.
