Re: [PATCH 01/16] Thermal: Make Thermal trip points writeable

[Zhang Rui <rui.zhang@intel.com>]

On 四, 2012-07-19 at 22:27 +0200, Rafael J. Wysocki wrote:
> On Thursday, July 19, 2012, Zhang Rui wrote:
> > From: Durgadoss R <dugardoss.r@intel.com>
> > 
> > Some of the thermal drivers using the Generic Thermal Framework
> > require (all/some) trip points to be writeable. This patch makes
> > the trip point temperatures writeable on a per-trip point basis,
> > and modifies the required function call in thermal.c. This patch
> > also updates the Documentation to reflect the new change.
> > 
> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  Documentation/thermal/sysfs-api.txt      |    4 +-
> >  drivers/acpi/thermal.c                   |    4 +-
> >  drivers/platform/x86/acerhdf.c           |    2 +-
> >  drivers/platform/x86/intel_mid_thermal.c |    2 +-
> >  drivers/thermal/spear_thermal.c          |    2 +-
> >  drivers/thermal/thermal_sys.c            |  147 +++++++++++++++++++++---------
> >  include/linux/thermal.h                  |   15 ++-
> >  7 files changed, 125 insertions(+), 51 deletions(-)
> > 
> > diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> > index 1733ab9..0c7c423 100644
> > --- a/Documentation/thermal/sysfs-api.txt
> > +++ b/Documentation/thermal/sysfs-api.txt
> > @@ -32,7 +32,8 @@ temperature) and throttle appropriate devices.
> >  
> >  1.1 thermal zone device interface
> >  1.1.1 struct thermal_zone_device *thermal_zone_device_register(char *name,
> > -		int trips, void *devdata, struct thermal_zone_device_ops *ops)
> > +		int trips, int flag, void *devdata,
> 
> The 'flags' argument should rather be called 'mask', or even 'writable_mask'.
> 
Agreed.

> >  #define to_cooling_device(_dev)	\
> >  	container_of(_dev, struct thermal_cooling_device, device)
> > @@ -1089,9 +1084,83 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
> >  EXPORT_SYMBOL(thermal_zone_device_update);
> >  >  /**
> > + * create_trip_attrs - create attributes for trip points
> > + * @tz:		the thermal zone device
> > + * @flag:	Writeable trip point bitmap.
> > + */
> > +static int create_trip_attrs(struct thermal_zone_device *tz, int flag)
> > +{
> > +	int indx;
> > +	int writeable;
> > +
> > +	tz->trip_type_attrs =
> > +		kzalloc(sizeof(struct thermal_attr) * tz->trips, GFP_KERNEL);
> > +	if (!tz->trip_type_attrs)
> > +		return -ENOMEM;
> > +
> > +	tz->trip_temp_attrs =
> > +		kzalloc(sizeof(struct thermal_attr) * tz->trips, GFP_KERNEL);
> > +	if (!tz->trip_temp_attrs) {
> > +		kfree(tz->trip_type_attrs);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	for (indx = 0; indx < tz->trips; indx++) {
> > +
> > +		/* create trip type attribute */
> > +		snprintf(tz->trip_type_attrs[indx].name, THERMAL_NAME_LENGTH,
> > +			 "trip_point_%d_type", indx);
> 
> If this really truncates the name, we'll run into problems in _show() and _store().
> Is there any solution to that?
> 
as THERMAL_MAX_TRIP is smaller than 100, so the answer is no.

> > +
> > +		sysfs_attr_init(&tz->trip_type_attrs[count].attr.attr);
> > +		tz->trip_type_attrs[indx].attr.attr.name =
> > +						tz->trip_type_attrs[indx].name;
> > +		tz->trip_type_attrs[indx].attr.attr.mode = S_IRUGO;
> > +		tz->trip_type_attrs[indx].attr.show = trip_point_type_show;
> > +
> > +		device_create_file(&tz->device,
> > +				   &tz->trip_type_attrs[indx].attr);
> > +
> > +		/* create trip temp attribute */
> > +		writeable = flag & (1 << indx);
> > +		snprintf(tz->trip_temp_attrs[indx].name, THERMAL_NAME_LENGTH,
> > +			 "trip_point_%d_temp", indx);
> > +
> > +		sysfs_attr_init(&tz->trip_temp_attrs[indx].attr.attr);
> > +		tz->trip_temp_attrs[indx].attr.attr.name =
> > +						tz->trip_temp_attrs[indx].name;
> > +		tz->trip_temp_attrs[indx].attr.attr.mode = S_IRUGO;
> > +		tz->trip_temp_attrs[indx].attr.show = trip_point_temp_show;
> > +		if (writeable) {
> 
> Why don't you put the "flag & (1 << indx)" here directly instead of 'writable'?
> 
agreed.

> > +			tz->trip_temp_attrs[indx].attr.attr.mode |= S_IWUSR;
> > +			tz->trip_temp_attrs[indx].attr.store =
> > +							trip_point_temp_store;
> > +		}
> > +
> > +		device_create_file(&tz->device,
> > +				   &tz->trip_temp_attrs[indx].attr);
> > +	}
> > +	return 0;
> > +}
> > +
> > +static void remove_trip_attrs(struct thermal_zone_device *tz)
> > +{
> > +	int indx;
> > +
> > +	for (indx = 0; indx < tz->trips; indx++) {
> > +		device_remove_file(&tz->device,
> > +				   &tz->trip_type_attrs[indx].attr);
> > +		device_remove_file(&tz->device,
> > +				   &tz->trip_temp_attrs[indx].attr);
> > +	}
> > +	kfree(tz->trip_type_attrs);
> > +	kfree(tz->trip_temp_attrs);
> > +}
> > +
> > +/**
> >   * thermal_zone_device_register - register a new thermal zone device
> >   * @type:	the thermal zone device type
> >   * @trips:	the number of trip points the thermal zone support
> > + * @flag:	a bit string indicating the writeablility of trip points
> >   * @devdata:	private device data
> >   * @ops:	standard thermal zone device callbacks
> >   * @tc1:	thermal coefficient 1 for passive calculations
> > @@ -1107,7 +1176,7 @@ EXPORT_SYMBOL(thermal_zone_device_update);
> >   * section 11.1.5.1 of the ACPI specification 3.0.
> >   */
> >  struct thermal_zone_device *thermal_zone_device_register(char *type,
> > -	int trips, void *devdata,
> > +	int trips, int flag, void *devdata,
> >  	const struct thermal_zone_device_ops *ops,
> >  	int tc1, int tc2, int passive_delay, int polling_delay)
> >  {
> > @@ -1124,6 +1193,9 @@ struct thermal_zone_device *thermal_zone_device_register(char *type,
> >  	if (trips > THERMAL_MAX_TRIPS || trips < 0)
> >  		return ERR_PTR(-EINVAL);
> >  
> > +	if (flag >> trips)
> > +		return ERR_PTR(-EINVAL);
> 
> I'm not sure if this check is necessary.  Does it actually matter is someone
> passes ones in the unused bit positions?
> 
I do not think we can trust this value if the caller even wants to set
writable mask for a non-exist trip point.

thanks,
rui

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html