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 ;)