[PATCH V5 0/2] watchdog: Sysfs status read support

[PATCH V5 0/2] watchdog: Sysfs status read support

[Pratyush Anand]

These patches provide support to read different watchdog device status
through sysfs interface.
 
Changes since V4:
* All sysfs code has been protected under CONFIG_WATCHDOG_SYSFS

Changes since V3:                                                                                    
* Added Reviewed by tag
* Corrected a checkpatch warning             

Changes since V2: 
* Used static struct class watchdog_class in stead of pointer. It helped to 
keep using device_create(). 
* Above logic was moved to a separate patch.  Changed subject line of other
patch to look more relevant  

Changes since V1(RFC): 
* Removed keepalive and start ABI
* timeout is read only now
* state returns text
* only supported ABI visible
* ABI contact changed to MAINTAINER
* unnecessary mutex removed
* aligned continuation with '('
* unnecessary initialization of status (= 0) corrected
* unnecessary else removed
* used __ATTRIBUTE_GROUPS
* removed watchdog_device_create and added functionality in
* watchdog_dev_register.
* optimized nowayout_show
* Now no -EOPNOTSUPP return for timeout read in case of
* wdd->timeout = 0.         

Pratyush Anand (2):
  watchdog: Use static struct class watchdog_class in stead of pointer
  watchdog: Read device status through sysfs attributes

 Documentation/ABI/testing/sysfs-class-watchdog |  51 +++++++++
 drivers/watchdog/Kconfig                       |   7 ++
 drivers/watchdog/watchdog_core.c               |  17 +--
 drivers/watchdog/watchdog_core.h               |   2 +-
 drivers/watchdog/watchdog_dev.c                | 140 ++++++++++++++++++++++++-
 5 files changed, 198 insertions(+), 19 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-watchdog

-- 
2.5.0

[PATCH V5 1/2] watchdog: Use static struct class watchdog_class in stead of pointer

[Pratyush Anand]

We need few sysfs attributes to know different status of a watchdog device.
To do that, we need to associate .dev_groups with watchdog_class. So
convert it from pointer to static.
Putting this static struct in watchdog_dev.c, so that static device
attributes defined in that file can be attached to it.

Signed-off-by: Pratyush Anand <panand@redhat.com>
Suggested-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/watchdog_core.c | 15 ++-------------
 drivers/watchdog/watchdog_core.h |  2 +-
 drivers/watchdog/watchdog_dev.c  | 26 ++++++++++++++++++++++----
 3 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 873f13972cf4..a55e846eec79 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -285,19 +285,9 @@ static int __init watchdog_deferred_registration(void)
 
 static int __init watchdog_init(void)
 {
-	int err;
-
-	watchdog_class = class_create(THIS_MODULE, "watchdog");
-	if (IS_ERR(watchdog_class)) {
-		pr_err("couldn't create class\n");
+	watchdog_class = watchdog_dev_init();
+	if (IS_ERR(watchdog_class))
 		return PTR_ERR(watchdog_class);
-	}
-
-	err = watchdog_dev_init();
-	if (err < 0) {
-		class_destroy(watchdog_class);
-		return err;
-	}
 
 	watchdog_deferred_registration();
 	return 0;
@@ -306,7 +296,6 @@ static int __init watchdog_init(void)
 static void __exit watchdog_exit(void)
 {
 	watchdog_dev_exit();
-	class_destroy(watchdog_class);
 	ida_destroy(&watchdog_ida);
 }
 
diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h
index 6c951418fca7..1c8d6b0e68c7 100644
--- a/drivers/watchdog/watchdog_core.h
+++ b/drivers/watchdog/watchdog_core.h
@@ -33,5 +33,5 @@
  */
 extern int watchdog_dev_register(struct watchdog_device *);
 extern int watchdog_dev_unregister(struct watchdog_device *);
-extern int __init watchdog_dev_init(void);
+extern struct class * __init watchdog_dev_init(void);
 extern void __exit watchdog_dev_exit(void);
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 56a649e66eb2..055a4e1b4c13 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -581,18 +581,35 @@ int watchdog_dev_unregister(struct watchdog_device *wdd)
 	return 0;
 }
 
+static struct class watchdog_class = {
+	.name =		"watchdog",
+	.owner =	THIS_MODULE,
+};
+
 /*
  *	watchdog_dev_init: init dev part of watchdog core
  *
  *	Allocate a range of chardev nodes to use for watchdog devices
  */
 
-int __init watchdog_dev_init(void)
+struct class * __init watchdog_dev_init(void)
 {
-	int err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
-	if (err < 0)
+	int err;
+
+	err = class_register(&watchdog_class);
+	if (err < 0) {
+		pr_err("couldn't register class\n");
+		return ERR_PTR(err);
+	}
+
+	err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
+	if (err < 0) {
 		pr_err("watchdog: unable to allocate char dev region\n");
-	return err;
+		class_unregister(&watchdog_class);
+		return ERR_PTR(err);
+	}
+
+	return &watchdog_class;
 }
 
 /*
@@ -604,4 +621,5 @@ int __init watchdog_dev_init(void)
 void __exit watchdog_dev_exit(void)
 {
 	unregister_chrdev_region(watchdog_devt, MAX_DOGS);
+	class_unregister(&watchdog_class);
 }
-- 
2.5.0

[PATCH V5 2/2] watchdog: Read device status through sysfs attributes

[Pratyush Anand]

This patch adds following attributes to watchdog device's sysfs interface
to read its different status.

* state - reads whether device is active or not
* identity - reads Watchdog device's identity string.
* timeout - reads current timeout.
* timeleft - reads timeleft before watchdog generates a reset
* bootstatus - reads status of the watchdog device at boot
* status - reads watchdog device's  internal status bits
* nowayout - reads whether nowayout feature was set or not

Testing with iTCO_wdt:
 # cd /sys/class/watchdog/watchdog1/
 # ls
bootstatus  dev  device  identity  nowayout  power  state
subsystem  timeleft  timeout  uevent
 # cat identity
iTCO_wdt
 # cat timeout
30
 # cat state
inactive
 # echo > /dev/watchdog1
 # cat timeleft
26
 # cat state
active
 # cat bootstatus
0
 # cat nowayout
0

Signed-off-by: Pratyush Anand <panand@redhat.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 Documentation/ABI/testing/sysfs-class-watchdog |  51 +++++++++++
 drivers/watchdog/Kconfig                       |   7 ++
 drivers/watchdog/watchdog_core.c               |   2 +-
 drivers/watchdog/watchdog_dev.c                | 114 +++++++++++++++++++++++++
 4 files changed, 173 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-watchdog

diff --git a/Documentation/ABI/testing/sysfs-class-watchdog b/Documentation/ABI/testing/sysfs-class-watchdog
new file mode 100644
index 000000000000..736046b33040
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-watchdog
@@ -0,0 +1,51 @@
+What:		/sys/class/watchdog/watchdogn/bootstatus
+Date:		August 2015
+Contact:	Wim Van Sebroeck <wim@iguana.be>
+Description:
+		It is a read only file. It contains status of the watchdog
+		device at boot. It is equivalent to WDIOC_GETBOOTSTATUS of
+		ioctl interface.
+
+What:		/sys/class/watchdog/watchdogn/identity
+Date:		August 2015
+Contact:	Wim Van Sebroeck <wim@iguana.be>
+Description:
+		It is a read only file. It contains identity string of
+		watchdog device.
+
+What:		/sys/class/watchdog/watchdogn/nowayout
+Date:		August 2015
+Contact:	Wim Van Sebroeck <wim@iguana.be>
+Description:
+		It is a read only file. While reading, it gives '1' if that
+		device supports nowayout feature else, it gives '0'.
+
+What:		/sys/class/watchdog/watchdogn/state
+Date:		August 2015
+Contact:	Wim Van Sebroeck <wim@iguana.be>
+Description:
+		It is a read only file. It gives active/inactive status of
+		watchdog device.
+
+What:		/sys/class/watchdog/watchdogn/status
+Date:		August 2015
+Contact:	Wim Van Sebroeck <wim@iguana.be>
+Description:
+		It is a read only file. It contains watchdog device's
+		internal status bits. It is equivalent to WDIOC_GETSTATUS
+		of ioctl interface.
+
+What:		/sys/class/watchdog/watchdogn/timeleft
+Date:		August 2015
+Contact:	Wim Van Sebroeck <wim@iguana.be>
+Description:
+		It is a read only file. It contains value of time left for
+		reset generation. It is equivalent to WDIOC_GETTIMELEFT of
+		ioctl interface.
+
+What:		/sys/class/watchdog/watchdogn/timeout
+Date:		August 2015
+Contact:	Wim Van Sebroeck <wim@iguana.be>
+Description:
+		It is a read only file. It is read to know about current
+		value of timeout programmed.
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 1c427beffadd..71e47dde7d4a 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -46,6 +46,13 @@ config WATCHDOG_NOWAYOUT
 	  get killed. If you say Y here, the watchdog cannot be stopped once
 	  it has been started.
 
+config WATCHDOG_SYSFS
+	bool "Read different watchdog information through sysfs"
+	default n
+	help
+	  Say Y here if you want to enable watchdog device status read through
+	  sysfs attributes.
+
 #
 # General Watchdog drivers
 #
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index a55e846eec79..62f4496b7975 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -194,7 +194,7 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 
 	devno = wdd->cdev.dev;
 	wdd->dev = device_create(watchdog_class, wdd->parent, devno,
-					NULL, "watchdog%d", wdd->id);
+					wdd, "watchdog%d", wdd->id);
 	if (IS_ERR(wdd->dev)) {
 		watchdog_dev_unregister(wdd);
 		ida_simple_remove(&watchdog_ida, id);
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 055a4e1b4c13..d93118e97d11 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -247,6 +247,117 @@ out_timeleft:
 	return err;
 }
 
+#ifdef CONFIG_WATCHDOG_SYSFS
+static ssize_t nowayout_show(struct device *dev, struct device_attribute *attr,
+				char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", !!test_bit(WDOG_NO_WAY_OUT, &wdd->status));
+}
+static DEVICE_ATTR_RO(nowayout);
+
+static ssize_t status_show(struct device *dev, struct device_attribute *attr,
+				char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+	ssize_t status;
+	unsigned int val;
+
+	status = watchdog_get_status(wdd, &val);
+	if (!status)
+		status = sprintf(buf, "%u\n", val);
+
+	return status;
+}
+static DEVICE_ATTR_RO(status);
+
+static ssize_t bootstatus_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", wdd->bootstatus);
+}
+static DEVICE_ATTR_RO(bootstatus);
+
+static ssize_t timeleft_show(struct device *dev, struct device_attribute *attr,
+				char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+	ssize_t status;
+	unsigned int val;
+
+	status = watchdog_get_timeleft(wdd, &val);
+	if (!status)
+		status = sprintf(buf, "%u\n", val);
+
+	return status;
+}
+static DEVICE_ATTR_RO(timeleft);
+
+static ssize_t timeout_show(struct device *dev, struct device_attribute *attr,
+				char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", wdd->timeout);
+}
+static DEVICE_ATTR_RO(timeout);
+
+static ssize_t identity_show(struct device *dev, struct device_attribute *attr,
+				char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%s\n", wdd->info->identity);
+}
+static DEVICE_ATTR_RO(identity);
+
+static ssize_t state_show(struct device *dev, struct device_attribute *attr,
+				char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+
+	if (watchdog_active(wdd))
+		return sprintf(buf, "active\n");
+
+	return sprintf(buf, "inactive\n");
+}
+static DEVICE_ATTR_RO(state);
+
+static umode_t wdt_is_visible(struct kobject *kobj, struct attribute *attr,
+				int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+	umode_t mode = attr->mode;
+
+	if (attr == &dev_attr_status.attr && !wdd->ops->status)
+		mode = 0;
+	else if (attr == &dev_attr_timeleft.attr && !wdd->ops->get_timeleft)
+		mode = 0;
+
+	return mode;
+}
+static struct attribute *wdt_attrs[] = {
+	&dev_attr_state.attr,
+	&dev_attr_identity.attr,
+	&dev_attr_timeout.attr,
+	&dev_attr_timeleft.attr,
+	&dev_attr_bootstatus.attr,
+	&dev_attr_status.attr,
+	&dev_attr_nowayout.attr,
+	NULL,
+};
+
+static const struct attribute_group wdt_group = {
+	.attrs = wdt_attrs,
+	.is_visible = wdt_is_visible,
+};
+__ATTRIBUTE_GROUPS(wdt);
+#endif
+
 /*
  *	watchdog_ioctl_op: call the watchdog drivers ioctl op if defined
  *	@wdd: the watchdog device to do the ioctl on
@@ -584,6 +695,9 @@ int watchdog_dev_unregister(struct watchdog_device *wdd)
 static struct class watchdog_class = {
 	.name =		"watchdog",
 	.owner =	THIS_MODULE,
+#ifdef CONFIG_WATCHDOG_SYSFS
+	.dev_groups =	wdt_groups,
+#endif
 };
 
 /*
-- 
2.5.0

[PATCH V5 2/2] watchdog: Read device status through sysfs attributes

[Pratyush Anand]

This patch adds following attributes to watchdog device's sysfs interface
to read its different status.

* state - reads whether device is active or not
* identity - reads Watchdog device's identity string.
* timeout - reads current timeout.
* timeleft - reads timeleft before watchdog generates a reset
* bootstatus - reads status of the watchdog device at boot
* status - reads watchdog device's  internal status bits
* nowayout - reads whether nowayout feature was set or not

Testing with iTCO_wdt:
 # cd /sys/class/watchdog/watchdog1/
 # ls
bootstatus  dev  device  identity  nowayout  power  state
subsystem  timeleft  timeout  uevent
 # cat identity
iTCO_wdt
 # cat timeout
30
 # cat state
inactive
 # echo > /dev/watchdog1
 # cat timeleft
26
 # cat state
active
 # cat bootstatus
0
 # cat nowayout
0

Signed-off-by: Pratyush Anand <panand@redhat.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 Documentation/ABI/testing/sysfs-class-watchdog |  51 +++++++++++
 drivers/watchdog/Kconfig                       |   7 ++
 drivers/watchdog/watchdog_core.c               |   2 +-
 drivers/watchdog/watchdog_dev.c                | 114 +++++++++++++++++++++++++
 4 files changed, 173 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-watchdog

diff --git a/Documentation/ABI/testing/sysfs-class-watchdog b/Documentation/ABI/testing/sysfs-class-watchdog
new file mode 100644
index 000000000000..736046b33040
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-watchdog
@@ -0,0 +1,51 @@
+What:		/sys/class/watchdog/watchdogn/bootstatus
+Date:		August 2015
+Contact:	Wim Van Sebroeck <wim@iguana.be>
+Description:
+		It is a read only file. It contains status of the watchdog
+		device at boot. It is equivalent to WDIOC_GETBOOTSTATUS of
+		ioctl interface.
+
+What:		/sys/class/watchdog/watchdogn/identity
+Date:		August 2015
+Contact:	Wim Van Sebroeck <wim@iguana.be>
+Description:
+		It is a read only file. It contains identity string of
+		watchdog device.
+
+What:		/sys/class/watchdog/watchdogn/nowayout
+Date:		August 2015
+Contact:	Wim Van Sebroeck <wim@iguana.be>
+Description:
+		It is a read only file. While reading, it gives '1' if that
+		device supports nowayout feature else, it gives '0'.
+
+What:		/sys/class/watchdog/watchdogn/state
+Date:		August 2015
+Contact:	Wim Van Sebroeck <wim@iguana.be>
+Description:
+		It is a read only file. It gives active/inactive status of
+		watchdog device.
+
+What:		/sys/class/watchdog/watchdogn/status
+Date:		August 2015
+Contact:	Wim Van Sebroeck <wim@iguana.be>
+Description:
+		It is a read only file. It contains watchdog device's
+		internal status bits. It is equivalent to WDIOC_GETSTATUS
+		of ioctl interface.
+
+What:		/sys/class/watchdog/watchdogn/timeleft
+Date:		August 2015
+Contact:	Wim Van Sebroeck <wim@iguana.be>
+Description:
+		It is a read only file. It contains value of time left for
+		reset generation. It is equivalent to WDIOC_GETTIMELEFT of
+		ioctl interface.
+
+What:		/sys/class/watchdog/watchdogn/timeout
+Date:		August 2015
+Contact:	Wim Van Sebroeck <wim@iguana.be>
+Description:
+		It is a read only file. It is read to know about current
+		value of timeout programmed.
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 1c427beffadd..71e47dde7d4a 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -46,6 +46,13 @@ config WATCHDOG_NOWAYOUT
 	  get killed. If you say Y here, the watchdog cannot be stopped once
 	  it has been started.
 
+config WATCHDOG_SYSFS
+	bool "Read different watchdog information through sysfs"
+	default n
+	help
+	  Say Y here if you want to enable watchdog device status read through
+	  sysfs attributes.
+
 #
 # General Watchdog drivers
 #
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index a55e846eec79..62f4496b7975 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -194,7 +194,7 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 
 	devno = wdd->cdev.dev;
 	wdd->dev = device_create(watchdog_class, wdd->parent, devno,
-					NULL, "watchdog%d", wdd->id);
+					wdd, "watchdog%d", wdd->id);
 	if (IS_ERR(wdd->dev)) {
 		watchdog_dev_unregister(wdd);
 		ida_simple_remove(&watchdog_ida, id);
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 055a4e1b4c13..d93118e97d11 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -247,6 +247,117 @@ out_timeleft:
 	return err;
 }
 
+#ifdef CONFIG_WATCHDOG_SYSFS
+static ssize_t nowayout_show(struct device *dev, struct device_attribute *attr,
+				char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", !!test_bit(WDOG_NO_WAY_OUT, &wdd->status));
+}
+static DEVICE_ATTR_RO(nowayout);
+
+static ssize_t status_show(struct device *dev, struct device_attribute *attr,
+				char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+	ssize_t status;
+	unsigned int val;
+
+	status = watchdog_get_status(wdd, &val);
+	if (!status)
+		status = sprintf(buf, "%u\n", val);
+
+	return status;
+}
+static DEVICE_ATTR_RO(status);
+
+static ssize_t bootstatus_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", wdd->bootstatus);
+}
+static DEVICE_ATTR_RO(bootstatus);
+
+static ssize_t timeleft_show(struct device *dev, struct device_attribute *attr,
+				char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+	ssize_t status;
+	unsigned int val;
+
+	status = watchdog_get_timeleft(wdd, &val);
+	if (!status)
+		status = sprintf(buf, "%u\n", val);
+
+	return status;
+}
+static DEVICE_ATTR_RO(timeleft);
+
+static ssize_t timeout_show(struct device *dev, struct device_attribute *attr,
+				char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", wdd->timeout);
+}
+static DEVICE_ATTR_RO(timeout);
+
+static ssize_t identity_show(struct device *dev, struct device_attribute *attr,
+				char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%s\n", wdd->info->identity);
+}
+static DEVICE_ATTR_RO(identity);
+
+static ssize_t state_show(struct device *dev, struct device_attribute *attr,
+				char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+
+	if (watchdog_active(wdd))
+		return sprintf(buf, "active\n");
+
+	return sprintf(buf, "inactive\n");
+}
+static DEVICE_ATTR_RO(state);
+
+static umode_t wdt_is_visible(struct kobject *kobj, struct attribute *attr,
+				int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+	umode_t mode = attr->mode;
+
+	if (attr == &dev_attr_status.attr && !wdd->ops->status)
+		mode = 0;
+	else if (attr == &dev_attr_timeleft.attr && !wdd->ops->get_timeleft)
+		mode = 0;
+
+	return mode;
+}
+static struct attribute *wdt_attrs[] = {
+	&dev_attr_state.attr,
+	&dev_attr_identity.attr,
+	&dev_attr_timeout.attr,
+	&dev_attr_timeleft.attr,
+	&dev_attr_bootstatus.attr,
+	&dev_attr_status.attr,
+	&dev_attr_nowayout.attr,
+	NULL,
+};
+
+static const struct attribute_group wdt_group = {
+	.attrs = wdt_attrs,
+	.is_visible = wdt_is_visible,
+};
+__ATTRIBUTE_GROUPS(wdt);
+#endif
+
 /*
  *	watchdog_ioctl_op: call the watchdog drivers ioctl op if defined
  *	@wdd: the watchdog device to do the ioctl on
@@ -584,6 +695,9 @@ int watchdog_dev_unregister(struct watchdog_device *wdd)
 static struct class watchdog_class = {
 	.name =		"watchdog",
 	.owner =	THIS_MODULE,
+#ifdef CONFIG_WATCHDOG_SYSFS
+	.dev_groups =	wdt_groups,
+#endif
 };
 
 /*
-- 
2.5.0

Re: [PATCH V5 2/2] watchdog: Read device status through sysfs attributes

[Guenter Roeck]

On 12/17/2015 04:23 AM, Pratyush Anand wrote:
> This patch adds following attributes to watchdog device's sysfs interface
> to read its different status.
>
> * state - reads whether device is active or not
> * identity - reads Watchdog device's identity string.
> * timeout - reads current timeout.
> * timeleft - reads timeleft before watchdog generates a reset
> * bootstatus - reads status of the watchdog device at boot
> * status - reads watchdog device's  internal status bits
> * nowayout - reads whether nowayout feature was set or not
>
> Testing with iTCO_wdt:
>   # cd /sys/class/watchdog/watchdog1/
>   # ls
> bootstatus  dev  device  identity  nowayout  power  state
> subsystem  timeleft  timeout  uevent
>   # cat identity
> iTCO_wdt
>   # cat timeout
> 30
>   # cat state
> inactive
>   # echo > /dev/watchdog1
>   # cat timeleft
> 26
>   # cat state
> active
>   # cat bootstatus
> 0
>   # cat nowayout
> 0
>
> Signed-off-by: Pratyush Anand <panand@redhat.com>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Almost. See below.

> ---
>   Documentation/ABI/testing/sysfs-class-watchdog |  51 +++++++++++
>   drivers/watchdog/Kconfig                       |   7 ++
>   drivers/watchdog/watchdog_core.c               |   2 +-
>   drivers/watchdog/watchdog_dev.c                | 114 +++++++++++++++++++++++++
>   4 files changed, 173 insertions(+), 1 deletion(-)
>   create mode 100644 Documentation/ABI/testing/sysfs-class-watchdog
>
> diff --git a/Documentation/ABI/testing/sysfs-class-watchdog b/Documentation/ABI/testing/sysfs-class-watchdog
> new file mode 100644
> index 000000000000..736046b33040
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-watchdog
> @@ -0,0 +1,51 @@
> +What:		/sys/class/watchdog/watchdogn/bootstatus
> +Date:		August 2015
> +Contact:	Wim Van Sebroeck <wim@iguana.be>
> +Description:
> +		It is a read only file. It contains status of the watchdog
> +		device at boot. It is equivalent to WDIOC_GETBOOTSTATUS of
> +		ioctl interface.
> +
> +What:		/sys/class/watchdog/watchdogn/identity
> +Date:		August 2015
> +Contact:	Wim Van Sebroeck <wim@iguana.be>
> +Description:
> +		It is a read only file. It contains identity string of
> +		watchdog device.
> +
> +What:		/sys/class/watchdog/watchdogn/nowayout
> +Date:		August 2015
> +Contact:	Wim Van Sebroeck <wim@iguana.be>
> +Description:
> +		It is a read only file. While reading, it gives '1' if that
> +		device supports nowayout feature else, it gives '0'.
> +
> +What:		/sys/class/watchdog/watchdogn/state
> +Date:		August 2015
> +Contact:	Wim Van Sebroeck <wim@iguana.be>
> +Description:
> +		It is a read only file. It gives active/inactive status of
> +		watchdog device.
> +
> +What:		/sys/class/watchdog/watchdogn/status
> +Date:		August 2015
> +Contact:	Wim Van Sebroeck <wim@iguana.be>
> +Description:
> +		It is a read only file. It contains watchdog device's
> +		internal status bits. It is equivalent to WDIOC_GETSTATUS
> +		of ioctl interface.
> +
> +What:		/sys/class/watchdog/watchdogn/timeleft
> +Date:		August 2015
> +Contact:	Wim Van Sebroeck <wim@iguana.be>
> +Description:
> +		It is a read only file. It contains value of time left for
> +		reset generation. It is equivalent to WDIOC_GETTIMELEFT of
> +		ioctl interface.
> +
> +What:		/sys/class/watchdog/watchdogn/timeout
> +Date:		August 2015
> +Contact:	Wim Van Sebroeck <wim@iguana.be>
> +Description:
> +		It is a read only file. It is read to know about current
> +		value of timeout programmed.
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 1c427beffadd..71e47dde7d4a 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -46,6 +46,13 @@ config WATCHDOG_NOWAYOUT
>   	  get killed. If you say Y here, the watchdog cannot be stopped once
>   	  it has been started.
>
> +config WATCHDOG_SYSFS
> +	bool "Read different watchdog information through sysfs"
> +	default n
> +	help
> +	  Say Y here if you want to enable watchdog device status read through
> +	  sysfs attributes.
> +
>   #
>   # General Watchdog drivers
>   #
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index a55e846eec79..62f4496b7975 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -194,7 +194,7 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>
>   	devno = wdd->cdev.dev;
>   	wdd->dev = device_create(watchdog_class, wdd->parent, devno,
> -					NULL, "watchdog%d", wdd->id);
> +					wdd, "watchdog%d", wdd->id);
>   	if (IS_ERR(wdd->dev)) {
>   		watchdog_dev_unregister(wdd);
>   		ida_simple_remove(&watchdog_ida, id);
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 055a4e1b4c13..d93118e97d11 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -247,6 +247,117 @@ out_timeleft:
>   	return err;
>   }
>
> +#ifdef CONFIG_WATCHDOG_SYSFS
> +static ssize_t nowayout_show(struct device *dev, struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d\n", !!test_bit(WDOG_NO_WAY_OUT, &wdd->status));
> +}
> +static DEVICE_ATTR_RO(nowayout);
> +
> +static ssize_t status_show(struct device *dev, struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +	ssize_t status;
> +	unsigned int val;
> +
> +	status = watchdog_get_status(wdd, &val);
> +	if (!status)
> +		status = sprintf(buf, "%u\n", val);
> +
> +	return status;
> +}
> +static DEVICE_ATTR_RO(status);
> +
> +static ssize_t bootstatus_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", wdd->bootstatus);
> +}
> +static DEVICE_ATTR_RO(bootstatus);
> +
> +static ssize_t timeleft_show(struct device *dev, struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +	ssize_t status;
> +	unsigned int val;
> +
> +	status = watchdog_get_timeleft(wdd, &val);
> +	if (!status)
> +		status = sprintf(buf, "%u\n", val);
> +
> +	return status;
> +}
> +static DEVICE_ATTR_RO(timeleft);
> +
> +static ssize_t timeout_show(struct device *dev, struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", wdd->timeout);
> +}
> +static DEVICE_ATTR_RO(timeout);
> +
> +static ssize_t identity_show(struct device *dev, struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%s\n", wdd->info->identity);
> +}
> +static DEVICE_ATTR_RO(identity);
> +
> +static ssize_t state_show(struct device *dev, struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(wdd))
> +		return sprintf(buf, "active\n");
> +
> +	return sprintf(buf, "inactive\n");
> +}
> +static DEVICE_ATTR_RO(state);
> +
> +static umode_t wdt_is_visible(struct kobject *kobj, struct attribute *attr,
> +				int n)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +	umode_t mode = attr->mode;
> +
> +	if (attr == &dev_attr_status.attr && !wdd->ops->status)
> +		mode = 0;
> +	else if (attr == &dev_attr_timeleft.attr && !wdd->ops->get_timeleft)
> +		mode = 0;
> +
> +	return mode;
> +}
> +static struct attribute *wdt_attrs[] = {
> +	&dev_attr_state.attr,
> +	&dev_attr_identity.attr,
> +	&dev_attr_timeout.attr,
> +	&dev_attr_timeleft.attr,
> +	&dev_attr_bootstatus.attr,
> +	&dev_attr_status.attr,
> +	&dev_attr_nowayout.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group wdt_group = {
> +	.attrs = wdt_attrs,
> +	.is_visible = wdt_is_visible,
> +};
> +__ATTRIBUTE_GROUPS(wdt);

#else

#define wdt_groups	NULL

Then you don't need the second #ifdef below.

> +#endif
> +
>   /*
>    *	watchdog_ioctl_op: call the watchdog drivers ioctl op if defined
>    *	@wdd: the watchdog device to do the ioctl on
> @@ -584,6 +695,9 @@ int watchdog_dev_unregister(struct watchdog_device *wdd)
>   static struct class watchdog_class = {
>   	.name =		"watchdog",
>   	.owner =	THIS_MODULE,
> +#ifdef CONFIG_WATCHDOG_SYSFS
> +	.dev_groups =	wdt_groups,
> +#endif
>   };
>
>   /*
>

Re: [PATCH V5 1/2] watchdog: Use static struct class watchdog_class in stead of pointer

[Guenter Roeck]

On 12/17/2015 04:23 AM, Pratyush Anand wrote:
> We need few sysfs attributes to know different status of a watchdog device.
> To do that, we need to associate .dev_groups with watchdog_class. So
> convert it from pointer to static.
> Putting this static struct in watchdog_dev.c, so that static device
> attributes defined in that file can be attached to it.
>
> Signed-off-by: Pratyush Anand <panand@redhat.com>
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

As things evolve, I'd by now prefer to move the call to device_create()
into watchdog_dev.c, but that can wait for a follow-up patch if Wim
is ok with this series.

Thanks,
Guenter

> ---
>   drivers/watchdog/watchdog_core.c | 15 ++-------------
>   drivers/watchdog/watchdog_core.h |  2 +-
>   drivers/watchdog/watchdog_dev.c  | 26 ++++++++++++++++++++++----
>   3 files changed, 25 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 873f13972cf4..a55e846eec79 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -285,19 +285,9 @@ static int __init watchdog_deferred_registration(void)
>
>   static int __init watchdog_init(void)
>   {
> -	int err;
> -
> -	watchdog_class = class_create(THIS_MODULE, "watchdog");
> -	if (IS_ERR(watchdog_class)) {
> -		pr_err("couldn't create class\n");
> +	watchdog_class = watchdog_dev_init();
> +	if (IS_ERR(watchdog_class))
>   		return PTR_ERR(watchdog_class);
> -	}
> -
> -	err = watchdog_dev_init();
> -	if (err < 0) {
> -		class_destroy(watchdog_class);
> -		return err;
> -	}
>
>   	watchdog_deferred_registration();
>   	return 0;
> @@ -306,7 +296,6 @@ static int __init watchdog_init(void)
>   static void __exit watchdog_exit(void)
>   {
>   	watchdog_dev_exit();
> -	class_destroy(watchdog_class);
>   	ida_destroy(&watchdog_ida);
>   }
>
> diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h
> index 6c951418fca7..1c8d6b0e68c7 100644
> --- a/drivers/watchdog/watchdog_core.h
> +++ b/drivers/watchdog/watchdog_core.h
> @@ -33,5 +33,5 @@
>    */
>   extern int watchdog_dev_register(struct watchdog_device *);
>   extern int watchdog_dev_unregister(struct watchdog_device *);
> -extern int __init watchdog_dev_init(void);
> +extern struct class * __init watchdog_dev_init(void);
>   extern void __exit watchdog_dev_exit(void);
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 56a649e66eb2..055a4e1b4c13 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -581,18 +581,35 @@ int watchdog_dev_unregister(struct watchdog_device *wdd)
>   	return 0;
>   }
>
> +static struct class watchdog_class = {
> +	.name =		"watchdog",
> +	.owner =	THIS_MODULE,
> +};
> +
>   /*
>    *	watchdog_dev_init: init dev part of watchdog core
>    *
>    *	Allocate a range of chardev nodes to use for watchdog devices
>    */
>
> -int __init watchdog_dev_init(void)
> +struct class * __init watchdog_dev_init(void)
>   {
> -	int err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
> -	if (err < 0)
> +	int err;
> +
> +	err = class_register(&watchdog_class);
> +	if (err < 0) {
> +		pr_err("couldn't register class\n");
> +		return ERR_PTR(err);
> +	}
> +
> +	err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
> +	if (err < 0) {
>   		pr_err("watchdog: unable to allocate char dev region\n");
> -	return err;
> +		class_unregister(&watchdog_class);
> +		return ERR_PTR(err);
> +	}
> +
> +	return &watchdog_class;
>   }
>
>   /*
> @@ -604,4 +621,5 @@ int __init watchdog_dev_init(void)
>   void __exit watchdog_dev_exit(void)
>   {
>   	unregister_chrdev_region(watchdog_devt, MAX_DOGS);
> +	class_unregister(&watchdog_class);
>   }
>

Re: [PATCH V5 1/2] watchdog: Use static struct class watchdog_class in stead of pointer

[Pratyush Anand]

On 17/12/2015:06:56:27 AM, Guenter Roeck wrote:
> On 12/17/2015 04:23 AM, Pratyush Anand wrote:
> >We need few sysfs attributes to know different status of a watchdog device.
> >To do that, we need to associate .dev_groups with watchdog_class. So
> >convert it from pointer to static.
> >Putting this static struct in watchdog_dev.c, so that static device
> >attributes defined in that file can be attached to it.
> >
> >Signed-off-by: Pratyush Anand <panand@redhat.com>
> >Suggested-by: Guenter Roeck <linux@roeck-us.net>
> >Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
> As things evolve, I'd by now prefer to move the call to device_create()
> into watchdog_dev.c, but that can wait for a follow-up patch if Wim
> is ok with this series.

Thanks for your quick review.

OK. I will wait for Wim's comment and then may be I will send another
version with your comment for patch 1/1 incorporated.

~Pratyush

Re: [PATCH V5 1/2] watchdog: Use static struct class watchdog_class in stead of pointer

[Guenter Roeck]

On 12/18/2015 08:22 PM, Pratyush Anand wrote:
> On 17/12/2015:06:56:27 AM, Guenter Roeck wrote:
>> On 12/17/2015 04:23 AM, Pratyush Anand wrote:
>>> We need few sysfs attributes to know different status of a watchdog device.
>>> To do that, we need to associate .dev_groups with watchdog_class. So
>>> convert it from pointer to static.
>>> Putting this static struct in watchdog_dev.c, so that static device
>>> attributes defined in that file can be attached to it.
>>>
>>> Signed-off-by: Pratyush Anand <panand@redhat.com>
>>> Suggested-by: Guenter Roeck <linux@roeck-us.net>
>>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>>
>> As things evolve, I'd by now prefer to move the call to device_create()
>> into watchdog_dev.c, but that can wait for a follow-up patch if Wim
>> is ok with this series.
>
> Thanks for your quick review.
>
> OK. I will wait for Wim's comment and then may be I will send another
> version with your comment for patch 1/1 incorporated.
>

I reworked my patch to call device_create() from watchdog_dev.c on top of
your patches and on top of Wim's watchdog-next branch. Hopefully we can get
that into v4.5.

Guenter

Re: [PATCH V5 1/2] watchdog: Use static struct class watchdog_class in stead of pointer

[Guenter Roeck]

On 12/18/2015 08:22 PM, Pratyush Anand wrote:
> On 17/12/2015:06:56:27 AM, Guenter Roeck wrote:
>> On 12/17/2015 04:23 AM, Pratyush Anand wrote:
>>> We need few sysfs attributes to know different status of a watchdog device.
>>> To do that, we need to associate .dev_groups with watchdog_class. So
>>> convert it from pointer to static.
>>> Putting this static struct in watchdog_dev.c, so that static device
>>> attributes defined in that file can be attached to it.
>>>
>>> Signed-off-by: Pratyush Anand <panand@redhat.com>
>>> Suggested-by: Guenter Roeck <linux@roeck-us.net>
>>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>>
>> As things evolve, I'd by now prefer to move the call to device_create()
>> into watchdog_dev.c, but that can wait for a follow-up patch if Wim
>> is ok with this series.
>
> Thanks for your quick review.
>
> OK. I will wait for Wim's comment and then may be I will send another
> version with your comment for patch 1/1 incorporated.
>

Thinking about it, my comment is really minor and should not hold up the
series from going forward. So let's just skip another version unless Wim
or someone else has any comments.

Thanks,
Guenter

Re: [PATCH V5 1/2] watchdog: Use static struct class watchdog_class in stead of pointer

[Pratyush Anand]

On 21/12/2015:07:52:59 AM, Guenter Roeck wrote:
> On 12/18/2015 08:22 PM, Pratyush Anand wrote:
> >On 17/12/2015:06:56:27 AM, Guenter Roeck wrote:
> >>On 12/17/2015 04:23 AM, Pratyush Anand wrote:
> >>>We need few sysfs attributes to know different status of a watchdog device.
> >>>To do that, we need to associate .dev_groups with watchdog_class. So
> >>>convert it from pointer to static.
> >>>Putting this static struct in watchdog_dev.c, so that static device
> >>>attributes defined in that file can be attached to it.
> >>>
> >>>Signed-off-by: Pratyush Anand <panand@redhat.com>
> >>>Suggested-by: Guenter Roeck <linux@roeck-us.net>
> >>>Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> >>
> >>As things evolve, I'd by now prefer to move the call to device_create()
> >>into watchdog_dev.c, but that can wait for a follow-up patch if Wim
> >>is ok with this series.
> >
> >Thanks for your quick review.
> >
> >OK. I will wait for Wim's comment and then may be I will send another
> >version with your comment for patch 1/1 incorporated.
> >
> 
> Thinking about it, my comment is really minor and should not hold up the
> series from going forward. So let's just skip another version unless Wim
> or someone else has any comments.

Thanks a lot :-)

~Pratyush

Re: [PATCH V5 0/2] watchdog: Sysfs status read support

[Wim Van Sebroeck]

Hi Pratyush,

> These patches provide support to read different watchdog device status
> through sysfs interface.
>  
> Changes since V4:
> * All sysfs code has been protected under CONFIG_WATCHDOG_SYSFS
> 
> Changes since V3:                                                                                    
> * Added Reviewed by tag
> * Corrected a checkpatch warning             
> 
> Changes since V2: 
> * Used static struct class watchdog_class in stead of pointer. It helped to 
> keep using device_create(). 
> * Above logic was moved to a separate patch.  Changed subject line of other
> patch to look more relevant  
> 
> Changes since V1(RFC): 
> * Removed keepalive and start ABI
> * timeout is read only now
> * state returns text
> * only supported ABI visible
> * ABI contact changed to MAINTAINER
> * unnecessary mutex removed
> * aligned continuation with '('
> * unnecessary initialization of status (= 0) corrected
> * unnecessary else removed
> * used __ATTRIBUTE_GROUPS
> * removed watchdog_device_create and added functionality in
> * watchdog_dev_register.
> * optimized nowayout_show
> * Now no -EOPNOTSUPP return for timeout read in case of
> * wdd->timeout = 0.         
> 
> Pratyush Anand (2):
>   watchdog: Use static struct class watchdog_class in stead of pointer
>   watchdog: Read device status through sysfs attributes
> 
>  Documentation/ABI/testing/sysfs-class-watchdog |  51 +++++++++
>  drivers/watchdog/Kconfig                       |   7 ++
>  drivers/watchdog/watchdog_core.c               |  17 +--
>  drivers/watchdog/watchdog_core.h               |   2 +-
>  drivers/watchdog/watchdog_dev.c                | 140 ++++++++++++++++++++++++-
>  5 files changed, 198 insertions(+), 19 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-watchdog

This patchset has been added to linux-watchdog-next.
The second patch has been altered as suggested by Guenter (with the #else ; #define wdt_groups NULL statements).

Kind regards,
Wim.

Re: [PATCH V5 0/2] watchdog: Sysfs status read support

[Pratyush Anand]

Hi Wim,

On 27/12/2015:05:57:56 PM, Wim Van Sebroeck wrote:
> 
> This patchset has been added to linux-watchdog-next.
> The second patch has been altered as suggested by Guenter (with the #else ; #define wdt_groups NULL statements).


Thanks a lot for adding them.

~Pratyush