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

[Zhang, Rui]

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

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
> 
> 
>