| Message ID | 1346041706-29642-7-git-send-email-durgadoss.r@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: R, Durgadoss > Sent: Monday, August 27, 2012 7:28 AM > To: lenb@kernel.org; Zhang, Rui > Cc: linux-acpi@vger.kernel.org; eduardo.valentin@ti.com; R, Durgadoss > Subject: [PATCHv2 06/14] Thermal: Add a policy sysfs attribute > Importance: High > > This patch adds a policy sysfs attribute to a thermal zone. > This attribute will give us the throttling policy used for the zone. > This is a RO attribute. > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com> > --- > drivers/thermal/thermal_sys.c | 32 ++++++++++++++++++++++++++++++-- > 1 file changed, 30 insertions(+), 2 deletions(-) > > diff --git a/drivers/thermal/thermal_sys.c > b/drivers/thermal/thermal_sys.c index 6adda39..8aa4200a6 100644 > --- a/drivers/thermal/thermal_sys.c > +++ b/drivers/thermal/thermal_sys.c > @@ -378,10 +378,32 @@ passive_show(struct device *dev, struct > device_attribute *attr, > return sprintf(buf, "%d\n", tz->forced_passive); } > > +static ssize_t > +policy_show(struct device *dev, struct device_attribute *devattr, char > +*buf) { > + struct thermal_zone_device *tz = to_thermal_zone(dev); > + struct thermal_zone_params *tzp = tz->tzp; > + > + if (!tzp) > + return sprintf(buf, "step_wise(default)\n"); > + > + switch (tzp->throttle_policy) { > + case THERMAL_FAIR_SHARE: > + return sprintf(buf, "fair_share\n"); > + case THERMAL_STEP_WISE: > + return sprintf(buf, "step_wise\n"); > + case THERMAL_USER_SPACE: > + return sprintf(buf, "user_space\n"); > + default: > + return sprintf(buf, "unknown\n"); > + } > +} > + > static DEVICE_ATTR(type, 0444, type_show, NULL); static > DEVICE_ATTR(temp, 0444, temp_show, NULL); static DEVICE_ATTR(mode, > 0644, mode_show, mode_store); static DEVICE_ATTR(passive, S_IRUGO | > S_IWUSR, passive_show, passive_store); > +static DEVICE_ATTR(throttle_policy, S_IRUGO, policy_show, NULL); > > /* sys I/F for cooling device */ > #define to_cooling_device(_dev) \ > @@ -1349,10 +1371,16 @@ static int retrieve_zone_params(struct > thermal_zone_device *tz) > > /* It is not an error to not have any platform data */ > ret = get_platform_thermal_params(tz); > - if (ret) > + if (ret) { > tz->tzp = NULL; > + return 0; > + } > > - return 0; > + ret = device_create_file(&tz->device, &dev_attr_throttle_policy); > + if (ret) > + dev_err(&tz->device, "creating policy attr failed:%d\n", > ret); > + > + return ret; > } What does this mean? We will not create "policy" attributes if there is no thermal_zone_params? What if platform thermal drivers chooses to provide .bind() and invoke thermal_zone_bind_cooling_device manually? Thanks, rui > > /** > -- > 1.7.9.5 -- 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
Hi Rui, > > +static ssize_t > > +policy_show(struct device *dev, struct device_attribute *devattr, char > > +*buf) { > > + struct thermal_zone_device *tz = to_thermal_zone(dev); > > + struct thermal_zone_params *tzp = tz->tzp; > > + > > + if (!tzp) > > + return sprintf(buf, "step_wise(default)\n"); > > + > > + switch (tzp->throttle_policy) { > > + case THERMAL_FAIR_SHARE: > > + return sprintf(buf, "fair_share\n"); > > + case THERMAL_STEP_WISE: > > + return sprintf(buf, "step_wise\n"); > > + case THERMAL_USER_SPACE: > > + return sprintf(buf, "user_space\n"); > > + default: > > + return sprintf(buf, "unknown\n"); > > + } > > +} > > + > > static DEVICE_ATTR(type, 0444, type_show, NULL); static > > DEVICE_ATTR(temp, 0444, temp_show, NULL); static DEVICE_ATTR(mode, > > 0644, mode_show, mode_store); static DEVICE_ATTR(passive, S_IRUGO | > > S_IWUSR, passive_show, passive_store); > > +static DEVICE_ATTR(throttle_policy, S_IRUGO, policy_show, NULL); > > > > /* sys I/F for cooling device */ > > #define to_cooling_device(_dev) \ > > @@ -1349,10 +1371,16 @@ static int retrieve_zone_params(struct > > thermal_zone_device *tz) > > > > /* It is not an error to not have any platform data */ > > ret = get_platform_thermal_params(tz); > > - if (ret) > > + if (ret) { > > tz->tzp = NULL; > > + return 0; > > + } > > > > - return 0; > > + ret = device_create_file(&tz->device, &dev_attr_throttle_policy); > > + if (ret) > > + dev_err(&tz->device, "creating policy attr failed:%d\n", > > ret); > > + > > + return ret; > > } > > What does this mean? > We will not create "policy" attributes if there is no thermal_zone_params? Yes, that's what I thought initially. Because if there is no 'throttle_policy' we assume that it is (by default) step_wise. But, if we make tz_params be provided through tzd_register function call, it makes sense for this to be a mandatory attribute, showing 'step_wise" if there is no thermal_zone_params. This needs a clean fix, will make it in v3. Thanks, Durga -- 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
> -----Original Message----- > From: R, Durgadoss > Sent: Monday, August 27, 2012 12:02 PM > To: Zhang, Rui; lenb@kernel.org > Cc: linux-acpi@vger.kernel.org; eduardo.valentin@ti.com > Subject: RE: [PATCHv2 06/14] Thermal: Add a policy sysfs attribute > Importance: High > > Hi Rui, > > > > +static ssize_t > > > +policy_show(struct device *dev, struct device_attribute *devattr, > > > +char > > > +*buf) { > > > + struct thermal_zone_device *tz = to_thermal_zone(dev); > > > + struct thermal_zone_params *tzp = tz->tzp; > > > + > > > + if (!tzp) > > > + return sprintf(buf, "step_wise(default)\n"); > > > + > > > + switch (tzp->throttle_policy) { > > > + case THERMAL_FAIR_SHARE: > > > + return sprintf(buf, "fair_share\n"); > > > + case THERMAL_STEP_WISE: > > > + return sprintf(buf, "step_wise\n"); > > > + case THERMAL_USER_SPACE: > > > + return sprintf(buf, "user_space\n"); > > > + default: > > > + return sprintf(buf, "unknown\n"); > > > + } > > > +} > > > + > > > static DEVICE_ATTR(type, 0444, type_show, NULL); static > > > DEVICE_ATTR(temp, 0444, temp_show, NULL); static DEVICE_ATTR(mode, > > > 0644, mode_show, mode_store); static DEVICE_ATTR(passive, S_IRUGO > | > > > S_IWUSR, passive_show, passive_store); > > > +static DEVICE_ATTR(throttle_policy, S_IRUGO, policy_show, NULL); > > > > > > /* sys I/F for cooling device */ > > > #define to_cooling_device(_dev) \ > > > @@ -1349,10 +1371,16 @@ static int retrieve_zone_params(struct > > > thermal_zone_device *tz) > > > > > > /* It is not an error to not have any platform data */ > > > ret = get_platform_thermal_params(tz); > > > - if (ret) > > > + if (ret) { > > > tz->tzp = NULL; > > > + return 0; > > > + } > > > > > > - return 0; > > > + ret = device_create_file(&tz->device, &dev_attr_throttle_policy); > > > + if (ret) > > > + dev_err(&tz->device, "creating policy attr failed:%d\n", > > > ret); > > > + > > > + return ret; > > > } > > > > What does this mean? > > We will not create "policy" attributes if there is no > thermal_zone_params? > > Yes, that's what I thought initially. Because if there is no > 'throttle_policy' > we assume that it is (by default) step_wise. > > But, if we make tz_params be provided through tzd_register function > call, it makes sense for this to be a mandatory attribute, showing > 'step_wise" > if there is no thermal_zone_params. > IMO, every thermal zone should have a policy. And they can be changed anytime if user wants to. 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
> -----Original Message----- > From: R, Durgadoss > Sent: Monday, August 27, 2012 7:28 AM > To: lenb@kernel.org; Zhang, Rui > Cc: linux-acpi@vger.kernel.org; eduardo.valentin@ti.com; R, Durgadoss > Subject: [PATCHv2 06/14] Thermal: Add a policy sysfs attribute > Importance: High > > This patch adds a policy sysfs attribute to a thermal zone. > This attribute will give us the throttling policy used for the zone. > This is a RO attribute. > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com> > --- > drivers/thermal/thermal_sys.c | 32 ++++++++++++++++++++++++++++++-- > 1 file changed, 30 insertions(+), 2 deletions(-) > > diff --git a/drivers/thermal/thermal_sys.c > b/drivers/thermal/thermal_sys.c index 6adda39..8aa4200a6 100644 > --- a/drivers/thermal/thermal_sys.c > +++ b/drivers/thermal/thermal_sys.c > @@ -378,10 +378,32 @@ passive_show(struct device *dev, struct > device_attribute *attr, > return sprintf(buf, "%d\n", tz->forced_passive); } > > +static ssize_t > +policy_show(struct device *dev, struct device_attribute *devattr, char > +*buf) { > + struct thermal_zone_device *tz = to_thermal_zone(dev); > + struct thermal_zone_params *tzp = tz->tzp; > + > + if (!tzp) > + return sprintf(buf, "step_wise(default)\n"); > + > + switch (tzp->throttle_policy) { > + case THERMAL_FAIR_SHARE: > + return sprintf(buf, "fair_share\n"); > + case THERMAL_STEP_WISE: > + return sprintf(buf, "step_wise\n"); > + case THERMAL_USER_SPACE: > + return sprintf(buf, "user_space\n"); > + default: > + return sprintf(buf, "unknown\n"); > + } > +} > + > static DEVICE_ATTR(type, 0444, type_show, NULL); static > DEVICE_ATTR(temp, 0444, temp_show, NULL); static DEVICE_ATTR(mode, > 0644, mode_show, mode_store); static DEVICE_ATTR(passive, S_IRUGO | > S_IWUSR, passive_show, passive_store); > +static DEVICE_ATTR(throttle_policy, S_IRUGO, policy_show, NULL); > > /* sys I/F for cooling device */ > #define to_cooling_device(_dev) \ > @@ -1349,10 +1371,16 @@ static int retrieve_zone_params(struct > thermal_zone_device *tz) > > /* It is not an error to not have any platform data */ > ret = get_platform_thermal_params(tz); > - if (ret) > + if (ret) { > tz->tzp = NULL; > + return 0; > + } > > - return 0; > + ret = device_create_file(&tz->device, &dev_attr_throttle_policy); > + if (ret) > + dev_err(&tz->device, "creating policy attr failed:%d\n", > ret); > + > + return ret; > } > > /** We should remove this attribute in thermal_zone_device_unregister(); 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
> > > > #define to_cooling_device(_dev) \ > > > > @@ -1349,10 +1371,16 @@ static int retrieve_zone_params(struct > > > > thermal_zone_device *tz) > > > > > > > > /* It is not an error to not have any platform data */ > > > > ret = get_platform_thermal_params(tz); > > > > - if (ret) > > > > + if (ret) { > > > > tz->tzp = NULL; > > > > + return 0; > > > > + } > > > > > > > > - return 0; > > > > + ret = device_create_file(&tz->device, &dev_attr_throttle_policy); > > > > + if (ret) > > > > + dev_err(&tz->device, "creating policy attr failed:%d\n", > > > > ret); > > > > + > > > > + return ret; > > > > } > > > > > > What does this mean? > > > We will not create "policy" attributes if there is no > > thermal_zone_params? > > > > Yes, that's what I thought initially. Because if there is no > > 'throttle_policy' > > we assume that it is (by default) step_wise. > > > > But, if we make tz_params be provided through tzd_register function > > call, it makes sense for this to be a mandatory attribute, showing > > 'step_wise" > > if there is no thermal_zone_params. > > > > IMO, every thermal zone should have a policy. And they can be changed > anytime if user wants to. Agree with you on the first part. Not sure if we want this to be writable. Thanks, Durga -- 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
> > +static ssize_t > > +policy_show(struct device *dev, struct device_attribute *devattr, char > > +*buf) { > > + struct thermal_zone_device *tz = to_thermal_zone(dev); > > + struct thermal_zone_params *tzp = tz->tzp; > > + > > + if (!tzp) > > + return sprintf(buf, "step_wise(default)\n"); > > + > > + switch (tzp->throttle_policy) { > > + case THERMAL_FAIR_SHARE: > > + return sprintf(buf, "fair_share\n"); > > + case THERMAL_STEP_WISE: > > + return sprintf(buf, "step_wise\n"); > > + case THERMAL_USER_SPACE: > > + return sprintf(buf, "user_space\n"); > > + default: > > + return sprintf(buf, "unknown\n"); > > + } > > +} > > + > > static DEVICE_ATTR(type, 0444, type_show, NULL); static > > DEVICE_ATTR(temp, 0444, temp_show, NULL); static DEVICE_ATTR(mode, > > 0644, mode_show, mode_store); static DEVICE_ATTR(passive, S_IRUGO | > > S_IWUSR, passive_show, passive_store); > > +static DEVICE_ATTR(throttle_policy, S_IRUGO, policy_show, NULL); > > > > /* sys I/F for cooling device */ > > #define to_cooling_device(_dev) \ > > @@ -1349,10 +1371,16 @@ static int retrieve_zone_params(struct > > thermal_zone_device *tz) > > > > /* It is not an error to not have any platform data */ > > ret = get_platform_thermal_params(tz); > > - if (ret) > > + if (ret) { > > tz->tzp = NULL; > > + return 0; > > + } > > > > - return 0; > > + ret = device_create_file(&tz->device, &dev_attr_throttle_policy); > > + if (ret) > > + dev_err(&tz->device, "creating policy attr failed:%d\n", > > ret); > > + > > + return ret; > > } > > > > /** > > We should remove this attribute in thermal_zone_device_unregister(); Oh yes, missed it :-( Will fix in v3.. Thanks for the catch, Durga -- 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
> -----Original Message----- > From: R, Durgadoss > Sent: Monday, August 27, 2012 1:24 PM > To: Zhang, Rui; lenb@kernel.org > Cc: linux-acpi@vger.kernel.org; eduardo.valentin@ti.com > Subject: RE: [PATCHv2 06/14] Thermal: Add a policy sysfs attribute > Importance: High > > > > > > #define to_cooling_device(_dev) \ > > > > > @@ -1349,10 +1371,16 @@ static int retrieve_zone_params(struct > > > > > thermal_zone_device *tz) > > > > > > > > > > /* It is not an error to not have any platform data */ > > > > > ret = get_platform_thermal_params(tz); > > > > > - if (ret) > > > > > + if (ret) { > > > > > tz->tzp = NULL; > > > > > + return 0; > > > > > + } > > > > > > > > > > - return 0; > > > > > + ret = device_create_file(&tz->device, > &dev_attr_throttle_policy); > > > > > + if (ret) > > > > > + dev_err(&tz->device, "creating policy attr > failed:%d\n", > > > > > ret); > > > > > + > > > > > + return ret; > > > > > } > > > > > > > > What does this mean? > > > > We will not create "policy" attributes if there is no > > > thermal_zone_params? > > > > > > Yes, that's what I thought initially. Because if there is no > > > 'throttle_policy' > > > we assume that it is (by default) step_wise. > > > > > > But, if we make tz_params be provided through tzd_register function > > > call, it makes sense for this to be a mandatory attribute, showing > > > 'step_wise" > > > if there is no thermal_zone_params. > > > > > > > IMO, every thermal zone should have a policy. And they can be changed > > anytime if user wants to. > > Agree with you on the first part. Not sure if we want this to be > writable. > Say, what if a user space application is loaded and want to take control of the thermal management from kernel? It should set the policy to "userspace" to stop the kernel actions first. BTW, just like the cpufreq governors, they can be changed any time. > Thanks, > Durga -- 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
diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c index 6adda39..8aa4200a6 100644 --- a/drivers/thermal/thermal_sys.c +++ b/drivers/thermal/thermal_sys.c @@ -378,10 +378,32 @@ passive_show(struct device *dev, struct device_attribute *attr, return sprintf(buf, "%d\n", tz->forced_passive); } +static ssize_t +policy_show(struct device *dev, struct device_attribute *devattr, char *buf) +{ + struct thermal_zone_device *tz = to_thermal_zone(dev); + struct thermal_zone_params *tzp = tz->tzp; + + if (!tzp) + return sprintf(buf, "step_wise(default)\n"); + + switch (tzp->throttle_policy) { + case THERMAL_FAIR_SHARE: + return sprintf(buf, "fair_share\n"); + case THERMAL_STEP_WISE: + return sprintf(buf, "step_wise\n"); + case THERMAL_USER_SPACE: + return sprintf(buf, "user_space\n"); + default: + return sprintf(buf, "unknown\n"); + } +} + static DEVICE_ATTR(type, 0444, type_show, NULL); static DEVICE_ATTR(temp, 0444, temp_show, NULL); static DEVICE_ATTR(mode, 0644, mode_show, mode_store); static DEVICE_ATTR(passive, S_IRUGO | S_IWUSR, passive_show, passive_store); +static DEVICE_ATTR(throttle_policy, S_IRUGO, policy_show, NULL); /* sys I/F for cooling device */ #define to_cooling_device(_dev) \ @@ -1349,10 +1371,16 @@ static int retrieve_zone_params(struct thermal_zone_device *tz) /* It is not an error to not have any platform data */ ret = get_platform_thermal_params(tz); - if (ret) + if (ret) { tz->tzp = NULL; + return 0; + } - return 0; + ret = device_create_file(&tz->device, &dev_attr_throttle_policy); + if (ret) + dev_err(&tz->device, "creating policy attr failed:%d\n", ret); + + return ret; } /**
This patch adds a policy sysfs attribute to a thermal zone. This attribute will give us the throttling policy used for the zone. This is a RO attribute. Signed-off-by: Durgadoss R <durgadoss.r@intel.com> --- drivers/thermal/thermal_sys.c | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-)