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

[Pratyush Anand <panand-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

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