[U-Boot] [PATCH] TPM: remove dead code

[Simon Glass]

Hi Wolfgang,

On Sun, Nov 4, 2012 at 1:54 AM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Simon,
>
> In message <CAPnjgZ0vRbsFWpGqsZPy=TjgJp9r80VnVv2Pcem5werL8bRwmw at 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 at denx.de
> Without facts, the decision cannot be made logically. You  must  rely
> on your human intuition.
>         -- Spock, "Assignment: Earth", stardate unknown