Re: [PATCH 1/3] thermal: core: Add indication for userspace usage

[Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>]

On Wed, 2020-12-09 at 10:30 +0100, Daniel Lezcano wrote:
> On 07/12/2020 06:36, Kai-Heng Feng wrote:
> > 
> > > On Dec 1, 2020, at 02:39, Srinivas Pandruvada <
> > > srinivas.pandruvada@linux.intel.com> wrote:
> > > 
> > > On Tue, 2020-12-01 at 02:22 +0800, Kai-Heng Feng wrote:
> > > > > On Dec 1, 2020, at 02:13, Srinivas Pandruvada <
> > > > > srinivas.pandruvada@linux.intel.com> wrote:
> > > > 
> > > > [snipped] 
> > > > 
> > > > > > > What about creating an new callback
> > > > > > > 
> > > > > > > enum thermal_trip_status {
> > > > > > > 	THERMAL_TRIP_DISABLED = 0,
> > > > > > > 	THERMAL_TRIP_ENABLED,
> > > > > > > };
> > > > > > > 
> > > > > > > int get_trip_status(struct thermal_zone_device *, int
> > > > > > > trip,
> > > > > > > enum
> > > > > > > thermal_trip_status *state);
> > > > > > > 
> > > > > > > Then in 
> > > > > > > static void handle_thermal_trip(struct
> > > > > > > thermal_zone_device *tz,
> > > > > > > int
> > > > > > > trip)
> > > > > > > {
> > > > > > > 
> > > > > > > /* before tz->ops->get_trip_temp(tz, trip, &trip_temp);
> > > > > > > */
> > > > > > > if (tz->ops->get_trip_status) {
> > > > > > > 	enum thermal_trip_status *status;
> > > > > > > 
> > > > > > > 	if (!tz->ops->get_trip_status(tz, trip, &status)) {
> > > > > > > 		if (status == THERMAL_TRIP_DISABLED)
> > > > > > > 			return;	
> > > > > > > 	}
> > > > > > > }
> > > > > > > ...
> > > > > > > ...
> > > > > > > 
> > > > > > > }
> > > > > > > 
> > > > > > > 
> > > > > > > This callback will help the cases:
> > > > > > > - Allows drivers to selectively disable certain trips
> > > > > > > during
> > > > > > > init
> > > > > > > state
> > > > > > > or system resume where there can be spikes or always.
> > > > > > > int340x
> > > > > > > drivers
> > > > > > > can disable always.
> > > > > > 
> > > > > > This sounds really great. This is indeed can happen on
> > > > > > system
> > > > > > resume,
> > > > > > before userspace process thaw.
> > > > > > 
> > > > > > > - Still give options for drivers to handle critical trip
> > > > > > > even
> > > > > > > if
> > > > > > > they
> > > > > > > are bound to user space governors. User space process may
> > > > > > > be
> > > > > > > dead,
> > > > > > > so
> > > > > > > still allow kernel to process graceful shutdown
> > > > > > 
> > > > > > To make the scenario happen, do we need a new sysfs to let
> > > > > > usespace
> > > > > > enable it with THERMAL_TRIP_ENABLED?
> > > > > This should be drivers call not user space.
> > > > 
> > > > Understood. So after thermal_zone_device_register(), the driver
> > > > can
> > > > decide to what to return on get_trip_temp().
> > > get_trip_status()
> > > 
> > > > Let me work on a new patch if there's no other concern.
> > > Better to wait for confirmation from Daniel and others.
> > 
> > Daniel,
> > 
> > Do you like Srinivas' proposed solution?
> > 
> > I hope we can find a solution in upstream kernel soon.
> 
> (just trying to figure out the full context)
> 
> If the device is enumerated outside of a thermal zone, the sensor
> should
> not register in the thermal zone no ?

Other trips are fine, so sensor registry is still required for passive
and active control. We just need to ignore critical trip. These table
are tested by OEM on Windows, where critical trip doesn't result in
immediate shutdown.

Thanks,
Srinivas

> 
> 
>