On Sat, 22 Jun 2024 at 19:36, Ilias Apalodimas
<ilias.apalodi...@linaro.org> wrote:
>
> Hi
>
> again many thanks for the quick review
>
> On Sat, 22 Jun 2024 at 19:25, Heinrich Schuchardt <xypron.g...@gmx.de> wrote:
> >
> > On 22.06.24 16:35, Ilias Apalodimas wrote:
> > > commit 97707f12fdab ("tpm: Support boot measurements") moved out code
> > > from the EFI subsystem into the TPM one to support measurements when
> > > booting with !EFI.
> > >
> > > Those were moved directly into the TPM subsystem and in the tpm-v2.c
> > > library. In hindsight, it would have been better to move it in new
> > > files since the TCG2 is governed by its own spec and it's cleaner
> > > when we want to enable certian parts of the TPM functionality.
> >
> > Nits:
> >
> > %s/certian/certain/
> >
> > >
> > > So let's create a header file and another library and move the TCG
> > > specific bits there.
> > >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodi...@linaro.org>
> > > ---
> > > boot/bootm.c | 1 +
> > > include/efi_tcg2.h | 1 +
> > > include/tpm-v2.h | 474 +++++-------------------------
> > > include/tpm_tcg2.h | 336 ++++++++++++++++++++++
> > > lib/Makefile | 2 +
> > > lib/tpm-v2.c | 676 +------------------------------------------
> > > lib/tpm_tcg2.c | 696 +++++++++++++++++++++++++++++++++++++++++++++
> >
> > The patch series were easier to review if moving header definitions were
> > separated from moving implementations.
So, I can't do that because I'll need an intermediate commit to
include tpm_tcg2.h to tpm-v2.h which I'd rather avoid.
I can make the diff smaller though. Are you ok with that ?
Thanks
/Ilias
> >
> > This patch contains changes that are not described in the commit
> > message, e.g.
> >
> > if (elog->log_size) {
> > if (log.found) {
> > if (elog->log_size < log.log_position)
> > - return -ENOBUFS;
> > + return -ENOSPC;
>
> And this is a great catch. this changed in patch#1 and the correct
> return is -ENOBUFS. I started working on 2 trees and obviously messed
> up this rebase... Thanks!
>
> >
> > I guess you wanted to put this into patch 1.
> >
> > Please, separate the patches adequately.
>
> Fair enough. I thought it was going to be hard not breaking
> compilation hence the big patch. I'll try splitting it
>
> >
> > + * Copyright (c) 2020 Linaro
> > + * Copyright (c) 2023 Linaro Limited
> >
> > The copyright lines look inconsistent. Linaro Limited exists under this
> > name since April 13th, 2010. Is the 2020 copyright for a different company?
> >
>
> I'll keep the older one, same company
>
> [...]
>
> > > +}
> > > +
> > > +__weak void tcg2_platform_startup_error(struct udevice *dev, int rc) {}
> > > +
> >
> > git warning: "new blank line at EOF".
> >
> > Otherwise looks good.
> >
> > Best regards
> >
> > Heinrich
>
> Thanks
> /Ilias
