mbox series

[v1,0/3] iwlwifi: mvm: Thermal management fixes

Message ID 1892445.tdWV9SEqCh@kreacher (mailing list archive)
Headers show
Series iwlwifi: mvm: Thermal management fixes | expand

Message

Rafael J. Wysocki Feb. 7, 2024, 7:08 p.m. UTC
Hi Everyone,

There are a few thermal management shortcomings in the iwlwifi driver that are
addressed by this series.

First off, the fw_trips_index[] array field in struct iwl_mvm_thermal_device
is only populated and never read, and the code populating it has problems,
so patch [1/3] removes it.

Second, iwl_mvm_thermal_zone_register() populates the trip table after passing
it to thermal_zone_device_register_with_trips() which is too late, because it
can get used before it is populated.  It also may as well use THERMAL_TEMP_INVALID
as the "invalid temperature" value.  Both these issues are addressed by patch [2/3].

Finally, iwl_mvm_send_temp_report_ths_cmd() accesses the trip tables used during
thermal zone registration directly in order to obtain the current trip point
temperature values, which is not guaranteed to work in the future, because the
core will store the trips information in its own copy of the trip table - see
this patch series:

https://lore.kernel.org/linux-pm/2728491.mvXUDI8C0e@kreacher/

If possible, I'd like to route the $subject series through the thermal tree,
it is requisite for the above one.

Thanks!

Comments

Kalle Valo Feb. 8, 2024, 6:13 a.m. UTC | #1
"Rafael J. Wysocki" <rjw@rjwysocki.net> writes:

> There are a few thermal management shortcomings in the iwlwifi driver that are
> addressed by this series.
>
> First off, the fw_trips_index[] array field in struct iwl_mvm_thermal_device
> is only populated and never read, and the code populating it has problems,
> so patch [1/3] removes it.
>
> Second, iwl_mvm_thermal_zone_register() populates the trip table after passing
> it to thermal_zone_device_register_with_trips() which is too late, because it
> can get used before it is populated.  It also may as well use THERMAL_TEMP_INVALID
> as the "invalid temperature" value.  Both these issues are addressed by patch [2/3].
>
> Finally, iwl_mvm_send_temp_report_ths_cmd() accesses the trip tables used during
> thermal zone registration directly in order to obtain the current trip point
> temperature values, which is not guaranteed to work in the future, because the
> core will store the trips information in its own copy of the trip table - see
> this patch series:
>
> https://lore.kernel.org/linux-pm/2728491.mvXUDI8C0e@kreacher/
>
> If possible, I'd like to route the $subject series through the thermal tree,
> it is requisite for the above one.

iwlwifi is getting a lot of patches lately, though I don't know if any
of them touch the thermal stuff. But if this patchset goes to the
thermal I am a bit worried about conflicts.
Johannes Berg Feb. 8, 2024, 9:28 a.m. UTC | #2
On Thu, 2024-02-08 at 08:13 +0200, Kalle Valo wrote:
> > 
> > If possible, I'd like to route the $subject series through the thermal tree,
> > it is requisite for the above one.
> 
> iwlwifi is getting a lot of patches lately, though I don't know if any
> of them touch the thermal stuff. But if this patchset goes to the
> thermal I am a bit worried about conflicts.

Should be OK, I checked now and apart from the trivial change in mvm.h
this is contained in tt.c, which isn't touched (even in our internal
feeder tree) after commit 0106cce5ad0c ("wifi: iwlwifi: mvm: drop NULL
pointer check in iwl_mvm_tzone_set_trip_temp()").

But I'll let Miri send an Acked-by to go through your tree, since she's
the maintainer :-)

johannes
Miri Korenblit Feb. 8, 2024, 1:24 p.m. UTC | #3
> Hi Everyone,
> 
> There are a few thermal management shortcomings in the iwlwifi driver that are
> addressed by this series.
> 
> First off, the fw_trips_index[] array field in struct iwl_mvm_thermal_device is
> only populated and never read, and the code populating it has problems, so
> patch [1/3] removes it.
> 
> Second, iwl_mvm_thermal_zone_register() populates the trip table after passing
> it to thermal_zone_device_register_with_trips() which is too late, because it can
> get used before it is populated.  It also may as well use
> THERMAL_TEMP_INVALID as the "invalid temperature" value.  Both these issues
> are addressed by patch [2/3].
> 
> Finally, iwl_mvm_send_temp_report_ths_cmd() accesses the trip tables used
> during thermal zone registration directly in order to obtain the current trip point
> temperature values, which is not guaranteed to work in the future, because the
> core will store the trips information in its own copy of the trip table - see this
> patch series:
> 
> https://lore.kernel.org/linux-pm/2728491.mvXUDI8C0e@kreacher/
> 
> If possible, I'd like to route the $subject series through the thermal tree, it is
> requisite for the above one.
> 
> Thanks!
> 
> 
Acked-by: Miri Korenblit <Miriam.rachel.korenblit@intel.com>
Miri Korenblit Feb. 8, 2024, 1:26 p.m. UTC | #4
> On Thu, 2024-02-08 at 08:13 +0200, Kalle Valo wrote:
> > >
> > > If possible, I'd like to route the $subject series through the
> > > thermal tree, it is requisite for the above one.
> >
> > iwlwifi is getting a lot of patches lately, though I don't know if any
> > of them touch the thermal stuff. But if this patchset goes to the
> > thermal I am a bit worried about conflicts.
> 
> Should be OK, I checked now and apart from the trivial change in mvm.h this is
> contained in tt.c, which isn't touched (even in our internal feeder tree) after
> commit 0106cce5ad0c ("wifi: iwlwifi: mvm: drop NULL pointer check in
> iwl_mvm_tzone_set_trip_temp()").
> 
> But I'll let Miri send an Acked-by to go through your tree, since she's the
> maintainer :-)
> 
> Johannes

Hi,
 
This seems fine to go through your tree, as Johannes said, we don't expect any conflicts.

Thanks,
Miri
Rafael J. Wysocki Feb. 8, 2024, 1:50 p.m. UTC | #5
On Thu, Feb 8, 2024 at 2:26 PM Korenblit, Miriam Rachel
<miriam.rachel.korenblit@intel.com> wrote:
>
>
>
> > On Thu, 2024-02-08 at 08:13 +0200, Kalle Valo wrote:
> > > >
> > > > If possible, I'd like to route the $subject series through the
> > > > thermal tree, it is requisite for the above one.
> > >
> > > iwlwifi is getting a lot of patches lately, though I don't know if any
> > > of them touch the thermal stuff. But if this patchset goes to the
> > > thermal I am a bit worried about conflicts.
> >
> > Should be OK, I checked now and apart from the trivial change in mvm.h this is
> > contained in tt.c, which isn't touched (even in our internal feeder tree) after
> > commit 0106cce5ad0c ("wifi: iwlwifi: mvm: drop NULL pointer check in
> > iwl_mvm_tzone_set_trip_temp()").
> >
> > But I'll let Miri send an Acked-by to go through your tree, since she's the
> > maintainer :-)
> >
> > Johannes
>
> Hi,
>
> This seems fine to go through your tree, as Johannes said, we don't expect any conflicts.

Thank you!

I'll go ahead and queue it up, then.