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

[Harald Hoyer <harald-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

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?