Re: Fix: Bind Cooling Device Weight

[Javi Merino <javi.merino@arm.com>]

On Mon, Dec 22, 2014 at 03:18:35AM +0000, Zhang Rui wrote:
> On Thu, 2014-12-18 at 09:43 -0400, Eduardo Valentin wrote:
> > On Thu, Dec 18, 2014 at 12:19:00PM +0000, Kapileshwar Singh wrote:
> > > The Problem:
> > > 
> > > In the current code, the weight of the cooling device, which is read as
> > > contribution from the device tree in of-thermal.c as:
> > > 
> > > 
> > >         ret = of_property_read_u32(np, "contribution", &prop);
> > >         if (ret == 0)
> > >                 __tbp->usage = prop;
> > > 
> > > This is only stored in the private devicedata as:
> > > 
> > >         ((__thernal_zone *)cdev->devdata)->__tbp->usage
> > > 
> > > As of-thermal.c specifies its "bind" operation and does not populate
> > > tzd->tzp->tbp, this causes an erroneous access in the fair-share
> > > governor when it tries to access the weight:
> > > 
> > > instance->target = get_target_state(tz, cdev,
> > >                                         tzp->tbp[i].weight,
> > > cur_trip_level);
> > > 
> > > 
> > > The Proposed solution:
> > > 
> > > The proposed solution has the following changes:
> > > 
> > >  1. Passing the weight as an argument to thermal_zone_bind_cooling_device
> > > 
> > >  2. Storing the weight in the thermal_instance data structure created in
> > > the thermal_zone_bind_cooling_device function
> > > 
> > >  3. Changing the accesses in the governor accordingly.
> > 
> > Shouldn't we simply:
> > 1. In of-thermal, populate tzd->tzp->tbp by passing a tbp with correct
> > tbp's in the registration call:
> > 	zone = thermal_zone_device_register(child->name, tz->ntrips,
> > 						0,
> > 						tz,
> > 						ops,
> > /* This guy needs to be filled properly */	tzp,
> > 						tz->passive_delay,
> > 						tz->polling_delay);
> > 
> Agreed.
> 
> > 2. Add a proper check in the governor to avoid accessing thermal zones
> > with missing data.
> > 
> > I know there is a check in place:
> >         if (!tz->tzp || !tz->tzp->tbp)
> > 	          return -EINVAL;
> > 
> > which based in your description seams to be failing. Here, I need more
> > info from your side describing what exactly you are seeing. Can you
> > please post the kernel log when the failure happens? Does it output a
> > kernel segfault or is the governor simply not working?
> > 
> > I would expect, with the current code, the governor be silent and
> > non-functional, which needs to be fixed too.
> 
> Plus, I think we should add an optional .validate() callback for each
> governor, to check if the thermal zone meets the requirement of the
> governor or not before switching to it.

The patch "thermal: let governors have private data for each thermal
zone" that you applied[0] allows you to do that.  Governors
can have a bind_to_tz() op that can fail.  If it does, then the switch
doesn't happen and you continue with the old governor.

[0] http://article.gmane.org/gmane.linux.power-management.general/54163

Cheers,
Javi