[Bug 1846329] Re: [FFe] 2019.07 to support Pi4 boot

[Robie Basak]

This is a review of your branch ubuntu-2019.07-pi4 with commit hash
94f89d8. I adjusted it to make my review eaiser (mainly splitting some
commits). I've pushed to
https://code.launchpad.net/~racb/ubuntu/+source/u-boot/+git/u-boot/+ref/review/waveform/ubuntu-2019.07-pi4
if you want to use it, but this isn't necessary.

In general, excellent work! The approach you've taken seems nice and
clean, which is impressive given how tricky things are in this area.

debian/changelog:

Please squash all changelog entries into a single one for 2019.07+dfsg-
1ubuntu1. That also means that the changelog should describe only the
differences from 2019.04+dfsg-2ubuntu3, describing only the end result
as visible from the archive and skipping any churn that took place
during development in your branch.

Looking at your PPA, rather than using the orig tarball as published by
upstream, that orig tarball is supposed to have been repacked to remove
non-DFSG code, according to Files-Excluded in debian/copyright. uscan(1)
can help with this. But in this case an upload of this upstream version
had already been made by Debian. Therefore, please use the repacked orig
tarball as published by Debian (eg. as archived at
https://launchpad.net/debian/+source/u-boot/2019.07+dfsg-1) to avoid
potential mismatches. There's no point in changing anything in your PPA
now; your sponsor (presumably me) should be able to correct this when
uploading.

Only debian/patches/am57xx/omap5_distro_bootcmd actually needed
updating, and it was more than a refresh. I suggest avoiding using the
word "refresh" to describe the change, and please drop the unnecessary
refresh of the other patches.

Bump to v2019.07:

This is not practical to review. I understand it is necessary for rpi4
support. I'll leave it to the release team to make a call on the
regression risk.

python-pyelftools dependency: note that this binary package is in
universe even though its source is in main. Are you expecting to put u
-boot-rpi in main? I only see it in universe currently.

debian/patches/rpi4.patch:

Typo "Descrpition"

Consider doing this with multiple patches, one per commit, depending on
how often you expect you need to update it against Andrei Gherzan's
work. Maybe easier to leave it now it's done. Your choice.

Makefile: DTB addition duplicates existing bcm2837-rpi-3-b.dtb entry

rpi_4_defconfig drops:
-CONFIG_USB_DWC2=y
-CONFIG_USB_ETHER_LAN78XX=y
-CONFIG_USB_ETHER_SMSC95XX=y
-CONFIG_SUPPORT_RAW_INITRD=y
-CONFIG_ENV_IS_IN_FAT=y
...are all of these intentional? CONFIG_SUPPORT_RAW_INITRD and CONFIG_ENV_IS_IN_FAT in particular strike me as non-pi4-specific which is why I found this surprising.

bcm2835_sdhci.c: dev_get_driver_data call: no error checking now this is
a dynamic lookup and not a constant?

Quite a bit of RPi code is touched - for example the introduction of
bcm2835-common.dtsi and status -> status_r/status_w. Are all rpi
hardware variants tested against this branch?

debian/patches/rpi-import-mkknlimg.patch:

As we discussed, I'm not sure if we should drop this change (as it's not
strictly necessary for the rpi4) or keep it (as it cleans things up and
will make future backports easier). The trade-off is regression risk due
to the late upload for thi cycle versus increased work in maintaining a
branch just for Eoan for the same of this change. I'll leave the
decision between you and the release team.

debian/u-boot-rpi.postinst:

Style: please quote all shell variables where it's trivial to do so.
Saves a reviewer having to verify that there definitely cannot be any
whitespace in all cases. $platform_path in particular please.

I would consider:

for uboot_binary in "$UBOOT_DIR"/*/u-boot.bin; do
    <something using dirname and basename on $uboot_binary to get the parent directory name>
    cp "$uboot_binary" "$BOOT/uboot_${...}.bin"
done

This would protect a little better against directories containing other
things appearing in "$UBOOT_DIR" in the future, although it's still not
perfect. The reason I noticed this is because rpi-config-migration is
now being added to /usr/lib/u-boot so the directory no longer contains
_only_ bootloaders, though your -d check is still perfectly functional
currently. I suppose what I'm saying is that what /usr/lib/u-boot/
contains exactly isn't clearly defined, though you are relying on such a
definition here, which might make it error-prone in the future. However
in mitigation the packaging is in full control of this and there isn't
very much of it. Your choice.

> if dpkg --compare-versions "$2" lt "2019.07+dfsg-1~" && is_old_config;
then

I find this surprising as 2019.07+dfsg-1 has been published by Debian,
but this test doesn't involve that version. How about 2019.07+dfsg-
1ubuntu1~ instead?

debian/bin/rpi-config-migration:

I would put rpi-config-migration in /usr/share, not /usr/lib. However
the standard does allow /usr/lib as an exception in this case. Is this a
conscious decision? I would use /usr/share anyway here, as would provide
a convenient separation between packaging and payload in this case.
However I leave this choice up to you. Revelant policy:
https://www.debian.org/doc/debian-policy/ch-opersys.html#file-system-
structure

Style: please quote all shell variables where it's trivial to do so.
Saves a reviewer having to verify that there definitely cannot be any
whitespace in all cases.

Perhaps check that the expected-to-be-new files being unconditionally
written to in migrate_config() do not exist before writing them, and
consider that another case of "should not migrate"? I guess those tests
would need to be in is_old_config(), and presumably some files are
expected to be overwritten so this would only apply to those that
aren't. Your choice.

Next steps:

Please address the review points above, except the ones labelled "your
choice" which are up to you. Note that I'm calling out things I find
surprising; the resolution may well be that what you've done is still
correct, and no change is needed. Where I'm wrong, that's fine but
please do explain :)

Due to the freeze, this freeze exception needs review and approval by
someone from the release team before I can sponsor this upload.

-- 
You received this bug notification because you are a member of Ubuntu
Foundations Bugs, which is subscribed to u-boot in Ubuntu.
https://bugs.launchpad.net/bugs/1846329

Title:
  [FFe] 2019.07 to support Pi4 boot

Status in u-boot package in Ubuntu:
  New

Bug description:
  Impact
  ======

  The proposed version supports booting Ubuntu on the Raspberry Pi 4,
  both by providing a Pi 4 compatible variant of u-boot (which requires
  the version bump to 2019.07), and by detecting and (if required)
  migrating the boot configuration to support the Pi 4 (by calling
  different u-boot binaries depending upon the host Pi's version).

  The package has been built in the following PPA:
  https://launchpad.net/~waveform/+archive/ubuntu/ubpi4/+packages

  A git-ubuntu branch with the proposed changes is available at:
  https://code.launchpad.net/~waveform/ubuntu/+source/u-boot/+git/u-boot/+ref/ubuntu-2019.07-pi4
  (the version in this branch is lower than the PPA as the branch has
  been rebased for clarity, compressing all the minor modifications done
  during testing)

  
  Test Case
  ==========

  Upon installing the new package on a system with the existing u-boot
  setup (a single u-boot binary under /boot/firmware and a config.txt
  that points to it as the kernel), the postinst script should detect
  this scenario and migrate the configuration to one with multiple
  u-boot binaries under /boot/firmware and a config.txt with selective
  sections pointing to them. After installation, the card should still
  boot successfully on all supported models (at present the Pi4 will
  only get to the u-boot stage, pending kernel work).

  This process has been tested on the Pi 2 (armhf only), 3, 3A+, 3B+,
  and 4 (under armhf and arm64 on relevant platforms). The scripts for
  detecting and migrating the old boot configuration to the new have
  been tested with the Eoan and Bionic boot configurations, with a view
  to eventual SRU of this package to Bionic (for Pi 4 support on LTS).

  
  Regression Potential
  ====================

  Testing with the older Bionic firmware led to several changes (that
  led to re-testing on the Eoan firmware), which has increased
  confidence in the low probability of regressions at least as regards
  the Pi. The certification team will test other relevant platforms
  using u-boot to ensure they are still capable of booting with the
  2019.07 version (other than the pi's postinst script, no other scripts
  have changed so boot capability is all that requires testing).

  The boot config migration code takes some pains to detect the old and
  new states, both by hash checking and (as fallback) by rudimentary
  content analysis. It is conservative in choosing when to migrate (only
  when both a package version check passes and detection confirms the
  old boot configuration is present), and provides a backup of the
  original configuration (in fact, an "unmigration" routine is included
  in the library, primarily for testing).

  At this point, the major concern for regression is other platforms
  relying upon u-boot.

To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/u-boot/+bug/1846329/+subscriptions