Hi Wolfgang,
On Sun, Nov 4, 2012 at 1:54 AM, Wolfgang Denk wd@denx.de wrote:
Dear Simon,
In message CAPnjgZ0vRbsFWpGqsZPy=TjgJp9r80VnVv2Pcem5werL8bRwmw@mail.gmail.com you wrote:
I think you may have missed the pending patches which make use of this. it is important functionality for the Chromebooks (secure boot).
No, I have not missed these. But all the patch does is set CONFIG_GENERIC_LPC_TPM - there is still not a single user defining CONFIG_CMD_TPM, so what does this help? We still have tons of dead code around. Dump it!
So we need a board that defines the command also? I did not realise that was a requirement - certainly I can add that command to the boards also.
No, you misunderstand. You stick at the surface, at the formal part of it. Indeed enabling this option for a board would be an improvement - at least then we could be sure that the code compiles. But that does not actually make it _used_. Similar to you enabling of CONFIG_GENERIC_LPC_TPM - the code that needs appears to be just more unused code itself so this is not actual _use_ in the sense of providing a functionality needed for the operation of the device.
OK, but I still don't quite get it. As I asked in the other thread, are you not interested in TPM functionality at all until we have a verified boot implementation, or are you happy to have things progress in stages? I'm will to work through this, and I have the time at present, but I believe that drivers for two common TPM chips are a useful addition to U-Boot regardless.
Upstreaming the code is a step-by-step process. The TPM is an important component of secure boot, and things have to progress in some sort of fashion. I do understand the dead code argument, but we can't submit high-level code without the drivers it uses (and there are many).
Well, we had this very same discussion when the TPM code was originally added, and I let myself talk into accepting it, based on the argument that code that needs it is available and will be mainlined "really soon now". More than a year has passed, and _nothing_ happened. Now I see the next wave of patches adding dead code in the same are, and I have to admit that this does not make me enthusiastic.
It's not obvious how to mainline this rather large feature except in pieces. For example, I think I can start on a feature to verify a kernel, but I do want to talk to the TPM as part of that.
I think your approach is wrong. You should try and get a minimal version (obviously with very restricted functionality) of your "high-level code" into mainline, and all needed drivers in the same patch series (which makes clear why this should be a minimal configuration). Then add drivers and functionality step by step. Adding a driver here and a driver there one by one with the slight chance that there might - in some unforeseeable future - a board submitted that actually uses these all is nothing I'm going to accept.
That's going to be a big series.... but in contrast, display, SATA and GPIO patches are ok, but TPM is not?
I'll see what I can do here, but in principle I feel that adding a driver should be OK, provided a board uses it, without necessarily the high-level code that uses it. After all, we don't necessarily expect to mainline all the scripts that drive the actual boot.
And especially not after the experience with the TPM code so far, where my patience has been exhausted after more than a year of nothing happening.
Well not nothing. Have been rather tied up getting a product out. I agree that things have been very quiet on verified boot - perhaps the original upstream author was exhausted after the effort of getting the driver into mainline and retired for a rest :-)
Finally, I also have license concerns about the newly submitted code. Statements like "code is governed by a BSD-style license" (in drivers/tpm/tis_i2c.c) are obviusly not acceptabl at all - there are several BSD-style licenses - am I to chose myself any of these, or what???
Hmmm, that is in one file only, and one that I can easily change without problem. It is not correct - I will change it to GPL.
I have the impression that you are trying to use the end of the merge window to get a whole a bunch of very green code into mainline, ignoring quite a number of rules well known to you.
Not intentionally. The common/ series includes a few patches that I have carried for quite a while (2 years in some cases), so I thought it was time to either mainline it or drop it. I know from experience that other people's patches can be very useful in certain circumstances even if not perfect. But coding hiding in a dark corner is no use to anyone.
However most of the common/ series is code that we depend on and I think might be generally useful. I do admit that in striving for a very high level of polish we have attacked problems that perhaps would not matter for many systems, and thus more patches.
Regards, Simon
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Without facts, the decision cannot be made logically. You must rely on your human intuition. -- Spock, "Assignment: Earth", stardate unknown