Re: [RFC PATCH 1/5] thermal: let governors have private data for each thermal zone

[Eduardo Valentin <edubezval@gmail.com>]

Javi,

On Thu, May 15, 2014 at 03:30:53PM +0100, Javi Merino wrote:
> On Tue, May 13, 2014 at 03:31:08PM +0100, edubezval@gmail.com wrote:
> > Hello Javi,
> > 
> > Thanks for your patch. Find some comment inline.
> > 

[cut]

> 
> Will do, but I find that the other governor op (throttle) is not
> documented.  Where do you suggest we should put it?
> 

The main entry for thermal Documentation is Documentation/thermal/. The
sysfs-api.txt file has major info about the thermal framework. But for
this case, I agree with you, we deserve a better explanation. I think
the fact that the governor API is becoming more interesting is a hint
that we need better documentation. I suggest having another file
describing the governor API and how it deals with other entities such
as sensor inputs and cooling actions.

Rui, any suggestion here?


> > > + if (tz->governor && tz->governor->bind_to_tz) {
> > > + if (tz->governor->bind_to_tz(tz))
> > > + /*
> > > + * The new governor failed to register
> > > + * and the previous one failed as well
> > > + */
> > 
> > I suggest having a dev_warn here to notify that a zone is now orphan due to
> > switching from one governor to another.
> 
> Done
> 
> > > + tz->governor = NULL;
> > > + }
> > > +
> > > + return ret;
> > > + }
> > > + }
> > > +
> > > + tz->governor = new_gov;
> > > +
> > > + return ret;
> > > +}
> > > +
> > >  int thermal_register_governor(struct thermal_governor *governor)
> > >  {
> > >   int err;
> > > @@ -104,8 +143,12 @@ int thermal_register_governor(struct thermal_governor *governor)
> > >
> > >   name = pos->tzp->governor_name;
> > >
> > > - if (!strnicmp(name, governor->name, THERMAL_NAME_LENGTH))
> > > - pos->governor = governor;
> > > + if (!strnicmp(name, governor->name, THERMAL_NAME_LENGTH)) {
> > > + int ret = thermal_set_governor(pos, governor);
> > 
> > nip: add a blank line between declarations and first statement.
> 
> Ok
> 
> > > + if (ret)
> > > + pr_warn("Failed to set governor %s for zone %d: %d\n",
> > > + governor->name, pos->id, ret);
> > 
> > dev_* family of logging functions is preferred in this file.
> 
> Done
> 
> > > + }
> > >   }
> > >
> > >   mutex_unlock(&thermal_list_lock);
> > > @@ -131,7 +174,7 @@ void thermal_unregister_governor(struct thermal_governor *governor)
> > >   list_for_each_entry(pos, &thermal_tz_list, node) {
> > >   if (!strnicmp(pos->governor->name, governor->name,
> > >   THERMAL_NAME_LENGTH))
> > > - pos->governor = NULL;
> > > + thermal_set_governor(pos, NULL);
> > >   }
> > >
> > >   mutex_unlock(&thermal_list_lock);
> > > @@ -756,8 +799,9 @@ policy_store(struct device *dev, struct device_attribute *attr,
> > >   if (!gov)
> > >   goto exit;
> > >
> > > - tz->governor = gov;
> > > - ret = count;
> > > + ret = thermal_set_governor(tz, gov);
> > > + if (!ret)
> > > + ret = count;
> > >
> > >  exit:
> > >   mutex_unlock(&thermal_governor_lock);
> > > @@ -1452,6 +1496,7 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
> > >   int result;
> > >   int count;
> > >   int passive = 0;
> > > + struct thermal_governor *governor;
> > >
> > >   if (type && strlen(type) >= THERMAL_NAME_LENGTH)
> > >   return ERR_PTR(-EINVAL);
> > > @@ -1542,9 +1587,15 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
> > >   mutex_lock(&thermal_governor_lock);
> > >
> > >   if (tz->tzp)
> > > - tz->governor = __find_governor(tz->tzp->governor_name);
> > > + governor = __find_governor(tz->tzp->governor_name);
> > >   else
> > > - tz->governor = def_governor;
> > > + governor = def_governor;
> > > +
> > > + result = thermal_set_governor(tz, governor);
> > > + if (result) {
> > > + mutex_unlock(&thermal_governor_lock);
> > > + goto unregister;
> > > + }
> > >
> > >   mutex_unlock(&thermal_governor_lock);
> > >
> > > @@ -1634,7 +1685,7 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
> > >   device_remove_file(&tz->device, &dev_attr_mode);
> > >   device_remove_file(&tz->device, &dev_attr_policy);
> > >   remove_trip_attrs(tz);
> > > - tz->governor = NULL;
> > > + thermal_set_governor(tz, NULL);
> > >
> > >   thermal_remove_hwmon_sysfs(tz);
> > >   release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
> > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > > index f7e11c7ea7d9..baac212f815e 100644
> > > --- a/include/linux/thermal.h
> > > +++ b/include/linux/thermal.h
> > > @@ -177,6 +177,7 @@ struct thermal_zone_device {
> > >   struct thermal_zone_device_ops *ops;
> > >   const struct thermal_zone_params *tzp;
> > >   struct thermal_governor *governor;
> > > + void *governor_data;
> > 
> > Please document this entry.
> 
> Where?  None of these entries are documented.  I guess I'll have to
> add a patch that describes each one of them and then add ours.
> 
> > >   struct list_head thermal_instances;
> > >   struct idr idr;
> > >   struct mutex lock; /* protect thermal_instances list */
> > > @@ -187,6 +188,8 @@ struct thermal_zone_device {
> > >  /* Structure that holds thermal governor information */
> > >  struct thermal_governor {
> > >   char name[THERMAL_NAME_LENGTH];
> > > + int (*bind_to_tz)(struct thermal_zone_device *tz);
> > > + void (*unbind_from_tz)(struct thermal_zone_device *tz);
> > 
> > a governor cannot prevent unbinding?
> 
> To me it's like open()/close() or malloc()/free().  You generally can
> fail to bind, but not fail to unbind.  I can change it to an int if
> you think it is something that we may need.
> 
> > Please, document the proposed operations.
> 
> Same as before, none of the entries in this struct are documented so
> I'll submit a patch that describes the current ones and then add our
> new ones in our patch.
> 
> > >   int (*throttle)(struct thermal_zone_device *tz, int trip);
> > >   struct list_head governor_list;
> > >  };
> > > --
> > > 1.7.9.5
> > 
> > I request that you update Documentation/thermal/sysfs-api.txt too.
> 
> Will do.
> 
> Cheers,
> Javi
>