Change in ...coreboot[master]: src/mainboard/emulation/Kconfig: disable DRIVERS_INTEL_WIFI by default
Hello Mike Banon,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/31308
to review the following change.
Change subject: src/mainboard/emulation/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
src/mainboard/emulation/Kconfig: disable DRIVERS_INTEL_WIFI by default
It is impossible to insert the Intel PCI-e WiFi adapter to any of the Emulation boards because they are not physical, so this driver option should be disabled by default.
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: Ia5802b3234f984d906c6a1048ed8176f85f9bb99 --- M src/mainboard/emulation/Kconfig 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/31308/1
diff --git a/src/mainboard/emulation/Kconfig b/src/mainboard/emulation/Kconfig index 759b1de..f9e1198 100644 --- a/src/mainboard/emulation/Kconfig +++ b/src/mainboard/emulation/Kconfig @@ -19,4 +19,8 @@ string default "Emulation"
+config DRIVERS_INTEL_WIFI + bool + default n + endif # VENDOR_EMULATION
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31308 )
Change subject: src/mainboard/emulation/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31308/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31308/1//COMMIT_MSG@9 PS1, Line 9: It is impossible to insert the Intel PCI-e WiFi adapter to any of the : Emulation boards because they are not physical Are you sure?
https://wiki.installgentoo.com/index.php/PCI_passthrough
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31308 )
Change subject: src/mainboard/emulation/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31308/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31308/1//COMMIT_MSG@9 PS1, Line 9: It is impossible to insert the Intel PCI-e WiFi adapter to any of the : Emulation boards because they are not physical
Are you sure? […]
(Do note this is not a very serious wiki page, but the concept still applies)
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31308 )
Change subject: src/mainboard/emulation/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31308/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31308/1//COMMIT_MSG@9 PS1, Line 9: It is impossible to insert the Intel PCI-e WiFi adapter to any of the : Emulation boards because they are not physical
(Do note this is not a very serious wiki page, but the concept still applies)
Thank you Angel, this is interesting. To be honest I think if the Intel WiFi module already got properly initialized (e.g. because DRIVERS_INTEL_WIFI was enabled at the coreboot BIOS of your computer) then attaching it to QEMU virtual machine by PCI passthrough doesn't require this option at QEMU's BIOS. Please let me know if I am wrong and could abandon this change. Wish you a nice weekends
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31308 )
Change subject: src/mainboard/emulation/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31308/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31308/1//COMMIT_MSG@9 PS1, Line 9: It is impossible to insert the Intel PCI-e WiFi adapter to any of the : Emulation boards because they are not physical
Thank you Angel, this is interesting. […]
Well, passing through a wireless card to an emulated machine is a rather unlikely scenario, but I wanted to point out it is not impossible.
w.r.t. whether it is needed or not, it could be it is still needed, but I don't know.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31308 )
Change subject: src/mainboard/emulation/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/31308/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31308/2//COMMIT_MSG@9 PS2, Line 9: It is impossible to insert the Intel PCI-e WiFi adapter to any of the unlikely
Hello Kyösti Mälkki, Patrick Rudolph, Angel Pons, Mike Banon, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31308
to look at the new patch set (#3).
Change subject: src/mainboard/emulation/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
src/mainboard/emulation/Kconfig: disable DRIVERS_INTEL_WIFI by default
The emulation boards are not physical and if the user would like to attach the Intel PCI-e WiFi adapter - it has been already initialized by BIOS and OS, and should work fine. So this driver option is not required and could be disabled.
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: Ia5802b3234f984d906c6a1048ed8176f85f9bb99 --- M src/mainboard/emulation/Kconfig 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/31308/3
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31308 )
Change subject: src/mainboard/emulation/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31308/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31308/2//COMMIT_MSG@9 PS2, Line 9: It is impossible to insert the Intel PCI-e WiFi adapter to any of the
unlikely
hope it is okay now
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31308 )
Change subject: src/mainboard/emulation/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 3:
(3 comments)
What does this fix?
https://review.coreboot.org/#/c/31308/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31308/3//COMMIT_MSG@7 PS3, Line 7: src/mainboard/emulation/Kconfig: disable DRIVERS_INTEL_WIFI by default This is much too long. The src/ and /Kconfig can be droped without really losing information.
https://review.coreboot.org/#/c/31308/3//COMMIT_MSG@11 PS3, Line 11: should work fine. So this driver option is not required and could be disabled. It doesn't matter if it is physical. The driver in question is 95% *not* about initialization, so this reasoning seems pointless.
Please adhere to the 72 chars line limit.
https://review.coreboot.org/#/c/31308/3/src/mainboard/emulation/Kconfig File src/mainboard/emulation/Kconfig:
https://review.coreboot.org/#/c/31308/3/src/mainboard/emulation/Kconfig@24 PS3, Line 24: default n If I'd see this (without this change in mind), I'd probably want to clean it up. So better provide a reasoning here why we need the clutter. e.g. optimization of the flash chip usage to the last emulated byte. oh, wait, looks like I didn't understand the reason.
Hello Kyösti Mälkki, Patrick Rudolph, Angel Pons, Mike Banon, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31308
to look at the new patch set (#4).
Change subject: mainboard/emulation: disable DRIVERS_INTEL_WIFI by default ......................................................................
mainboard/emulation: disable DRIVERS_INTEL_WIFI by default
This helps to reduce the occupied space inside of a build by about 1% without any downsides.
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: Ia5802b3234f984d906c6a1048ed8176f85f9bb99 --- M src/mainboard/emulation/Kconfig 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/31308/4
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31308 )
Change subject: mainboard/emulation: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/#/c/31308/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31308/3//COMMIT_MSG@7 PS3, Line 7: src/mainboard/emulation/Kconfig: disable DRIVERS_INTEL_WIFI by default
This is much too long. The src/ and /Kconfig can be droped without […]
Done.
https://review.coreboot.org/#/c/31308/3//COMMIT_MSG@11 PS3, Line 11: should work fine. So this driver option is not required and could be disabled.
It doesn't matter if it is physical. The driver in question is 95% […]
Done.
https://review.coreboot.org/#/c/31308/3/src/mainboard/emulation/Kconfig File src/mainboard/emulation/Kconfig:
https://review.coreboot.org/#/c/31308/3/src/mainboard/emulation/Kconfig@24 PS3, Line 24: default n
If I'd see this (without this change in mind), I'd probably want to clean […]
I updated the reasoning and also checked how much we win by disabling this config alone. Turned out it is 44476-43965 = 511 bytes = 1.2% for BOARD_EMULATION_QEMU_X86_I440FX
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31308 )
Change subject: mainboard/emulation: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 4: Code-Review-1
(2 comments)
Saving 511 bytes on the default build for a target that does not have a flash chip seems rather pointless to me IMHO.
https://review.coreboot.org/#/c/31308/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31308/3//COMMIT_MSG@11 PS3, Line 11: should work fine. So this driver option is not required and could be disabled.
Done.
I checked said Intel driver's source code (src/drivers/intel/wifi) and it mainly adds data to ACPI tables and applies some quirks on a certain card series. I am pretty sure even PCI passthrough would need this driver.
https://review.coreboot.org/#/c/31308/3/src/mainboard/emulation/Kconfig File src/mainboard/emulation/Kconfig:
https://review.coreboot.org/#/c/31308/3/src/mainboard/emulation/Kconfig@24 PS3, Line 24: default n
I updated the reasoning and also checked how much we win by disabling this config alone. […]
I am pretty sure nobody will ever need 511 extra bytes on QEMU using defaults...
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31308 )
Change subject: mainboard/emulation: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/31308/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31308/3//COMMIT_MSG@11 PS3, Line 11: should work fine. So this driver option is not required and could be disabled.
I checked said Intel driver's source code (src/drivers/intel/wifi) and it mainly adds data to ACPI t […]
Okay, then I'm abandoning this change.
mikeb mikeb has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31308 )
Change subject: mainboard/emulation: disable DRIVERS_INTEL_WIFI by default ......................................................................
Abandoned
Abandoned after reading the Angel Pons reasoning.
-
Angel Pons (Code Review) -
mikeb mikeb (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)