Re: [RFC] flash-kernel hook to prepend to boot script
On Thu, Jun 05, 2014 at 03:13:33PM -0600, dann frazier wrote:
> On Thu, Jun 05, 2014 at 08:43:02AM +0100, Ian Campbell wrote:
> > On Wed, 2014-06-04 at 11:40 -0600, dann frazier wrote:
> > > On Fri, May 30, 2014 at 09:50:37AM +0100, Ian Campbell wrote:
> > > > On Wed, 2014-05-28 at 16:30 -0600, dann frazier wrote:
> > > > > On Tue, May 27, 2014 at 10:35:18AM -0600, dann frazier wrote:
> > > > > > On Mon, May 26, 2014 at 11:54:31AM +0100, Ian Campbell wrote:
> > > > > > > On Wed, 2014-05-21 at 14:59 -0600, dann frazier wrote:
> > > > > > > > hey,
> > > > > > > > A couple of projects we're working on at work require some
> > > > > > > > tweaking of u-boot settings. These requirements can be summed up by:
> > > > > > > >
> > > > > > > > A) Maintain the console= setting, and ideally all userargs (the
> > > > > > > > cmdline args after "--") after install. This seems like standard
> > > > > > > > Debian functionality that exists on other architectures, but is
> > > > > > > > currently missing on flash-kernel platforms.
> > > > > > > >
> > > > > > > > B) The ability to let a package change settings in the u-boot
> > > > > > > > environment. We have a case where a u-boot envvar setting
> > > > > > > > needs to differ depending on whether or not certain software
> > > > > > > > will be used (their u-boot has special code that reconfigures
> > > > > > > > the hardware depending on the setting of this variable).
> > > > > > > >
> > > > > > > > The systems we're dealing with use a boot.scr script generated by
> > > > > > > > flash-kernel. So, we figure we could solve both by letting packages
> > > > > > > > drop in u-boot code snippets that will be prepended to the
> > > > > > > > boot.scr. To do this, we propose a scheme similar to initramfs-tools
> > > > > > > > where packages can drop snippets in a path under /usr/share (solving
> > > > > > > > B), and users can add their own new setings or override the /usr/share
> > > > > > > > versions by dropping snippets under /etc. With this scheme in place,
> > > > > > > > flash-kernel-installer could be extended to drop in a file in /etc
> > > > > > > > that does a 'setenv bootargs $userargs' to solve (A). Comments?
> > > > > > >
> > > > > > > I think snippets like this are a useful idea in general, but I wonder if
> > > > > > > something like the command line isn't deserving of "higher billing",
> > > > > > > e.g. via a setting in /etc/defaults/flash-kernel:COMMAND_LINE?
> > > > > >
> > > > > > It looks like Ubuntu is carrying a patch that does this today:
> > > > > >
> > > > > > http://bazaar.launchpad.net/~ubuntu-branches/ubuntu/utopic/flash-kernel/utopic/revision/443.2.7
> > > > > >
> > > > > > There the the variable is called "UBOOT_DEFAULTS". I think
> > > > > > "KERNEL_CMDLINE" would be a more obvious name, but it would also be
> > > > > > nice to be reducing their diff.
> > > > >
> > > > > Looking at grub as an example, I think we'll want to make the cmdline
> > > > > paramaters a debconf setting, giving the user the option to modify the
> > > > > proposed cmdline in expert mode.
> > > > >
> > > > > 1) f-k-i would use user-params to generate a reasonable default set
> > > > > cmdline args and set flash-kernel/linux_cmdline.
> > > > > 2) f-k/configure would prompt the user (priority=high) for changes to
> > > > > this default during configuration.
> > > > > 3) f-k/postinst would generate /etc/default/flash-kernel, and
> > > > > presumably using ucf to manage it from there on
> > > > >
> > > > > Sound reasonable?
> > > >
> > > > It does.
> > > >
> > > > I'm travelling right now but I took a brief look through the patches, I
> > > > think you should go ahead and push them.
> > >
> > > My target platform is actually an arm64 system, which I can't easily
> > > test it with Debian's d-i (yet). But I did manage to find an armhf
> > > system to test on this week. There were a few issues with my changes
> > > that I found/fixed (multiline support for ubootenv stubs,
> > > brokendebconf-set-selections call, etc), but it seems to be working as
> > > expected now. I went ahead and also added support for the armhf test
> > > system I used (Calxeda Highbank) and my target platform (APM Mustang),
> > > which also required turning on arm64 support.
> > >
> > > > Reading the diffs in my mail
> > > > client suggested there was some mixing of hard and soft tabs, butthat's
> > > > only a minor thing.
> > >
> > > Should be fixed in the merged version.
> > >
> > > > I didn't see any boot.scr using this stuff, I suppose that is to
> > > > come?
> > >
> > > Yeah - added this for both new platforms I've introduced.
> > >
> > > > I'll probably make the sunxi/cubietruck stuff use it at some point.
> > >
> > > Cool. In the meantime, I'll plan to do an f-k upload tomorrow, in case
> > > anyone wants to take some time to review the merged changes.
> >
> > Mostly looks good too me.
> >
> > The xgene script is missing the extra bootenv marker.
>
> Oh, good catch - obviously I haven't tested a Debian install on that
> system yet :) Fix pushed.
>
> > Bootloader-Sets-Incorrect-Root is marked "required" in the README but is
> > omitted from X-gene. I think since 850c8fc3 the behaviour when missing
> > is now to treat it as "no" which is the sane default, so perhaps the fix
> > is to make the README reflect that rather than fiddle with the Xgene db
> > entry.
>
> Either works for me.
>
> > flash-kernel upgrade rather than on install via di is a worry. Bootargs
> > gets overridden to the default from /etc/default/flash-kernel which will
> > be missing root= etc. On my cubietruck after dpkg -i of 3.20 I 've ended
> > up with:
> > setenv bootargs 'quiet'
> > in my boot.scr, which isn't going to boot I think. (actually, I as
> > discovered below it probably would, although missing console= is a
> > concern...)
> >
> > This could just mean that we need to be a bit careful when adding
> > @@LINUX_KERNEL_CMDLINE@@ to existing bootscripts, since we've not
> > released with CT support I think it would be safe to do in that case.
> > Other than that I'm at a loss what to suggest, perhaps something probed
> > from /proc/cmdline when first installing a version >= 3.20?
>
> That sounds reasonable/correct to me - I don't want to make anyone
> unbootable, even in sid->sid upgrades. But my debconf foo is weak, so
> I'm not sure what the best way to do this is. Maybe we could add a
> mapping of model strings and the corresponding f-k version at which
> each model started including L_K_C support to the postinst. When
> crossing that boundary (version test), we could preload any
> non-duplicate /proc/cmdline options to flash-kernel/linux_cmdline.
>
> Here's an untested patch to explain using code:
>
> diff --git a/debian/flash-kernel.postinst b/debian/flash-kernel.postinst
> index 7ef963a..13a74a6 100755
> --- a/debian/flash-kernel.postinst
> +++ b/debian/flash-kernel.postinst
> @@ -2,6 +2,45 @@
>
> set -e
>
> +# Transition from using a static bootargs
> +# to one provided by /etc/flash/kernel:LINUX_KERNEL_CMDLINE
> +transition_cmdline() {
> + local configver="$1"
> + local transver=""
> +
> + . /usr/share/flash-kernel/functions
> +
> + get_machine
> +
> + case "$machine" in
> + "Cubietech Cubietruck")
> + transver=1.20 ;;
> + *)
> + ;;
> + esac
> +
> + if [ -z "$transver" ]; then
> + return
> + fi
> +
> + if dpkg --compare-versions "$configver" ge "$transver"; then
hm.. that should probably be s/ge/ge-nl/. In the case of a fresh
install, I don't think we want to incorporate /proc/cmdline. f-k-i
will do that for us at d-i install time, and we probably don't want to
make assumptions about the environment if the user is installing f-k
outside of d-i.
-dann
> + return
> + fi
> +
> + local def_params=""
> + local new_params=""
> + db_get flash-kernel/linux_cmdline && def_params="$RET"
> +
> + new_params="$def_params"
> +
> + for param in $(cat /proc/cmdline); do
> + if ! echo "$def_params" | tr ' ' '\n' | grep -q "^${param}$"; then
> + new_params="${new_params:+$new_params }${param}"
> + fi
> + done
> + db_set flash-kernel/linux_cmdline "$new_params"
> +}
> +
> # cargo-culted from grub
> merge_debconf_into_conf() {
> local tmpfile; tmpfile="$1"
> @@ -19,10 +58,13 @@ merge_debconf_into_conf() {
> fi
> }
>
> +configured_ver="$2"
> +
> case "$1" in
> configure)
> . /usr/share/debconf/confmodule
>
> + transition_cmdline "$configured_ver"
> tmp_default_fk="$(mktemp -t flash-kernel.XXXXXXXXXX)"
> trap "rm -f ${tmp_default_fk}" EXIT
> cp -p /usr/share/flash-kernel/default/flash-kernel \
>
> > Then I edited /etc/default/flash-kernel by hand to
> > LINUX_KERNEL_CMDLINE="quiet root=/dev/sda2
> > console=ttyS0,115200n"
> >
> > But:
> > # flash-kernel
> > Installing sun7i-a20-cubietruck.dtb 3.15-rc7-armmp into /boot
> > flash-kernel: installing version 3.15-rc7-armmp
> > Generating boot script u-boot image... sed: -e expression #2, char 40: unknown option to `s'
> >
> > Because of the / in /dev/sda2 perhaps. It turns out root= can be omitted
> > here because it's also done in
> > initramfs-tools/hooks/flash_kernel_set_rootm which sets the default in
> > case the command line is missing it. That's OK, but it would be nice if
> > we could be arranging for / to be allowed here -- someone is eventually
> > going to add root= to /etc/defaults and break their system...
>
> I pushed a fix for that, thanks!
>
> -dann
>
> > With root= gone from there things work on my CT.
> >
> > Cheers,
> > Ian.
> >
> >
>
>
Reply to: