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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
>
> 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@xxxxxxxxx>.
>
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
