Change in ...coreboot[master]: src/mainboard/lenovo/g505s/Kconfig: disable DRIVERS_INTEL_WIFI by def...
Hello Mike Banon,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/31225
to review the following change.
Change subject: src/mainboard/lenovo/g505s/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
src/mainboard/lenovo/g505s/Kconfig: disable DRIVERS_INTEL_WIFI by default
There were no G505S shipped with pre-installed Intel PCI-e WiFi adapters, almost all such adapters (except Intel 135) could not be installed under proprietary UEFI because of a whitelist, and I am not aware of anyone who installed them after upgrading to coreboot - maybe because Atheros ath9k have a much better software freedom status and are the preferred choice.
Disable DRIVERS_INTEL_WIFI option by default for this G505S laptop.
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I73c3c2587690dca653dd1b2a8d6ee9ff8b769ad4 --- M src/mainboard/lenovo/g505s/Kconfig 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/31225/1
diff --git a/src/mainboard/lenovo/g505s/Kconfig b/src/mainboard/lenovo/g505s/Kconfig index 883ef27..d0701a7 100644 --- a/src/mainboard/lenovo/g505s/Kconfig +++ b/src/mainboard/lenovo/g505s/Kconfig @@ -55,4 +55,8 @@ string default "1002,990b"
+config DRIVERS_INTEL_WIFI + bool + default n + endif # BOARD_LENOVO_G505S
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31225 )
Change subject: src/mainboard/lenovo/g505s/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 2: Code-Review-1
If there is any chance that this device hosts such a card, it should default to `y`, IMHO.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31225 )
Change subject: src/mainboard/lenovo/g505s/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31225/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31225/2//COMMIT_MSG@13 PS2, Line 13: have a much better software freedom status and are the preferred choice. Because they hide their firmware better?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31225 )
Change subject: src/mainboard/lenovo/g505s/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 2: Code-Review-1
I agree with git blame on how conditionals are currently set.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31225 )
Change subject: src/mainboard/lenovo/g505s/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 2:
(1 comment)
Patch Set 2: Code-Review-1
If there is any chance that this device hosts such a card, it should default to `y`, IMHO.
As said at the commit message, this chance is very very small because all G505S shipped without Intel cards preinstalled, people couldn't install them because of the whitelist while there was a proprietary UEFI, and those who upgraded to coreboot are security conscious enough to choose ath9k over intel.
I know a lot of coreboot+G505S owners, but none of them have Intel WiFi installed. I am confident that we should not select a configuration if 99.9% or even 100% of coreboot+G505S users will not ever need it. The remaining 0%-0.1% could enable this configuration at Generic Drivers : it is still there, just disabled by default.
https://review.coreboot.org/#/c/31225/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31225/2//COMMIT_MSG@13 PS2, Line 13: have a much better software freedom status and are the preferred choice.
Because they hide their firmware better?
No non-free firmware is required to operate Atheros ath9k cards, and this is why many of these cards have been shipped at librebooted FSF-endorsed laptops. Meanwhile, all the Intel cards require a non-free firmware, so their freedom status is much worse. See more info at https://en.wikipedia.org/wiki/Comparison_of_open-source_wireless_drivers
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31225 )
Change subject: src/mainboard/lenovo/g505s/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 2:
Patch Set 2: Code-Review-1
I agree with git blame on how conditionals are currently set.
How could I improve this change? Please could you provide any ideas?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31225 )
Change subject: src/mainboard/lenovo/g505s/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 2:
(1 comment)
Patch Set 2:
(1 comment)
Patch Set 2: Code-Review-1
If there is any chance that this device hosts such a card, it should default to `y`, IMHO.
As said at the commit message, this chance is very very small because all G505S shipped without Intel cards preinstalled, people couldn't install them because of the whitelist while there was a proprietary UEFI,
It doesn't matter how small the chance is. It can hurt if the option is set to `n` falsely. But can't if set to `y`.
and those who upgraded to coreboot are security conscious enough to choose ath9k over intel.
You are confusing software freedom with security.
I know a lot of coreboot+G505S owners, but none of them have Intel WiFi installed. I am confident that we should not select a configuration if 99.9% or even 100% of coreboot+G505S users will not ever need it. The remaining 0%-0.1% could enable this configuration at Generic Drivers : it is still there, just disabled by default.
Anybody is already free to disable it if they don't need it.
https://review.coreboot.org/#/c/31225/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31225/2//COMMIT_MSG@13 PS2, Line 13: have a much better software freedom status and are the preferred choice.
No non-free firmware is required to operate Atheros ath9k cards, and this is why many of these cards […]
Um, I know that page... the only reference it quotes seems to predate the chips in question by two years. It's nuts. And it's not a reliable source anyway. If Atheros ever publishes a list of chips which (don't) have firware, I would trust it (if the selection of chips with firmware seems reasonable).
Generally, don't trust wikis. Especially not about the winner of a popularity contest, which ath9k clearly is.
Btw. the current FSF-endorsement can only have a negative impact on free firmware, IMHO. They reward those that hide their firmware dee- pest and/or ensure that bugs are never fixed (everything in a ROM seems to be endorsed). But punish those that a least try to have a secured update mechanism. But you need that update mechanism for free firmware... They also endorse Linux distributions that push proprietary parts into the firmware. If you ask me, the FSF is accidentally hostile to free firmware, so better don't try to use them as an argument here.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31225 )
Change subject: src/mainboard/lenovo/g505s/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 2: Code-Review-1
Patch Set 2:
(1 comment)
Patch Set 2: Code-Review-1
If there is any chance that this device hosts such a card, it should default to `y`, IMHO.
this chance is very very small because
Not zero, however.
I do not see any good reason to merge this. This makes the default configuration less compatible, which IMHO is a bad idea.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31225 )
Change subject: src/mainboard/lenovo/g505s/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 2:
(1 comment)
.
It doesn't matter how small the chance is. It can hurt if the option is set to `n` falsely. But can't if set to `y`.
It consumes some kilobytes at CBFS and adds extra complexity, so from my point of view it would be better if this doesn't stay as default 'y'. Of course if I will see at least one owner of coreboot'ed G505S with Intel WiFi, this will be a good reason to consider abandoning/reverting this change. However I haven't seen anyone who could suffer from this default 'n'.
and those who upgraded to coreboot are security conscious enough to choose ath9k over intel.
You are confusing software freedom with security.
To me they go hand in hand because the proprietary closed source firmware could contain the evil backdoors, while if the sources are open there is significantly less chance of it. By the way, a lot of G505S people are using security-inclined Qubes OS as their primary distro, and they went through the trouble of finding/refurbishing this laptop because they see that more software freedom at their computer's firmware automatically means much better security.
. Anybody is already free to disable it if they don't need it.
The point of my recent commits is to minimize the amount of local configurations the people need to do at their freshly cloned coreboot. If no-one is using G505S with Intel WiFi, there are no reasons for it to stay as default 'y', but I could change my point of view if I would see at least one person with such a system configuration.
https://review.coreboot.org/#/c/31225/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31225/2//COMMIT_MSG@13 PS2, Line 13: have a much better software freedom status and are the preferred choice.
Um, I know that page... the only reference it quotes seems to predate […]
As far as I know there haven't been any significant architecture changes between the various chips of ath9k family and they enjoy the same freedom status (unlike the problematic ath10k). Also, it seems that ath9k modules contain only 4 kilobytes EEPROM with ART and other settings and they don't have a ROM with a fully fledged proprietary firmware controlling it, so the chips at these modules are controlled completely by this opensource driver - which is very easy to update if there would be any security problems. While for ath9k_htc (USB modules, not PCIe) there is a ROM with firmware - but it is unactive if your driver loads the open source ath9k_htc firmware officially released by Atheros and available at Github.
P.S. Sorry I can't understand why every chip in existence requires a ROM to function; depending on the architecture it could simply obey the driver's commands. I think it could be "hardwired" inside the hardware without any ROM that chip will be loading the instructions starting from i.e. 0x1000 offset at its' RAM, and driver could be simply putting the various instructions there, software handle the interrupts coming from the physical IRQ lines of this chip and read/write the chip's registers to handle them.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31225 )
Change subject: src/mainboard/lenovo/g505s/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31225/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31225/2//COMMIT_MSG@13 PS2, Line 13: have a much better software freedom status and are the preferred choice.
As far as I know there haven't been any significant architecture changes between the various chips o […]
Again, the comment about firmware free Atheros seems to predate all ath9k effort / chips. I didn't check all of them, just few random but they were from 2006 and the comment is from 2004. In fact, nobody ever said ath9k chips would be firmware free (beside the people copy pasting in the tables of the mentioned wiki page). Please don't try to change the facts by arguing. If you want to know, ask Atheros.
You don't have to convince me that hardwired implementations are possible. You have to convince enough consumers to pay a chip vendor for the effort.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31225 )
Change subject: src/mainboard/lenovo/g505s/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31225/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31225/2//COMMIT_MSG@13 PS2, Line 13: have a much better software freedom status and are the preferred choice.
Again, the comment about firmware free Atheros seems to predate all ath9k […]
Sadly after Qualcomm bought Atheros I found it impossible to get any support or even a human reply from them, it seems that Qualcomm does not care about the opensource. E.g. despite my best contact efforts, no one from Qualcomm ever gave a reply to my datasheet requests for AR8161/8162/8171/8172 chips (would've been helpful for KolibriOS driver). And the scope of their developer portal topics is extremely limited, it's mostly for some devboards and Internet of ShitThings related.
Personally I think that all ath9k are fine. I may be wrong of course. Still, when I have to choose between "maybe contains the non-free firmware but hopefully not (Atheros ath9k)" VS "guaranteed to contain the non-free firmware (Intel)", of course I will choose ath9k because it is more likely to be free and also is more ethical (haven't been made by the same company that first put a ME inside their CPUs). Although Qualcomm ath10k is much worse than Atheros ath9k, and I hope that Qualcomm will be bought by someone else who is better (of course not by Broadcom or Qualcomm will turn from not caring about opensource to hating the opensource).
Back to the topic, it is hard for me to find any reasons why any coreboot'er would consciously choose Intel over Atheros ath9k especially at this AMD-based computer. So I hope that this change will make into the default configuration some day, at least for "remove kebab" -err- "remove Intel" reason ;) Meanwhile I'm going to submit a "disable DRIVERS_INTEL_WIFI" change for Emulation boards like QEMU... Wish you a nice weekends
awokd@danwin1210.me has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31225 )
Change subject: src/mainboard/lenovo/g505s/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 2:
Patch Set 2:
(1 comment)
.
It doesn't matter how small the chance is. It can hurt if the option is set to `n` falsely. But can't if set to `y`.
It consumes some kilobytes at CBFS and adds extra complexity, so from my point of view it would be better if this doesn't stay as default 'y'. Of course if I will see at least one owner of coreboot'ed G505S with Intel WiFi, this will be a good reason to consider abandoning/reverting this change. However I haven't seen anyone who could suffer from this default 'n'.
I suppose this depends on which "hurts" more, which can be subjective... FWIW, I'm not aware of any G505s's shipped with an Intel wifi adapter either, but I can't really prove a negative.
and those who upgraded to coreboot are security conscious enough to choose ath9k over intel.
You are confusing software freedom with security.
To me they go hand in hand because the proprietary closed source firmware could contain the evil backdoors, while if the sources are open there is significantly less chance of it. By the way, a lot of G505S people are using security-inclined Qubes OS as their primary distro, and they went through the trouble of finding/refurbishing this laptop because they see that more software freedom at their computer's firmware automatically means much better security.
I think Nico's point here is that software freedom =/= automatic security. You can have 100% open software with 0% security, as well as 0% open with 100% security. That said, I'm going to trust coreboot with as few blobs as possible over a random manufacturer's implementation every time.
. Anybody is already free to disable it if they don't need it.
The point of my recent commits is to minimize the amount of local configurations the people need to do at their freshly cloned coreboot. If no-one is using G505S with Intel WiFi, there are no reasons for it to stay as default 'y', but I could change my point of view if I would see at least one person with such a system configuration.
Looking through random mainboards in menuconfig, this Intel PCI-e wifi adapter appears enabled on every one. https://review.coreboot.org/c/coreboot/+/15931 seems to be the introduction. To play devil's advocate, if "most x86 platforms could have" any of the other listed generic drivers, should they be enabled globally too so people can disable them?
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31225 )
Change subject: src/mainboard/lenovo/g505s/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 2:
I think that until we find at least one G505S user who needs Intel WiFi we could safely default it to "n". Why default this to "y" if we don't know anyone who needs it?
Patch Set 2:
I'm not aware of any G505s's shipped with an Intel wifi adapter either, but I can't really prove a negative.
There is a hardware maintenance manual PDF at https://github.com/g505s-opensource-researcher/g505s-proprietary/tree/master... , which among others lists only Intel 135 as the supported replacement, but if you search the Internet you could not find any G505S with it - only some other Ideapad G-series Intel laptops which have been mentioned at this manual. To me it's a good enough proof.
Patch Set 2:
You can have 100% open software with 0% security, as well as 0% open with 100% security.
If there is no tivoization you could upgrade the security of your open software to 100% or pay someone to do it, meanwhile you could not consider 0% open software to be 100% secure because you couldn't just blindly trust the maker of this software who claims it is "100% secure" and the community couldn't audit it since the sources are closed.
Patch Set 2:
If "most x86 platforms could have" any of the other listed generic drivers, should they be enabled globally too so people can disable them?
I think the difference with other generic drivers is that e.g. if there is no TI TPS65913 at your board then it couldn't ever have it, while these MiniPCI-e modules are add-on and could be added. But because I haven't heard of anyone who has G505S with Intel WiFi inside, I think it is safe to disable by default.
My point is that the default settings should suit the needs of majority, especially considering that in this particular case it's likely that minority doesn't exist. Meanwhile I've already found myself a couple of times in a situation where I had to disable DRIVERS_INTEL_WIFI in order to save a few kilobytes to make enough remaining room for the last floppy image of my interest
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31225 )
Change subject: src/mainboard/lenovo/g505s/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 2:
Patch Set 2:
I think that until we find at least one G505S user who needs Intel WiFi we could safely default it to "n". Why default this to "y" if we don't know anyone who needs it?
My g505s now has Intel WiFi. Previously it had none.
Patch Set 2:
I'm not aware of any G505s's shipped with an Intel wifi adapter either, but I can't really prove a negative.
Irrelevant what it shipped with. It was alreydy open up once for flashing?
There is a hardware maintenance manual PDF at https://github.com/g505s-opensource-researcher/g505s-proprietary/tree/master... , which among others lists only Intel 135 as the supported replacement, but if you search the Internet you could not find any G505S with it - only some other Ideapad G-series Intel laptops which have been mentioned at this manual. To me it's a good enough proof.
It's a slot, making WiFi card user changeable.
My point is that the default settings should suit the needs of majority, especially considering that in this particular case it's likely that minority doesn't exist. Meanwhile I've already found myself a couple of times in a situation where I had to disable DRIVERS_INTEL_WIFI in order to save a few kilobytes to make enough remaining room for the last floppy image of my interest
You are in the minority with your need to tune up last kilobytes from ramstage. You need to adopt, probably should be building from local branch anyways. Default build is clearly not for you.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31225 )
Change subject: src/mainboard/lenovo/g505s/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
I think that until we find at least one G505S user who needs Intel WiFi we could safely default it to "n". Why default this to "y" if we don't know anyone who needs it?
My g505s now has Intel WiFi. Previously it had none.
This means my -1 is staying. I can see no valid reason to make the default less compatible.
My point is that the default settings should suit the needs of majority, especially considering that in this particular case it's likely that minority doesn't exist. Meanwhile I've already found myself a couple of times in a situation where I had to disable DRIVERS_INTEL_WIFI in order to save a few kilobytes to make enough remaining room for the last floppy image of my interest
You are in the minority with your need to tune up last kilobytes from ramstage. You need to adopt, probably should be building from local branch anyways. Default build is clearly not for you.
Definitely agree with this.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31225 )
Change subject: src/mainboard/lenovo/g505s/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
I think that until we find at least one G505S user who needs Intel WiFi we could safely default it to "n". Why default this to "y" if we don't know anyone who needs it?
My g505s now has Intel WiFi. Previously it had none.
Thank you very much for your feedback; although I couldn't understand your choice of Intel over ath9k, I see it as a good enough reason to abandon this change. Please also take a look at CB:31308 (disable for the emulation boards)
mikeb mikeb has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31225 )
Change subject: src/mainboard/lenovo/g505s/Kconfig: disable DRIVERS_INTEL_WIFI by default ......................................................................
Abandoned
Thank you very much, friends, now I see clearly this change is wrong.
-
Angel Pons (Code Review) -
Kyösti Mälkki (Code Review)
-
mikeb mikeb (Code Review)
-
Name of user not set (Code Review)
-
Nico Huber (Code Review)