Comment 3 for bug 1846329

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.