[PATCH V2 dracut 0/2] Install kernel module for active watchdog

[PATCH V2 dracut 0/2] Install kernel module for active watchdog

[Pratyush Anand]

Changes since RFC:
 -- Added one more patch to take care that dracut does not add watchdog
hooks when systemd module is included. In that case systemd daemon will
manage watchdog for kicking.
 -- Removed --nowdt argument. User can use -o or --omit when wathdog module
is not needed. Thanks Harald for this suggestion.
 -- Now I also do not check for "hostonly" to add modules for active
watchdog. It seems reasonable to add kernel watchdog modules whenever,
dracut watchdog module has been added.

Pratyush Anand (2):
  watchdog: Do not add hooks if systemd module is included
  watchdog: install module for active watchdog

 modules.d/04watchdog/module-setup.sh | 38 ++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

-- 
2.5.0

[PATCH V2 dracut 1/2] watchdog: Do not add hooks if systemd module is included

[Pratyush Anand]

When systemd is present, let it manage watchdog feed.

Signed-off-by: Pratyush Anand <panand-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 modules.d/04watchdog/module-setup.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/modules.d/04watchdog/module-setup.sh b/modules.d/04watchdog/module-setup.sh
index 576c589c198d..7ec757aec032 100755
--- a/modules.d/04watchdog/module-setup.sh
+++ b/modules.d/04watchdog/module-setup.sh
@@ -12,6 +12,11 @@ depends() {
 
 # called by dracut
 install() {
+    # Do not add watchdog hooks if systemd module is included
+    # In that case, systemd will manage watchdog kick
+    if dracut_module_included "systemd"; then
+	    return
+    fi
     inst_hook cmdline   00 "$moddir/watchdog.sh"
     inst_hook cmdline   50 "$moddir/watchdog.sh"
     inst_hook pre-trigger 00 "$moddir/watchdog.sh"
-- 
2.5.0

[PATCH V2 dracut 2/2] watchdog: install module for active watchdog

[Pratyush Anand]

Recently following patches have been added in upstream Linux kernel, which
(1) fixes parent of watchdog_device so that
/sys/class/watchdog/watchdogn/device is populated. (2) adds some sysfs
device attributes so that different watchdog status can be read.

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6551881c86c791237a3bebf11eb3bd70b60ea782
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=906d7a5cfeda508e7361f021605579a00cd82815
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=33b711269ade3f6bc9d9d15e4343e6fa922d999b

With the above support, now we can find out whether a watchdog is active or
not. We can also find out the driver/module responsible for that watchdog
device.

Proposed patch uses above support and then adds module of active watchdog
in initramfs generated by dracut.

It is reasonable to always add kernel watchdog module if dracut watchdog
module has been added. When an user does not want to add kernel module,
then he/she should exclude complete dracut watchdog module with --omit.

Testing:
-- When watchdog is active watchdog modules were added
	# cat /sys/class/watchdog/watchdog0/identity
	iTCO_wdt
	# cat /sys/class/watchdog/watchdog0/state
	active
	# dracut initramfs-test.img -a watchdog
	# lsinitrd initramfs-test.img | grep iTCO
	-rw-r--r--   1 root     root         9100 Feb 24 09:19 usr/lib/modules/.../kernel/drivers/watchdog/iTCO_vendor_support.ko
	-rw-r--r--   1 root     root        19252 Feb 24 09:19 usr/lib/modules/.../kernel/drivers/watchdog/iTCO_wdt.ko
	# lsinitrd -f tmp/active-watchdogs  initramfs-test.img
	iTCO_wdt

-- When watchdog is inactive then watchdog modules were not added
	# cat /sys/class/watchdog/watchdog0/state
	inactive
	# dracut initramfs-test.img -a watchdog
	# lsinitrd initramfs-test.img | grep iTCO
	# lsinitrd -f tmp/active-watchdogs  initramfs-test.img

Signed-off-by: Pratyush Anand <panand-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Don Zickus <dzickus-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Harald Hoyer <harald-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 modules.d/04watchdog/module-setup.sh | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/modules.d/04watchdog/module-setup.sh b/modules.d/04watchdog/module-setup.sh
index 7ec757aec032..6232afb0d9ca 100755
--- a/modules.d/04watchdog/module-setup.sh
+++ b/modules.d/04watchdog/module-setup.sh
@@ -32,3 +32,36 @@ install() {
     inst_multiple -o wdctl
 }
 
+installkernel() {
+    cd /sys/class/watchdog
+    for dir in */; do
+	    cd $dir
+	    active=`[ -f state ] && cat state`
+	    if [ "$active" =  "active" ]; then
+		    # applications like kdump need to know that, which
+		    # watchdog modules have been added into initramfs
+		    echo `cat identity` >> "$initdir/tmp/active-watchdogs"
+		    # device/modalias will return driver of this device
+		    wdtdrv=`cat device/modalias`
+		    # There can be more than one module represented by same
+		    # modalias. Currently load all of them.
+		    # TODO: Need to find a way to avoid any unwanted module
+		    # represented by modalias
+		    wdtdrv=`modprobe -R $wdtdrv | tr "\n" "," | sed 's/.$//'`
+		    instmods $wdtdrv
+		    # however in some cases, we also need to check that if
+		    # there is a specific driver for the parent bus/device.
+		    # In such cases we also need to enable driver for parent
+		    # bus/device.
+		    wdtppath="device/..";
+		    while [ -f "$wdtppath/modalias" ]
+		    do
+			    wdtpdrv=`cat $wdtppath/modalias`
+			    wdtpdrv=`modprobe -R $wdtpdrv | tr "\n" "," | sed 's/.$//'`
+			    instmods $wdtpdrv
+			    wdtppath="$wdtppath/.."
+		    done
+	    fi
+	    cd ..
+    done
+}
-- 
2.5.0

Re: [PATCH V2 dracut 2/2] watchdog: install module for active watchdog

[Dracut GitHub Import Bot]

Patchset imported to github.
Pull request:
<https://github.com/haraldh/dracut/compare/master...dracut-mailing-devs:0d7adcfdf08f4eb753a0e65e7e65d5fe0ef22f70.1456919929.git.panand%40redhat.com>

Re: [PATCH V2 dracut 2/2] watchdog: install module for active watchdog

[Dave Young]

Hi, Pratyush

[snip]
> 
> Signed-off-by: Pratyush Anand <panand-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Don Zickus <dzickus-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Harald Hoyer <harald-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  modules.d/04watchdog/module-setup.sh | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/modules.d/04watchdog/module-setup.sh b/modules.d/04watchdog/module-setup.sh
> index 7ec757aec032..6232afb0d9ca 100755
> --- a/modules.d/04watchdog/module-setup.sh
> +++ b/modules.d/04watchdog/module-setup.sh
> @@ -32,3 +32,36 @@ install() {
>      inst_multiple -o wdctl
>  }
>  
> +installkernel() {
> +    cd /sys/class/watchdog
> +    for dir in */; do
> +	    cd $dir
> +	    active=`[ -f state ] && cat state`
> +	    if [ "$active" =  "active" ]; then
> +		    # applications like kdump need to know that, which
> +		    # watchdog modules have been added into initramfs
> +		    echo `cat identity` >> "$initdir/tmp/active-watchdogs"
> +		    # device/modalias will return driver of this device

Seems I do not find where the identiy being used..

OTOH if we use it in kdump module, we should find it in kdump module code instead
of depending on the tmp file created by wdt module.

Thanks
Dave

Re: [PATCH V2 dracut 0/2] Install kernel module for active watchdog

[Dave Young]

Hi, Pratyush

On 03/02/16 at 05:36pm, Pratyush Anand wrote:
> Changes since RFC:
>  -- Added one more patch to take care that dracut does not add watchdog
> hooks when systemd module is included. In that case systemd daemon will
> manage watchdog for kicking.
>  -- Removed --nowdt argument. User can use -o or --omit when wathdog module
> is not needed. Thanks Harald for this suggestion.
>  -- Now I also do not check for "hostonly" to add modules for active
> watchdog. It seems reasonable to add kernel watchdog modules whenever,
> dracut watchdog module has been added.

From my understanding hostonly is a different mode comparing with the default
one which being used for distribution provided initramfs. In the latter case
all possible drivers should be included. But hostonly mode only adding the
drivers being used in this boot. From the code logic we provided it will detect
active wdt module and add the driver, the behavior is just "hostonly", no?

Thanks
Dave

Re: [PATCH V2 dracut 0/2] Install kernel module for active watchdog

[Pratyush Anand]

Hi Dave,

On 04/03/2016:11:16:33 AM, Dave Young wrote:
> Hi, Pratyush
> 
> On 03/02/16 at 05:36pm, Pratyush Anand wrote:
> > Changes since RFC:
> >  -- Added one more patch to take care that dracut does not add watchdog
> > hooks when systemd module is included. In that case systemd daemon will
> > manage watchdog for kicking.
> >  -- Removed --nowdt argument. User can use -o or --omit when wathdog module
> > is not needed. Thanks Harald for this suggestion.
> >  -- Now I also do not check for "hostonly" to add modules for active
> > watchdog. It seems reasonable to add kernel watchdog modules whenever,
> > dracut watchdog module has been added.
> 
> >From my understanding hostonly is a different mode comparing with the default
> one which being used for distribution provided initramfs. In the latter case
> all possible drivers should be included. But hostonly mode only adding the
> drivers being used in this boot. From the code logic we provided it will detect
> active wdt module and add the driver, the behavior is just "hostonly", no?

I think, I am agreeing to this.

So, should we also add no "hostonly" case where driver will be present
irrespective of whether wdt is active or not? May be we can leave it for now,
and can be added if someone needs it latter.

~Pratyush

Re: [PATCH V2 dracut 2/2] watchdog: install module for active watchdog

[Pratyush Anand]

Hi Dave,

On 04/03/2016:11:13:34 AM, Dave Young wrote:
> Hi, Pratyush
> 
> [snip]
> > 
> > Signed-off-by: Pratyush Anand <panand-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Cc: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Cc: Don Zickus <dzickus-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Cc: Harald Hoyer <harald-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  modules.d/04watchdog/module-setup.sh | 33 +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> > 
> > diff --git a/modules.d/04watchdog/module-setup.sh b/modules.d/04watchdog/module-setup.sh
> > index 7ec757aec032..6232afb0d9ca 100755
> > --- a/modules.d/04watchdog/module-setup.sh
> > +++ b/modules.d/04watchdog/module-setup.sh
> > @@ -32,3 +32,36 @@ install() {
> >      inst_multiple -o wdctl
> >  }
> >  
> > +installkernel() {
> > +    cd /sys/class/watchdog
> > +    for dir in */; do
> > +	    cd $dir
> > +	    active=`[ -f state ] && cat state`
> > +	    if [ "$active" =  "active" ]; then
> > +		    # applications like kdump need to know that, which
> > +		    # watchdog modules have been added into initramfs
> > +		    echo `cat identity` >> "$initdir/tmp/active-watchdogs"
> > +		    # device/modalias will return driver of this device
> 
> Seems I do not find where the identiy being used..

Yes, as stated in comment it is being used by kdump.
https://github.com/pratyushanand/kexec-tools/commit/88c2c9c931c6d2acc68bdd0adf9ac04a93a1a864#diff-c8f5f515755d435842acf0793a131c28R345

> 
> OTOH if we use it in kdump module, we should find it in kdump module code instead
> of depending on the tmp file created by wdt module.

Unfortunately, I do not see any nice way to do it in kdump until we replicate
almost all of this code (except module installation) again in kdump. In kdump
dracut module we will have to parse each directory in sysfs again to determine
active watchdogs, and then putting all this stuff in watchdog dracut is almost
useless for kdump.

Another implementation which has occurred to me was as follows:
is_wdt_modified() of kdump can rely on "driver name" instead of "identity". For
example if watchdog module installs mei_me then is_wdt_modified() just looks for
a filename  in initrd having "mei_me" string in it. But I am not sure about it.
Same string might be present in other name as well. 

However, if we can get full path of ".ko" from module name easily, that would be
helpful.

~Pratyush

Re: [PATCH V2 dracut 2/2] watchdog: install module for active watchdog

[Dave Young]

On 03/04/16 at 09:45am, Pratyush Anand wrote:
> Hi Dave,
> 
> On 04/03/2016:11:13:34 AM, Dave Young wrote:
> > Hi, Pratyush
> > 
> > [snip]
> > > 
> > > Signed-off-by: Pratyush Anand <panand-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > Cc: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > Cc: Don Zickus <dzickus-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > Cc: Harald Hoyer <harald-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  modules.d/04watchdog/module-setup.sh | 33 +++++++++++++++++++++++++++++++++
> > >  1 file changed, 33 insertions(+)
> > > 
> > > diff --git a/modules.d/04watchdog/module-setup.sh b/modules.d/04watchdog/module-setup.sh
> > > index 7ec757aec032..6232afb0d9ca 100755
> > > --- a/modules.d/04watchdog/module-setup.sh
> > > +++ b/modules.d/04watchdog/module-setup.sh
> > > @@ -32,3 +32,36 @@ install() {
> > >      inst_multiple -o wdctl
> > >  }
> > >  
> > > +installkernel() {
> > > +    cd /sys/class/watchdog
> > > +    for dir in */; do
> > > +	    cd $dir
> > > +	    active=`[ -f state ] && cat state`
> > > +	    if [ "$active" =  "active" ]; then
> > > +		    # applications like kdump need to know that, which
> > > +		    # watchdog modules have been added into initramfs
> > > +		    echo `cat identity` >> "$initdir/tmp/active-watchdogs"
> > > +		    # device/modalias will return driver of this device
> > 
> > Seems I do not find where the identiy being used..
> 
> Yes, as stated in comment it is being used by kdump.
> https://github.com/pratyushanand/kexec-tools/commit/88c2c9c931c6d2acc68bdd0adf9ac04a93a1a864#diff-c8f5f515755d435842acf0793a131c28R345
> 
> > 
> > OTOH if we use it in kdump module, we should find it in kdump module code instead
> > of depending on the tmp file created by wdt module.
> 
> Unfortunately, I do not see any nice way to do it in kdump until we replicate
> almost all of this code (except module installation) again in kdump. In kdump
> dracut module we will have to parse each directory in sysfs again to determine
> active watchdogs, and then putting all this stuff in watchdog dracut is almost
> useless for kdump.
> 
> Another implementation which has occurred to me was as follows:
> is_wdt_modified() of kdump can rely on "driver name" instead of "identity". For
> example if watchdog module installs mei_me then is_wdt_modified() just looks for
> a filename  in initrd having "mei_me" string in it. But I am not sure about it.
> Same string might be present in other name as well. 
> 
> However, if we can get full path of ".ko" from module name easily, that would be
> helpful.

Ok, got it. But from design point of view is sounds odd. Maybe adding a function
to dracut-functions.sh will help us? Previously we changed dracut-functions.shso
that it can be used other external scripts, see below dracut commit:
777f2db0373ccbc1a44fc2d960ecefbe50195055

Thanks
Dave

Re: [PATCH V2 dracut 2/2] watchdog: install module for active watchdog

[Dave Young]

Hi, Pratyush

[snip]
> diff --git a/modules.d/04watchdog/module-setup.sh b/modules.d/04watchdog/module-setup.sh
> index 7ec757aec032..6232afb0d9ca 100755
> --- a/modules.d/04watchdog/module-setup.sh
> +++ b/modules.d/04watchdog/module-setup.sh
> @@ -32,3 +32,36 @@ install() {
>      inst_multiple -o wdctl
>  }
>  
> +installkernel() {
> +    cd /sys/class/watchdog
> +    for dir in */; do
> +	    cd $dir
> +	    active=`[ -f state ] && cat state`
> +	    if [ "$active" =  "active" ]; then
> +		    # applications like kdump need to know that, which
> +		    # watchdog modules have been added into initramfs
> +		    echo `cat identity` >> "$initdir/tmp/active-watchdogs"
> +		    # device/modalias will return driver of this device
> +		    wdtdrv=`cat device/modalias`
> +		    # There can be more than one module represented by same
> +		    # modalias. Currently load all of them.
> +		    # TODO: Need to find a way to avoid any unwanted module
> +		    # represented by modalias
> +		    wdtdrv=`modprobe -R $wdtdrv | tr "\n" "," | sed 's/.$//'`
> +		    instmods $wdtdrv
> +		    # however in some cases, we also need to check that if
> +		    # there is a specific driver for the parent bus/device.
> +		    # In such cases we also need to enable driver for parent
> +		    # bus/device.
> +		    wdtppath="device/..";
> +		    while [ -f "$wdtppath/modalias" ]
> +		    do
> +			    wdtpdrv=`cat $wdtppath/modalias`
> +			    wdtpdrv=`modprobe -R $wdtpdrv | tr "\n" "," | sed 's/.$//'`
> +			    instmods $wdtpdrv
> +			    wdtppath="$wdtppath/.."
> +		    done
> +	    fi
> +	    cd ..
> +    done
> +}

Another thing coming into mind is it possible more than one wdt being active
at the same time? If so it will cause problem because only the first occupies
/dev/watchdog device for systemd use?

How about just break the loop when we find an active wdt? Is it safe?

Thanks
Dave

Re: [PATCH V2 dracut 2/2] watchdog: install module for active watchdog

[Pratyush Anand]

On 04/03/2016:12:56:10 PM, Dave Young wrote:
> Hi, Pratyush
> 
> [snip]
> > diff --git a/modules.d/04watchdog/module-setup.sh b/modules.d/04watchdog/module-setup.sh
> > index 7ec757aec032..6232afb0d9ca 100755
> > --- a/modules.d/04watchdog/module-setup.sh
> > +++ b/modules.d/04watchdog/module-setup.sh
> > @@ -32,3 +32,36 @@ install() {
> >      inst_multiple -o wdctl
> >  }
> >  
> > +installkernel() {
> > +    cd /sys/class/watchdog
> > +    for dir in */; do
> > +	    cd $dir
> > +	    active=`[ -f state ] && cat state`
> > +	    if [ "$active" =  "active" ]; then
> > +		    # applications like kdump need to know that, which
> > +		    # watchdog modules have been added into initramfs
> > +		    echo `cat identity` >> "$initdir/tmp/active-watchdogs"
> > +		    # device/modalias will return driver of this device
> > +		    wdtdrv=`cat device/modalias`
> > +		    # There can be more than one module represented by same
> > +		    # modalias. Currently load all of them.
> > +		    # TODO: Need to find a way to avoid any unwanted module
> > +		    # represented by modalias
> > +		    wdtdrv=`modprobe -R $wdtdrv | tr "\n" "," | sed 's/.$//'`
> > +		    instmods $wdtdrv
> > +		    # however in some cases, we also need to check that if
> > +		    # there is a specific driver for the parent bus/device.
> > +		    # In such cases we also need to enable driver for parent
> > +		    # bus/device.
> > +		    wdtppath="device/..";
> > +		    while [ -f "$wdtppath/modalias" ]
> > +		    do
> > +			    wdtpdrv=`cat $wdtppath/modalias`
> > +			    wdtpdrv=`modprobe -R $wdtpdrv | tr "\n" "," | sed 's/.$//'`
> > +			    instmods $wdtpdrv
> > +			    wdtppath="$wdtppath/.."
> > +		    done
> > +	    fi
> > +	    cd ..
> > +    done
> > +}
> 
> Another thing coming into mind is it possible more than one wdt being active
> at the same time? If so it will cause problem because only the first occupies
> /dev/watchdog device for systemd use?

I had thought about it. Practically I do not see an use case of two active
watchdogs in a system. May be Don can comment better. However, technically it is
possible to have two active watchdog and systemd is using second active watchdog
for system reset. First watchdog might not be connected to reset circuitry and
its interrupt is being used for some other purpose. So, I left it like this.

~Pratyush
> 
> How about just break the loop when we find an active wdt? Is it safe?
> 
> Thanks
> Dave

Re: [PATCH V2 dracut 2/2] watchdog: install module for active watchdog

[Pratyush Anand]

On 04/03/2016:09:45:50 AM, Pratyush Anand wrote:
> However, if we can get full path of ".ko" from module name easily, that would be
> helpful.

modinfo might be helpful. Will give it a try. Will send V3 accordingly.

~Pratyush

Re: [PATCH V2 dracut 0/2] Install kernel module for active watchdog

[Dave Young]

On 03/04/16 at 09:04am, Pratyush Anand wrote:
> Hi Dave,
> 
> On 04/03/2016:11:16:33 AM, Dave Young wrote:
> > Hi, Pratyush
> > 
> > On 03/02/16 at 05:36pm, Pratyush Anand wrote:
> > > Changes since RFC:
> > >  -- Added one more patch to take care that dracut does not add watchdog
> > > hooks when systemd module is included. In that case systemd daemon will
> > > manage watchdog for kicking.
> > >  -- Removed --nowdt argument. User can use -o or --omit when wathdog module
> > > is not needed. Thanks Harald for this suggestion.
> > >  -- Now I also do not check for "hostonly" to add modules for active
> > > watchdog. It seems reasonable to add kernel watchdog modules whenever,
> > > dracut watchdog module has been added.
> > 
> > >From my understanding hostonly is a different mode comparing with the default
> > one which being used for distribution provided initramfs. In the latter case
> > all possible drivers should be included. But hostonly mode only adding the
> > drivers being used in this boot. From the code logic we provided it will detect
> > active wdt module and add the driver, the behavior is just "hostonly", no?
> 
> I think, I am agreeing to this.
> 
> So, should we also add no "hostonly" case where driver will be present
> irrespective of whether wdt is active or not? May be we can leave it for now,
> and can be added if someone needs it latter.

Agreed, maybe keep the current code within if hostonly, later if one want to
add other non hostonly stuff he can add it out of the if-fi

Thanks
Dave

Re: [PATCH V2 dracut 2/2] watchdog: install module for active watchdog

[Dave Young]

On 03/04/16 at 10:39am, Pratyush Anand wrote:
> On 04/03/2016:12:56:10 PM, Dave Young wrote:
> > Hi, Pratyush
> > 
> > [snip]
> > > diff --git a/modules.d/04watchdog/module-setup.sh b/modules.d/04watchdog/module-setup.sh
> > > index 7ec757aec032..6232afb0d9ca 100755
> > > --- a/modules.d/04watchdog/module-setup.sh
> > > +++ b/modules.d/04watchdog/module-setup.sh
> > > @@ -32,3 +32,36 @@ install() {
> > >      inst_multiple -o wdctl
> > >  }
> > >  
> > > +installkernel() {
> > > +    cd /sys/class/watchdog
> > > +    for dir in */; do
> > > +	    cd $dir
> > > +	    active=`[ -f state ] && cat state`
> > > +	    if [ "$active" =  "active" ]; then
> > > +		    # applications like kdump need to know that, which
> > > +		    # watchdog modules have been added into initramfs
> > > +		    echo `cat identity` >> "$initdir/tmp/active-watchdogs"
> > > +		    # device/modalias will return driver of this device
> > > +		    wdtdrv=`cat device/modalias`
> > > +		    # There can be more than one module represented by same
> > > +		    # modalias. Currently load all of them.
> > > +		    # TODO: Need to find a way to avoid any unwanted module
> > > +		    # represented by modalias
> > > +		    wdtdrv=`modprobe -R $wdtdrv | tr "\n" "," | sed 's/.$//'`
> > > +		    instmods $wdtdrv
> > > +		    # however in some cases, we also need to check that if
> > > +		    # there is a specific driver for the parent bus/device.
> > > +		    # In such cases we also need to enable driver for parent
> > > +		    # bus/device.
> > > +		    wdtppath="device/..";
> > > +		    while [ -f "$wdtppath/modalias" ]
> > > +		    do
> > > +			    wdtpdrv=`cat $wdtppath/modalias`
> > > +			    wdtpdrv=`modprobe -R $wdtpdrv | tr "\n" "," | sed 's/.$//'`
> > > +			    instmods $wdtpdrv
> > > +			    wdtppath="$wdtppath/.."
> > > +		    done
> > > +	    fi
> > > +	    cd ..
> > > +    done
> > > +}
> > 
> > Another thing coming into mind is it possible more than one wdt being active
> > at the same time? If so it will cause problem because only the first occupies
> > /dev/watchdog device for systemd use?
> 
> I had thought about it. Practically I do not see an use case of two active
> watchdogs in a system. May be Don can comment better. However, technically it is
> possible to have two active watchdog and systemd is using second active watchdog
> for system reset. First watchdog might not be connected to reset circuitry and
> its interrupt is being used for some other purpose. So, I left it like this.

Ok, let's see if Don has more idea about the problem.

BTW, should we add the $wdtdrv $wdtpdrv info rd.driver.pre after the instmods in
another patch?

Thanks
Dave

Re: [PATCH V2 dracut 2/2] watchdog: install module for active watchdog

[Pratyush Anand]

On 04/03/2016:03:30:07 PM, Dave Young wrote:
> BTW, should we add the $wdtdrv $wdtpdrv info rd.driver.pre after the instmods in
> another patch?

You mean to ensure that watchdog module is loaded as early as possible?
Will do that.

~Pratyush

Re: [PATCH V2 dracut 2/2] watchdog: install module for active watchdog

[Dave Young]

On 03/04/16 at 01:14pm, Pratyush Anand wrote:
> On 04/03/2016:03:30:07 PM, Dave Young wrote:
> > BTW, should we add the $wdtdrv $wdtpdrv info rd.driver.pre after the instmods in
> > another patch?
> 
> You mean to ensure that watchdog module is loaded as early as possible?
> Will do that.

Pratyush, yes.

Thanks a lot
Dave

Re: [PATCH V2 dracut 2/2] watchdog: install module for active watchdog

[Harald Hoyer]

On 02.03.2016 13:06, Pratyush Anand wrote:
> Recently following patches have been added in upstream Linux kernel, which
> (1) fixes parent of watchdog_device so that
> /sys/class/watchdog/watchdogn/device is populated. (2) adds some sysfs
> device attributes so that different watchdog status can be read.
> 
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6551881c86c791237a3bebf11eb3bd70b60ea782
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=906d7a5cfeda508e7361f021605579a00cd82815
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=33b711269ade3f6bc9d9d15e4343e6fa922d999b
> 
> With the above support, now we can find out whether a watchdog is active or
> not. We can also find out the driver/module responsible for that watchdog
> device.
> 
> Proposed patch uses above support and then adds module of active watchdog
> in initramfs generated by dracut.
> 
> It is reasonable to always add kernel watchdog module if dracut watchdog
> module has been added. When an user does not want to add kernel module,
> then he/she should exclude complete dracut watchdog module with --omit.
> 
> Testing:
> -- When watchdog is active watchdog modules were added
> 	# cat /sys/class/watchdog/watchdog0/identity
> 	iTCO_wdt
> 	# cat /sys/class/watchdog/watchdog0/state
> 	active
> 	# dracut initramfs-test.img -a watchdog
> 	# lsinitrd initramfs-test.img | grep iTCO
> 	-rw-r--r--   1 root     root         9100 Feb 24 09:19 usr/lib/modules/.../kernel/drivers/watchdog/iTCO_vendor_support.ko
> 	-rw-r--r--   1 root     root        19252 Feb 24 09:19 usr/lib/modules/.../kernel/drivers/watchdog/iTCO_wdt.ko
> 	# lsinitrd -f tmp/active-watchdogs  initramfs-test.img
> 	iTCO_wdt
> 
> -- When watchdog is inactive then watchdog modules were not added
> 	# cat /sys/class/watchdog/watchdog0/state
> 	inactive
> 	# dracut initramfs-test.img -a watchdog
> 	# lsinitrd initramfs-test.img | grep iTCO
> 	# lsinitrd -f tmp/active-watchdogs  initramfs-test.img
> 
> Signed-off-by: Pratyush Anand <panand-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Don Zickus <dzickus-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Harald Hoyer <harald-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  modules.d/04watchdog/module-setup.sh | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/modules.d/04watchdog/module-setup.sh b/modules.d/04watchdog/module-setup.sh
> index 7ec757aec032..6232afb0d9ca 100755
> --- a/modules.d/04watchdog/module-setup.sh
> +++ b/modules.d/04watchdog/module-setup.sh
> @@ -32,3 +32,36 @@ install() {
>      inst_multiple -o wdctl
>  }
>  
> +installkernel() {
> +    cd /sys/class/watchdog
> +    for dir in */; do
> +	    cd $dir
> +	    active=`[ -f state ] && cat state`
> +	    if [ "$active" =  "active" ]; then
> +		    # applications like kdump need to know that, which
> +		    # watchdog modules have been added into initramfs
> +		    echo `cat identity` >> "$initdir/tmp/active-watchdogs"
> +		    # device/modalias will return driver of this device
> +		    wdtdrv=`cat device/modalias`
> +		    # There can be more than one module represented by same
> +		    # modalias. Currently load all of them.
> +		    # TODO: Need to find a way to avoid any unwanted module
> +		    # represented by modalias
> +		    wdtdrv=`modprobe -R $wdtdrv | tr "\n" "," | sed 's/.$//'`
> +		    instmods $wdtdrv
> +		    # however in some cases, we also need to check that if
> +		    # there is a specific driver for the parent bus/device.
> +		    # In such cases we also need to enable driver for parent
> +		    # bus/device.
> +		    wdtppath="device/..";
> +		    while [ -f "$wdtppath/modalias" ]
> +		    do
> +			    wdtpdrv=`cat $wdtppath/modalias`
> +			    wdtpdrv=`modprobe -R $wdtpdrv | tr "\n" "," | sed 's/.$//'`
> +			    instmods $wdtpdrv
> +			    wdtppath="$wdtppath/.."
> +		    done
> +	    fi
> +	    cd ..
> +    done
> +}
> 


This part should only matter/execute in the "hostonly" case. Please use $()
instead of backticks. $() can be nested.

$(cat file) can be expressed as $(< file)

you have to catch the case:
a) there is no /sys/class/watchdog
b) there is no subdir in /sys/class/watchdog

cd $dir and cd .. is not ideal
you might want to use subshells with () or use pushd/popd


something like this:

installkernel() {
  [[ $hostonly ]] || return
  for dir in /sys/class/watchdog/*; do
 	[[ -d "$dir" ]] || continue
        [[ -f "$dir/state" ]] || continue
        active=$(< "$dir/state")
	[[ "$active" ==  "active" ]] || continue
        (
		cd $dir
	   	echo $(< identity) >> "$initdir/tmp/active-watchdogs"
[…]
	        # no need for "cd .."
	)
}

Also I don't know, if the ".." method works for the wdtppath with the symlinks.
You might want:
wdtppath=$(readlink -f device/..)
instead of:
wdtppath="device/.."


why do you use tr "\n" "," | sed 's/.$//' to construct the instmods string?

Re: [PATCH V2 dracut 2/2] watchdog: install module for active watchdog

[Pratyush Anand]

Hi Harald,

Thanks for your feedback.

On 07/03/2016:02:49:08 PM, Harald Hoyer wrote:
> On 02.03.2016 13:06, Pratyush Anand wrote:
> > Recently following patches have been added in upstream Linux kernel, which
> > (1) fixes parent of watchdog_device so that
> > /sys/class/watchdog/watchdogn/device is populated. (2) adds some sysfs
> > device attributes so that different watchdog status can be read.
> > 
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6551881c86c791237a3bebf11eb3bd70b60ea782
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=906d7a5cfeda508e7361f021605579a00cd82815
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=33b711269ade3f6bc9d9d15e4343e6fa922d999b
> > 
> > With the above support, now we can find out whether a watchdog is active or
> > not. We can also find out the driver/module responsible for that watchdog
> > device.
> > 
> > Proposed patch uses above support and then adds module of active watchdog
> > in initramfs generated by dracut.
> > 
> > It is reasonable to always add kernel watchdog module if dracut watchdog
> > module has been added. When an user does not want to add kernel module,
> > then he/she should exclude complete dracut watchdog module with --omit.
> > 
> > Testing:
> > -- When watchdog is active watchdog modules were added
> > 	# cat /sys/class/watchdog/watchdog0/identity
> > 	iTCO_wdt
> > 	# cat /sys/class/watchdog/watchdog0/state
> > 	active
> > 	# dracut initramfs-test.img -a watchdog
> > 	# lsinitrd initramfs-test.img | grep iTCO
> > 	-rw-r--r--   1 root     root         9100 Feb 24 09:19 usr/lib/modules/.../kernel/drivers/watchdog/iTCO_vendor_support.ko
> > 	-rw-r--r--   1 root     root        19252 Feb 24 09:19 usr/lib/modules/.../kernel/drivers/watchdog/iTCO_wdt.ko
> > 	# lsinitrd -f tmp/active-watchdogs  initramfs-test.img
> > 	iTCO_wdt
> > 
> > -- When watchdog is inactive then watchdog modules were not added
> > 	# cat /sys/class/watchdog/watchdog0/state
> > 	inactive
> > 	# dracut initramfs-test.img -a watchdog
> > 	# lsinitrd initramfs-test.img | grep iTCO
> > 	# lsinitrd -f tmp/active-watchdogs  initramfs-test.img
> > 
> > Signed-off-by: Pratyush Anand <panand-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Cc: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Cc: Don Zickus <dzickus-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Cc: Harald Hoyer <harald-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  modules.d/04watchdog/module-setup.sh | 33 +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> > 
> > diff --git a/modules.d/04watchdog/module-setup.sh b/modules.d/04watchdog/module-setup.sh
> > index 7ec757aec032..6232afb0d9ca 100755
> > --- a/modules.d/04watchdog/module-setup.sh
> > +++ b/modules.d/04watchdog/module-setup.sh
> > @@ -32,3 +32,36 @@ install() {
> >      inst_multiple -o wdctl
> >  }
> >  
> > +installkernel() {
> > +    cd /sys/class/watchdog
> > +    for dir in */; do
> > +	    cd $dir
> > +	    active=`[ -f state ] && cat state`
> > +	    if [ "$active" =  "active" ]; then
> > +		    # applications like kdump need to know that, which
> > +		    # watchdog modules have been added into initramfs
> > +		    echo `cat identity` >> "$initdir/tmp/active-watchdogs"
> > +		    # device/modalias will return driver of this device
> > +		    wdtdrv=`cat device/modalias`
> > +		    # There can be more than one module represented by same
> > +		    # modalias. Currently load all of them.
> > +		    # TODO: Need to find a way to avoid any unwanted module
> > +		    # represented by modalias
> > +		    wdtdrv=`modprobe -R $wdtdrv | tr "\n" "," | sed 's/.$//'`
> > +		    instmods $wdtdrv
> > +		    # however in some cases, we also need to check that if
> > +		    # there is a specific driver for the parent bus/device.
> > +		    # In such cases we also need to enable driver for parent
> > +		    # bus/device.
> > +		    wdtppath="device/..";
> > +		    while [ -f "$wdtppath/modalias" ]
> > +		    do
> > +			    wdtpdrv=`cat $wdtppath/modalias`
> > +			    wdtpdrv=`modprobe -R $wdtpdrv | tr "\n" "," | sed 's/.$//'`
> > +			    instmods $wdtpdrv
> > +			    wdtppath="$wdtppath/.."
> > +		    done
> > +	    fi
> > +	    cd ..
> > +    done
> > +}
> > 
> 
> 
> This part should only matter/execute in the "hostonly" case. Please use $()

I had changed it in V3. In V3, for hostonly we add modules for active watchdogs
and for no-hostonly for all watchdog present. Please let me know if you see any
issue with that.

> instead of backticks. $() can be nested.
> 
> $(cat file) can be expressed as $(< file)
> 
> you have to catch the case:
> a) there is no /sys/class/watchdog
> b) there is no subdir in /sys/class/watchdog
> 
> cd $dir and cd .. is not ideal
> you might want to use subshells with () or use pushd/popd
> 
> 
> something like this:
> 
> installkernel() {
>   [[ $hostonly ]] || return
>   for dir in /sys/class/watchdog/*; do
>  	[[ -d "$dir" ]] || continue
>         [[ -f "$dir/state" ]] || continue
>         active=$(< "$dir/state")
> 	[[ "$active" ==  "active" ]] || continue
>         (
> 		cd $dir
> 	   	echo $(< identity) >> "$initdir/tmp/active-watchdogs"
> […]
> 	        # no need for "cd .."
> 	)
> }
> 
> Also I don't know, if the ".." method works for the wdtppath with the symlinks.
> You might want:
> wdtppath=$(readlink -f device/..)
> instead of:
> wdtppath="device/.."
> 

Agreeing to all of the above suggestions.
> 
> why do you use tr "\n" "," | sed 's/.$//' to construct the instmods string?

In some cases there could be an alias representing more than one module like as
follows
# modprobe -R pci:v00008086d000024D0sv00000000sd00000000bc06sc01i00
intel_rng
lpc_ich

In such cases, we need to convert the above output of `modprobe -R` into the
format like intel_rng,lpc_ich, so that we can ask instmods to install all of them.

~Pratyush

Re: [PATCH V2 dracut 2/2] watchdog: install module for active watchdog

[Harald Hoyer]

On 07.03.2016 17:28, Pratyush Anand wrote:
> 
> In some cases there could be an alias representing more than one module like as
> follows
> # modprobe -R pci:v00008086d000024D0sv00000000sd00000000bc06sc01i00
> intel_rng
> lpc_ich
> 
> In such cases, we need to convert the above output of `modprobe -R` into the
> format like intel_rng,lpc_ich, so that we can ask instmods to install all of them.

But instmods wants space separated arguments

Re: [PATCH V2 dracut 2/2] watchdog: install module for active watchdog

[Pratyush Anand]

On 07/03/2016:05:52:47 PM, Harald Hoyer wrote:
> On 07.03.2016 17:28, Pratyush Anand wrote:
> > 
> > In some cases there could be an alias representing more than one module like as
> > follows
> > # modprobe -R pci:v00008086d000024D0sv00000000sd00000000bc06sc01i00
> > intel_rng
> > lpc_ich
> > 
> > In such cases, we need to convert the above output of `modprobe -R` into the
> > format like intel_rng,lpc_ich, so that we can ask instmods to install all of them.
> 
> But instmods wants space separated arguments

Ohh, I mistaken with rd.driver.pre :(.
Will correct.

Thanks for pointing it out.

~Pratyush