Hello Kyösti Mälkki, Werner Zeh, David Hendricks, Aaron Durbin, Angel Pons, Arthur Heymans, Patrick Rudolph, Stefan Reinauer, Philip Prindeville, Patrick Georgi,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/34282
to review the following change.
Change subject: Revert "soc/intel/cannonlake: Add option to select FSP_CAR" ......................................................................
Revert "soc/intel/cannonlake: Add option to select FSP_CAR"
This reverts commit 9e3ba212f34c6d9f2eb7dac8e4651f8ce12ab0c0.
It rudely changed the default CAR implementation from open-source to the proprietary FSP-T. That it was reviewed by Google and contains a safeguard so that it doesn't affect Google reeks of corporate arrogance. My attempts to find a civilized solution failed, because the author is blocking any progress.
Change-Id: Ief7b85a9988d1a3f4a4639e9fd304959295958bd --- M src/soc/intel/cannonlake/Kconfig 1 file changed, 2 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/34282/1
diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig index fed7f02..f30ff11 100644 --- a/src/soc/intel/cannonlake/Kconfig +++ b/src/soc/intel/cannonlake/Kconfig @@ -68,6 +68,7 @@ select INTEL_DESCRIPTOR_MODE_CAPABLE select HAVE_SMI_HANDLER select IDT_IN_EVERY_STAGE + select INTEL_CAR_NEM_ENHANCED select INTEL_GMA_ACPI select INTEL_GMA_ADD_VBT if RUN_FSP_GOP select IOAPIC @@ -86,6 +87,7 @@ select SOC_INTEL_COMMON_ACPI_WAKE_SOURCE select SOC_INTEL_COMMON_BLOCK select SOC_INTEL_COMMON_BLOCK_ACPI + select SOC_INTEL_COMMON_BLOCK_CAR select SOC_INTEL_COMMON_BLOCK_CHIP_CONFIG select SOC_INTEL_COMMON_BLOCK_CPU select SOC_INTEL_COMMON_BLOCK_CPU_MPINIT @@ -267,33 +269,6 @@ This will enable a workaround in ASL _PS3 and _PS0 methods to force SD_PWR_ENABLE to stay low in D3.
-choice - prompt "Cache-as-ram implementation" - default USE_CANNONLAKE_CAR_NEM_ENHANCED if MAINBOARD_HAS_CHROMEOS - default USE_CANNONLAKE_FSP_CAR - help - This option allows you to select how cache-as-ram (CAR) is set up. - -config USE_CANNONLAKE_CAR_NEM_ENHANCED - bool "Enhanced Non-evict mode" - select SOC_INTEL_COMMON_BLOCK_CAR - select INTEL_CAR_NEM_ENHANCED - help - A current limitation of NEM (Non-Evict mode) is that code and data - sizes are derived from the requirement to not write out any modified - cache line. With NEM, if there is no physical memory behind the - cached area, the modified data will be lost and NEM results will be - inconsistent. ENHANCED NEM guarantees that modified data is always - kept in cache while clean data is replaced. - -config USE_CANNONLAKE_FSP_CAR - bool "Use FSP CAR" - select FSP_CAR - help - Use FSP APIs to initialize and tear down the Cache-As-Ram. - -endchoice - config FSP_HEADER_PATH string "Location of FSP headers" default "3rdparty/fsp/CoffeeLakeFspBinPkg/Include/" if SOC_INTEL_COFFEELAKE || SOC_INTEL_WHISKEYLAKE
Hello Kyösti Mälkki, David Hendricks, Patrick Rudolph, Aaron Durbin, Angel Pons, Arthur Heymans, Patrick Rudolph, Stefan Reinauer, build bot (Jenkins), Philip Prindeville, Patrick Georgi, Werner Zeh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34282
to look at the new patch set (#2).
Change subject: Revert "soc/intel/cannonlake: Add option to select FSP_CAR" ......................................................................
Revert "soc/intel/cannonlake: Add option to select FSP_CAR"
This reverts commit 9e3ba212f34c6d9f2eb7dac8e4651f8ce12ab0c0.
It rudely changed the default CAR implementation from open-source to the proprietary FSP-T. That it was reviewed by Google and contains a safeguard so that it doesn't affect Google reeks of corporate arrogance. My attempts to find a civilized solution failed, because the author is blocking any progress.
Change-Id: Ief7b85a9988d1a3f4a4639e9fd304959295958bd Signed-off-by: Nico Huber nico.huber@secunet.com --- M src/soc/intel/cannonlake/Kconfig 1 file changed, 2 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/34282/2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34282 )
Change subject: Revert "soc/intel/cannonlake: Add option to select FSP_CAR" ......................................................................
Patch Set 1:
-2 to re justify your commit msg
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34282 )
Change subject: Revert "soc/intel/cannonlake: Add option to select FSP_CAR" ......................................................................
Patch Set 2:
-2 to re justify your commit msg
Subrata, be more specific, remove your vote immediately, or I'll ask to remove you from the "coreboot developers" group. So your abuse of (completely unwarranted btw.) power can finally have an end.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34282 )
Change subject: Revert "soc/intel/cannonlake: Add option to select FSP_CAR" ......................................................................
Patch Set 2:
Patch Set 2: Subrata, [to] be more specific, remove your vote immediately, or
Nico, calm down.
I'll ask to remove you from the "coreboot developers" group. So your abuse of (completely unwarranted btw.) power can finally have an end.
Request denied.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34282 )
Change subject: Revert "soc/intel/cannonlake: Add option to select FSP_CAR" ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2: Subrata, [to] be more specific, remove your vote immediately, or
Nico, calm down.
It's mandated by our guidelines to give *concrete* recommendations to address a -2. Do I have to quote them to you too now? I regularly have to point Subrata to them...
I'll ask to remove you from the "coreboot developers" group. So your abuse of (completely unwarranted btw.) power can finally have an end.
Request denied.
Are you some kind of Metatron? If I would complain formally, I would take it to our leadership.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34282 )
Change subject: Revert "soc/intel/cannonlake: Add option to select FSP_CAR" ......................................................................
Patch Set 2: Code-Review-2
Nico, the commit you're trying to revert has been in for a year. It was reviewed by Aaron, not by "Google". We are still people, not corporate shills.
If you disagree with the defaults, let's discuss it, but I don't think this revert is appropriate. The commit message and the threats against Subrata are also inappropriate.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34282 )
Change subject: Revert "soc/intel/cannonlake: Add option to select FSP_CAR" ......................................................................
Patch Set 2:
Nico, the commit you're trying to revert has been in for a year. It was reviewed by Aaron, not by "Google". We are still people, not corporate shills.
Yeah, can't make everybody happy. If I say Google, that's bad, if I say Martin, that's too personal, I get it.
Are you trying to tell me that Aaron's employment has nothing to do with the fact that he didn't ask questions about the `if MAINBOARD_HAS_CHROMEOS` in the patch?!?
If you disagree with the defaults, let's discuss it, but I don't think this revert is appropriate. The commit message and the threats against Subrata are also inappropriate.
Martin, seriously, what the fuck? Subrata is violating our Gerrit guidelines all day and when I complain about it, then that is inappropriate?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34282 )
Change subject: Revert "soc/intel/cannonlake: Add option to select FSP_CAR" ......................................................................
Patch Set 2:
If you disagree with the defaults, let's discuss it
I tried to btw. Guess what: Subrata jumped in and gave an immediate -2 based on a hunch that wasn't true at all (that happens regularly btw. sometimes with sometimes without -2). We had a discussion with different folks at Intel based on that hunch. Funny, nobody noticed that it was all void.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34282 )
Change subject: Revert "soc/intel/cannonlake: Add option to select FSP_CAR" ......................................................................
Patch Set 2:
Martin, please see our Gerrit guidelines here: https://doc.coreboot.org/getting_started/gerrit_guidelines.html
Please give concrete recommendations what I'm supposed to change.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34282 )
Change subject: Revert "soc/intel/cannonlake: Add option to select FSP_CAR" ......................................................................
Patch Set 2:
Patch Set 2:
Yeah, can't make everybody happy. If I say Google, that's bad, if I say Martin, that's too personal, I get it.
It's not too personal to use are names at all. I felt that the last time we butted heads it was a personal attack that is at least partially due to my employer. I feel like there's an attitude with some that GOOGLE==BAD and anyone who works for them is bad. That's seems to be what you're saying in your commit message here too, so it doesn't seem like I was wrong.
Are you trying to tell me that Aaron's employment has nothing to do with the fact that he didn't ask questions about the `if MAINBOARD_HAS_CHROMEOS` in the patch?!?
Honestly, I don't have any idea what Aaron's thinking, I'm not trying to speak for him, just point out that it was an individual, not "Google". Subrata explained his rationale in his commit message. You may disagree with his thinking, but that's a difference of opinion. Maybe Aaron agreed with him.
If you disagree with the defaults, let's discuss it, but I don't think this revert is appropriate. The commit message and the threats against Subrata are also inappropriate.
Martin, seriously, what the fuck? Subrata is violating our Gerrit guidelines all day and when I complain about it, then that is inappropriate?
If you feel that Subrata is violating guidelines, then bring that up as an issue with examples instead of attacking him and being rude. It seems to me that you're violating guidelines here too - "Be respectful to others when commenting."
If you disagree with the defaults, let's discuss it
I tried to btw. Guess what: Subrata jumped in and gave an immediate -2 based on a hunch that wasn't true at all (that happens regularly btw. sometimes with sometimes without -2). We had a discussion with different folks at Intel based on that hunch. Funny, nobody noticed that it was all void.
Can you give a pointer to that discussion? Let's discuss it in the leadership meeting.
Martin, please see our Gerrit guidelines here: https://doc.coreboot.org/getting_started/gerrit_guidelines.html
Thanks for the pointer Nico, I'm aware of what the guidelines say. I did write them: http://review.coreboot.org/12256
Please give concrete recommendations what I'm supposed to change.
When I said "If you disagree with the defaults, let's discuss it", that's what I was trying to do. Instead of reverting the commit, let's look at changing the defaults.
-----
Look, I'm not opposed to making the open-source CAR the default solution, and I'm not arguing against that. I'm apposed to the tone of your message and the heavy-handed way you're trying to change things.
I know you're trying to do what you think is technically the best for the project, and I can appreciate that. Subrata's doing the same, but he has to work within his constraints. Let's work together to make the project the best we can.
Unless I'm misunderstanding something, Subrata's patch that you're trying to remove here didn't remove any functionality, it just provided an alternative. I get that you disagree with that alternative, but you don't have to use it, and other people might see some advantage to it.
There are different ways of looking at this. Some people see it as corrupting the open-source project. I see it as bringing users to an open source project that might otherwise go with a fully-proprietary firmware. My hope is that we can convince vendors to open-source their currently proprietary binaries, but attacking the vendors and pushing them away from the project certainly won't help with that.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34282 )
Change subject: Revert "soc/intel/cannonlake: Add option to select FSP_CAR" ......................................................................
Patch Set 2:
Yeah, can't make everybody happy. If I say Google, that's bad, if I say Martin, that's too personal, I get it.
It's not too personal to use are names at all.
In a commit message?
I felt that the last time we butted heads it was a personal attack that is at least partially due to my employer. I feel like there's an attitude with some that GOOGLE==BAD and anyone who works for them is bad. That's seems to be what you're saying in your commit message here too, so it doesn't seem like I was wrong.
I have no bad feelings towards Google in general. It's good that you tell what you read between the lines so we can fix that. Subrata forced me into the revert and I had to write something. So I just looked at the original review briefly and wrote down my thoughts. But no matter if I wrote this in anger, I think changing
select reasonable
to
default reasonable if VENDOR default unreasonable-and-likely-untested
Is very bad and deserves to be pointed out in the commit message of a revert somehow. And I really don't care who that VENDOR is, but you might understand my feelings towards the pattern.
Are you trying to tell me that Aaron's employment has nothing to do with the fact that he didn't ask questions about the `if MAINBOARD_HAS_CHROMEOS` in the patch?!?
Honestly, I don't have any idea what Aaron's thinking, I'm not trying to speak for him, just point out that it was an individual, not "Google". Subrata explained his rationale in his commit message. You may disagree with his thinking, but that's a difference of opinion. Maybe Aaron agreed with him.
If you disagree with the defaults, let's discuss it, but I don't think this revert is appropriate. The commit message and the threats against Subrata are also inappropriate.
Martin, seriously, what the fuck? Subrata is violating our Gerrit guidelines all day and when I complain about it, then that is inappropriate?
If you feel that Subrata is violating guidelines, then bring that up as an issue with examples instead of attacking him and being rude. It seems to me that you're violating guidelines here too - "Be respectful to others when commenting."
Yes, I know that's one of my weak spots. But I think also a cultural issue. If have bad feelings and wouldn't express them, I would feel like a liar.
If you disagree with the defaults, let's discuss it
I tried to btw. Guess what: Subrata jumped in and gave an immediate -2 based on a hunch that wasn't true at all (that happens regularly btw. sometimes with sometimes without -2). We had a discussion with different folks at Intel based on that hunch. Funny, nobody noticed that it was all void.
Can you give a pointer to that discussion? Let's discuss it in the leadership meeting.
There was no real discussion (CB:34166). It started with a -2 for wrong assumptions (which I only discovered after abandoning the change). And after everybody agreed to the change, and nothing else was left to bike- shed, Subrata started to pick on my commit message (probably because he felt personally reasponsible for the code; I didn't know he wrote it). Which I didn't want to change too much because it felt dishonest.
There's another similar change on Gerrit now.
Martin, please see our Gerrit guidelines here: https://doc.coreboot.org/getting_started/gerrit_guidelines.html
Thanks for the pointer Nico, I'm aware of what the guidelines say. I did write them: http://review.coreboot.org/12256
Please give concrete recommendations what I'm supposed to change.
When I said "If you disagree with the defaults, let's discuss it", that's what I was trying to do. Instead of reverting the commit, let's look at changing the defaults.
Look, I'm not opposed to making the open-source CAR the default solution, and I'm not arguing against that. I'm apposed to the tone of your message and the heavy-handed way you're trying to change things.
It's just the way
I know you're trying to do what you think is technically the best for the project, and I can appreciate that. Subrata's doing the same, but he has to work within his constraints. Let's work together to make the project the best we can.
Agreed.
Unless I'm misunderstanding something, Subrata's patch that you're trying to remove here didn't remove any functionality, it just provided an alternative. I get that you disagree with that alternative, but you don't have to use it, and other people might see some advantage to it.
I wouldn't care about the FSP-T as an alternative. The problem really is that Intel constantly makes it harder to configure coreboot properly if you don't do some hidden magic (in this case select MAINBOARD_HAS_ CHROMEOS or manually switch FSP-T off or manually configure other related settings, the latter only under the assumption that the code actually works which is usually not the case).
I have to fix these things for every new Intel platform over again and that's already very exhausting. Guess how it feels when I then have to endure some hours of nonsense review to fix two lines!
There are different ways of looking at this. Some people see it as corrupting the open-source project. I see it as bringing users to an open source project that might otherwise go with a fully-proprietary firmware. My hope is that we can convince vendors to open-source their currently proprietary binaries, but attacking the vendors and pushing them away from the project certainly won't help with that.
That's a completely different issue here. I could lament about how Intel step by step replaces coreboot with blobs (they really do btw.). But that has nothing to do with making a two-line review a pain; for, what I believe now, personal reasons. Now I could try to babysit more to hope to prevent that things get merged that would be hard for me to get fixed later. But they know already how to avoid reviews: CB:33568 ;)
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34282 )
Change subject: Revert "soc/intel/cannonlake: Add option to select FSP_CAR" ......................................................................
Patch Set 2:
How about this, folks:
If anybody can get FSP-T working in 24h. I'll abandon this. If not, I'll write a nice commit message :)
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34282 )
Change subject: Revert "soc/intel/cannonlake: Add option to select FSP_CAR" ......................................................................
Patch Set 2:
Patch Set 2:
How about this, folks:
If anybody can get FSP-T working in 24h. I'll abandon this. If not, I'll write a nice commit message :)
Can we just go with CB:34287?
I'm going to move the rest of this discussion to private email.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34282 )
Change subject: Revert "soc/intel/cannonlake: Add option to select FSP_CAR" ......................................................................
Patch Set 2:
Look, I'm not opposed to making the open-source CAR the default solution, and I'm not arguing against that. I'm apposed to the tone of your message and the heavy-handed way you're trying to change things.
It's just the way
Sorry I don't know what I wanted to type here... obviously didn't finish the thought. I think this is the way I was pushed into; by having people in coreboot's Developers group that use their -2 and submission rights too agressively. It obviously results in more agressive commits.
Nico Huber has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34282 )
Change subject: Revert "soc/intel/cannonlake: Add option to select FSP_CAR" ......................................................................
Abandoned
why that option exists is still unclear, but at least it's disabled by default now
-
Martin Roth (Code Review) -
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Subrata Banik (Code Review)