Bug 1088933 - update grubby to support device tree options for arm
Summary: update grubby to support device tree options for arm
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: grubby
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Brian Lane
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard: AcceptedBlocker AcceptedFreezeException
: 1119937 (view as bug list)
Depends On:
Blocks: ARMTracker F21AlphaFreezeException F21BetaBlocker 1078911
TreeView+ depends on / blocked
 
Reported: 2014-04-17 13:06 UTC by Dennis Gilmore
Modified: 2014-11-04 00:07 UTC (History)
12 users (show)

Fixed In Version: grubby-8.35-7.fc21
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-10-20 23:02:34 UTC


Attachments (Terms of Use)
add --devtree support to extlinux (3.81 KB, patch)
2014-04-21 17:40 UTC, Brian Lane
no flags Details | Diff
add support to new-kernel-pkg to update fdtdir on 32 bit arm (3.08 KB, patch)
2014-04-24 03:30 UTC, Dennis Gilmore
no flags Details | Diff
patch to update fdt entries in extlinux.conf on arm (6.30 KB, patch)
2014-07-02 01:51 UTC, Dennis Gilmore
no flags Details | Diff
patch to update fdtdir entries in extlinux.conf on arm (6.00 KB, patch)
2014-07-02 01:51 UTC, Dennis Gilmore
no flags Details | Diff
add devtree support to extlinux (6.42 KB, patch)
2014-10-13 01:46 UTC, Dennis Gilmore
no flags Details | Diff
add support to new-kernel-pkg to update fdtdir on 32 bit arm (7.33 KB, patch)
2014-10-13 01:47 UTC, Dennis Gilmore
no flags Details | Diff
cleanup the dtb option handling (1.98 KB, patch)
2014-10-13 01:49 UTC, Dennis Gilmore
no flags Details | Diff

Description Dennis Gilmore 2014-04-17 13:06:02 UTC
Description of problem:
u-boot has two different types of ways to specify how it finds a devicetree blob in its syslinux parsing code
fdt/devicetree and fdtdir/devicetreedir 

fdt and devicetree specify a path to an exact devicetree blob
fdtdir and devicetreedir specify a directory where devicetree blobs can be found 

the kernel puts the devicetree blobs in /boot/dtb-( kernel version)

we need grubby to update the line appropriately 

some valid configs are as follows

label Fedora (3.15.0-0.rc1.git0.1.fc21.armv7hl) 21 (Rawhide)
        kernel /vmlinuz-3.15.0-0.rc1.git0.1.fc21.armv7hl
        fdtdir /dtb-3.15.0-0.rc1.git0.1.fc21.armv7hl/
        append console=ttymxc0,115200 root=UUID=7ee85ed8-de4a-4779-8658-2daed0d35e97 ro rhgb quiet LANG=en_US.UTF-8
        initrd /initramfs-3.15.0-0.rc1.git0.1.fc21.armv7hl.img

label Fedora (3.15.0-0.rc1.git0.1.fc21.armv7hl) 21 (Rawhide)
        kernel /vmlinuz-3.15.0-0.rc1.git0.1.fc21.armv7hl
        devicetreedir /dtb-3.15.0-0.rc1.git0.1.fc21.armv7hl/
        append console=ttymxc0,115200 root=UUID=7ee85ed8-de4a-4779-8658-2daed0d35e97 ro rhgb quiet LANG=en_US.UTF-8
        initrd /initramfs-3.15.0-0.rc1.git0.1.fc21.armv7hl.img

label Fedora (3.15.0-0.rc1.git0.1.fc21.armv7hl) 21 (Rawhide)
        kernel /vmlinuz-3.15.0-0.rc1.git0.1.fc21.armv7hl
        fdt /dtb-3.15.0-0.rc1.git0.1.fc21.armv7hl/imx6q-wandboard.dtb
        append console=ttymxc0,115200 root=UUID=7ee85ed8-de4a-4779-8658-2daed0d35e97 ro rhgb quiet LANG=en_US.UTF-8
        initrd /initramfs-3.15.0-0.rc1.git0.1.fc21.armv7hl.img

label Fedora (3.15.0-0.rc1.git0.1.fc21.armv7hl) 21 (Rawhide)
        kernel /vmlinuz-3.15.0-0.rc1.git0.1.fc21.armv7hl
        devicetree /dtb-3.15.0-0.rc1.git0.1.fc21.armv7hl/imx6q-wandboard.dtb
        append console=ttymxc0,115200 root=UUID=7ee85ed8-de4a-4779-8658-2daed0d35e97 ro rhgb quiet LANG=en_US.UTF-8
        initrd /initramfs-3.15.0-0.rc1.git0.1.fc21.armv7hl.img


on an update we need to ensure that the kernel nvr is updated the same as in a kernel or initrd line.

any questions please ask

Comment 1 Brian Lane 2014-04-21 17:40:23 UTC
Created attachment 888179 [details]
add --devtree support to extlinux

Comment 2 Dennis Gilmore 2014-04-24 03:30:10 UTC
Created attachment 889181 [details]
add support to new-kernel-pkg to update fdtdir on 32 bit arm

Additionally this patch is needed for everything to work right

Comment 3 Matthew Garrett 2014-04-25 16:09:53 UTC
Are you sure this is right?

+    if [ "x$devtreefile" != "x" -a -f "$devtreefile" -o -d "$devtreefile" ]; then

This is meant to be if $devtree && (is file or is dir), but I think it's actually (if $devtree && is file) or is dir

Also, allowing $devtreefile to be a directory is a touch confusing.

Comment 4 Dennis Gilmore 2014-04-25 18:48:00 UTC
fdtdir in extlinux.conf tells u-boot a directory to find the dtb files. 
fdt in extlinux.conf tells u-boot a specific dtb file to use.

we want to tell u-boot here to find the dtb files and then let u-boot use its internal knowledge to select the specific dtb file to use.

perhaps the better way to deal with it is to have another option to grubby --devtreedir to update fdtdir and have --devtree update a fdt entry 

if we keep the existing logic then we should make it clearer what it is supposed to do.

Comment 5 Dennis Gilmore 2014-07-02 01:51:06 UTC
Created attachment 913961 [details]
patch to update fdt entries in extlinux.conf on arm

Comment 6 Dennis Gilmore 2014-07-02 01:51:51 UTC
Created attachment 913962 [details]
patch to update fdtdir entries in extlinux.conf on arm

Comment 7 Dennis Gilmore 2014-07-02 01:53:41 UTC
I have tested these two patches and I think they are a better way to deal with things. I do need to do some testing on highbank to make sure fdtdir entries don't get added on new kernels as their u-boot will stop processing when encountering an unknown entry.

Comment 8 Brian Lane 2014-07-08 16:55:21 UTC
You don't need to use x in the checks:

if [ "x$devtreefile" != "x"

this should work fine:

-n "$devtreefile"

And in the devtreedir patch you set DEVTREE instead of DEVTREEDIR

Comment 9 Dennis Gilmore 2014-07-16 00:46:28 UTC
*** Bug 1119937 has been marked as a duplicate of this bug. ***

Comment 10 Adam Williamson 2014-07-16 16:38:30 UTC
Discussed at the 2014-07-16 blocker review meeting: http://meetbot.fedoraproject.org/fedora-blocker-review/2014-07-16/f21-blocker-review.2014-07-16-15.59.log.txt . Accepted as a blocker per criterion https://fedoraproject.org/wiki/Fedora_21_Alpha_Release_Criteria#Release-blocking_images_must_boot , "All release-blocking images must boot in their supported configurations."

Comment 11 Adam Williamson 2014-07-30 17:08:28 UTC
Discussed at 2014-07-30 blocker review meeting: http://meetbot.fedoraproject.org/fedora-blocker-review/2014-07-30/f21-blocker-review.2014-07-30-15.59.log.txt . Work on this bug seems to have stalled: does anyone have a progress update? Thanks!

Comment 12 Stephen Warren 2014-08-06 21:47:09 UTC
Dennis,

In attachment 913962 [details] "patch to update fdtdir entries in extlinux.conf on arm", I don't think that making both entries in extlinuxKeywords[] be LT_DEVTREE is correct.

If you do this, there's no way to direct getKeywordByType() to return "fdt" or "fdtdir", since they have the same type. Instead, the first entry, "fdt", will always be returned.

In turn, this means that the code in addNewKernel() which adds an LT_DEVTREE entry in the case where there's a template, but not fdt/fdtdir entry in the template, will always add an "fdt" entry rather than sometimes and "fdt" and sometimes and "fdtdir" entry:

            } else if (tmplLine->type == LT_ENTRY_END && needs & NEED_DEVTREE) {
                const char *ndtp = newDevTreePath;
                if (!strncmp(newDevTreePath, prefix, strlen(prefix)))
                    ndtp += strlen(prefix);
                newLine = addLine(new, config->cfi, LT_DEVTREE,
                                  config->secondaryIndent,
                                  ndtp);
                needs &= ~NEED_DEVTREE;
                newLine = addLineTmpl(new, tmplLine, newLine, NULL, config->cfi);
            } else

I think fixing this would require a new LT_DEVTREEDIR in lineType_e, and perhaps logic like:

when copying fdt from template to new:
    if newDevTree is set:
       emit an updated fdt line
    else:
       remove the line /* or error? */

when copying fdtdir from template to new:
    if newDevTreeDir is set:
       emit an updated fdtdir line
    else:
       remove the line /* or error? */

at LT_ENTRY_END, if newDevTree was set but not yet copied from template:
    emit a new fdt line

at LT_ENTRY_END, if newDevTreeDir was set but not yet copied from template:
    emit a new fdtdir line

That probably also requires a new NEED_DEVTREEDIR flag too.

?

Comment 13 Kamil Páral 2014-08-13 17:44:41 UTC
Discussed at the 2014-08-13 blocker review meeting:
http://meetbot.fedoraproject.org/fedora-blocker-review/2014-08-13/
<dgilmore> ive started on updating things but ive not yet gotten it right
<dgilmore> it will be done when i can get time to finish it
<pschindl> #info dgilmore has this on his probably veeery full todo list. When there will be time he will finish it.

Comment 14 Stephen Warren 2014-08-20 16:59:04 UTC
satellit, did you mean to remove the needinfo flag, or just to add yourself to CC?

Comment 15 satellitgo 2014-08-20 23:13:09 UTC
add to cc only

Comment 16 Stephen Warren 2014-08-20 23:17:35 UTC
Restoring the needinfo flag.

Comment 17 Adam Williamson 2014-09-10 18:51:36 UTC
This has been sitting around on the Alpha blocker list for like two months now, we really need some clarification on the status here. Is it blocking Alpha? Is it not? Is it fixed? Thanks!

Comment 18 Peter Robinson 2014-09-10 20:22:59 UTC
I propose we drop this back to a beta blocker and document what needs to be done to work around it. 

It affects two things:
1) kernel updates of images (primarily once the initial kernel is rotated out)
2) network installs (can be worked around if done using a kickstart)

Comment 19 Adam Williamson 2014-09-11 22:36:40 UTC
This was discussed at today's go/no-go meeting: http://meetbot.fedoraproject.org/fedora-meeting-2/2014-09-11/f21_alpha_gono-go_meeting_-_2.2014-09-11-17.03.log.html . We agreed to make it BetaBlocker, AlphaFreezeException, and document the workaround.

Comment 20 Adam Williamson 2014-09-22 18:34:39 UTC
Peter, can you please either write this into CommonBugs or send me a plain text commonbugs-ish note I can add to the page? I can tweak the language and formatting and links and so on, but I need the details about what the bug actually involves and what people have to do. Thanks!

Comment 21 Paul Whalen 2014-09-22 19:47:27 UTC
Adam, Peter, 

Added to common bugs under https://fedoraproject.org/wiki/Common_F21_bugs#ARM_issues

Comment 22 Adam Williamson 2014-10-08 17:00:57 UTC
Discussed at 2014-10-08 blocker review meeting: http://meetbot.fedoraproject.org/fedora-blocker-review/2014-10-08/f21-blocker-review.2014-10-08-15.58.log.txt . pwhalen says pbrobinson is working on this and it's expected soon.

Comment 23 Dennis Gilmore 2014-10-13 01:46:46 UTC
Created attachment 946192 [details]
add devtree support to extlinux

Comment 24 Dennis Gilmore 2014-10-13 01:47:40 UTC
Created attachment 946193 [details]
add support to new-kernel-pkg to update fdtdir on 32 bit arm

Comment 25 Dennis Gilmore 2014-10-13 01:49:34 UTC
Created attachment 946195 [details]
cleanup the dtb option handling

with the latest 3 patches everything on the grubby side works as expected in all supported use cases.

Comment 26 Fedora Update System 2014-10-15 21:16:43 UTC
grubby-8.35-6.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/grubby-8.35-6.fc21

Comment 27 Adam Williamson 2014-10-16 00:05:25 UTC
There seems to be a problem in one of Dennis' patches for this. Building a live image with the new grubby I get these errors:

/sbin/new-kernel-pkg: line 194: [: too many arguments
/sbin/new-kernel-pkg: line 200: [: too many arguments
No '/dev/log' or 'logger' included for syslog logging
/sbin/new-kernel-pkg: line 460: [: too many arguments
/sbin/new-kernel-pkg: line 466: [: too many arguments
No '/dev/log' or 'logger' included for syslog logging
/sbin/new-kernel-pkg: line 194: [: too many arguments
/sbin/new-kernel-pkg: line 200: [: too many arguments

The offending bits seem to be these blocks of Dennis' "add support for devicetree directories for use on arm":

@@ -191,11 +191,17 @@ install() {
     fi
 
     DEVTREE=""
-    if [ "x$devtreefile" != "x" -a -f "$devtreefile" ]; then
+    if [ -n $devtreefile -a -f "$devtreefile" ]; then
 	[ -n "$verbose" ] && echo "found $devtreefile and using it with grubby"
 	DEVTREE="--devtree $devtreefile"
     fi
 
+    DEVTREEDIR=""
+    if [ -n $devtreedir -a -d "$devtreedir" ]; then
+	[ -n "$verbose" ] && echo "found $devtreedir and using it with grubby"
+	DEVTREEDIR="--devtreedir $devtreedir"
+    fi
+
     # FIXME: is this a good heuristic to find out if we're on iSeries?
     if [ -d /proc/iSeries ]; then
 	[ -n "$verbose" ] && echo "On an iSeries, just making img file"

and:

@@ -451,11 +457,17 @@ update() {
     fi
 
     DEVTREE=""
-    if [ "x$devtreefile" != "x" -a -f "$devtreefile" ]; then
+    if [ -n $devtreefile -a -f "$devtreefile" ]; then
         [ -n "$verbose" ] && echo "found $devtreefile and using it with grubby"
         DEVTREE="--devtree $devtreefile"
     fi
 
+    DEVTREEDIR=""
+    if [ -n $devtreedir -a -d "$devtreedir" ]; then
+        [ -n "$verbose" ] && echo "found $devtreedir and using it with grubby"
+        DEVTREEDIR="--devtreedir $devtreedir"
+    fi
+
     if [ -n "$cfgGrub" ]; then
 	[ -n "$verbose" ] && echo "updating $version from $grubConfig"
 	ARGS="--grub -c $grubConfig --update-kernel=$kernelImage $INITRD \

Comment 28 Adam Williamson 2014-10-16 00:14:43 UTC
So I think this is a non-fatal error, but it should still be fixed. I did a quick test, and it looks like statements of that form break if the variable is unassigned.

this works, whether /tmp/foo exists or not:

devtreefile="/tmp/foo"
if [ -n $devtreefile -a -f "$devtreefile" ]; then
    echo "True!"
else
    echo "False!"
fi

but if you comment out the assignment of devtreefile:

#devtreefile="/tmp/foo"
if [ -n $devtreefile -a -f "$devtreefile" ]; then
    echo "True!"
else
    echo "False!"
fi

you get:

[adamw@adam grubby (f21 %)]$ sh /tmp/test.sh 
/tmp/test.sh: line 4: [: too many arguments
False!

so it seems to continue executing (or else False! wouldn't be printed), but does show an error.

Comment 29 Adam Williamson 2014-10-16 00:18:57 UTC
Oh, now I see the problem, it's simple - the first occurrence of $devtreefile is unquoted in every case. This works:

#devtreefile="/tmp/foo"
if [ -n "$devtreefile" -a -f "$devtreefile" ]; then
    echo "True!"
else
    echo "False!"
fi

Comment 30 Adam Williamson 2014-10-16 01:16:53 UTC
Well, there's a more serious bug in the new grubby - it writes an invalid grub2.cfg and the installed system doesn't boot. There's no initramfs line, and all the LV locations are wrong - it has "root=/dev/mapper/fedora-root" , "rd.lvm.lv=fedora/swap" , "rd.lvm.lv=fedora/root" when the correct location is fedora_localhost/ in each case, i.e. the hostname is missing from the VG path, I guess.

If I manually edit all three locations and add the initramfs16 line, the system boots.

I don't know if this is somehow caused by the issue I noted above (even though in my test script execution seemed to continue after the 'error'), I can check that by editing new-kernel-pkg in place, I guess.

Comment 31 Adam Williamson 2014-10-16 01:18:38 UTC
correction - it's just the missing initrd16 line that's incorrect, the VG paths are correct (different between my two test VMs for some reason).

Comment 32 Adam Williamson 2014-10-16 03:34:45 UTC
I've filed the new bug as https://bugzilla.redhat.com/show_bug.cgi?id=1153410 , since it doesn't look like it's directly related to the ARM device tree stuff.

Comment 33 Fedora Update System 2014-10-16 17:17:36 UTC
Package grubby-8.35-6.fc21:
* should fix your issue,
* was pushed to the Fedora 21 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing grubby-8.35-6.fc21'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2014-12932/grubby-8.35-6.fc21
then log in and leave karma (feedback).

Comment 34 Fedora Update System 2014-10-17 19:45:11 UTC
Package grubby-8.35-7.fc21:
* should fix your issue,
* was pushed to the Fedora 21 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing grubby-8.35-7.fc21'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2014-12932/grubby-8.35-7.fc21
then log in and leave karma (feedback).

Comment 35 Fedora Update System 2014-10-20 23:02:34 UTC
grubby-8.35-7.fc21 has been pushed to the Fedora 21 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 36 Adam Williamson 2014-11-04 00:07:58 UTC
fixed, commonbugs no longer needed.


Note You need to log in before you can comment on or make changes to this bug.