Thank you ron for your comments!
actually I was not aware the code I look at is more than 5 years old. You are absolutely right in that the patch was written after reading public ICH7 documentation and I do not know of any problem the current implementation causes. Moreover, I was not aware that the code might do something else than what one can know by reading the code and the public documentation. To be honest, if that was true, I would be disappointed, because I expected coreboot to be a possibility of getting away from private software and "you don't know what your machine does". However, I did debug the code by reading the write-once registers in question before and after the several write operations. The registers do not change their contents but due to the first write operation. By that I concluded the public documentation describes the chip's behavior correctly. After reading your comments about private documentation of course this conclusion might be wrong though.
At the moment I believe that applying the patch does no harm. At least this is my current knowledge by running it at the MacBook2,1 for a couple of hours. Switching forth and back between coreboot versions (back to safe ones) works. I do not know if this was true for other boards.
Who knows, it might(!) fix some 5 year old bug?
In case you were interessted, I uploaded the output of inteltool when running the macbook without the patch, that is by compiling [1] and by compiling the patch on top of [1]. Please see 5324 without patch 5387: http://paste.flashrom.org/view.php?id=2051 5324 plus patch 5387: http://paste.flashrom.org/view.php?id=2052 diff -y without vs with patch 5387: http://paste.flashrom.org/view.php?id=2053
any light shedding is much appreciated! best regards, Mono
[1] http://review.coreboot.org/#/c/5324/
On Sun, Mar 16, 2014 at 09:26:09AM -0700, ron minnich wrote:
So do I understand it correctly, that this patch is for hardware that has been working, in some cases for 5 years; that it has been written after reading a public doc; and that it was not developed in response to a known bug or issue?
Code that does not conform to a public doc is NOT a bug.
A word to the wise: in all too many cases, the public docs and the private docs disagree. In some cases, the private docs directly contradict the public docs. And, in still other cases, the hardware contradicts the private docs. That's what makes this so much fun.
It's the implementation of the hardware, not the documentation, that matters.
Changes made in response to a reading of public docs, that are not known to resolve an actual observed problem, are a very risky thing to do, especially with old chipsets. They should be rejected outright unless they are confirmed to fix a problem.
We've had people push this kind of change in the past and break boards.
So tread carefully.
ron
-- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
On 17.03.2014 08:00, Mono wrote:
Who knows, it might(!) fix some 5 year old bug?
Then it should be no problem for you to point one to us. coreboot is not openhardware. It never was and never will be. Openhardware is the only way to know what your hardware actually. Coreboot is only how to kick the hardware into working.
First, Mono, thank *you* for your interest and involvement in coreboot.
On Sun, Mar 16, 2014 at 2:00 PM, Mono mono@posteo.de wrote:
Moreover, I was not aware that the code might do something else than what one can know by reading the code and the public documentation. To be honest, if that was true, I would be disappointed, because I expected coreboot to be a possibility of getting away from private software and "you don't know what your machine does".
Yeah, if only it were this simple ::-) It was, at some point, but The Empire Struck Back.
At the moment I believe that applying the patch does no harm. At least this is my current knowledge by running it at the MacBook2,1 for a couple of hours. Switching forth and back between coreboot versions (back to safe ones) works. I do not know if this was true for other boards.
Yes, your test is just not a sufficient test. There's a fair amount of systems out there with this chipset. It's easy to create code that works on *your* version of the macbook with *your* rev of the chipset. But what if it breaks an older version, and we don't know for a year until someone reports that his getac just broke? This happens and it's very painful. So the rule is not to change things unless you know it fixes a bug -- and you really need to document that bug.
The problem solving flowchart is very helpful here.
Here's a true story. We once had a 256-or-so node cluster at LANL. Same vendor, same server node, same mainboards, same chipset, 1/2 the boards ran at 15% lower PCI speed than the other half. All chipsets had the same PCI devid, same rev. What was different? We finally got to realizing that 1/2 the chipsets were made at one fab, half another. And that was the reason.
It's very subtle at the lower levels.
ron
Dear Mono,
Am Sonntag, den 16.03.2014, 22:00 +0100 schrieb Mono:
[…]
In case you were interessted, I uploaded the output of inteltool when running the macbook without the patch, that is by compiling [1] and by compiling the patch on top of [1]. Please see 5324 without patch 5387: http://paste.flashrom.org/view.php?id=2051 5324 plus patch 5387: http://paste.flashrom.org/view.php?id=2052 diff -y without vs with patch 5387: http://paste.flashrom.org/view.php?id=2053
could you please attach/paste the same with/against the inteltool output when running the vendor BIOS please? (I prefer attaching the output, as no browser has to be opened then.)
[…]
Thanks,
Paul
Hallo Paul,
in the attachement are two tarballs (compressed to not send a 2 MB email).
Each tarball contains three files with inteltool output when running the machines a) under vendor BIOS b) under coreboot without the patch c) under coreboot with patch applied. there are three more files with the diffs of the three configurations.
First tarball contains output from a MacBook2,1 second from a Lenovo X60.
For the X60 test case a) runs with wifi card removed. b) and c) with wifi installed.
best regards, Mono
On Mon, Mar 17, 2014 at 10:23:08AM +0100, Paul Menzel wrote:
Dear Mono,
Am Sonntag, den 16.03.2014, 22:00 +0100 schrieb Mono:
[…]
In case you were interessted, I uploaded the output of inteltool when running the macbook without the patch, that is by compiling [1] and by compiling the patch on top of [1]. Please see 5324 without patch 5387: http://paste.flashrom.org/view.php?id=2051 5324 plus patch 5387: http://paste.flashrom.org/view.php?id=2052 diff -y without vs with patch 5387: http://paste.flashrom.org/view.php?id=2053
could you please attach/paste the same with/against the inteltool output when running the vendor BIOS please? (I prefer attaching the output, as no browser has to be opened then.)
[…]
Thanks,
Paul
-- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
-
Mono -
Paul Menzel -
ron minnich -
Vladimir 'φ-coder/phcoder' Serbinenko