Re: [Regression] cached max_state breaks ACPI processor cooling device

[srinivas pandruvada]

On Sat, 2023-02-25 at 05:34 +0000, Zhang, Rui wrote:
> On Sat, 2023-02-25 at 13:29 +0800, Zhang Rui wrote:
> > Hi, All,
> > 
> > Starting from commit c408b3d1d9bb("thermal: Validate new state in
> > cur_state_store()") and commit a365105c685c("thermal: sysfs: Reuse
> > cdev->max_state"), the cdev->get_max_state() is evaluated only once
> > during cooling device registration.
> > 
> > This is done to fix the below Smatch static checker warning:
> >         drivers/thermal/thermal_sysfs.c:656
> > thermal_cooling_device_stats_update()
> >         warn: potential integer overflow from user 'stats->state *
> > stats->max_states + new_state'
> > reported here https://lore.kernel.org/all/Y0ltRJRjO7AkawvE@kili/.
> > 
> > But this actually breaks cooling devices which could have dynamic
> > max
> > cooling state, like ACPI processor cooling device.
> > 
> > acpi_processor_driver_init
> >         driver_register(&acpi_processor_driver);
> >                 acpi_processor_start
> >                         acpi_processor_thermal_init
> >                                 thermal_cooling_device_register
> >                                         processor_get_max_state
> >         acpi_processor_cpufreq_init = true
> > The driver doesn't count cpufreq as cooling state until
> > acpi_processor_cpufreq_init is set later.
> > 
> > As a result, without the commits above,
> > /sys/class/thermal/cooling_device10/cur_state:0
> > /sys/class/thermal/cooling_device10/max_state:3
> > /sys/class/thermal/cooling_device10/type:Processor
> > after the commits above, 
> > /sys/class/thermal/cooling_device10/cur_state:0
> > /sys/class/thermal/cooling_device10/max_state:0
> > /sys/class/thermal/cooling_device10/type:Processor
> 
You really need a core API, which updates max_state. When I was trying
to update max_state for powerclamp driver, I thought of a function to
do this.
This basically works under  cdev->lock, something similar to
__thermal_cdev_update. For each thermal instances if the shallowest
state is more than the new max_state, fail the request, otherwise set.


Thanks,
Srinivas


> Forgot to mention that this problem is
> 
> Reported-by: Wang, Quanxian <quanxian.wang@xxxxxxxxx>
> 
> thanks,
> rui
> > 
> > In order to fix this, there are something worth clarification IMO.
> > 1. should we support dynamic max_state or not?
> >    ACPI processor cooling state is a combination of processor p-
> > states
> >    and t-states.
> >    t-states are static, but p-states can vary depends on processor 
> >    frequency driver loaded or not.
> >    I'm not sure if there is any other user like this, but still
> > this
> >    is a valid use case of dynamic max_state.
> > 2. how to handle dynamic max_state for cooling device statistics in
> >    sysfs?
> >    IMO, when max_state changes, the definition of previous cooling
> >    device is changed as well, thus we should abandon the previous
> >    statistics and restart counting.
> > 3. anything else needs to be handled when max_state changes?
> >    Say, as the definition of each cooling state is changed when
> >    max_state changes, should we invalidate and re-update the
> >    thermal instances of this cdev in each thermal zone device?
> > 4. how to detect/handle max cooling states changes?
> >    Should we do this as before, which invokes .get_max_state()
> >    everywhere and do updates when necessary , or
> >    a. cache max_state like we do today
> >    b. introduce a new API for max_state change
> >    c. invoke the new API in the cooling device driver explicitly
> > when
> >       max_state changes
> >    d. update cached max_state value, statistics sysfs and
> >       thermal_instances in the API
> >    e. remove .get_max_state() callback as we register and update
> > the
> >       max_state with a fixed value each time.
> > 
> > thanks,
> > rui
> > 
> > 
> >