When we can't find a .ko module to satisfy the firmware request, try
harder by looking for a file to read in directly. We compuse this file's
name by appening the imagename to the firmware path (currently
hard-wired at /boot/firmware). Allow this file to be unload when
firmware_put() releases the last reference, but we don't need to do the
indirection and dance we need to do when unloading the .ko that will
unregister the firmware.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 55597 Build 52486: arc lint + arc unit
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Make this work, generally,
But also don't change the names we try to load by adding / trimming .fw
That was a brain artifact of my trying to be safe or the link_firmware.c
path that can't really be taken.
| sys/kern/subr_firmware.c | ||
|---|---|---|
| 145 | Maybe I'll default this to 2MB or 4MB then. | |
| 286 | Stupid reasons... I'll kill it. I had another review that did this via a linker (link_firmware.c). And for that I'd only link in .fw files as a safety measure (to make sure my linker didn't accidentally consume a .ko that's loaded). But that method fell part for other reasons... leaving just a couple of .fw floating in the ether in the version. | |
Comment Actions
With the latest, kldload if_rtw88 gives me
rtw880: <rtw_8822be> port 0x3000-0x30ff mem 0x56200000-0x5620ffff at device 0.0 on pci1 rtw8822b_fw.bin: could not load /boot/firmware/rtw8822b_fw.bin either rtw88/rtw8822b_fw.bin: Loaded using /boot/firmware/rtw88/rtw8822b_fw.bin rtw880: successfully loaded firmware image 'rtw88/rtw8822b_fw.bin' rtw880: Firmware version 27.2.0, H2C version 13 wlan0: Ethernet address: 28:3a:4d:45:95:9f
the 'either' is because sometimes we warn about not loading the .ko. and maybe the debugging
is too much...
| sys/kern/subr_firmware.c | ||
|---|---|---|
| 145 | A default of 4MB look sane to me. | |
| sys/kern/subr_firmware.c | ||
|---|---|---|
| 145 | -rw-r--r-- 1 bz staff 3963336 Jun 12 2023 ./ath11kfw/ath11k/QCN9074/hw1.0/amss.bin?h=20230515 Just about if this doesn't get any bigger... Maybe we should scroll through linux-firmware.git and see about file sizes... okay, we lost... -rw-r--r-- 1 bz staff 6308684 Dec 28 20:00 ./ath11k/WCN6855/hw2.0/board-2.bin Not sure if we still support Netronome NICs but some of their firmware is also lager ... The largets file if I am not wrong seems to be: | |
| 270 | Can we please not call this "fallback" but name it by what it is? | |
| 596 | If you want to do this we should keep the hierarchy, indeed. | |
| sys/kern/subr_firmware.c | ||
|---|---|---|
| 145 | Let's go to 8MB then. | |
Comment Actions
I claim I've addressed all the comments :).
If I've missed something, please comment again. It was unintentional.
| sys/kern/subr_firmware.c | ||
|---|---|---|
| 270 | I'll think and cope in next pass. Maybe try_raw_file? try_direct_file? | |
| 596 | I've lost the context for this comment... however, guessing I think it's addressed by the changes to not strip paths. | |
| sys/kern/subr_firmware.c | ||
|---|---|---|
| 270 | I think I've settled with 'try_native_file' and use the term native in preference to 'raw' in other places. | |
Comment Actions
Few comments but looks ok.
(BTW I prefered the 'raw' name compared to 'native' but don't much care too tbh :P)
| sys/kern/subr_firmware.c | ||
|---|---|---|
| 273 | Is this still "XXX" ? | |
| 309 | As a non native english speaker "longer" seems wrong and "larger" seems correct but I don't know which one is correct english :) | |
| 316 | Maybe tweak this comment as we might load firmware that execute code even if they are less popular than back then in the "good old days" | |
| 327 | For linuxkpi drivers we will have two printfs (the other one comes from linuxkpi firmware functions) but for native FreeBSD driver I guess we will not so it's good to see that a firmware was loaded without boot verbose I think. | |
Comment Actions
I'm not sure that native is exactly right either. "Raw" is relative to the "cooked" version in .ko files.
As long as we're consistent, I'd be happy with either of the names.
| sys/kern/subr_firmware.c | ||
|---|---|---|
| 273 | The comment was cut and pasted from elsewhere in this file. That's a good question... | |
| 309 | "larger" likely is more correct now that I read this.... "longer" might also be correct, but it's not as good a fit. But "Firmware of %ld bytes is too big. Max %ld.\n" is likely even better. | |
| 316 | This is about making the pages backing 'data' read-only. Currently, firmware is build with it as const, which makes it read-only so this would be a difference. | |
Comment Actions
Yes. I thought of "ordinary", "regular" and "normal", but so far "plain" is the one I like the most.
| sys/kern/subr_firmware.c | ||
|---|---|---|
| 129 | loading director? | |
| 267 | FEATURE REQUEST: This should probably come from kenv at some point only with this as a default here. And then it can probably be multiple places. | |
| 292 | bootverbose? | |
| 322 | That's probably related to the manual unload question. Driver code likely will load foo/bar.bin and want to unload foo/bar.bin and not /boot/firmware/foo/bar.bin but if kldload support was there the latter should be able to find the former but we can probably do that by trying to strip a prefix so I would suggest "no full path". | |
| 327 | Do what the .ko path does. From memory -- unless there is an error things are only printed with bootverbose? | |
| 355 | I would pass in flags btw. Will save us some trouble one day in the future. | |
| 526 | Do you want to change that flag to FW_NATIVE then as well? | |
Comment Actions
I've updated to binary, per an IRC conversation. I've created a draft to cover the bigger issues and will work towards that. Anybody reviewing this is welcome to comment on the draft..
I'll be letting this cook while I implement the extras requested there.
But extra features will be extra commits.
| sys/kern/subr_firmware.c | ||
|---|---|---|
| 267 | Yes. This will eventually be firmware_path, come in via the boot loader, and have a sensible set of defaults. That will be a future commit in this list. | |
| 270 | After talking on IRC, and working on things. We've settled on binary here as the name. | |
| 273 | I think this is no longer XXX. | |
| 292 | ah, yes. done. | |
| 322 | We use the same lookup code for both, so this works. | |
| 327 | I'll do bootverbose... | |
| 355 | I'll pass in the flags... If we need to rework it's easy enough. | |
| 526 | I've renamed it to FW_BINARY with the shift of the name to binary. | |
Comment Actions
Rename to binary, plus review comments
I claim all the comments are addressed, but if I'm in error let me know
| sys/kern/subr_firmware.c | ||
|---|---|---|
| 270 | Well I'll add my comment here; there's some of text files to be loaded as well as firmware; mostly settings and calibrations and stuff. | |
| sys/kern/subr_firmware.c | ||
|---|---|---|
| 312 | s/bug/big/ | |
| sys/kern/subr_firmware.c | ||
|---|---|---|
| 270 | Are those text files supposed to be read by the kernel or uploaded as a blob to the chip ? | |
| sys/kern/subr_firmware.c | ||
|---|---|---|
| 270 | For example, yes. | |
Comment Actions
A few typos in the commit log:
- s/compuse/compose/
- s/appening/appending/
- s/imagename/image name/?
- s/hard-wired at/hard-wired to/?
- s/be unload/be unloaded/
| sys/kern/subr_firmware.c | ||
|---|---|---|
| 270 | cxgbe has text config files it also currently loads via firmware_get(), but I think just treating those as binary files is fine | |
| 307–308 | ||
| 526 | ||