From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pratyush Anand Subject: Re: [PATCH V2 dracut 2/2] watchdog: install module for active watchdog Date: Mon, 7 Mar 2016 21:58:13 +0530 Message-ID: <20160307162813.GC20953@dhcppc3.redhat.com> References: <0d7adcfdf08f4eb753a0e65e7e65d5fe0ef22f70.1456919929.git.panand@redhat.com> <56DD86D4.8040800@redhat.com> Mime-Version: 1.0 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: <56DD86D4.8040800-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> Sender: initramfs-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: Content-Type: text/plain; charset="windows-1252" To: Harald Hoyer Cc: initramfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, dzickus-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 sy= sfs > > device attributes so that different watchdog status can be read. > >=20 > > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/comm= it/?id=3D6551881c86c791237a3bebf11eb3bd70b60ea782 > > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/comm= it/?id=3D906d7a5cfeda508e7361f021605579a00cd82815 > > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/comm= it/?id=3D33b711269ade3f6bc9d9d15e4343e6fa922d999b > >=20 > > With the above support, now we can find out whether a watchdog is a= ctive or > > not. We can also find out the driver/module responsible for that wa= tchdog > > device. > >=20 > > Proposed patch uses above support and then adds module of active wa= tchdog > > in initramfs generated by dracut. > >=20 > > It is reasonable to always add kernel watchdog module if dracut wat= chdog > > module has been added. When an user does not want to add kernel mod= ule, > > then he/she should exclude complete dracut watchdog module with --o= mit. > >=20 > > 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/mod= ules/.../kernel/drivers/watchdog/iTCO_vendor_support.ko > > -rw-r--r-- 1 root root 19252 Feb 24 09:19 usr/lib/mod= ules/.../kernel/drivers/watchdog/iTCO_wdt.ko > > # lsinitrd -f tmp/active-watchdogs initramfs-test.img > > iTCO_wdt > >=20 > > -- 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 > >=20 > > Signed-off-by: Pratyush Anand > > Cc: Dave Young > > Cc: Don Zickus > > Cc: Harald Hoyer > > --- > > modules.d/04watchdog/module-setup.sh | 33 ++++++++++++++++++++++++= +++++++++ > > 1 file changed, 33 insertions(+) > >=20 > > diff --git a/modules.d/04watchdog/module-setup.sh b/modules.d/04wat= chdog/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 > > } > > =20 > > +installkernel() { > > + cd /sys/class/watchdog > > + for dir in */; do > > + cd $dir > > + active=3D`[ -f state ] && cat state` > > + if [ "$active" =3D "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=3D`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=3D`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=3D"device/.."; > > + while [ -f "$wdtppath/modalias" ] > > + do > > + wdtpdrv=3D`cat $wdtppath/modalias` > > + wdtpdrv=3D`modprobe -R $wdtpdrv | tr "\n" "," | sed 's/.$//= '` > > + instmods $wdtpdrv > > + wdtppath=3D"$wdtppath/.." > > + done > > + fi > > + cd .. > > + done > > +} > >=20 >=20 >=20 > This part should only matter/execute in the "hostonly" case. Please u= se $() I had changed it in V3. In V3, for hostonly we add modules for active w= atchdogs 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. >=20 > $(cat file) can be expressed as $(< file) >=20 > you have to catch the case: > a) there is no /sys/class/watchdog > b) there is no subdir in /sys/class/watchdog >=20 > cd $dir and cd .. is not ideal > you might want to use subshells with () or use pushd/popd >=20 >=20 > something like this: >=20 > installkernel() { > [[ $hostonly ]] || return > for dir in /sys/class/watchdog/*; do > [[ -d "$dir" ]] || continue > [[ -f "$dir/state" ]] || continue > active=3D$(< "$dir/state") > [[ "$active" =3D=3D "active" ]] || continue > ( > cd $dir > echo $(< identity) >> "$initdir/tmp/active-watchdogs" > [=E2=80=A6] > # no need for "cd .." > ) > } >=20 > Also I don't know, if the ".." method works for the wdtppath with the= symlinks. > You might want: > wdtppath=3D$(readlink -f device/..) > instead of: > wdtppath=3D"device/.." >=20 Agreeing to all of the above suggestions. >=20 > why do you use tr "\n" "," | sed 's/.$//' to construct the instmods s= tring? 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` int= o the format like intel_rng,lpc_ich, so that we can ask instmods to install a= ll of them. ~Pratyush