[PATCH RFC] watchdog: Add watchdog device control through sysfs attributes

[PATCH RFC] watchdog: Add watchdog device control through sysfs attributes

[Pratyush Anand]

This patch adds following attributes to watchdog device's sysfs interface.

* start - writes non zero value to start and zero to stop
* state - reads whether device is active or not(1 for active and 0 for
inactive)
* identity - reads Watchdog device's identity string.
* keepalive - writes to ping a watchdog device
* timeout - reads current timeout and writes to program a new 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  keepalive  nowayout  power  start  state
status  subsystem  timeleft  timeout  uevent
 # cat identity
iTCO_wdt
 # cat timeout
30
 # echo 1 > start
 # cat timeleft
26
 # echo 120 > timeout
 # cat timeleft
116
 # echo > keepalive
 # cat timeleft
118
 # cat state
1
 # echo 0 > start
 # cat state
0
 # cat bootstatus
0
 # cat nowayout
0
 # cat status
cat: status: Operation not supported

Signed-off-by: Pratyush Anand <panand@redhat.com>
---
 Documentation/ABI/testing/sysfs-class-watchdog |  74 +++++++++
 drivers/watchdog/watchdog_core.c               |   6 +-
 drivers/watchdog/watchdog_core.h               |   2 +
 drivers/watchdog/watchdog_dev.c                | 206 +++++++++++++++++++++++++
 4 files changed, 284 insertions(+), 4 deletions(-)
 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..31e7be53edf8
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-watchdog
@@ -0,0 +1,74 @@
+What:		/sys/class/watchdog/watchdogn/bootstatus
+Date:		August 2015
+Contact:	Pratyush Anand <panand@redhat.com>
+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:	Pratyush Anand <panand@redhat.com>
+Description:
+		It is a read only file. It contains identity string of
+		watchdog device.
+
+What:		/sys/class/watchdog/watchdogn/keepalive
+Date:		August 2015
+Contact:	Pratyush Anand <panand@redhat.com>
+Description:
+		It is a write only file. It is written to ping a watchdog
+		device to keep it alive. It is equivalent to
+		WDIOC_KEEPALIVE of ioctl interface.
+
+What:		/sys/class/watchdog/watchdogn/nowayout
+Date:		August 2015
+Contact:	Pratyush Anand <panand@redhat.com>
+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/start
+Date:		August 2015
+Contact:	Pratyush Anand <panand@redhat.com>
+Description:
+		It is a write only file. Writing '1' will trigger that
+		watchdog device and writing '0' will stop it. These are
+		equivalent to WDIOS_ENABLECARD and WDIOS_DISABLECARD of ioctl
+		interface. If a device has nowayout programmed, then that
+		can not be stopped. Therefore, it is recommended to read
+		state file to insure that whether watchdog device was
+		stopped or not after writing '0'.
+
+What:		/sys/class/watchdog/watchdogn/state
+Date:		August 2015
+Contact:	Pratyush Anand <panand@redhat.com>
+Description:
+		It is a read only file. If it is read as '1' then a watchdog
+		device is active. If it is read as '0' then a watchdog
+		device is inactive.
+
+What:		/sys/class/watchdog/watchdogn/status
+Date:		August 2015
+Contact:	Pratyush Anand <panand@redhat.com>
+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:	Pratyush Anand <panand@redhat.com>
+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:	Pratyush Anand <panand@redhat.com>
+Description:
+		It is a read/write file. It is read to know about current
+		timeout and written to program a new timeout value. These
+		are equivalent to WDIOC_SETTIMEOUT and WDIOC_GETTIMEOUT of
+		ioctl interface.
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 1a8059455413..703ff7b23f31 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -139,7 +139,7 @@ EXPORT_SYMBOL_GPL(watchdog_init_timeout);
 
 static int __watchdog_register_device(struct watchdog_device *wdd)
 {
-	int ret, id, devno;
+	int ret, id;
 
 	if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
 		return -EINVAL;
@@ -181,9 +181,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->dev = watchdog_device_create(watchdog_class, wdd);
 	if (IS_ERR(wdd->dev)) {
 		watchdog_dev_unregister(wdd);
 		ida_simple_remove(&watchdog_ida, id);
diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h
index 6c951418fca7..36abd15ffcea 100644
--- a/drivers/watchdog/watchdog_core.h
+++ b/drivers/watchdog/watchdog_core.h
@@ -31,6 +31,8 @@
 /*
  *	Functions/procedures to be called by the core
  */
+struct device * watchdog_device_create(struct class *,
+				struct watchdog_device *);
 extern int watchdog_dev_register(struct watchdog_device *);
 extern int watchdog_dev_unregister(struct watchdog_device *);
 extern int __init watchdog_dev_init(void);
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 6aaefbad303e..1186b5a918e9 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -248,6 +248,212 @@ out_timeleft:
 	return err;
 }
 
+static ssize_t nowayout_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+	bool nowayout = false;
+
+	mutex_lock(&wdd->lock);
+	if (test_bit(WDOG_NO_WAY_OUT, &wdd->status))
+		nowayout = true;
+	mutex_unlock(&wdd->lock);
+
+	return sprintf(buf, "%d\n", nowayout);
+}
+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);
+	ssize_t status;
+
+	mutex_lock(&wdd->lock);
+	status = sprintf(buf, "%u\n", wdd->bootstatus);
+	mutex_unlock(&wdd->lock);
+
+	return status;
+}
+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_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+	unsigned int val;
+	ssize_t status = 0;
+
+	status = kstrtouint(buf, 0, &val);
+	if (!status) {
+		status = watchdog_set_timeout(wdd, val);
+		if (status >= 0)
+			status = watchdog_ping(wdd);
+	}
+
+	if (status < 0)
+		return status;
+	else
+		return size;
+}
+
+static ssize_t timeout_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+	ssize_t status;
+
+	mutex_lock(&wdd->lock);
+	if (wdd->timeout == 0)
+		status = -EOPNOTSUPP;
+	else
+		status = sprintf(buf, "%u\n", wdd->timeout);
+	mutex_unlock(&wdd->lock);
+
+	return status;
+}
+static DEVICE_ATTR_RW(timeout);
+
+static ssize_t keepalive_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+	ssize_t status = 0;
+
+	mutex_lock(&wdd->lock);
+	if (!(wdd->info->options & WDIOF_KEEPALIVEPING))
+		status = -EOPNOTSUPP;
+	mutex_unlock(&wdd->lock);
+
+	if (!status)
+		status = watchdog_ping(wdd);
+
+	if (status < 0)
+		return status;
+	else
+		return size;
+}
+static DEVICE_ATTR_WO(keepalive);
+
+static ssize_t identity_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+	ssize_t status;
+
+	mutex_lock(&wdd->lock);
+	status = sprintf(buf, "%s\n", wdd->info->identity);
+	mutex_unlock(&wdd->lock);
+
+	return status;
+}
+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);
+	bool active;
+
+	mutex_lock(&wdd->lock);
+	active = watchdog_active(wdd);
+	mutex_unlock(&wdd->lock);
+
+	return sprintf(buf, "%d\n", active);
+}
+static DEVICE_ATTR_RO(state);
+
+static ssize_t start_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+	ssize_t status = 0;
+	unsigned int val;
+
+	status = kstrtouint(buf, 0, &val);
+	if (status)
+		return status;
+	if (val)
+		status = watchdog_start(wdd);
+	else
+		status = watchdog_stop(wdd);
+
+	if (status < 0)
+		return status;
+	else
+		return size;
+}
+static DEVICE_ATTR_WO(start);
+
+static struct attribute *wdt_attrs[] = {
+	&dev_attr_start.attr,
+	&dev_attr_state.attr,
+	&dev_attr_identity.attr,
+	&dev_attr_keepalive.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,
+};
+
+static const struct attribute_group *wdt_groups[] = {
+	&wdt_group,
+	NULL
+};
+
+/*
+ *	watchdog_device_create: create a struct device for a given watchdog
+ *	device
+ *	@wdc: watchdog class with which created device is associated
+ *	@wdd: watchdog device
+ *
+ *	Creates a struct device corresponding to given watchdog device and
+ *	associates a device attribute_group with it.
+ */
+struct device * watchdog_device_create(struct class *wdc,
+					struct watchdog_device *wdd)
+{
+	return device_create_with_groups(wdc, wdd->parent, wdd->cdev.dev,
+			wdd, wdt_groups, "watchdog%d", wdd->id);
+}
+
 /*
  *	watchdog_ioctl_op: call the watchdog drivers ioctl op if defined
  *	@wddev: the watchdog device to do the ioctl on
-- 
2.4.3

Re: [PATCH RFC] watchdog: Add watchdog device control through sysfs attributes

[Pratyush Anand]

Hi Guenter,

On 21/08/2015:11:18:12 PM, Pratyush Anand wrote:
> This patch adds following attributes to watchdog device's sysfs interface.

Please see if you can review it.

Does this patch look fine to you? If yes then do I need to resend it by removing
RFC tag.

~Pratyush

Re: [PATCH RFC] watchdog: Add watchdog device control through sysfs attributes

[Pratyush Anand]

On 25/08/2015:10:20:01 AM, Guenter Roeck wrote:
> On 08/25/2015 09:45 AM, Pratyush Anand wrote:
> >Hi Guenter,
> >
> >On 21/08/2015:11:18:12 PM, Pratyush Anand wrote:
> >>This patch adds following attributes to watchdog device's sysfs interface.
> >
> >Please see if you can review it.
> >
> >Does this patch look fine to you? If yes then do I need to resend it by removing
> >RFC tag.
> >
> 
> I didn't have time to review your patch in detail. Sorry, but please keep in mind
> that this is all volunteer work for me.

It's understandable. Sorry for my impatience. Please take your time.

> 
> I don't feel comfortable with modifying the watchdog state through sysfs.
> That changes the ABI substantially, since it is no longer necessary to have
> an application to activate the watchdog. It also messes up internal state keeping,
> since there is now no longer a well defined state indicating that the kernel
> expects user space to send heartbeats, or if the kernel needs to do it (in the
> driver or, with the patch set I submitted a couple of weeks ago, in the watchdog
> core).

I will be OK with removing 'start'.

> 
> Given that, and I really don't have time to look into it more deeply,
> I put the whole thing on a back burner for now.
> 

No issue, will wait :-)

~Pratyush

Re: [RFC] watchdog: Add watchdog device control through sysfs attributes

[Guenter Roeck]

On Fri, Aug 21, 2015 at 11:18:12PM +0530, Pratyush Anand wrote:
> This patch adds following attributes to watchdog device's sysfs interface.
> 
> * start - writes non zero value to start and zero to stop

I would suggest to drop this attribute, as well as 'keepalive'.
Both will make keeping the internal state very difficult, especially
when we add support for heartbeats triggered by the watchdog core.

> * state - reads whether device is active or not(1 for active and 0 for
> inactive)

How about reporting the state as text ?

> * identity - reads Watchdog device's identity string.
> * keepalive - writes to ping a watchdog device
> * timeout - reads current timeout and writes to program a new 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  keepalive  nowayout  power  start  state
> status  subsystem  timeleft  timeout  uevent
>  # cat identity
> iTCO_wdt
>  # cat timeout
> 30
>  # echo 1 > start
>  # cat timeleft
> 26
>  # echo 120 > timeout
>  # cat timeleft
> 116
>  # echo > keepalive
>  # cat timeleft
> 118
>  # cat state
> 1
>  # echo 0 > start
>  # cat state
> 0
>  # cat bootstatus
> 0
>  # cat nowayout
> 0
>  # cat status
> cat: status: Operation not supported
> 
Unsupported attributes should not appear in the first place.
Please use is_visible to determine if an attribute should
be there or not.

> Signed-off-by: Pratyush Anand <panand@redhat.com>
> ---
>  Documentation/ABI/testing/sysfs-class-watchdog |  74 +++++++++
>  drivers/watchdog/watchdog_core.c               |   6 +-
>  drivers/watchdog/watchdog_core.h               |   2 +
>  drivers/watchdog/watchdog_dev.c                | 206 +++++++++++++++++++++++++
>  4 files changed, 284 insertions(+), 4 deletions(-)
>  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..31e7be53edf8
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-watchdog
> @@ -0,0 +1,74 @@
> +What:		/sys/class/watchdog/watchdogn/bootstatus
> +Date:		August 2015
> +Contact:	Pratyush Anand <panand@redhat.com>

Who is normally listed here ? Shouldn't it be the maintainer ?

> +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:	Pratyush Anand <panand@redhat.com>
> +Description:
> +		It is a read only file. It contains identity string of
> +		watchdog device.
> +
> +What:		/sys/class/watchdog/watchdogn/keepalive
> +Date:		August 2015
> +Contact:	Pratyush Anand <panand@redhat.com>
> +Description:
> +		It is a write only file. It is written to ping a watchdog
> +		device to keep it alive. It is equivalent to
> +		WDIOC_KEEPALIVE of ioctl interface.
> +
> +What:		/sys/class/watchdog/watchdogn/nowayout
> +Date:		August 2015
> +Contact:	Pratyush Anand <panand@redhat.com>
> +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/start
> +Date:		August 2015
> +Contact:	Pratyush Anand <panand@redhat.com>
> +Description:
> +		It is a write only file. Writing '1' will trigger that
> +		watchdog device and writing '0' will stop it. These are
> +		equivalent to WDIOS_ENABLECARD and WDIOS_DISABLECARD of ioctl
> +		interface. If a device has nowayout programmed, then that
> +		can not be stopped. Therefore, it is recommended to read
> +		state file to insure that whether watchdog device was
> +		stopped or not after writing '0'.
> +
> +What:		/sys/class/watchdog/watchdogn/state
> +Date:		August 2015
> +Contact:	Pratyush Anand <panand@redhat.com>
> +Description:
> +		It is a read only file. If it is read as '1' then a watchdog
> +		device is active. If it is read as '0' then a watchdog
> +		device is inactive.
> +
> +What:		/sys/class/watchdog/watchdogn/status
> +Date:		August 2015
> +Contact:	Pratyush Anand <panand@redhat.com>
> +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:	Pratyush Anand <panand@redhat.com>
> +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:	Pratyush Anand <panand@redhat.com>
> +Description:
> +		It is a read/write file. It is read to know about current
> +		timeout and written to program a new timeout value. These
> +		are equivalent to WDIOC_SETTIMEOUT and WDIOC_GETTIMEOUT of
> +		ioctl interface.
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 1a8059455413..703ff7b23f31 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -139,7 +139,7 @@ EXPORT_SYMBOL_GPL(watchdog_init_timeout);
>  
>  static int __watchdog_register_device(struct watchdog_device *wdd)
>  {
> -	int ret, id, devno;
> +	int ret, id;
>  
>  	if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
>  		return -EINVAL;
> @@ -181,9 +181,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->dev = watchdog_device_create(watchdog_class, wdd);

Can we do this in watchdog_dev_register() ?
Seems to make more sense than adding another callback into watchdog_dev.c.

>  	if (IS_ERR(wdd->dev)) {
>  		watchdog_dev_unregister(wdd);
>  		ida_simple_remove(&watchdog_ida, id);
> diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h
> index 6c951418fca7..36abd15ffcea 100644
> --- a/drivers/watchdog/watchdog_core.h
> +++ b/drivers/watchdog/watchdog_core.h
> @@ -31,6 +31,8 @@
>  /*
>   *	Functions/procedures to be called by the core
>   */
> +struct device * watchdog_device_create(struct class *,
> +				struct watchdog_device *);
>  extern int watchdog_dev_register(struct watchdog_device *);
>  extern int watchdog_dev_unregister(struct watchdog_device *);
>  extern int __init watchdog_dev_init(void);
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 6aaefbad303e..1186b5a918e9 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -248,6 +248,212 @@ out_timeleft:
>  	return err;
>  }
>  
> +static ssize_t nowayout_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +	bool nowayout = false;
> +
> +	mutex_lock(&wdd->lock);
> +	if (test_bit(WDOG_NO_WAY_OUT, &wdd->status))
> +		nowayout = true;
> +	mutex_unlock(&wdd->lock);
> +
> +	return sprintf(buf, "%d\n", nowayout);

	return sprintf(buf, "%d\n", !!test_bit(WDOG_NO_WAY_OUT, &wdd->status));

should do it, and the lock doesn't seem to be very helpful,
as it doesn't make a difference when the flag is evaluated.

> +}
> +static DEVICE_ATTR_RO(nowayout);
> +
> +static ssize_t status_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)

Please align continuation lines with '('.

> +{
> +	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);
> +

This attribute should only be visible if supported.

> +	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);
> +	ssize_t status;
> +
> +	mutex_lock(&wdd->lock);
> +	status = sprintf(buf, "%u\n", wdd->bootstatus);
> +	mutex_unlock(&wdd->lock);
> +
Useless lock.

> +	return status;
> +}
> +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);
> +
This attribute should only be visible if supported.

> +	return status;
> +}
> +static DEVICE_ATTR_RO(timeleft);
> +
> +static ssize_t timeout_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +	unsigned int val;
> +	ssize_t status = 0;

Unnecessary initialization.

> +
> +	status = kstrtouint(buf, 0, &val);
> +	if (!status) {
> +		status = watchdog_set_timeout(wdd, val);
> +		if (status >= 0)
> +			status = watchdog_ping(wdd);
> +	}
> +
> +	if (status < 0)
> +		return status;
> +	else

Unnecessary else.

> +		return size;
> +}
> +
> +static ssize_t timeout_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +	ssize_t status;
> +
> +	mutex_lock(&wdd->lock);
> +	if (wdd->timeout == 0)
> +		status = -EOPNOTSUPP;

Why ?

> +	else
> +		status = sprintf(buf, "%u\n", wdd->timeout);
> +	mutex_unlock(&wdd->lock);
> +
Unnecessary lock.

> +	return status;
> +}
> +static DEVICE_ATTR_RW(timeout);
> +
> +static ssize_t keepalive_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +	ssize_t status = 0;
> +
> +	mutex_lock(&wdd->lock);
> +	if (!(wdd->info->options & WDIOF_KEEPALIVEPING))
> +		status = -EOPNOTSUPP;

In that case the attribute should not be there to start with.

Anyway, I would prefer if this attribute would not be there.
Otherwise you'd at least have to check the current state to see
if the watchdog is running in the first place, and you would have
to define what to do if it isn't. Lots of complexity for no clear gain.

> +	mutex_unlock(&wdd->lock);
> +
The lock is unnecessary.

> +	if (!status)
> +		status = watchdog_ping(wdd);
> +
> +	if (status < 0)
> +		return status;
> +	else
> +		return size;
> +}
> +static DEVICE_ATTR_WO(keepalive);
> +
> +static ssize_t identity_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +	ssize_t status;
> +
> +	mutex_lock(&wdd->lock);
> +	status = sprintf(buf, "%s\n", wdd->info->identity);
> +	mutex_unlock(&wdd->lock);
> +
Unnecessary lock.

> +	return status;
> +}
> +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);
> +	bool active;
> +
> +	mutex_lock(&wdd->lock);
> +	active = watchdog_active(wdd);
> +	mutex_unlock(&wdd->lock);
> +
Unnecessary lock.

> +	return sprintf(buf, "%d\n", active);
> +}
> +static DEVICE_ATTR_RO(state);
> +
> +static ssize_t start_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{

I would prefer not to have this attribute.

> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +	ssize_t status = 0;
> +	unsigned int val;
> +
> +	status = kstrtouint(buf, 0, &val);
> +	if (status)
> +		return status;
> +	if (val)
> +		status = watchdog_start(wdd);
> +	else
> +		status = watchdog_stop(wdd);
> +
> +	if (status < 0)
> +		return status;
> +	else
> +		return size;
> +}
> +static DEVICE_ATTR_WO(start);
> +
> +static struct attribute *wdt_attrs[] = {
> +	&dev_attr_start.attr,
> +	&dev_attr_state.attr,
> +	&dev_attr_identity.attr,
> +	&dev_attr_keepalive.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,
> +};
> +
> +static const struct attribute_group *wdt_groups[] = {
> +	&wdt_group,
> +	NULL
> +};

You should be able to use ATTRIBUTE_GROUPS or __ATTRIBUTE_GROUPS.

> +
> +/*
> + *	watchdog_device_create: create a struct device for a given watchdog
> + *	device
> + *	@wdc: watchdog class with which created device is associated
> + *	@wdd: watchdog device
> + *
> + *	Creates a struct device corresponding to given watchdog device and
> + *	associates a device attribute_group with it.
> + */
> +struct device * watchdog_device_create(struct class *wdc,
> +					struct watchdog_device *wdd)
> +{
> +	return device_create_with_groups(wdc, wdd->parent, wdd->cdev.dev,
> +			wdd, wdt_groups, "watchdog%d", wdd->id);
> +}
> +
>  /*
>   *	watchdog_ioctl_op: call the watchdog drivers ioctl op if defined
>   *	@wddev: the watchdog device to do the ioctl on

Re: [RFC] watchdog: Add watchdog device control through sysfs attributes

[Pratyush Anand]

Hi Guenter,

Thanks for review.

On 29/08/2015:09:51:24 AM, Guenter Roeck wrote:
> On Fri, Aug 21, 2015 at 11:18:12PM +0530, Pratyush Anand wrote:
> > This patch adds following attributes to watchdog device's sysfs interface.
> > 
> > * start - writes non zero value to start and zero to stop
> 
> I would suggest to drop this attribute, as well as 'keepalive'.
> Both will make keeping the internal state very difficult, especially
> when we add support for heartbeats triggered by the watchdog core.
> 

OK.

> > * state - reads whether device is active or not(1 for active and 0 for
> > inactive)
> 
> How about reporting the state as text ?

Will change

> 
> > * identity - reads Watchdog device's identity string.
> > * keepalive - writes to ping a watchdog device
> > * timeout - reads current timeout and writes to program a new 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  keepalive  nowayout  power  start  state
> > status  subsystem  timeleft  timeout  uevent
> >  # cat identity
> > iTCO_wdt
> >  # cat timeout
> > 30
> >  # echo 1 > start
> >  # cat timeleft
> > 26
> >  # echo 120 > timeout
> >  # cat timeleft
> > 116
> >  # echo > keepalive
> >  # cat timeleft
> > 118
> >  # cat state
> > 1
> >  # echo 0 > start
> >  # cat state
> > 0
> >  # cat bootstatus
> > 0
> >  # cat nowayout
> > 0
> >  # cat status
> > cat: status: Operation not supported
> > 
> Unsupported attributes should not appear in the first place.
> Please use is_visible to determine if an attribute should
> be there or not.

Thanks :-).. Will modify.

> 
> > Signed-off-by: Pratyush Anand <panand@redhat.com>
> > ---
> >  Documentation/ABI/testing/sysfs-class-watchdog |  74 +++++++++
> >  drivers/watchdog/watchdog_core.c               |   6 +-
> >  drivers/watchdog/watchdog_core.h               |   2 +
> >  drivers/watchdog/watchdog_dev.c                | 206 +++++++++++++++++++++++++
> >  4 files changed, 284 insertions(+), 4 deletions(-)
> >  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..31e7be53edf8
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-watchdog
> > @@ -0,0 +1,74 @@
> > +What:		/sys/class/watchdog/watchdogn/bootstatus
> > +Date:		August 2015
> > +Contact:	Pratyush Anand <panand@redhat.com>
> 
> Who is normally listed here ? Shouldn't it be the maintainer ?

I am not sure.. Will be happy to change it to
Wim Van Sebroeck <wim@iguana.be>.

> 
> >  static int __watchdog_register_device(struct watchdog_device *wdd)
> >  {
> > -	int ret, id, devno;
> > +	int ret, id;
> >  
> >  	if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
> >  		return -EINVAL;
> > @@ -181,9 +181,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->dev = watchdog_device_create(watchdog_class, wdd);
> 
> Can we do this in watchdog_dev_register() ?
> Seems to make more sense than adding another callback into watchdog_dev.c.

I had thought of this, but then will have to change prototype of
watchdog_dev_register() to accept struct class *wdc and which in turn will cause
to change all users of watchdog_dev_register().
Other option could be to add struct class *wdc in struct watchdog_device, but it
did not look nice to me.

> 
> >  	if (IS_ERR(wdd->dev)) {
> > +static ssize_t nowayout_show(struct device *dev,
> > +		struct device_attribute *attr, char *buf)
> > +{
> > +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> > +	bool nowayout = false;
> > +
> > +	mutex_lock(&wdd->lock);
> > +	if (test_bit(WDOG_NO_WAY_OUT, &wdd->status))
> > +		nowayout = true;
> > +	mutex_unlock(&wdd->lock);
> > +
> > +	return sprintf(buf, "%d\n", nowayout);
> 
> 	return sprintf(buf, "%d\n", !!test_bit(WDOG_NO_WAY_OUT, &wdd->status));
> 
> should do it, and the lock doesn't seem to be very helpful,
> as it doesn't make a difference when the flag is evaluated.

OK.

> 
> > +}
> > +static DEVICE_ATTR_RO(nowayout);
> > +
> > +static ssize_t status_show(struct device *dev,
> > +		struct device_attribute *attr, char *buf)
> 
> Please align continuation lines with '('.

OK.

> 
> > +{
> > +	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);
> > +
> 
> This attribute should only be visible if supported.

yes.

> 
> > +	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);
> > +	ssize_t status;
> > +
> > +	mutex_lock(&wdd->lock);
> > +	status = sprintf(buf, "%u\n", wdd->bootstatus);
> > +	mutex_unlock(&wdd->lock);
> > +
> Useless lock.

yes.

> 
> > +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);
> > +
> This attribute should only be visible if supported.

yes
> 
> > +	return status;
> > +}
> > +static DEVICE_ATTR_RO(timeleft);
> > +
> > +static ssize_t timeout_store(struct device *dev,
> > +		struct device_attribute *attr, const char *buf, size_t size)
> > +{
> > +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> > +	unsigned int val;
> > +	ssize_t status = 0;
> 
> Unnecessary initialization.

will correct.

> 
> > +
> > +	status = kstrtouint(buf, 0, &val);
> > +	if (!status) {
> > +		status = watchdog_set_timeout(wdd, val);
> > +		if (status >= 0)
> > +			status = watchdog_ping(wdd);
> > +	}
> > +
> > +	if (status < 0)
> > +		return status;
> > +	else
> 
> Unnecessary else.


ok

> 
> > +		return size;
> > +}
> > +
> > +static ssize_t timeout_show(struct device *dev,
> > +		struct device_attribute *attr, char *buf)
> > +{
> > +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> > +	ssize_t status;
> > +
> > +	mutex_lock(&wdd->lock);
> > +	if (wdd->timeout == 0)
> > +		status = -EOPNOTSUPP;
> 
> Why ?

It has been copied from case WDIOC_GETTIMEOUT: which says:
timeout == 0 means that we don't know the timeout.

> > +
> > +static const struct attribute_group wdt_group = {
> > +	.attrs = wdt_attrs,
> > +};
> > +
> > +static const struct attribute_group *wdt_groups[] = {
> > +	&wdt_group,
> > +	NULL
> > +};
> 
> You should be able to use ATTRIBUTE_GROUPS or __ATTRIBUTE_GROUPS.

Yes, with is_visible __ATTRIBUTE_GROUPS can still be used.

~Pratyush

Re: [RFC] watchdog: Add watchdog device control through sysfs attributes

[Pratyush Anand]

Hi Guenter,
On 30/08/2015:07:58:27 AM, Guenter Roeck wrote:
> 
> I can only see a single caller of watchdog_dev_register().
> Changing that one call should not be such a problem.

Sure..Probably I was sleeping when I greped watchdog_dev_register :(

> 
> Also, you could name the groups variable something like wdc_class_dev_groups,
> make it public, and assign it in watchdog_core.c as follows.

I will avoid making public whereever possible.

> 
> static struct class watchdog_class = {
> 	.name = "watchdog",
> 	.owner = THIS_MODULE,
> 	.dev_groups = wdc_class_dev_groups,
> };
> 
> Then use class_register() and class_unregister() instead of class_create()
> and class_destroy(), and you would be done w/o any further API change.
> 
> >>
> >>>+		return size;
> >>>+}
> >>>+
> >>>+static ssize_t timeout_show(struct device *dev,
> >>>+		struct device_attribute *attr, char *buf)
> >>>+{
> >>>+	struct watchdog_device *wdd = dev_get_drvdata(dev);
> >>>+	ssize_t status;
> >>>+
> >>>+	mutex_lock(&wdd->lock);
> >>>+	if (wdd->timeout == 0)
> >>>+		status = -EOPNOTSUPP;
> >>
> >>Why ?
> >
> >It has been copied from case WDIOC_GETTIMEOUT: which says:
> >timeout == 0 means that we don't know the timeout.
> >
> 
> Just return 0 to mean "the timeout is unknown", and make it part of the
> sysfs ABI. Returning an error just makes user space code more complicated.

Now the new code will check wdd->timeout == 0 in is_visible. So should be fine.
What do you say.?

~Pratyush

Re: [RFC] watchdog: Add watchdog device control through sysfs attributes

[Pratyush Anand]

On 30/08/2015:11:39:38 AM, Guenter Roeck wrote:
> On 08/30/2015 11:17 AM, Pratyush Anand wrote:
> >Hi Guenter,
> >On 30/08/2015:07:58:27 AM, Guenter Roeck wrote:
> >>
> >>I can only see a single caller of watchdog_dev_register().
> >>Changing that one call should not be such a problem.
> >
> >Sure..Probably I was sleeping when I greped watchdog_dev_register :(
> >
> >>
> >>Also, you could name the groups variable something like wdc_class_dev_groups,
> >>make it public, and assign it in watchdog_core.c as follows.
> >
> >I will avoid making public whereever possible.
> >
> Your argument seems to be that it is better to introduce a new public
> single-use API function to avoid introducing a public variable.
> 
> Not sure if I understand the logic.

Sorry, if it was not clear..I agreed to change watchdog_dev_register instead of
introducing a new public variable.

> 
> >>
> >>static struct class watchdog_class = {
> >>	.name = "watchdog",
> >>	.owner = THIS_MODULE,
> >>	.dev_groups = wdc_class_dev_groups,
> >>};
> >>
> >>Then use class_register() and class_unregister() instead of class_create()
> >>and class_destroy(), and you would be done w/o any further API change.
> >>
> >>>>
> >>>>>+		return size;
> >>>>>+}
> >>>>>+
> >>>>>+static ssize_t timeout_show(struct device *dev,
> >>>>>+		struct device_attribute *attr, char *buf)
> >>>>>+{
> >>>>>+	struct watchdog_device *wdd = dev_get_drvdata(dev);
> >>>>>+	ssize_t status;
> >>>>>+
> >>>>>+	mutex_lock(&wdd->lock);
> >>>>>+	if (wdd->timeout == 0)
> >>>>>+		status = -EOPNOTSUPP;
> >>>>
> >>>>Why ?
> >>>
> >>>It has been copied from case WDIOC_GETTIMEOUT: which says:
> >>>timeout == 0 means that we don't know the timeout.
> >>>
> >>
> >>Just return 0 to mean "the timeout is unknown", and make it part of the
> >>sysfs ABI. Returning an error just makes user space code more complicated.
> >
> >Now the new code will check wdd->timeout == 0 in is_visible. So should be fine.
> >What do you say.?
> >
> timeout can be configured through ioctl, unless I am missing something,
> so I am not sure if I agree. It could still end up being 0.

Hummm..it did not allow to set timeout=0 for iTCO_wdt..but I can see that there
are some devices which sets min_timeout=0 and so your argument seems to be
correct and I will change as you suggested.
However, I am wondering should a device be allowed to set timeout=0. What
purpose will that serve?... may be for testing purpose can be used..

~Pratyush

Re: [RFC] watchdog: Add watchdog device control through sysfs attributes

[Pratyush Anand]

On 29/08/2015:09:51:24 AM, Guenter Roeck wrote:
> > * timeout - reads current timeout and writes to program a new timeout.

Now, this is the only file which has write permission. I hope, that is fine. Pl
let me know if you expected this to be as RO as well.

~Pratyush

Re: [RFC] watchdog: Add watchdog device control through sysfs attributes

[Pratyush Anand]

On 30/08/2015:08:45:00 PM, Guenter Roeck wrote:
> On 08/30/2015 08:11 PM, Pratyush Anand wrote:
> >On 29/08/2015:09:51:24 AM, Guenter Roeck wrote:
> >>>* timeout - reads current timeout and writes to program a new timeout.
> >
> >Now, this is the only file which has write permission. I hope, that is fine. Pl
> >let me know if you expected this to be as RO as well.
> >
> Good question. Thinking about it, we should make it read-only. Reason is
> that the application sending the heartbeats would not know about timeout
> changes made through sysfs, which could (and likely would) be fatal.

OK..will change accordingly.

~Pratyush