thermal: Avoid CONFIG_NET compile dependency

thermal: Avoid CONFIG_NET compile dependency

[R, Durgadoss]

Hi Len,

This patch from Thomas fixes the compile dependency.
Looks fine as far as I have tested.

Could you please apply it on top of the old patch ?

Thanks,
Durga
---
thermal: Avoid CONFIG_NET compile dependency

Tested-by: Durgadoss R <durgadoss.r@intel.com>
Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: R.Durgadoss <durgadoss.r@intel.com>
CC: Len Brown <len.brown@intel.com>

---
 drivers/thermal/Kconfig       |    1 -
 drivers/thermal/thermal_sys.c |  177 ++++++++++++++++++++++-------------------
 include/linux/thermal.h       |    5 +-
 3 files changed, 98 insertions(+), 85 deletions(-)

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index f7a5dba..bf7c687 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -4,7 +4,6 @@
 
 menuconfig THERMAL
 	tristate "Generic Thermal sysfs driver"
-	depends on NET
 	help
 	  Generic Thermal Sysfs driver offers a generic mechanism for
 	  thermal management. Usually it's made up of one or more thermal
diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 7d0e63c..5bbacff 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -32,8 +32,6 @@
 #include <linux/thermal.h>
 #include <linux/spinlock.h>
 #include <linux/reboot.h>
-#include <net/netlink.h>
-#include <net/genetlink.h>
 
 MODULE_AUTHOR("Zhang Rui");
 MODULE_DESCRIPTION("Generic thermal management sysfs support");
@@ -60,6 +58,10 @@ static LIST_HEAD(thermal_tz_list);
 static LIST_HEAD(thermal_cdev_list);
 static DEFINE_MUTEX(thermal_list_lock);
 
+#ifdef CONFIG_NET /* needed for netlink messages */
+#include <net/netlink.h>
+#include <net/genetlink.h>
+
 static unsigned int thermal_event_seqnum;
 
 static struct genl_family thermal_event_genl_family = {
@@ -76,6 +78,96 @@ static struct genl_multicast_group thermal_event_mcgrp = {
 static int genetlink_init(void);
 static void genetlink_exit(void);
 
+int generate_netlink_event(u32 orig, enum events event)
+{
+	struct sk_buff *skb;
+	struct nlattr *attr;
+	struct thermal_genl_event *thermal_event;
+	void *msg_header;
+	int size;
+	int result;
+
+	/* allocate memory */
+	size = nla_total_size(sizeof(struct thermal_genl_event)) + \
+				nla_total_size(0);
+
+	skb = genlmsg_new(size, GFP_ATOMIC);
+	if (!skb)
+		return -ENOMEM;
+
+	/* add the genetlink message header */
+	msg_header = genlmsg_put(skb, 0, thermal_event_seqnum++,
+				 &thermal_event_genl_family, 0,
+				 THERMAL_GENL_CMD_EVENT);
+	if (!msg_header) {
+		nlmsg_free(skb);
+		return -ENOMEM;
+	}
+
+	/* fill the data */
+	attr = nla_reserve(skb, THERMAL_GENL_ATTR_EVENT, \
+			sizeof(struct thermal_genl_event));
+
+	if (!attr) {
+		nlmsg_free(skb);
+		return -EINVAL;
+	}
+
+	thermal_event = nla_data(attr);
+	if (!thermal_event) {
+		nlmsg_free(skb);
+		return -EINVAL;
+	}
+
+	memset(thermal_event, 0, sizeof(struct thermal_genl_event));
+
+	thermal_event->orig = orig;
+	thermal_event->event = event;
+
+	/* send multicast genetlink message */
+	result = genlmsg_end(skb, msg_header);
+	if (result < 0) {
+		nlmsg_free(skb);
+		return result;
+	}
+
+	result = genlmsg_multicast(skb, 0, thermal_event_mcgrp.id, GFP_ATOMIC);
+	if (result)
+		printk(KERN_INFO "failed to send netlink event:%d", result);
+
+	return result;
+}
+EXPORT_SYMBOL(generate_netlink_event);
+
+static int genetlink_init(void)
+{
+	int result;
+
+	result = genl_register_family(&thermal_event_genl_family);
+	if (result)
+		return result;
+
+	result = genl_register_mc_group(&thermal_event_genl_family,
+					&thermal_event_mcgrp);
+	if (result)
+		genl_unregister_family(&thermal_event_genl_family);
+	return result;
+}
+
+static void genetlink_exit(void)
+{
+	genl_unregister_family(&thermal_event_genl_family);
+}
+
+#else
+
+static void genetlink_exit(void) {};
+static int genetlink_init(void) { return 0; }
+int generate_netlink_event(u32 orig, enum events event) { return 0; }
+EXPORT_SYMBOL(generate_netlink_event);
+
+#endif /* CONFIG_NET */
+
 static int get_idr(struct idr *idr, struct mutex *lock, int *id)
 {
 	int err;
@@ -1225,82 +1317,6 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 
 EXPORT_SYMBOL(thermal_zone_device_unregister);
 
-int generate_netlink_event(u32 orig, enum events event)
-{
-	struct sk_buff *skb;
-	struct nlattr *attr;
-	struct thermal_genl_event *thermal_event;
-	void *msg_header;
-	int size;
-	int result;
-
-	/* allocate memory */
-	size = nla_total_size(sizeof(struct thermal_genl_event)) + \
-				nla_total_size(0);
-
-	skb = genlmsg_new(size, GFP_ATOMIC);
-	if (!skb)
-		return -ENOMEM;
-
-	/* add the genetlink message header */
-	msg_header = genlmsg_put(skb, 0, thermal_event_seqnum++,
-				 &thermal_event_genl_family, 0,
-				 THERMAL_GENL_CMD_EVENT);
-	if (!msg_header) {
-		nlmsg_free(skb);
-		return -ENOMEM;
-	}
-
-	/* fill the data */
-	attr = nla_reserve(skb, THERMAL_GENL_ATTR_EVENT, \
-			sizeof(struct thermal_genl_event));
-
-	if (!attr) {
-		nlmsg_free(skb);
-		return -EINVAL;
-	}
-
-	thermal_event = nla_data(attr);
-	if (!thermal_event) {
-		nlmsg_free(skb);
-		return -EINVAL;
-	}
-
-	memset(thermal_event, 0, sizeof(struct thermal_genl_event));
-
-	thermal_event->orig = orig;
-	thermal_event->event = event;
-
-	/* send multicast genetlink message */
-	result = genlmsg_end(skb, msg_header);
-	if (result < 0) {
-		nlmsg_free(skb);
-		return result;
-	}
-
-	result = genlmsg_multicast(skb, 0, thermal_event_mcgrp.id, GFP_ATOMIC);
-	if (result)
-		printk(KERN_INFO "failed to send netlink event:%d", result);
-
-	return result;
-}
-EXPORT_SYMBOL(generate_netlink_event);
-
-static int genetlink_init(void)
-{
-	int result;
-
-	result = genl_register_family(&thermal_event_genl_family);
-	if (result)
-		return result;
-
-	result = genl_register_mc_group(&thermal_event_genl_family,
-					&thermal_event_mcgrp);
-	if (result)
-		genl_unregister_family(&thermal_event_genl_family);
-	return result;
-}
-
 static int __init thermal_init(void)
 {
 	int result = 0;
@@ -1316,11 +1332,6 @@ static int __init thermal_init(void)
 	return result;
 }
 
-static void genetlink_exit(void)
-{
-	genl_unregister_family(&thermal_event_genl_family);
-}
-
 static void __exit thermal_exit(void)
 {
 	class_unregister(&thermal_class);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 8651556..1c31614 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -127,6 +127,8 @@ struct thermal_zone_device {
 	struct thermal_hwmon_attr temp_crit;	/* hwmon sys attr */
 #endif
 };
+
+#ifdef CONFIG_NET
 /* Adding event notification support elements */
 #define THERMAL_GENL_FAMILY_NAME                "thermal_event"
 #define THERMAL_GENL_VERSION                    0x01
@@ -158,6 +160,8 @@ enum {
 	__THERMAL_GENL_CMD_MAX,
 };
 #define THERMAL_GENL_CMD_MAX (__THERMAL_GENL_CMD_MAX - 1)
+#endif
+extern int generate_netlink_event(u32 orig, enum events event);
 
 struct thermal_zone_device *thermal_zone_device_register(char *, int, void *,
 		const struct thermal_zone_device_ops *, int tc1, int tc2,
@@ -172,6 +176,5 @@ void thermal_zone_device_update(struct thermal_zone_device *);
 struct thermal_cooling_device *thermal_cooling_device_register(char *, void *,
 		const struct thermal_cooling_device_ops *);
 void thermal_cooling_device_unregister(struct thermal_cooling_device *);
-extern int generate_netlink_event(u32 orig, enum events event);
 
 #endif /* __THERMAL_H__ */

Re: thermal: Avoid CONFIG_NET compile dependency

[Thomas Renninger]

On Friday, January 21, 2011 11:52:39 AM R, Durgadoss wrote:
> Hi Len,
> 
> This patch from Thomas fixes the compile dependency.
> Looks fine as far as I have tested.
> 
> Could you please apply it on top of the old patch ?
I tried to not export thermal_netlink_event in #ifndef CONFIG_NET
case, but as it uses an enum which should only exist in CONFIG_NET
case it doesn't work out that easy...

Therefore please apply this one as it is.

Thanks,

   Thomas

Re: thermal: Avoid CONFIG_NET compile dependency

[Zhang Rui]

the "Rename exported func generate_netlink_event to
thermal_netlink_event" patch is on top of this one.

you'd better either send them in one patch series, or state this in the
email. :)

On Fri, 2011-01-21 at 18:52 +0800, R, Durgadoss wrote:
> Hi Len,
> 
> This patch from Thomas fixes the compile dependency.
> Looks fine as far as I have tested.
> 
> Could you please apply it on top of the old patch ?
> 
> Thanks,
> Durga
> ---
> thermal: Avoid CONFIG_NET compile dependency
> 
> Tested-by: Durgadoss R <durgadoss.r@intel.com>
> Signed-off-by: Thomas Renninger <trenn@suse.de>
> CC: R.Durgadoss <durgadoss.r@intel.com>
> CC: Len Brown <len.brown@intel.com>
> 
Acked-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui

> ---
>  drivers/thermal/Kconfig       |    1 -
>  drivers/thermal/thermal_sys.c |  177 ++++++++++++++++++++++-------------------
>  include/linux/thermal.h       |    5 +-
>  3 files changed, 98 insertions(+), 85 deletions(-)
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index f7a5dba..bf7c687 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -4,7 +4,6 @@
>  
>  menuconfig THERMAL
>  	tristate "Generic Thermal sysfs driver"
> -	depends on NET
>  	help
>  	  Generic Thermal Sysfs driver offers a generic mechanism for
>  	  thermal management. Usually it's made up of one or more thermal
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 7d0e63c..5bbacff 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -32,8 +32,6 @@
>  #include <linux/thermal.h>
>  #include <linux/spinlock.h>
>  #include <linux/reboot.h>
> -#include <net/netlink.h>
> -#include <net/genetlink.h>
>  
>  MODULE_AUTHOR("Zhang Rui");
>  MODULE_DESCRIPTION("Generic thermal management sysfs support");
> @@ -60,6 +58,10 @@ static LIST_HEAD(thermal_tz_list);
>  static LIST_HEAD(thermal_cdev_list);
>  static DEFINE_MUTEX(thermal_list_lock);
>  
> +#ifdef CONFIG_NET /* needed for netlink messages */
> +#include <net/netlink.h>
> +#include <net/genetlink.h>
> +
>  static unsigned int thermal_event_seqnum;
>  
>  static struct genl_family thermal_event_genl_family = {
> @@ -76,6 +78,96 @@ static struct genl_multicast_group thermal_event_mcgrp = {
>  static int genetlink_init(void);
>  static void genetlink_exit(void);
>  
> +int generate_netlink_event(u32 orig, enum events event)

> +{
> +	struct sk_buff *skb;
> +	struct nlattr *attr;
> +	struct thermal_genl_event *thermal_event;
> +	void *msg_header;
> +	int size;
> +	int result;
> +
> +	/* allocate memory */
> +	size = nla_total_size(sizeof(struct thermal_genl_event)) + \
> +				nla_total_size(0);
> +
> +	skb = genlmsg_new(size, GFP_ATOMIC);
> +	if (!skb)
> +		return -ENOMEM;
> +
> +	/* add the genetlink message header */
> +	msg_header = genlmsg_put(skb, 0, thermal_event_seqnum++,
> +				 &thermal_event_genl_family, 0,
> +				 THERMAL_GENL_CMD_EVENT);
> +	if (!msg_header) {
> +		nlmsg_free(skb);
> +		return -ENOMEM;
> +	}
> +
> +	/* fill the data */
> +	attr = nla_reserve(skb, THERMAL_GENL_ATTR_EVENT, \
> +			sizeof(struct thermal_genl_event));
> +
> +	if (!attr) {
> +		nlmsg_free(skb);
> +		return -EINVAL;
> +	}
> +
> +	thermal_event = nla_data(attr);
> +	if (!thermal_event) {
> +		nlmsg_free(skb);
> +		return -EINVAL;
> +	}
> +
> +	memset(thermal_event, 0, sizeof(struct thermal_genl_event));
> +
> +	thermal_event->orig = orig;
> +	thermal_event->event = event;
> +
> +	/* send multicast genetlink message */
> +	result = genlmsg_end(skb, msg_header);
> +	if (result < 0) {
> +		nlmsg_free(skb);
> +		return result;
> +	}
> +
> +	result = genlmsg_multicast(skb, 0, thermal_event_mcgrp.id, GFP_ATOMIC);
> +	if (result)
> +		printk(KERN_INFO "failed to send netlink event:%d", result);
> +
> +	return result;
> +}
> +EXPORT_SYMBOL(generate_netlink_event);
> +
> +static int genetlink_init(void)
> +{
> +	int result;
> +
> +	result = genl_register_family(&thermal_event_genl_family);
> +	if (result)
> +		return result;
> +
> +	result = genl_register_mc_group(&thermal_event_genl_family,
> +					&thermal_event_mcgrp);
> +	if (result)
> +		genl_unregister_family(&thermal_event_genl_family);
> +	return result;
> +}
> +
> +static void genetlink_exit(void)
> +{
> +	genl_unregister_family(&thermal_event_genl_family);
> +}
> +
> +#else
> +
> +static void genetlink_exit(void) {};
> +static int genetlink_init(void) { return 0; }
> +int generate_netlink_event(u32 orig, enum events event) { return 0; }
> +EXPORT_SYMBOL(generate_netlink_event);
> +
> +#endif /* CONFIG_NET */
> +
>  static int get_idr(struct idr *idr, struct mutex *lock, int *id)
>  {
>  	int err;
> @@ -1225,82 +1317,6 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>  
>  EXPORT_SYMBOL(thermal_zone_device_unregister);
>  
> -int generate_netlink_event(u32 orig, enum events event)
> -{
> -	struct sk_buff *skb;
> -	struct nlattr *attr;
> -	struct thermal_genl_event *thermal_event;
> -	void *msg_header;
> -	int size;
> -	int result;
> -
> -	/* allocate memory */
> -	size = nla_total_size(sizeof(struct thermal_genl_event)) + \
> -				nla_total_size(0);
> -
> -	skb = genlmsg_new(size, GFP_ATOMIC);
> -	if (!skb)
> -		return -ENOMEM;
> -
> -	/* add the genetlink message header */
> -	msg_header = genlmsg_put(skb, 0, thermal_event_seqnum++,
> -				 &thermal_event_genl_family, 0,
> -				 THERMAL_GENL_CMD_EVENT);
> -	if (!msg_header) {
> -		nlmsg_free(skb);
> -		return -ENOMEM;
> -	}
> -
> -	/* fill the data */
> -	attr = nla_reserve(skb, THERMAL_GENL_ATTR_EVENT, \
> -			sizeof(struct thermal_genl_event));
> -
> -	if (!attr) {
> -		nlmsg_free(skb);
> -		return -EINVAL;
> -	}
> -
> -	thermal_event = nla_data(attr);
> -	if (!thermal_event) {
> -		nlmsg_free(skb);
> -		return -EINVAL;
> -	}
> -
> -	memset(thermal_event, 0, sizeof(struct thermal_genl_event));
> -
> -	thermal_event->orig = orig;
> -	thermal_event->event = event;
> -
> -	/* send multicast genetlink message */
> -	result = genlmsg_end(skb, msg_header);
> -	if (result < 0) {
> -		nlmsg_free(skb);
> -		return result;
> -	}
> -
> -	result = genlmsg_multicast(skb, 0, thermal_event_mcgrp.id, GFP_ATOMIC);
> -	if (result)
> -		printk(KERN_INFO "failed to send netlink event:%d", result);
> -
> -	return result;
> -}
> -EXPORT_SYMBOL(generate_netlink_event);
> -
> -static int genetlink_init(void)
> -{
> -	int result;
> -
> -	result = genl_register_family(&thermal_event_genl_family);
> -	if (result)
> -		return result;
> -
> -	result = genl_register_mc_group(&thermal_event_genl_family,
> -					&thermal_event_mcgrp);
> -	if (result)
> -		genl_unregister_family(&thermal_event_genl_family);
> -	return result;
> -}
> -
>  static int __init thermal_init(void)
>  {
>  	int result = 0;
> @@ -1316,11 +1332,6 @@ static int __init thermal_init(void)
>  	return result;
>  }
>  
> -static void genetlink_exit(void)
> -{
> -	genl_unregister_family(&thermal_event_genl_family);
> -}
> -
>  static void __exit thermal_exit(void)
>  {
>  	class_unregister(&thermal_class);
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 8651556..1c31614 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -127,6 +127,8 @@ struct thermal_zone_device {
>  	struct thermal_hwmon_attr temp_crit;	/* hwmon sys attr */
>  #endif
>  };
> +
> +#ifdef CONFIG_NET
>  /* Adding event notification support elements */
>  #define THERMAL_GENL_FAMILY_NAME                "thermal_event"
>  #define THERMAL_GENL_VERSION                    0x01
> @@ -158,6 +160,8 @@ enum {
>  	__THERMAL_GENL_CMD_MAX,
>  };
>  #define THERMAL_GENL_CMD_MAX (__THERMAL_GENL_CMD_MAX - 1)
> +#endif
> +extern int generate_netlink_event(u32 orig, enum events event);
>  
>  struct thermal_zone_device *thermal_zone_device_register(char *, int, void *,
>  		const struct thermal_zone_device_ops *, int tc1, int tc2,
> @@ -172,6 +176,5 @@ void thermal_zone_device_update(struct thermal_zone_device *);
>  struct thermal_cooling_device *thermal_cooling_device_register(char *, void *,
>  		const struct thermal_cooling_device_ops *);
>  void thermal_cooling_device_unregister(struct thermal_cooling_device *);
> -extern int generate_netlink_event(u32 orig, enum events event);
>  
>  #endif /* __THERMAL_H__ */
> --
> 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

Re: thermal: Avoid CONFIG_NET compile dependency

[Zhang Rui]

On Fri, 2011-01-21 at 20:12 +0800, Thomas Renninger wrote:
> On Friday, January 21, 2011 11:52:39 AM R, Durgadoss wrote:
> > Hi Len,
> > 
> > This patch from Thomas fixes the compile dependency.
> > Looks fine as far as I have tested.
> > 
> > Could you please apply it on top of the old patch ?
> I tried to not export thermal_netlink_event in #ifndef CONFIG_NET
> case, but as it uses an enum which should only exist in CONFIG_NET
> case it doesn't work out that easy...
> 
oh, right.

IMO,
enum events {
       THERMAL_AUX0,
       THERMAL_AUX1,
       THERMAL_CRITICAL,
       THERMAL_DEV_FAULT,
};
should not be included in the ifdef case.

BTW: Durgadoss, what does THERMAL_AUX0 and THERMAL_AUX1 mean?
Sorry I missed this in the original patch, but these two events do not
look like something the generic thermal driver knows.

If you really need these kind of events, I'd prefer something like this:
enum events {
	THERMAL_CRITICAL,
	/* user defined thermal events */
	TEHRMAL_USER0,
	TEHRMAL_USER1,
	THERMAL_DEV_FAULT,
};

or
enum events {
	THERMAL_CRITICAL,
	/* user defined thermal events */
	TEHRMAL_USER_AUX0,
	TEHRMAL_USER_AUX1,
	THERMAL_DEV_FAULT,
};


> Therefore please apply this one as it is.
> 
which one? do I miss something again? :)

thanks,
rui

> Thanks,
> 
>    Thomas
> --
> 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

RE: thermal: Avoid CONFIG_NET compile dependency

[R, Durgadoss]

Hi Rui,

> 
> IMO,
> enum events {
>        THERMAL_AUX0,
>        THERMAL_AUX1,
>        THERMAL_CRITICAL,
>        THERMAL_DEV_FAULT,
> };
> should not be included in the ifdef case.
> 
> BTW: Durgadoss, what does THERMAL_AUX0 and THERMAL_AUX1 mean?
> Sorry I missed this in the original patch, but these two events do not
> look like something the generic thermal driver knows.
> 
> If you really need these kind of events, I'd prefer something like this:
> enum events {
> 	THERMAL_CRITICAL,
> 	/* user defined thermal events */
> 	TEHRMAL_USER0,
> 	TEHRMAL_USER1,
> 	THERMAL_DEV_FAULT,
> };
> 
> or
> enum events {
> 	THERMAL_CRITICAL,
> 	/* user defined thermal events */
> 	TEHRMAL_USER_AUX0,
> 	TEHRMAL_USER_AUX1,
> 	THERMAL_DEV_FAULT,
> };
> 
> 
> > Therefore please apply this one as it is.
> >
> which one? do I miss something again? :)
> 

This refers to the patch that you acknowledged.
So, you are not missing any patch in the middle.

Also, the thermal_aux0 and _aux1, we can use the final format specified by you.
enum events {
 	THERMAL_CRITICAL,
 	/* user defined thermal events */
 	THERMAL_USER_AUX0,
 	THERMAL_USER_AUX1,
 	THERMAL_DEV_FAULT,
 };
Thanks for the suggestion, Rui.

Regards,
Durga

Re: thermal: Avoid CONFIG_NET compile dependency

[Thomas Renninger]

Hi,

On Monday, January 24, 2011 05:39:26 AM R, Durgadoss wrote:
> Hi Rui,
> 
...
> This refers to the patch that you acknowledged.
> So, you are not missing any patch in the middle.
> 
> Also, the thermal_aux0 and _aux1, we can use the final format specified by you.
> enum events {
>  	THERMAL_CRITICAL,
>  	/* user defined thermal events */
>  	THERMAL_USER_AUX0,
>  	THERMAL_USER_AUX1,
>  	THERMAL_DEV_FAULT,
>  };
Something else...
I've now seen your HW specific core/pkg temp threshold support.
I didn't look at it that closely, but if you introduce generic
thermal netlink events in the thermal driver those should match
all kind of possible thermal events, not only the once introduced
by the driver you added.

For example it would be great to get the ACPI specific thermal
events which are currently exported as ACPI driven ones:
drivers/acpi/thermal.c:acpi_thermal_notify()
integrated there as well. So there should also be events like:
>  	THERMAL_CRITICAL,
>  	/* user defined thermal events */
>  	THERMAL_USER_AUX0,
>  	THERMAL_USER_AUX1,
>  	THERMAL_DEV_FAULT,
THERMAL_PASSIVE
THERMAL_ACTIVE
THERMAL_HOT

At some point of time we should be able
to get rid of the ACPI specific thermal events at all?
There seem to be nothing like a kind of
"available netlink events and their format" API in Documentation.
Each driver seem to do this and document it itself.
But as it is an interface to userspace, these thermal netlink events
should get documented in:
Documentation/thermal

Currently you only export one of the above 4 thermal events as a
number via netlink, right?
Are there any userspace programs you have in mind which should make
use of it?

Some data which should get exported as well:
   - The temperature if available
   - The number of the thermal zone, e.g. if 0, userspace can look
     up sysfs for further info in:
     /sys/devices/virtual/thermal/thermal_zone0/*
   - The trip point affected if any, e.g. if 0, userspace can look it up
     in sysfs for further info:
     /sys/devices/virtual/thermal/thermal_zone0/trip_point_0_{temp,type}
   - more?

Ehh, this core and package thermal driver you added threshold support,
does this somehow register as a thermal driver?
I can see that it registers for hwmon: hwmon_device_register(&pdev->dev);
But is it somehow connected to thermal_sys.c other than that it just
throws your newly introduced thermal_netlink_events?
This driver should first register as a thermal driver, export
trip points to:
/sys/devices/virtual/thermal/thermal_zone0/trip_point_0_{temp,type}
e.g.
cat trip_point_0_type
critical
cat trip_point_1_type
user_aux0

The thermal_netlink_events could then only be declared for
thermal_sys.c scope, so that other drivers cannot misuse them
and the netlink event can then get triggered by the driver through
thermal driver callbacks.

Rui: What do you think?
This implementation looks much too static, could you help to better
adjust it to the generic thermal driver needs, you know this stuff best.

This is merged already. I wonder whether we still could get a thermal
event netlink API for 2.6.38 which is more robust so that userspace does
not need to differ its version with the next kernel.
Maybe it's best to remove the thermal netlink parts again for now.


     Thomas

Thermal kernel events API to userspace - Was: Re: thermal: Avoid CONFIG_NET compile dependency

[Thomas Renninger]

Hi,

I wonder whether netlink is the way to go for thermal
events at all.
Sending an udev event would already contain the sysfs
path to the thermal device. A variable which thermal event
got thrown could get added and userspace can read out the rest
easily from sysfs files. But I expect udev is not intended
for such general events?

There is a lot talking about perf events...
perf events are/were for debugging as they get exported
through /sys/kernel/debug.
Recently people came up with the idea to add a perf event API
for MCE (Machine Check Exceptions) events which clearly is nothing
for debugging, but productive applications want to catch
and process them.
So either MCEs should better get a kind of netlink/ioctl or
whatever (not depending on debugging) interface to userspace
or thermal events might also fit as perf events.

This should get discussed by a broader audience, adding some
people to CC.

    Thomas


On Monday, January 24, 2011 11:35:23 AM Thomas Renninger wrote:
> Hi,
> 
> On Monday, January 24, 2011 05:39:26 AM R, Durgadoss wrote:
> > Hi Rui,
> > 
> ...
> > This refers to the patch that you acknowledged.
> > So, you are not missing any patch in the middle.
> > 
> > Also, the thermal_aux0 and _aux1, we can use the final format specified by you.
> > enum events {
> >  	THERMAL_CRITICAL,
> >  	/* user defined thermal events */
> >  	THERMAL_USER_AUX0,
> >  	THERMAL_USER_AUX1,
> >  	THERMAL_DEV_FAULT,
> >  };
> Something else...
> I've now seen your HW specific core/pkg temp threshold support.
> I didn't look at it that closely, but if you introduce generic
> thermal netlink events in the thermal driver those should match
> all kind of possible thermal events, not only the once introduced
> by the driver you added.
> 
> For example it would be great to get the ACPI specific thermal
> events which are currently exported as ACPI driven ones:
> drivers/acpi/thermal.c:acpi_thermal_notify()
> integrated there as well. So there should also be events like:
> >  	THERMAL_CRITICAL,
> >  	/* user defined thermal events */
> >  	THERMAL_USER_AUX0,
> >  	THERMAL_USER_AUX1,
> >  	THERMAL_DEV_FAULT,
> THERMAL_PASSIVE
> THERMAL_ACTIVE
> THERMAL_HOT
> 
> At some point of time we should be able
> to get rid of the ACPI specific thermal events at all?
> There seem to be nothing like a kind of
> "available netlink events and their format" API in Documentation.
> Each driver seem to do this and document it itself.
> But as it is an interface to userspace, these thermal netlink events
> should get documented in:
> Documentation/thermal
> 
> Currently you only export one of the above 4 thermal events as a
> number via netlink, right?
> Are there any userspace programs you have in mind which should make
> use of it?
> 
> Some data which should get exported as well:
>    - The temperature if available
>    - The number of the thermal zone, e.g. if 0, userspace can look
>      up sysfs for further info in:
>      /sys/devices/virtual/thermal/thermal_zone0/*
>    - The trip point affected if any, e.g. if 0, userspace can look it up
>      in sysfs for further info:
>      /sys/devices/virtual/thermal/thermal_zone0/trip_point_0_{temp,type}
>    - more?
> 
> Ehh, this core and package thermal driver you added threshold support,
> does this somehow register as a thermal driver?
> I can see that it registers for hwmon: hwmon_device_register(&pdev->dev);
> But is it somehow connected to thermal_sys.c other than that it just
> throws your newly introduced thermal_netlink_events?
> This driver should first register as a thermal driver, export
> trip points to:
> /sys/devices/virtual/thermal/thermal_zone0/trip_point_0_{temp,type}
> e.g.
> cat trip_point_0_type
> critical
> cat trip_point_1_type
> user_aux0
> 
> The thermal_netlink_events could then only be declared for
> thermal_sys.c scope, so that other drivers cannot misuse them
> and the netlink event can then get triggered by the driver through
> thermal driver callbacks.
> 
> Rui: What do you think?
> This implementation looks much too static, could you help to better
> adjust it to the generic thermal driver needs, you know this stuff best.
> 
> This is merged already. I wonder whether we still could get a thermal
> event netlink API for 2.6.38 which is more robust so that userspace does
> not need to differ its version with the next kernel.
> Maybe it's best to remove the thermal netlink parts again for now.
> 
> 
>      Thomas

Re: Thermal kernel events API to userspace - Was: Re: thermal: Avoid CONFIG_NET compile dependency

[Henrique de Moraes Holschuh]

On Mon, 24 Jan 2011, Thomas Renninger wrote:
> I wonder whether netlink is the way to go for thermal
> events at all.
> Sending an udev event would already contain the sysfs
> path to the thermal device. A variable which thermal event
> got thrown could get added and userspace can read out the rest
> easily from sysfs files. But I expect udev is not intended
> for such general events?

udev is heavyweight in the userspace side, we'd be much better off using the
ACPI event interface (which is netlink), or a new one to deliver system
status events, instead of continously abusing udev for this stuff.

> > > Also, the thermal_aux0 and _aux1, we can use the final format specified by you.
> > > enum events {
> > >  	THERMAL_CRITICAL,
> > >  	/* user defined thermal events */
> > >  	THERMAL_USER_AUX0,
> > >  	THERMAL_USER_AUX1,
> > >  	THERMAL_DEV_FAULT,
> > >  };

Please give us at least two levels of thermal alarm: critical and emergency
(or warning and critical -- it doesn't matter much, as long as there are at
least two levels, and which one comes first is defined by the
specification).  I'd have immediate use for them on thinkpads.

It is probably best to have three levels (warning, critical, emergency).
Best not to tie the API/ABI to the notion of "too hot", one can also alarm
when it starts to get to cold.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

RE: thermal: Avoid CONFIG_NET compile dependency

[R, Durgadoss]

Hi Thomas,

> enum events {
> >  	THERMAL_CRITICAL,
> >  	/* user defined thermal events */
> >  	THERMAL_USER_AUX0,
> >  	THERMAL_USER_AUX1,
> >  	THERMAL_DEV_FAULT,
> >  };
> Something else...
> I've now seen your HW specific core/pkg temp threshold support.
> I didn't look at it that closely, but if you introduce generic
> thermal netlink events in the thermal driver those should match
> all kind of possible thermal events, not only the once introduced
> by the driver you added.
> 
> For example it would be great to get the ACPI specific thermal
> events which are currently exported as ACPI driven ones:
> drivers/acpi/thermal.c:acpi_thermal_notify()
> integrated there as well. So there should also be events like:
> >  	THERMAL_CRITICAL,
> >  	/* user defined thermal events */
> >  	THERMAL_USER_AUX0,
> >  	THERMAL_USER_AUX1,
> >  	THERMAL_DEV_FAULT,
> THERMAL_PASSIVE
> THERMAL_ACTIVE
> THERMAL_HOT
> 

I am fine with adding these events also to the enum here..

> At some point of time we should be able
> to get rid of the ACPI specific thermal events at all?
> There seem to be nothing like a kind of
> "available netlink events and their format" API in Documentation.
> Each driver seem to do this and document it itself.
> But as it is an interface to userspace, these thermal netlink events
> should get documented in:
> Documentation/thermal
> 

I did add a simple documentation to Documentation/thermal/sysfs_api.txt.
This is under 4.Event Notification..But I did not account for _PASSIVE,
_HOT etc.. May if we all agree to merge all events under this one enum,
I can add more description there.

> Currently you only export one of the above 4 thermal events as a
> number via netlink, right?

Right.

> Are there any userspace programs you have in mind which should make
> use of it?

Yes. I am working on a simple user space program to catch these netlink events.

> Some data which should get exported as well:
>    - The temperature if available
>    - The number of the thermal zone, e.g. if 0, userspace can look
>      up sysfs for further info in:
>      /sys/devices/virtual/thermal/thermal_zone0/*
>    - The trip point affected if any, e.g. if 0, userspace can look it up
>      in sysfs for further info:
>      /sys/devices/virtual/thermal/thermal_zone0/trip_point_0_{temp,type}
>    - more?
> 
> Ehh, this core and package thermal driver you added threshold support,
> does this somehow register as a thermal driver?
> I can see that it registers for hwmon: hwmon_device_register(&pdev->dev);
> But is it somehow connected to thermal_sys.c other than that it just
> throws your newly introduced thermal_netlink_events?
> This driver should first register as a thermal driver, export
> trip points to:
> /sys/devices/virtual/thermal/thermal_zone0/trip_point_0_{temp,type}
> e.g.
> cat trip_point_0_type
> critical
> cat trip_point_1_type
> user_aux0
> 
> The thermal_netlink_events could then only be declared for
> thermal_sys.c scope, so that other drivers cannot misuse them
> and the netlink event can then get triggered by the driver through
> thermal driver callbacks.
> 

Yep!! This was my initial idea, with which I added the netlink support here.

If we make the coretemp register with the thermal framework, we have to modify
the sysfs _store and _show functions a bit. Hence, the coretemp people were not
very happy with the idea of making coretemp register with the thermal framework.
So, I just added support to the core thresholds, and when current 
temperature crosses them, it sends a netlink event.

Moreover, I thought by making the coretemp register with the framework,
We can aggregate all thermal related components in one place.
This way, /sys/class/thermal/thermal_zone[0-n] will give information on
every thermal sensor on the system.

The second point to this is, any sensor can send a thermal netlink event,
With a unique id(when any driver registers with the framework..it
gives a unique id). If there is a user land code that can catch netlink events,
it can differentiate the sensors with the id, and take actions based on that.

> Rui: What do you think?
> This implementation looks much too static, could you help to better
> adjust it to the generic thermal driver needs, you know this stuff best.

Rui helped me a lot in cleaning up the netlink code and getting it in.
But to make this approach work, we need support from coretemp code also.

Copying Guenter and Fenghua who maintain the coretemp code.
Guenter and Fenghua, kindly let us know your opinion on this..

Thanks,
Durga

Re: thermal: Avoid CONFIG_NET compile dependency

[Zhang Rui]

On Mon, 2011-01-24 at 18:35 +0800, Thomas Renninger wrote:
> Hi,
> 
> On Monday, January 24, 2011 05:39:26 AM R, Durgadoss wrote:
> > Hi Rui,
> > 
> ...
> > This refers to the patch that you acknowledged.
> > So, you are not missing any patch in the middle.
> > 
> > Also, the thermal_aux0 and _aux1, we can use the final format specified by you.
> > enum events {
> >  	THERMAL_CRITICAL,
> >  	/* user defined thermal events */
> >  	THERMAL_USER_AUX0,
> >  	THERMAL_USER_AUX1,
> >  	THERMAL_DEV_FAULT,
> >  };
> Something else...
> I've now seen your HW specific core/pkg temp threshold support.
> I didn't look at it that closely, but if you introduce generic
> thermal netlink events in the thermal driver those should match
> all kind of possible thermal events, not only the once introduced
> by the driver you added.
> 
> For example it would be great to get the ACPI specific thermal
> events which are currently exported as ACPI driven ones:
> drivers/acpi/thermal.c:acpi_thermal_notify()
> integrated there as well. So there should also be events like:
> >  	THERMAL_CRITICAL,
> >  	/* user defined thermal events */
> >  	THERMAL_USER_AUX0,
> >  	THERMAL_USER_AUX1,
> >  	THERMAL_DEV_FAULT,
> THERMAL_PASSIVE
> THERMAL_ACTIVE
> THERMAL_HOT
> 
Agree.

> At some point of time we should be able
> to get rid of the ACPI specific thermal events at all?

well. I'm not sure about this.
currently, we have acpid that handles all acpi events. And our plan is
to convert acpid to support ACPI genetlink events instead
of /proc/acpi/events. so that this is transparent to user space
applications, e.g. the scripts under /etc/acpi/, that depend on acpid.
And this means we should export ACPI thermal events via the ACPI
genetlink family like we do today.

But on the other side, exporting generic thermal netlink events from
ACPI thermal driver also makes sense. I think this depends on what kind
of user space applications we will have. Ideally, we should have user
space app. that handles the generic thermal netlink events and remove
all the thermal stuff in acpid.

BTW: to be honest, I don't see any scripts in /etc/acpi that handle ACPI
thermal events so far, what about you?

> There seem to be nothing like a kind of
> "available netlink events and their format" API in Documentation.
> Each driver seem to do this and document it itself.
> But as it is an interface to userspace, these thermal netlink events
> should get documented in:
> Documentation/thermal
> 
> Currently you only export one of the above 4 thermal events as a
> number via netlink, right?
> Are there any userspace programs you have in mind which should make
> use of it?
> 
> Some data which should get exported as well:
>    - The temperature if available
>    - The number of the thermal zone, e.g. if 0, userspace can look
>      up sysfs for further info in:
>      /sys/devices/virtual/thermal/thermal_zone0/*
>    - The trip point affected if any, e.g. if 0, userspace can look it up
>      in sysfs for further info:
>      /sys/devices/virtual/thermal/thermal_zone0/trip_point_0_{temp,type}
>    - more?
> 
I think a thermal zone id and an event id is enough.
Applications should rescan /sys/class/power_supply/ to see what is
actually going on. :)

> Ehh, this core and package thermal driver you added threshold support,
> does this somehow register as a thermal driver?
> I can see that it registers for hwmon: hwmon_device_register(&pdev->dev);
> But is it somehow connected to thermal_sys.c other than that it just
> throws your newly introduced thermal_netlink_events?
> This driver should first register as a thermal driver, export
> trip points to:
> /sys/devices/virtual/thermal/thermal_zone0/trip_point_0_{temp,type}
> e.g.
> cat trip_point_0_type
> critical
> cat trip_point_1_type
> user_aux0
> 
> The thermal_netlink_events could then only be declared for
> thermal_sys.c scope, so that other drivers cannot misuse them
> and the netlink event can then get triggered by the driver through
> thermal driver callbacks.
> 
> Rui: What do you think?
> This implementation looks much too static, could you help to better
> adjust it to the generic thermal driver needs, you know this stuff best.
> 
> This is merged already. I wonder whether we still could get a thermal
> event netlink API for 2.6.38 which is more robust so that userspace does
> not need to differ its version with the next kernel.
> Maybe it's best to remove the thermal netlink parts again for now.
> 
a question for Durgadoss,
int thermal_netlink_event(u32 orig, enum events event)
what does parameter "orig" stand for?

If we only need to change the enum event, what about a patch to fix the
enum event first and then shipping the coretemp/pkgtemp driver patch on
top of it?

thanks,
rui
> 
>      Thomas
> --
> 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

Re: Thermal kernel events API to userspace - Was: Re: thermal: Avoid CONFIG_NET compile dependency

[Zhang Rui]

On Tue, 2011-01-25 at 00:07 +0800, Henrique de Moraes Holschuh wrote:
> On Mon, 24 Jan 2011, Thomas Renninger wrote:
> > I wonder whether netlink is the way to go for thermal
> > events at all.
> > Sending an udev event would already contain the sysfs
> > path to the thermal device. A variable which thermal event
> > got thrown could get added and userspace can read out the rest
> > easily from sysfs files. But I expect udev is not intended
> > for such general events?
> 
> udev is heavyweight in the userspace side, we'd be much better off using the
> ACPI event interface (which is netlink), or a new one to deliver system
> status events, instead of continously abusing udev for this stuff.
> 
> > > > Also, the thermal_aux0 and _aux1, we can use the final format specified by you.
> > > > enum events {
> > > >  	THERMAL_CRITICAL,
> > > >  	/* user defined thermal events */
> > > >  	THERMAL_USER_AUX0,
> > > >  	THERMAL_USER_AUX1,
> > > >  	THERMAL_DEV_FAULT,
> > > >  };
> 
> Please give us at least two levels of thermal alarm: critical and emergency
> (or warning and critical -- it doesn't matter much, as long as there are at
> least two levels, and which one comes first is defined by the
> specification).  I'd have immediate use for them on thinkpads.
> 
> It is probably best to have three levels (warning, critical, emergency).
> Best not to tie the API/ABI to the notion of "too hot", one can also alarm
> when it starts to get to cold.
> 
when it's the "too hot" case, what kind of action should be taken upon
the warning/critical/emergency events?
I mean what's the difference between these three levels.

thanks,
rui

Re: Thermal kernel events API to userspace - Was: Re: thermal: Avoid CONFIG_NET compile dependency

[Zhang Rui]

On Tue, 2011-01-25 at 00:07 +0800, Henrique de Moraes Holschuh wrote:
> On Mon, 24 Jan 2011, Thomas Renninger wrote:
> > I wonder whether netlink is the way to go for thermal
> > events at all.
> > Sending an udev event would already contain the sysfs
> > path to the thermal device. A variable which thermal event
> > got thrown could get added and userspace can read out the rest
> > easily from sysfs files. But I expect udev is not intended
> > for such general events?
> 
> udev is heavyweight in the userspace side, we'd be much better off using the
> ACPI event interface (which is netlink), or a new one to deliver system
> status events, instead of continously abusing udev for this stuff.
> 
> > > > Also, the thermal_aux0 and _aux1, we can use the final format specified by you.
> > > > enum events {
> > > >  	THERMAL_CRITICAL,
> > > >  	/* user defined thermal events */
> > > >  	THERMAL_USER_AUX0,
> > > >  	THERMAL_USER_AUX1,
> > > >  	THERMAL_DEV_FAULT,
> > > >  };
> 
> Please give us at least two levels of thermal alarm: critical and emergency
> (or warning and critical -- it doesn't matter much, as long as there are at
> least two levels, and which one comes first is defined by the
> specification).  I'd have immediate use for them on thinkpads.
> 
> It is probably best to have three levels (warning, critical, emergency).
> Best not to tie the API/ABI to the notion of "too hot", one can also alarm
> when it starts to get to cold.
> 
when it's the "too hot" case, what kind of action should be taken upon
the warning/critical/emergency events?
I mean what's the difference between these three levels.

thanks,
rui

RE: thermal: Avoid CONFIG_NET compile dependency

[R, Durgadoss]

Hi Rui,

> I think a thermal zone id and an event id is enough.
> Applications should rescan /sys/class/power_supply/ to see what is
> actually going on. :)
> 

I agree with you here Rui.

> a question for Durgadoss,
> int thermal_netlink_event(u32 orig, enum events event)
> what does parameter "orig" stand for?
> 
As you mentioned, it is the thermal_zone id.
I named it "orig" for "originator" of that particular event.

> If we only need to change the enum event, what about a patch to fix the
> enum event first and then shipping the coretemp/pkgtemp driver patch on
> top of it?
> 
> thanks,
> rui
> >
> >      Thomas
> > --
> > 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
> 

Re: thermal: Avoid CONFIG_NET compile dependency

[Thomas Renninger]

On Tuesday, January 25, 2011 05:47:56 AM R, Durgadoss wrote:
> Hi Thomas,
...
> If we make the coretemp register with the thermal framework, we have to modify
> the sysfs _store and _show functions a bit. Hence, the coretemp people were not
> very happy with the idea of making coretemp register with the thermal framework.
> So, I just added support to the core thresholds, and when current 
> temperature crosses them, it sends a netlink event.
Can you give a pointer to this discussion if there is any.

I think a generic thermal netlink interface can get
implemented separately, best without looking at any specific
driver implementation.

Best would be to revert the current patch and send a more generic
implementation with at least these in CC (or other lists maintaining
drivers who might make use of it):
platform-driver-x86@vger.kernel.org
linux-pm@lists.linux-foundation.org

Then people can bring in their needs and eventually
come up with code making use of it.

   Thomas

RE: thermal: Avoid CONFIG_NET compile dependency

[R, Durgadoss]

Hi Thomas,

> 
> I think a generic thermal netlink interface can get
> implemented separately, best without looking at any specific
> driver implementation.

By generic interface, I understand, any thermal related driver,
Should be able to use it, by sending netlink events with a
Originator of the event, and the type of the event(as we discussed
earlier, this type is one in the enum..)

We can add more details, to the enum and enhance the generic netlink
Interface.

The problem I see is, how do we get a unique originator id,
for every thermal driver that wants to send events, unless it
registers with the thermal framework?
Some thoughts:
1. We should define a list of originators as an enum(like event_type)
2. Just use the get_id function(to generate unique ids) from thermal framework
   without registering with it.

Kindly let me know your thoughts on this.

> Best would be to revert the current patch and send a more generic
> implementation with at least these in CC (or other lists maintaining
> drivers who might make use of it):
> platform-driver-x86@vger.kernel.org
> linux-pm@lists.linux-foundation.org
> 
> Then people can bring in their needs and eventually
> come up with code making use of it.

I can do the cleanup, once we finalize how to go about it.

Thanks,
Durga

Re: thermal: Avoid CONFIG_NET compile dependency

[Jean Delvare]

On Tuesday 25 January 2011 05:47:56 am R, Durgadoss wrote:
> > The thermal_netlink_events could then only be declared for
> > thermal_sys.c scope, so that other drivers cannot misuse them
> > and the netlink event can then get triggered by the driver through
> > thermal driver callbacks.
> 
> Yep!! This was my initial idea, with which I added the netlink
>  support here.
> 
> If we make the coretemp register with the thermal framework, we have
>  to modify the sysfs _store and _show functions a bit. Hence, the
>  coretemp people were not very happy with the idea of making coretemp
>  register with the thermal framework. So, I just added support to the
>  core thresholds, and when current temperature crosses them, it sends
>  a netlink event.

I don't remember the details, but one thing for sure if that you can't 
change the format of existing attributes the hwmon subsystem (and 
libsensors) is using. These are standardized per 
Documentation/hwmon/sysfs-interface, so they can't be changed. But there 
is no problem with registering a second class device with the thermal 
subsystem and adding whatever attributes you want to have there.

Obviously we want to avoid too much redundancy between hwmon and 
thermal. But I can understand that this isn't always possible due to 
historical constraints.

Another problem is that we really want pkgtemp to be merged with 
coretemp before intrusive changes are made to these drivers.

-- 
Jean Delvare
Suse L3

Re: Thermal kernel events API to userspace - Was: Re: thermal: Avoid CONFIG_NET compile dependency

[Thomas Renninger]

On Tuesday, January 25, 2011 08:57:49 AM Zhang Rui wrote:
> On Tue, 2011-01-25 at 00:07 +0800, Henrique de Moraes Holschuh wrote:
> > On Mon, 24 Jan 2011, Thomas Renninger wrote:
> > > I wonder whether netlink is the way to go for thermal
> > > events at all.
> > > Sending an udev event would already contain the sysfs
> > > path to the thermal device. A variable which thermal event
> > > got thrown could get added and userspace can read out the rest
> > > easily from sysfs files. But I expect udev is not intended
> > > for such general events?
> > 
> > udev is heavyweight in the userspace side, we'd be much better off using the
> > ACPI event interface (which is netlink), or a new one to deliver system
> > status events, instead of continously abusing udev for this stuff.
> > 
> > > > > Also, the thermal_aux0 and _aux1, we can use the final format specified by you.
> > > > > enum events {
> > > > >  	THERMAL_CRITICAL,
> > > > >  	/* user defined thermal events */
> > > > >  	THERMAL_USER_AUX0,
> > > > >  	THERMAL_USER_AUX1,
> > > > >  	THERMAL_DEV_FAULT,
> > > > >  };
> > 
> > Please give us at least two levels of thermal alarm: critical and emergency
> > (or warning and critical -- it doesn't matter much, as long as there are at
> > least two levels, and which one comes first is defined by the
> > specification).  I'd have immediate use for them on thinkpads.
What kind of thinkpad specific events are these and what actions
should be taken if they happen?
 
> > It is probably best to have three levels (warning, critical, emergency).
> > Best not to tie the API/ABI to the notion of "too hot", one can also alarm
> > when it starts to get to cold.
> > 
> when it's the "too hot" case, what kind of action should be taken upon
> the warning/critical/emergency events?
> I mean what's the difference between these three levels.
I could imagine there is quite some HW out there
with quite different thermal events.
What other actions userspace would take beside logging the event,
telling the user that a fan is switched on, CPU or whatever device
gets throttled. Logging such specific info can only be implemented
in the driver itself and would get lost when exporting things through
a generic thermal interface.

I wonder which events would need userspace to take specific
(configured) actions at all and what kind of action it could be.

What is THERMAL_USER_AUX0?
When will it get thrown and what is userspace expected to do?

    Thomas

Re: Thermal kernel events API to userspace - Was: Re: thermal: Avoid CONFIG_NET compile dependency

[Thomas Renninger]

On Tuesday, January 25, 2011 08:57:49 AM Zhang Rui wrote:
> On Tue, 2011-01-25 at 00:07 +0800, Henrique de Moraes Holschuh wrote:
> > On Mon, 24 Jan 2011, Thomas Renninger wrote:
> > > I wonder whether netlink is the way to go for thermal
> > > events at all.
> > > Sending an udev event would already contain the sysfs
> > > path to the thermal device. A variable which thermal event
> > > got thrown could get added and userspace can read out the rest
> > > easily from sysfs files. But I expect udev is not intended
> > > for such general events?
> > 
> > udev is heavyweight in the userspace side, we'd be much better off using the
> > ACPI event interface (which is netlink), or a new one to deliver system
> > status events, instead of continously abusing udev for this stuff.
> > 
> > > > > Also, the thermal_aux0 and _aux1, we can use the final format specified by you.
> > > > > enum events {
> > > > >  	THERMAL_CRITICAL,
> > > > >  	/* user defined thermal events */
> > > > >  	THERMAL_USER_AUX0,
> > > > >  	THERMAL_USER_AUX1,
> > > > >  	THERMAL_DEV_FAULT,
> > > > >  };
> > 
> > Please give us at least two levels of thermal alarm: critical and emergency
> > (or warning and critical -- it doesn't matter much, as long as there are at
> > least two levels, and which one comes first is defined by the
> > specification).  I'd have immediate use for them on thinkpads.
What kind of thinkpad specific events are these and what actions
should be taken if they happen?
 
> > It is probably best to have three levels (warning, critical, emergency).
> > Best not to tie the API/ABI to the notion of "too hot", one can also alarm
> > when it starts to get to cold.
> > 
> when it's the "too hot" case, what kind of action should be taken upon
> the warning/critical/emergency events?
> I mean what's the difference between these three levels.
I could imagine there is quite some HW out there
with quite different thermal events.
What other actions userspace would take beside logging the event,
telling the user that a fan is switched on, CPU or whatever device
gets throttled. Logging such specific info can only be implemented
in the driver itself and would get lost when exporting things through
a generic thermal interface.

I wonder which events would need userspace to take specific
(configured) actions at all and what kind of action it could be.

What is THERMAL_USER_AUX0?
When will it get thrown and what is userspace expected to do?

    Thomas

Re: thermal: Avoid CONFIG_NET compile dependency

[Guenter Roeck]

On Tue, Jan 25, 2011 at 04:48:43AM -0500, Jean Delvare wrote:
> On Tuesday 25 January 2011 05:47:56 am R, Durgadoss wrote:
> > > The thermal_netlink_events could then only be declared for
> > > thermal_sys.c scope, so that other drivers cannot misuse them
> > > and the netlink event can then get triggered by the driver through
> > > thermal driver callbacks.
> > 
> > Yep!! This was my initial idea, with which I added the netlink
> >  support here.
> > 
> > If we make the coretemp register with the thermal framework, we have
> >  to modify the sysfs _store and _show functions a bit. Hence, the
> >  coretemp people were not very happy with the idea of making coretemp
> >  register with the thermal framework. So, I just added support to the
> >  core thresholds, and when current temperature crosses them, it sends
> >  a netlink event.
> 
> I don't remember the details, but one thing for sure if that you can't 
> change the format of existing attributes the hwmon subsystem (and 
> libsensors) is using. These are standardized per 
> Documentation/hwmon/sysfs-interface, so they can't be changed. But there 
> is no problem with registering a second class device with the thermal 
> subsystem and adding whatever attributes you want to have there.
> 
A secondary problem is that thermal subsystem drivers register themselves
with the hwmon subsystem - at least if CONFIG_THERMAL_HWMON is defined.
So it doesn't really make much sense for a driver to register itself
as thermal driver _and_ as hwmon driver, since it may end up being listed
twice as hwmon device.

> 
> Another problem is that we really want pkgtemp to be merged with 
> coretemp before intrusive changes are made to these drivers.
> 
Unfortunately, that request seems to go to deaf ears.

Guenter

Re: Thermal kernel events API to userspace - Was: Re: thermal: Avoid CONFIG_NET compile dependency

[Henrique de Moraes Holschuh]

On Tue, 25 Jan 2011, Zhang Rui wrote:
> On Tue, 2011-01-25 at 00:07 +0800, Henrique de Moraes Holschuh wrote:
> > > > > Also, the thermal_aux0 and _aux1, we can use the final format specified by you.
> > > > > enum events {
> > > > >  	THERMAL_CRITICAL,
> > > > >  	/* user defined thermal events */
> > > > >  	THERMAL_USER_AUX0,
> > > > >  	THERMAL_USER_AUX1,
> > > > >  	THERMAL_DEV_FAULT,
> > > > >  };
> > 
> > Please give us at least two levels of thermal alarm: critical and emergency
> > (or warning and critical -- it doesn't matter much, as long as there are at
> > least two levels, and which one comes first is defined by the
> > specification).  I'd have immediate use for them on thinkpads.
> > 
> > It is probably best to have three levels (warning, critical, emergency).
> > Best not to tie the API/ABI to the notion of "too hot", one can also alarm
> > when it starts to get to cold.
> > 
> when it's the "too hot" case, what kind of action should be taken upon
> the warning/critical/emergency events?
> I mean what's the difference between these three levels.

*ASSUMING* it is monitoring the box (which is a damn big assumption, and
should be stated outright):

Warning: do something to stop generating so much heat, warn user, try to
increase cooling.  Let userspace bother with this.

Critical: *STOP* generating so much heat (throttle cpu, throttle GPU,
shutdown non-critical devices, etc).  Increase cooling to max.  WARN the
user that a machine emergency exists and shutdown is imminent, and that he
has to power off or sleep to ram *NOW*.

Emergency: emergency sync, and initiate immediate suspend to ram. If that
fails, initiate emergency powerdown.

Likely one wants kernel notifiers for this stuff, too, so that we can have
in-kernel handlers (optional or not, configurable or not, that is orthogonal
to the issue) that can try to do something to help.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: Thermal kernel events API to userspace - Was: Re: thermal: Avoid CONFIG_NET compile dependency

[Henrique de Moraes Holschuh]

On Tue, 25 Jan 2011, Zhang Rui wrote:
> On Tue, 2011-01-25 at 00:07 +0800, Henrique de Moraes Holschuh wrote:
> > > > > Also, the thermal_aux0 and _aux1, we can use the final format specified by you.
> > > > > enum events {
> > > > >  	THERMAL_CRITICAL,
> > > > >  	/* user defined thermal events */
> > > > >  	THERMAL_USER_AUX0,
> > > > >  	THERMAL_USER_AUX1,
> > > > >  	THERMAL_DEV_FAULT,
> > > > >  };
> > 
> > Please give us at least two levels of thermal alarm: critical and emergency
> > (or warning and critical -- it doesn't matter much, as long as there are at
> > least two levels, and which one comes first is defined by the
> > specification).  I'd have immediate use for them on thinkpads.
> > 
> > It is probably best to have three levels (warning, critical, emergency).
> > Best not to tie the API/ABI to the notion of "too hot", one can also alarm
> > when it starts to get to cold.
> > 
> when it's the "too hot" case, what kind of action should be taken upon
> the warning/critical/emergency events?
> I mean what's the difference between these three levels.

*ASSUMING* it is monitoring the box (which is a damn big assumption, and
should be stated outright):

Warning: do something to stop generating so much heat, warn user, try to
increase cooling.  Let userspace bother with this.

Critical: *STOP* generating so much heat (throttle cpu, throttle GPU,
shutdown non-critical devices, etc).  Increase cooling to max.  WARN the
user that a machine emergency exists and shutdown is imminent, and that he
has to power off or sleep to ram *NOW*.

Emergency: emergency sync, and initiate immediate suspend to ram. If that
fails, initiate emergency powerdown.

Likely one wants kernel notifiers for this stuff, too, so that we can have
in-kernel handlers (optional or not, configurable or not, that is orthogonal
to the issue) that can try to do something to help.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: Thermal kernel events API to userspace - Was: Re: thermal: Avoid CONFIG_NET compile dependency

[Henrique de Moraes Holschuh]

On Tue, 25 Jan 2011, Thomas Renninger wrote:
> > > Please give us at least two levels of thermal alarm: critical and emergency
> > > (or warning and critical -- it doesn't matter much, as long as there are at
> > > least two levels, and which one comes first is defined by the
> > > specification).  I'd have immediate use for them on thinkpads.
> What kind of thinkpad specific events are these and what actions
> should be taken if they happen?

So far:  Battery temperature critical and emergency, Generic system sensor
temperature critical and emergency.

In all cases, the recommended actions are imediate notification for the
user, and in the case of the emergency level, immediate action, where action
is "suspend to ram" or shutdown.

Right now all they do is to prinkt at suitable "horrible things are about to
happen" severity levels (KERN_CRIT for critical, and KERN_ALERT for
emergency).  In a few sensible desktop environments and distros, this causes
a notification to show up on the user's screen.

Oh, and it also relays the thinkpad-specific event through the ACPI event
pipe, but I don't know of any userspace application doing something with it,
and nobody ever tried to bribe me into writing one by suppling me with a new
T-series thinkpad :)

> I wonder which events would need userspace to take specific
> (configured) actions at all and what kind of action it could be.

All of them can have sensible generic actions.  See my other email.

> What is THERMAL_USER_AUX0?
> When will it get thrown and what is userspace expected to do?

Good question.  What use are those "user defined" events in a generic
interface, anyway?  You will have to know exactly what device is issuing the
"generic user defined" event, and what it means for that device.

When you need a device-specific interface, you design one that is well
defined, such as thinkpad-acpi's thinkpad-specific "acpi" events.  If you
get any thinkpad-acpi specific event, you know exactly what it is, and
nothing else ever issues those events, so you will never get them from
somewhere else with a different meaning.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: Thermal kernel events API to userspace - Was: Re: thermal: Avoid CONFIG_NET compile dependency

[Henrique de Moraes Holschuh]

On Tue, 25 Jan 2011, Thomas Renninger wrote:
> > > Please give us at least two levels of thermal alarm: critical and emergency
> > > (or warning and critical -- it doesn't matter much, as long as there are at
> > > least two levels, and which one comes first is defined by the
> > > specification).  I'd have immediate use for them on thinkpads.
> What kind of thinkpad specific events are these and what actions
> should be taken if they happen?

So far:  Battery temperature critical and emergency, Generic system sensor
temperature critical and emergency.

In all cases, the recommended actions are imediate notification for the
user, and in the case of the emergency level, immediate action, where action
is "suspend to ram" or shutdown.

Right now all they do is to prinkt at suitable "horrible things are about to
happen" severity levels (KERN_CRIT for critical, and KERN_ALERT for
emergency).  In a few sensible desktop environments and distros, this causes
a notification to show up on the user's screen.

Oh, and it also relays the thinkpad-specific event through the ACPI event
pipe, but I don't know of any userspace application doing something with it,
and nobody ever tried to bribe me into writing one by suppling me with a new
T-series thinkpad :)

> I wonder which events would need userspace to take specific
> (configured) actions at all and what kind of action it could be.

All of them can have sensible generic actions.  See my other email.

> What is THERMAL_USER_AUX0?
> When will it get thrown and what is userspace expected to do?

Good question.  What use are those "user defined" events in a generic
interface, anyway?  You will have to know exactly what device is issuing the
"generic user defined" event, and what it means for that device.

When you need a device-specific interface, you design one that is well
defined, such as thinkpad-acpi's thinkpad-specific "acpi" events.  If you
get any thinkpad-acpi specific event, you know exactly what it is, and
nothing else ever issues those events, so you will never get them from
somewhere else with a different meaning.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: thermal: Avoid CONFIG_NET compile dependency

[Thomas Renninger]

On Tuesday, January 25, 2011 02:43:15 PM Guenter Roeck wrote:
> On Tue, Jan 25, 2011 at 04:48:43AM -0500, Jean Delvare wrote:
> > On Tuesday 25 January 2011 05:47:56 am R, Durgadoss wrote:
...
> > I don't remember the details, but one thing for sure if that you can't 
> > change the format of existing attributes the hwmon subsystem (and 
> > libsensors) is using. These are standardized per 
> > Documentation/hwmon/sysfs-interface, so they can't be changed. But there 
> > is no problem with registering a second class device with the thermal 
> > subsystem and adding whatever attributes you want to have there.
> > 
> A secondary problem is that thermal subsystem drivers register themselves
> with the hwmon subsystem - at least if CONFIG_THERMAL_HWMON is defined.
> So it doesn't really make much sense for a driver to register itself
> as thermal driver _and_ as hwmon driver, since it may end up being listed
> twice as hwmon device.
Having a very quick look at coretemp, this one only provides thermal data,
right?
So it should register as a thermal driver which in turn tells userspace
that lsmsensors can retrieve data from it.
The data would then get exported via:
/sys/devices/virtual/thermal/
and the whole platform code can get removed.

Do I miss something?

   Thomas

Re: thermal: Avoid CONFIG_NET compile dependency

[Guenter Roeck]

On Tue, Jan 25, 2011 at 11:18:36AM -0500, Thomas Renninger wrote:
> On Tuesday, January 25, 2011 02:43:15 PM Guenter Roeck wrote:
> > On Tue, Jan 25, 2011 at 04:48:43AM -0500, Jean Delvare wrote:
> > > On Tuesday 25 January 2011 05:47:56 am R, Durgadoss wrote:
> ...
> > > I don't remember the details, but one thing for sure if that you can't 
> > > change the format of existing attributes the hwmon subsystem (and 
> > > libsensors) is using. These are standardized per 
> > > Documentation/hwmon/sysfs-interface, so they can't be changed. But there 
> > > is no problem with registering a second class device with the thermal 
> > > subsystem and adding whatever attributes you want to have there.
> > > 
> > A secondary problem is that thermal subsystem drivers register themselves
> > with the hwmon subsystem - at least if CONFIG_THERMAL_HWMON is defined.
> > So it doesn't really make much sense for a driver to register itself
> > as thermal driver _and_ as hwmon driver, since it may end up being listed
> > twice as hwmon device.
> Having a very quick look at coretemp, this one only provides thermal data,
> right?
> So it should register as a thermal driver which in turn tells userspace
> that lsmsensors can retrieve data from it.
> The data would then get exported via:
> /sys/devices/virtual/thermal/
> and the whole platform code can get removed.
> 
That would be an option.

Guenter

RE: Thermal kernel events API to userspace - Was: Re: thermal: Avoid CONFIG_NET compile dependency

[Zhang, Rui]

> -----Original Message-----
> From: Henrique de Moraes Holschuh [mailto:hmh@hmh.eng.br]
> Sent: Wednesday, January 26, 2011 12:11 AM
> To: Thomas Renninger
> Cc: Zhang, Rui; R, Durgadoss; jdelvare@novell.com; Len Brown; linux-
> acpi@vger.kernel.org; Kay Sievers; linux-perf-users@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-trace-users@vger.kernel.org
> Subject: Re: Thermal kernel events API to userspace - Was: Re: thermal:
> Avoid CONFIG_NET compile dependency
> Importance: High
> 
> On Tue, 25 Jan 2011, Thomas Renninger wrote:
> > > > Please give us at least two levels of thermal alarm: critical and
> emergency
> > > > (or warning and critical -- it doesn't matter much, as long as
> there are at
> > > > least two levels, and which one comes first is defined by the
> > > > specification).  I'd have immediate use for them on thinkpads.
> > What kind of thinkpad specific events are these and what actions
> > should be taken if they happen?
> 
> So far:  Battery temperature critical and emergency, Generic system
> sensor
> temperature critical and emergency.
> 
> In all cases, the recommended actions are imediate notification for the
> user, and in the case of the emergency level, immediate action, where
> action
> is "suspend to ram" or shutdown.
> 
> Right now all they do is to prinkt at suitable "horrible things are
> about to
> happen" severity levels (KERN_CRIT for critical, and KERN_ALERT for
> emergency).  In a few sensible desktop environments and distros, this
> causes
> a notification to show up on the user's screen.
> 
> Oh, and it also relays the thinkpad-specific event through the ACPI
> event
> pipe, but I don't know of any userspace application doing something
> with it,
> and nobody ever tried to bribe me into writing one by suppling me with
> a new
> T-series thinkpad :)
> 
> > I wonder which events would need userspace to take specific
> > (configured) actions at all and what kind of action it could be.
> 
> All of them can have sensible generic actions.  See my other email.
> 
> > What is THERMAL_USER_AUX0?
> > When will it get thrown and what is userspace expected to do?
> 
> Good question.  What use are those "user defined" events in a generic
> interface, anyway?  You will have to know exactly what device is
> issuing the
> "generic user defined" event, and what it means for that device.
> 
> When you need a device-specific interface, you design one that is well
> defined, such as thinkpad-acpi's thinkpad-specific "acpi" events.  If
> you
> get any thinkpad-acpi specific event, you know exactly what it is, and
> nothing else ever issues those events, so you will never get them from
> somewhere else with a different meaning.
>
I think you are talking about something like this:
enum events {
	/* generic thermal event */
	THERMAL_WARN,
	THERMAL_EMERG,
	THERMAL_CRIT,
	/* coretemp thermal events */
	TEHRMAL_CT_AUX0,
	TEHRMAL_CT_AUX1,
	/* Thinkpad battery thermal events */
	THERMAL_TP_BAT_CRIT,
	THERMAL_TP_BAT_EMERG,
	THERMAL_DEV_FAULT,
};
Right?


> --
>   "One disk to rule them all, One disk to find them. One disk to bring
>   them all and in the darkness grind them. In the Land of Redmond
>   where the shadows lie." -- The Silicon Valley Tarot
>   Henrique Holschuh

RE: Thermal kernel events API to userspace - Was: Re: thermal: Avoid CONFIG_NET compile dependency

[Zhang, Rui]

> -----Original Message-----
> From: Henrique de Moraes Holschuh [mailto:hmh@hmh.eng.br]
> Sent: Wednesday, January 26, 2011 12:11 AM
> To: Thomas Renninger
> Cc: Zhang, Rui; R, Durgadoss; jdelvare@novell.com; Len Brown; linux-
> acpi@vger.kernel.org; Kay Sievers; linux-perf-users@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-trace-users@vger.kernel.org
> Subject: Re: Thermal kernel events API to userspace - Was: Re: thermal:
> Avoid CONFIG_NET compile dependency
> Importance: High
> 
> On Tue, 25 Jan 2011, Thomas Renninger wrote:
> > > > Please give us at least two levels of thermal alarm: critical and
> emergency
> > > > (or warning and critical -- it doesn't matter much, as long as
> there are at
> > > > least two levels, and which one comes first is defined by the
> > > > specification).  I'd have immediate use for them on thinkpads.
> > What kind of thinkpad specific events are these and what actions
> > should be taken if they happen?
> 
> So far:  Battery temperature critical and emergency, Generic system
> sensor
> temperature critical and emergency.
> 
> In all cases, the recommended actions are imediate notification for the
> user, and in the case of the emergency level, immediate action, where
> action
> is "suspend to ram" or shutdown.
> 
> Right now all they do is to prinkt at suitable "horrible things are
> about to
> happen" severity levels (KERN_CRIT for critical, and KERN_ALERT for
> emergency).  In a few sensible desktop environments and distros, this
> causes
> a notification to show up on the user's screen.
> 
> Oh, and it also relays the thinkpad-specific event through the ACPI
> event
> pipe, but I don't know of any userspace application doing something
> with it,
> and nobody ever tried to bribe me into writing one by suppling me with
> a new
> T-series thinkpad :)
> 
> > I wonder which events would need userspace to take specific
> > (configured) actions at all and what kind of action it could be.
> 
> All of them can have sensible generic actions.  See my other email.
> 
> > What is THERMAL_USER_AUX0?
> > When will it get thrown and what is userspace expected to do?
> 
> Good question.  What use are those "user defined" events in a generic
> interface, anyway?  You will have to know exactly what device is
> issuing the
> "generic user defined" event, and what it means for that device.
> 
> When you need a device-specific interface, you design one that is well
> defined, such as thinkpad-acpi's thinkpad-specific "acpi" events.  If
> you
> get any thinkpad-acpi specific event, you know exactly what it is, and
> nothing else ever issues those events, so you will never get them from
> somewhere else with a different meaning.
>
I think you are talking about something like this:
enum events {
	/* generic thermal event */
	THERMAL_WARN,
	THERMAL_EMERG,
	THERMAL_CRIT,
	/* coretemp thermal events */
	TEHRMAL_CT_AUX0,
	TEHRMAL_CT_AUX1,
	/* Thinkpad battery thermal events */
	THERMAL_TP_BAT_CRIT,
	THERMAL_TP_BAT_EMERG,
	THERMAL_DEV_FAULT,
};
Right?


> --
>   "One disk to rule them all, One disk to find them. One disk to bring
>   them all and in the darkness grind them. In the Land of Redmond
>   where the shadows lie." -- The Silicon Valley Tarot
>   Henrique Holschuh

Re: Thermal kernel events API to userspace - Was: Re: thermal: Avoid CONFIG_NET compile dependency

[Henrique de Moraes Holschuh]

On Wed, 26 Jan 2011, Zhang, Rui wrote:
> > > I wonder which events would need userspace to take specific
> > > (configured) actions at all and what kind of action it could be.
> > 
> > All of them can have sensible generic actions.  See my other email.
> > 
> > > What is THERMAL_USER_AUX0?
> > > When will it get thrown and what is userspace expected to do?
> > 
> > Good question.  What use are those "user defined" events in a generic
> > interface, anyway?  You will have to know exactly what device is
> > issuing the
> > "generic user defined" event, and what it means for that device.
> > 
> > When you need a device-specific interface, you design one that is well
> > defined, such as thinkpad-acpi's thinkpad-specific "acpi" events.  If
> > you
> > get any thinkpad-acpi specific event, you know exactly what it is, and
> > nothing else ever issues those events, so you will never get them from
> > somewhere else with a different meaning.
> >
> I think you are talking about something like this:
> enum events {
> 	/* generic thermal event */
> 	THERMAL_WARN,
> 	THERMAL_EMERG,
> 	THERMAL_CRIT,
> 	/* coretemp thermal events */
> 	TEHRMAL_CT_AUX0,
> 	TEHRMAL_CT_AUX1,
> 	/* Thinkpad battery thermal events */
> 	THERMAL_TP_BAT_CRIT,
> 	THERMAL_TP_BAT_EMERG,
> 	THERMAL_DEV_FAULT,
> };
> Right?

Hmm, no.  I already perfectly fine, strictly defined driver-specific events.
And driver-specific events are useless crap in practice.  Nobody really
codes userspace utilities for them.  Maybe some 10-20 power users write ACPI
rules and scripts to deal with them, but that's it.

I am quite willing and happy to keep the driver-specific events for those 20
people in the world that actually used them to make something nice, and even
add new ones when necessary.  But that is NOT the way to go forward when
you have any sort of choice.

I.e.  I'd like THERMAL_BAT_CRIT, THERMAL_BAT_EMERG, and a strict enough
definition of what they mean, that userspace (or a kernel thermal event
handler) could process them and do the right thing regardless of which
driver generated the events, because it would be sure of what they mean.

You might want to change that to a two-axis or three-axis event, actually:

What:   THERMAL_NORMAL, THERMAL_WARN, THERMAL_EMERG, THERMAL_CRIT,
        THERMAL_SENSOR_FAULT...

Where:  SYSTEM (aka: unknown/generic), BATTERY, CPU, GPU, PSU, PCIDEV, USBDEV...

Extended code: (what,where)-specific.  For example, could be the specific
core or package for (*, CPU) events...

BTW: thermal_normal means "alarm condition cleared".  Like thermal_warn, I
wouldn't expect every device to be able to raise it.  Thinkpads, for
example, won't tell you when they're back to normal operating conditions
AFAIK.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: Thermal kernel events API to userspace - Was: Re: thermal: Avoid CONFIG_NET compile dependency

[Henrique de Moraes Holschuh]

On Wed, 26 Jan 2011, Zhang, Rui wrote:
> > > I wonder which events would need userspace to take specific
> > > (configured) actions at all and what kind of action it could be.
> > 
> > All of them can have sensible generic actions.  See my other email.
> > 
> > > What is THERMAL_USER_AUX0?
> > > When will it get thrown and what is userspace expected to do?
> > 
> > Good question.  What use are those "user defined" events in a generic
> > interface, anyway?  You will have to know exactly what device is
> > issuing the
> > "generic user defined" event, and what it means for that device.
> > 
> > When you need a device-specific interface, you design one that is well
> > defined, such as thinkpad-acpi's thinkpad-specific "acpi" events.  If
> > you
> > get any thinkpad-acpi specific event, you know exactly what it is, and
> > nothing else ever issues those events, so you will never get them from
> > somewhere else with a different meaning.
> >
> I think you are talking about something like this:
> enum events {
> 	/* generic thermal event */
> 	THERMAL_WARN,
> 	THERMAL_EMERG,
> 	THERMAL_CRIT,
> 	/* coretemp thermal events */
> 	TEHRMAL_CT_AUX0,
> 	TEHRMAL_CT_AUX1,
> 	/* Thinkpad battery thermal events */
> 	THERMAL_TP_BAT_CRIT,
> 	THERMAL_TP_BAT_EMERG,
> 	THERMAL_DEV_FAULT,
> };
> Right?

Hmm, no.  I already perfectly fine, strictly defined driver-specific events.
And driver-specific events are useless crap in practice.  Nobody really
codes userspace utilities for them.  Maybe some 10-20 power users write ACPI
rules and scripts to deal with them, but that's it.

I am quite willing and happy to keep the driver-specific events for those 20
people in the world that actually used them to make something nice, and even
add new ones when necessary.  But that is NOT the way to go forward when
you have any sort of choice.

I.e.  I'd like THERMAL_BAT_CRIT, THERMAL_BAT_EMERG, and a strict enough
definition of what they mean, that userspace (or a kernel thermal event
handler) could process them and do the right thing regardless of which
driver generated the events, because it would be sure of what they mean.

You might want to change that to a two-axis or three-axis event, actually:

What:   THERMAL_NORMAL, THERMAL_WARN, THERMAL_EMERG, THERMAL_CRIT,
        THERMAL_SENSOR_FAULT...

Where:  SYSTEM (aka: unknown/generic), BATTERY, CPU, GPU, PSU, PCIDEV, USBDEV...

Extended code: (what,where)-specific.  For example, could be the specific
core or package for (*, CPU) events...

BTW: thermal_normal means "alarm condition cleared".  Like thermal_warn, I
wouldn't expect every device to be able to raise it.  Thinkpads, for
example, won't tell you when they're back to normal operating conditions
AFAIK.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: thermal: Avoid CONFIG_NET compile dependency

[Thomas Renninger]

On Tuesday, January 25, 2011 05:25:36 PM Guenter Roeck wrote:
> On Tue, Jan 25, 2011 at 11:18:36AM -0500, Thomas Renninger wrote:
> > On Tuesday, January 25, 2011 02:43:15 PM Guenter Roeck wrote:
> > > On Tue, Jan 25, 2011 at 04:48:43AM -0500, Jean Delvare wrote:
> > > > On Tuesday 25 January 2011 05:47:56 am R, Durgadoss wrote:
> > ...
> > > > I don't remember the details, but one thing for sure if that you can't 
> > > > change the format of existing attributes the hwmon subsystem (and 
> > > > libsensors) is using. These are standardized per 
> > > > Documentation/hwmon/sysfs-interface, so they can't be changed. But there 
> > > > is no problem with registering a second class device with the thermal 
> > > > subsystem and adding whatever attributes you want to have there.
> > > > 
> > > A secondary problem is that thermal subsystem drivers register themselves
> > > with the hwmon subsystem - at least if CONFIG_THERMAL_HWMON is defined.
> > > So it doesn't really make much sense for a driver to register itself
> > > as thermal driver _and_ as hwmon driver, since it may end up being listed
> > > twice as hwmon device.
> > Having a very quick look at coretemp, this one only provides thermal data,
> > right?
> > So it should register as a thermal driver which in turn tells userspace
> > that lsmsensors can retrieve data from it.
> > The data would then get exported via:
> > /sys/devices/virtual/thermal/
> > and the whole platform code can get removed.
> > 
> That would be an option.
What would be the next steps then?
Whatabout:
   1) Reverting the current thermal_netlink implementation

   2) Make coretemp register as a thermal driver.
      This can be done on top of Durgadoss' enhancements.
      Revert the one thermal_netlink_event call

   3) Come up with a more generic and more thermal bound
      (do not export things to the whole kernel if possible)
      thermal netlink implementation.
      Important is a robust interface. No need to define all
      possible events, they can be added one by one if thinkpad
      or whatever users need them.
      But the format should not need to be changed in the recent
      future.
      Might be worth to put this into an own file:
      drivers/thermal/thermal_netlink.c

   4) Ideally coretemp.c does not need a special hook to trigger
      the event. If, it could be some kind of thermal driver
      callback it got when registering as a thermal driver.
      Like that the thermal netlink events are hidden and cannot be
      mis-used by arbitrary drivers, but drivers must register
      as a thermal driver to be able to trigger them.
      Like that it can also be insured that the netlink event
      always provides a pointer to a thermal device which can
      be looked up via sysfs.

Not sure whether 4. will work out as described.
2. + 3. can be implemented in parallel and connected with
a tiny on top patch with 4 (if necessary at all).

If the current thermal_netlink_event implementation is not
reverted, there would be one kernel revision with a thermal
netlink userspace notification implementation which should
not get used. Hm and in the end userspace apps need to be
able to detect the never used one, it should really get
reverted.
Len?

Is there still someone who could veto above or a mailinglist
that should get added or can this be started without
much risk of getting a NAK afterwards?

Thanks,

    Thomas

RE: thermal: Avoid CONFIG_NET compile dependency

[R, Durgadoss]

> -----Original Message-----
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> owner@vger.kernel.org] On Behalf Of Thomas Renninger
> Sent: Thursday, January 27, 2011 3:18 PM
> To: Guenter Roeck
> Cc: Jean Delvare; R, Durgadoss; Zhang, Rui; Len Brown; linux-
> acpi@vger.kernel.org; Yu, Fenghua
> Subject: Re: thermal: Avoid CONFIG_NET compile dependency
> 
> On Tuesday, January 25, 2011 05:25:36 PM Guenter Roeck wrote:
> > On Tue, Jan 25, 2011 at 11:18:36AM -0500, Thomas Renninger wrote:
> > > On Tuesday, January 25, 2011 02:43:15 PM Guenter Roeck wrote:
> > > > On Tue, Jan 25, 2011 at 04:48:43AM -0500, Jean Delvare wrote:
> > > > > On Tuesday 25 January 2011 05:47:56 am R, Durgadoss wrote:
> > > ...
> > > > > I don't remember the details, but one thing for sure if that you can't
> > > > > change the format of existing attributes the hwmon subsystem (and
> > > > > libsensors) is using. These are standardized per
> > > > > Documentation/hwmon/sysfs-interface, so they can't be changed. But
> there
> > > > > is no problem with registering a second class device with the thermal
> > > > > subsystem and adding whatever attributes you want to have there.
> > > > >
> > > > A secondary problem is that thermal subsystem drivers register themselves
> > > > with the hwmon subsystem - at least if CONFIG_THERMAL_HWMON is defined.
> > > > So it doesn't really make much sense for a driver to register itself
> > > > as thermal driver _and_ as hwmon driver, since it may end up being listed
> > > > twice as hwmon device.
> > > Having a very quick look at coretemp, this one only provides thermal data,
> > > right?
> > > So it should register as a thermal driver which in turn tells userspace
> > > that lsmsensors can retrieve data from it.
> > > The data would then get exported via:
> > > /sys/devices/virtual/thermal/
> > > and the whole platform code can get removed.
> > >
> > That would be an option.
> What would be the next steps then?
> Whatabout:
>    1) Reverting the current thermal_netlink implementation
> 
>    2) Make coretemp register as a thermal driver.
>       This can be done on top of Durgadoss' enhancements.
>

Yes. 1) and 2) are fine with me.

> 
>    3) Come up with a more generic and more thermal bound
>       (do not export things to the whole kernel if possible)
>       thermal netlink implementation.
>       Important is a robust interface. No need to define all
>       possible events, they can be added one by one if thinkpad
>       or whatever users need them.
>       But the format should not need to be changed in the recent
>       future.
>       Might be worth to put this into an own file:
>       drivers/thermal/thermal_netlink.c

Since we are saying any driver that uses the netlink call _must_ register with
the thermal framework, its better this code be in thermal_sys.c.

>    4) Ideally coretemp.c does not need a special hook to trigger
>       the event. If, it could be some kind of thermal driver
>       callback it got when registering as a thermal driver.
>       Like that the thermal netlink events are hidden and cannot be
>       mis-used by arbitrary drivers, but drivers must register
>       as a thermal driver to be able to trigger them.

In this case, we should consider Jean's point of redundancy.
If CONFIG_THERMAL_HWMON is defined, the interfaces will be present in two
different places.
Or should we make it optional in the thermal framework to register or
Not register with hwmon ?
May be by passing some flag in thermal_zone_device_register?
Rui, need your suggestion here.

Moreover, if we make any driver that wants to use netlink, register with
the thermal framework (redundancy issue should be resolved though..), the problem of
unique originator id will be solved. Then, we can use enum like
the one Rui suggested, to start with. Later, when the need arises, we can add more.


Thanks,
Durga

Re: thermal: Avoid CONFIG_NET compile dependency

[Thomas Renninger]

On Thursday, January 27, 2011 02:34:10 PM R, Durgadoss wrote:
> > -----Original Message-----
> > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> > owner@vger.kernel.org] On Behalf Of Thomas Renninger
> > Sent: Thursday, January 27, 2011 3:18 PM
> > To: Guenter Roeck
> > Cc: Jean Delvare; R, Durgadoss; Zhang, Rui; Len Brown; linux-
> > acpi@vger.kernel.org; Yu, Fenghua
> > Subject: Re: thermal: Avoid CONFIG_NET compile dependency
> > 
> > On Tuesday, January 25, 2011 05:25:36 PM Guenter Roeck wrote:
> > > On Tue, Jan 25, 2011 at 11:18:36AM -0500, Thomas Renninger wrote:
> > > > On Tuesday, January 25, 2011 02:43:15 PM Guenter Roeck wrote:
> > > > > On Tue, Jan 25, 2011 at 04:48:43AM -0500, Jean Delvare wrote:
> > > > > > On Tuesday 25 January 2011 05:47:56 am R, Durgadoss wrote:
> > > > ...
> > > > > > I don't remember the details, but one thing for sure if that you can't
> > > > > > change the format of existing attributes the hwmon subsystem (and
> > > > > > libsensors) is using. These are standardized per
> > > > > > Documentation/hwmon/sysfs-interface, so they can't be changed. But
> > there
> > > > > > is no problem with registering a second class device with the thermal
> > > > > > subsystem and adding whatever attributes you want to have there.
> > > > > >
> > > > > A secondary problem is that thermal subsystem drivers register themselves
> > > > > with the hwmon subsystem - at least if CONFIG_THERMAL_HWMON is defined.
> > > > > So it doesn't really make much sense for a driver to register itself
> > > > > as thermal driver _and_ as hwmon driver, since it may end up being listed
> > > > > twice as hwmon device.
> > > > Having a very quick look at coretemp, this one only provides thermal data,
> > > > right?
> > > > So it should register as a thermal driver which in turn tells userspace
> > > > that lsmsensors can retrieve data from it.
> > > > The data would then get exported via:
> > > > /sys/devices/virtual/thermal/
> > > > and the whole platform code can get removed.
> > > >
> > > That would be an option.
> > What would be the next steps then?
> > Whatabout:
> >    1) Reverting the current thermal_netlink implementation
> > 
> >    2) Make coretemp register as a thermal driver.
> >       This can be done on top of Durgadoss' enhancements.
> >
> 
> Yes. 1) and 2) are fine with me.
> 
> > 
> >    3) Come up with a more generic and more thermal bound
> >       (do not export things to the whole kernel if possible)
> >       thermal netlink implementation.
> >       Important is a robust interface. No need to define all
> >       possible events, they can be added one by one if thinkpad
> >       or whatever users need them.
> >       But the format should not need to be changed in the recent
> >       future.
> >       Might be worth to put this into an own file:
> >       drivers/thermal/thermal_netlink.c
> 
> Since we are saying any driver that uses the netlink call _must_ register with
> the thermal framework, its better this code be in thermal_sys.c.
Why?
The netlink stuff is rather separate code.
If possible it should live in an own file which makes things
nicer structured and easier to find.
If needed you could also add a local header file:
drivers/thermal/thermal.h
to achieve this.

> >    4) Ideally coretemp.c does not need a special hook to trigger
> >       the event. If, it could be some kind of thermal driver
> >       callback it got when registering as a thermal driver.
> >       Like that the thermal netlink events are hidden and cannot be
> >       mis-used by arbitrary drivers, but drivers must register
> >       as a thermal driver to be able to trigger them.
> 
> In this case, we should consider Jean's point of redundancy.
> If CONFIG_THERMAL_HWMON is defined, the interfaces will be present in two
> different places.
coretemp should not register as a hwmon interface anymore.
These includes and the corresponding code can get ripped out of
coretemp.c:
#include <linux/hwmon.h>
#include <linux/sysfs.h>
#include <linux/hwmon-sysfs.h>
#include <linux/platform_device.h>
and coretemp's size should shrink rather significantly, only the
sensor logics and thermal driver communication should remain.

> Or should we make it optional in the thermal framework to register or
> Not register with hwmon ?
> May be by passing some flag in thermal_zone_device_register?
> Rui, need your suggestion here.
> 
> Moreover, if we make any driver that wants to use netlink, register with
> the thermal framework (redundancy issue should be resolved though..), the problem of
> unique originator id will be solved.
Yep.
> Then, we can use enum like
> the one Rui suggested, to start with. Later, when the need arises, we can add more.
I also like Henriques on-top suggestions.
Having an extra value for "which device" which could be CPU, Battery, Unkown,...
sounds like a good placeholder for upcoming needs.
If unsure one can simply send Unknown, but it might be very interesting
for the user whether graphics, cpu or whatever device is complaining.

    Thomas