Bug#1030519: check-missing-firmware: patch for files with space characters, mediamount and more (with code)

[Alexander Dalm]

Package: hw-detect
Version: 1.154
Severity: normal
Tags: d-i
X-Debbugs-Cc: a.dal...@googlemail.com

Dear Maintainer,

based on my previous bug report #1029843 (wrong package selected), so I tried to fix it myself.

First up, I had a really hard time setting things up for debugging. Building the debian-installer is quite easy but modifying an existing package is not mentioned in the wiki. Since packages are downloaded during build/rebuild, even if they are already existing in /debian-installer/installer/build/apt.udeb/cache/archives you can't replace these files. As it turns out you can simply put them here: /debian-installer/installer/build/localudebs
Please add this link: https://d-i.debian.org/doc/internals/ch04.html#idm692
to the Questions section of: https://wiki.debian.org/DebianInstaller/Build
And please mention that localudebs can replace the ones on the mirror here as well: /debian-installer/installer/build/README

Building the hw-detect udep was challenging as well. The package Makefile contains no useful information, it seems there are only options to install this package. This link is slightly helpful since it mentions that they are build like normal packages: https://wiki.debian.org/BuildingUDebs
btw: "XC-Package-Type: udeb" should be changed to "Package-Type: udeb"
So, I build it with dpkg-deb like just like my driver packages:

1. get the source:
git clone https://salsa.debian.org/installer-team/hw-detect.git hw-detect

2. extract and copy the missing templates file from hw-detect_1.154_amd64.udeb/control.tar.xz/./templates to /hw-detect/debian/

3. create folder structure, control file and copy files:
mkdir -p hw-detect_patched/DEBIAN && mkdir hw-detect_patched/bin && mkdir -p hw-detect_patched/etc/hotplug.d/net && mkdir -p hw-detect_patched/lib/udev/rules.d && mkdir -p hw- detect_patched/usr/lib/finish-install.d && mkdir hw-detect_patched/usr/lib/post-base-installer.d && mkdir hw-detect_patched/usr/lib/pre-pkgsel.d

echo $'Package: hw-detect\nVersion: 1.154\nArchitecture: amd64\nMaintainer: Debian Install System Team <debia...@lists.debian.org>\nInstalled-Size: 326\nDepends: rootskel, archdetect, cdebconf-udeb, di-utils, pciutils-udeb, udpkg\nSection: debian-installer\nPriority: standard\nDescription: Detect hardware and load kernel drivers for it' > hw-detect_patched/DEBIAN/control

cp hw-detect/debian/templates hw-detect_patched/DEBIAN/templates
cp hw-detect/check-missing-firmware.sh hw-detect_patched/bin/check-missing-firmware
cp hw-detect/hotplug-pcmcia.sh hw-detect_patched/bin/hotplug-pcmcia
cp hw-detect/hw-detect.sh hw-detect_patched/bin/hw-detect
cp hw-detect/sysfs-update-devnames.sh hw-detect_patched/bin/sysfs-update-devnames
cp hw-detect/net-hotplug.sh hw-detect_patched/etc/hotplug.d/net/hw-detect.hotplug
cp hw-detect/net-hotplug.rules hw-detect_patched/lib/udev/rules.d/010_net-hotplug.rules
cp hw-detect/hw-detect.finish-install.d/08hw-detect hw-detect_patched/usr/lib/finish-install.d/08hw-detect
cp hw-detect/hw-detect.finish-install.d/30hw-detect hw-detect_patched/usr/lib/finish-install.d/30hw-detect
cp hw-detect/hw-detect.post-base-installer.d/50install-firmware hw-detect_patched/usr/lib/post-base-installer.d/50install-firmware
cp hw-detect/hw-detect.post-base-installer.d/60install-mouseemu hw-detect_patched/usr/lib/post-base-installer.d/60install-mouseemu
cp hw-detect/hw-detect.pre-pkgsel.d/20install-hwpackages hw-detect_patched/usr/lib/pre-pkgsel.d/20install-hwpackages
cp hw-detect/hw-detect.pre-pkgsel.d/50install-firmware hw-detect_patched/usr/lib/pre-pkgsel.d/50install-firmware

4. replace/edit source files

5. build package into localudebs/ directory:
dpkg-deb --build hw-detect_patched debian-installer/installer/build/localudebs/hw-detect_patched.udeb

dpkg-deb has no option to output udeb (apart from the filename), specifying "XC-Package-Type: udeb" or "Package-Type: udeb" doesn't work either. It seems like the udep on the mirror is also just a .dep file named .udep since "Package-Type: udeb" is not specified in the control file despite being specified in the source control file.

Can these steps be added to the Makefile or what is the proper way to build the hw-detect udep?
I also found this link but I didn't tried it since the manual way works fine:
https://wiki.debian.org/DebianInstaller/Modify/CD#Create_a_udeb_packages_file

---

check-missing-firmware(.sh) changes:
nic_is_configured()
added skip_modules variable to skip removing/loading of modules that cause problems
(intel_sst_acpi -> modprobe: FATAL: Module snd_intel_acpi is in use.)
check_missing()
added support for firmware-filenames with spaces:
files variable stored as files file in tmp/
dmesg set regex and output modified as pipe separated
"while IFS='|' read" for pipe separated list
bug fixed in file listing:
when modules aren't or can't be removed/loaded, files don't appear in get_fresh_dmesg again
dmesg list is appended to fwlist instead of overwriting the file
actual removed/loaded modules are removed from fwlist (in main loop)
added filter for one file requested multiple times in dmesg
(actually happened on my Z83 Mini PC / Beelink BT3)
optimized modules listing -> no additional Uniquify needed
try_copy()
added source path parameter
this allows to use plain files in /firmware and /cdrom/firmware mounts (PXE initrd / CD / USB drive)
currently only .deb files can be used here
-> main use case: simply flash iso with Rufus and drag and dop plain files to firmware folder
check_for_firmware()
grepfor file removed since the files file is already line-separated
"for filename in $dir/*.deb; do" changed to "for filename in $(ls $dir/*.deb); do"
otherwise only one item e.g. "/cdrom/firmware/*.deb" is checked instead of the actual files
this is probably a compatibility issue with the used ash shell in the netboot mini-ISO
(I generally had a hard time testing/debugging between installed debian sh and iso, >30x bulding netinst)
main loop (while check_missing && ask_load_firmware; do)
added modified try_copy to /firmware and /cdrom/firmware to allow plain files
(see try_copy)
mountmedia; / mountmedia driver;
despite discussed in #1029543, no one actually tested this, I did:
calling mountmedia; works fine
calling mountmedia driver; causes long loading times and multiple errors (see Logs)
crude disabling removed (if [ "$loop" -lt 1 ]; then)
"check_for_firmware /media /media/firmware" moved from "mountmedia driver;" to "mountmedia;"
"mountmedia driver;" removed
remove files from fwlist after remove/load of module
(see bug in check_missing)
multiple locations
various changes for line-separated files file instad of space-separated variable
(use diff tool to spot the changes)

I will submit this file and a screenshot as attachment in a further information mail (instead of creating a Gitlab account)

suggested change to templates file (localisations):
With my changes in check-missing-firmware files are displayed with line breaks in the graphical installer but the first file is still shown in the same line.
This looks a bit confusing especially when dealing with multiple files.
I tried adding a leading line break using $(cat $flist | sed '1s/^/\n/') or creating a new file without success.
but the teplates file can be changed from:
The missing firmware files are: ${FILES}
.
If you have such media available now, insert it, and continue.
to:
The missing firmware files are:
.
${FILES}
.
If you have such media available now, insert it, and continue.
to solve this issue. There might be also a way without inserting an empty line on top but simply adding a line break doesn't work.


Logs:

mainloop iteration #1
DEBUG: firmware directory not found
DEBUG: cdrom-firmware directory not found

[point where mountmedia is called]
kernel: FAT-fs (mmcblk0p1): Volume was not properly unmounted. Some data may be corrupt. Please run fsck.
kernel: FAT-fs (mmcblk0p1): Volume was not properly unmounted. Some data may be corrupt. Please run fsck.
DEBUG: mountmedia ok
DEBUG: try copy (fw_sst_22a8.bin)
DEBUG: try copy (fw_sst_22a8.binbrcmfmac43340-sdio.To be filled by O.E.M.-Z83.bin)
DEBUG: try copy (brcmfmac43340-sdio.bin)
kernel: FAT-fs (mmcblk0p1): Volume was not properly unmounted. Some data may be corrupt. Please run fsck.
kernel: FAT-fs (mmcblk0p1): Volume was not properly unmounted. Some data may be corrupt. Please run fsck.

[point where mountmedia driver is called]
mount: mounting /dev/mmcblk0 on /media failed: Invalid argument
unmount: can't unmount /media: Invalid argument
mount: mounting /dev/mmcblk0 on /media failed: Invalid argument
mount: mounting /dev/mmcblk0boot0 on /media failed: Invalid argument
unmount: can't unmount /media: Invalid argument
mount: mounting /dev/mmcblk0boot0 on /media failed: Invalid argument
mount: mounting /dev/mmcblk0boot1 on /media failed: Invalid argument
unmount: can't unmount /media: Invalid argument
mount: mounting /dev/mmcblk0boot1 on /media failed: Invalid argument
mount: mounting /dev/sda on /media failed: Invalid argument
unmount: can't unmount /media: Invalid argument
mount: mounting /dev/sda on /media failed: Invalid argument
mount: mounting /dev/mmcblk0boot0 on /media failed: Invalid argument
unmount: can't unmount /media: Invalid argument
mount: mounting /dev/mmcblk0boot0 on /media failed: Invalid argument
mount: mounting /dev/mmcblk0boot1 on /media failed: Invalid argument
unmount: can't unmount /media: Invalid argument
mount: mounting /dev/mmcblk0boot1 on /media failed: Invalid argument
mount: mounting /dev/mmcblk0 on /media failed: Invalid argument
unmount: can't unmount /media: Invalid argument
mount: mounting /dev/mmcblk0 on /media failed: Invalid argument
mount: mounting /dev/mmcblk0boot0 on /media failed: Invalid argument
unmount: can't unmount /media: Invalid argument
mount: mounting /dev/mmcblk0boot0 on /media failed: Invalid argument
mount: mounting /dev/mmcblk0boot1 on /media failed: Invalid argument
unmount: can't unmount /media: Invalid argument
mount: mounting /dev/mmcblk0boot1 on /media failed: Invalid argument
mount: mounting /dev/sda on /media failed: Invalid argument
unmount: can't unmount /media: Invalid argument
mount: mounting /dev/sda on /media failed: Invalid argument
mount: mounting /dev/mmcblk0boot0 on /media failed: Invalid argument
unmount: can't unmount /media: Invalid argument
mount: mounting /dev/mmcblk0boot0 on /media failed: Invalid argument
mount: mounting /dev/mmcblk0boot1 on /media failed: Invalid argument
unmount: can't unmount /media: Invalid argument
mount: mounting /dev/mmcblk0boot1 on /media failed: Invalid argument
mount: mounting /dev/mmcblk0p2 on /media failed: Invalid argument
unmount: can't unmount /media: Invalid argument
mount: mounting /dev/mmcblk0p2 on /media failed: Invalid argument
mount: mounting /dev/mmcblk0p3 on /media failed: Invalid argument
unmount: can't unmount /media: Invalid argument
mount: mounting /dev/mmcblk0p3 on /media failed: Invalid argument
mount: mounting /dev/sda1 on /media failed: Invalid argument
unmount: can't unmount /media: Invalid argument
mount: mounting /dev/sda1 on /media failed: Invalid argument
mount: mounting /dev/mmcblk0 on /media failed: Invalid argument
unmount: can't unmount /media: Invalid argument
mount: mounting /dev/mmcblk0 on /media failed: Invalid argument
mount: mounting /dev/mmcblk0boot0 on /media failed: Invalid argument
unmount: can't unmount /media: Invalid argument
mount: mounting /dev/mmcblk0boot0 on /media failed: Invalid argument
mount: mounting /dev/mmcblk0boot1 on /media failed: Invalid argument
unmount: can't unmount /media: Invalid argument
mount: mounting /dev/mmcblk0boot1 on /media failed: Invalid argument
kernel: FAT-fs (mmcblk0p1): Volume was not properly unmounted. Some data may be corrupt. Please run fsck.
kernel: FAT-fs (mmcblk0p1): Volume was not properly unmounted. Some data may be corrupt. Please run fsck.
DEBUG: mountmedia driver not ok

[error at: remove and reload modules so they see the new firmware]
removing and loding kernel module intel_sst_acpi
modprobe: FATAL: Module intel_sst_acpi not found.
modprobe: FATAL: Module intel_sst_acpi not found in directory /lib/modules/6.1.0-3-amd64
modprobe: removing and loading kernel module snd_intel_sst_acpi as well (actual module for intel_sst_acpi)
modprobe: FATAL: Module snd_intel_acpi is in use.

-> intel/fw_sst_22a8.bin not requested in next loop


-- System Information:
Debian Release: 11.6
APT prefers stable-updates
APT policy: (500, 'stable-updates'), (500, 'stable-security'), (500, 'stable')
Architecture: amd64 (x86_64)

Kernel: Linux 5.10.0-21-amd64 (SMP w/4 CPU threads)
Locale: LANG=de_DE.UTF-8, LC_CTYPE=de_DE.UTF-8 (charmap=UTF-8), LANGUAGE not set
Shell: /bin/sh linked to /usr/bin/dash
Init: systemd (via /run/systemd/system)
LSM: AppArmor: enabled

[Cyril Brulebois]

Control: tag -1 - d-i

Hi Alexander,

Alexander Dalm <a.dal...@googlemail.com> (2023-02-04):

> So, I build it with dpkg-deb like just like my driver packages:

[…]

> Can these steps be added to the Makefile or what is the proper way to build the hw-detect udep?

That's a Debian package, it's built with dpkg-buildpackage, just like
any other Debian package. That's why there's no specific information in
any Makefile, README, or wiki.

> I also found this link but I didn't tried it since the manual way works fine:
> https://wiki.debian.org/DebianInstaller/Modify/CD#Create_a_udeb_packages_file
>
> ---
>

> I will submit this file and a screenshot as attachment in a further

> information mail (instead of creating a Gitlab account) […]

Attaching patches is best done by using git diff / git format-patch.

> With my changes in check-missing-firmware files are displayed with
> line breaks in the graphical installer but the first file is still
> shown in the same line.

Not a good idea, seeing iwlwifi requests 68 files…


Cheers,
--
Cyril Brulebois (ki...@debian.org) <https://debamax.com/>
D-I release manager -- Release team member -- Freelance Consultant

[Cyril Brulebois]

Hi,

Alexander Dalm <a.dal...@googlemail.com> (2023-02-05):
> Hi Cyril,

>
> > Attaching patches is best done by using git diff / git format-patch.

> sure, but this is an inconvenience for me as outside contributor and might
> deter other ones as well.
> I'm sure you and other core maintainers can import one file with ease...

The burden goes both ways. Sending your file over the wall is easy for
you but that means we end up reviewing this huge diff, without any kind
of logical split between changes:

check-missing-firmware.sh | 185 ++++++++++++++++++++++++++++++++++------------
1 file changed, 139 insertions(+), 46 deletions(-)

Anyway, playing with IFS is a recipe for a disaster, and why are we even
talking about files with space characters in the first place?

This is a little sad because there might be interesting bits in there
(like side-stepping moutmedia which is apparently not quite doing what
some users might expect, see #1029543, #1029962).

[James Addison]

Followup-For: Bug #1030519
Control: unmerge -1
Control: reassign -1 hw-detect
Control: retitle -1 hw-detect: firmware file path handling is fragile

[James Addison]

Source: hw-detect
Followup-For: Bug #1030519
X-Debbugs-Cc: a.dal...@googlemail.com

Hi Alexander,

I've been reviewing your patch and would like to suggest extracting the
following changes from it to consider and apply individually:

1. Supporting firmware filenames that contain spaces.

2. Removing (or at least reducing) the 5s wait[1] for USB devices to settle.

3. Refactoring the fwfile 'for' loop[2] to use less-complicated parameter
expansion (your changes didn't modify this but did highlight it
potentially more complicated than necessary).

Each of these changes would require some description and a small patch -
writing those may require more time, I admit; the reward is that it makes it
easier for the maintainer to accept the changes.


During review, I considered these as possible other changes:

* Loading the 'vfat' kernel module before mountpoint search.

* Consulting the 'maybe-usb-floppy' mountmedia device as an origin.

However, it seems that mountmedia already handles these?

https://sources.debian.org/src/mountmedia/0.26/mountmedia/?hl=20#L69
https://sources.debian.org/src/mountmedia/0.26/mountmedia/?hl=20#L20

Thank you!
James

[1] - https://sources.debian.org/src/mountmedia/0.26/mountmedia/?hl=20#L82

[2] - https://sources.debian.org/src/hw-detect/1.159/check-missing-firmware.sh/#L210