On Thu, Jun 09, 2022 at 10:11:59AM +0200, Morten Linderud wrote:
> On Thu, Jun 09, 2022 at 07:46:46AM +0300, Jarkko Sakkinen wrote:
> > On Wed, Jun 08, 2022 at 02:31:08PM +0200, Morten Linderud wrote:
> > > tpm_read_log_acpi() should return -ENODEV when no eventlog from the ACPI
> > > table is found. If the firmware vendor includes an invalid log address
> > > we are unable to map from the ACPI memory and the function returns -EIO
> > > which would abort discovery of the eventlog.
> > >
> > > This change ensure we always return -ENODEV in tpm_read_log_acpi() and
> > > fallback to the EFI configuration table.
> >
> > Please do not use "we" in commit messages. Or start a sentence
> > with "this patch", "this commit" or "this change". It is always
> > best just to go down to the roots and use imperative form.
> >
> > E.g. you could rephrase the last paragraph as
> >
> > "Change the return value from -EIO to -ENODEV when acpi_os_map_iomem()
> > fails to map the event log."
>
> ack
>
> > > The following hardware was used to test this issue:
> > > Framework Laptop (Pre-production)
> > > BIOS: INSYDE Corp, Revision: 3.2
> > > TPM Device: NTC, Firmware Revision: 7.2
> > >
> > > Dump of the faulty ACPI TPM2 table:
> > > [000h 0000 4] Signature : "TPM2" [Trusted Platform Module hardware interface Table]
> > > [004h 0004 4] Table Length : 0000004C
> > > [008h 0008 1] Revision : 04
> > > [009h 0009 1] Checksum : 2B
> > > [00Ah 0010 6] Oem ID : "INSYDE"
> > > [010h 0016 8] Oem Table ID : "TGL-ULT"
> > > [018h 0024 4] Oem Revision : 00000002
> > > [01Ch 0028 4] Asl Compiler ID : "ACPI"
> > > [020h 0032 4] Asl Compiler Revision : 00040000
> > >
> > > [024h 0036 2] Platform Class : 0000
> > > [026h 0038 2] Reserved : 0000
> > > [028h 0040 8] Control Address : 0000000000000000
> > > [030h 0048 4] Start Method : 06 [Memory Mapped I/O]
> > >
> > > [034h 0052 12] Method Parameters : 00 00 00 00 00 00 00 00 00 00 00 00
> > > [040h 0064 4] Minimum Log Length : 00010000
> > > [044h 0068 8] Log Address : 000000004053D000
> > >
> > > Signed-off-by: Morten Linderud <morten@xxxxxxxxxxx>
> > >
> > > ---
> > >
> > > v2: Tweak commit message and opt to return -ENODEV instead of loosening up the
> > > if condition in tpm_read_log()
> > >
> > > ---
> > > drivers/char/tpm/eventlog/acpi.c | 6 +++++-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c
> > > index 1b18ce5ebab1..2b15d6eebd69 100644
> > > --- a/drivers/char/tpm/eventlog/acpi.c
> > > +++ b/drivers/char/tpm/eventlog/acpi.c
> > > @@ -136,8 +136,12 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
> > >
> > > ret = -EIO;
> > > virt = acpi_os_map_iomem(start, len);
> > > - if (!virt)
> > > + if (!virt) {
> > > + dev_warn(&chip->dev, "%s: Failed to map ACPI memory\n", __func__);
> > > + /* try EFI log next */
> > > + ret = -ENODEV;
> > > goto err;
> > > + }
> >
> > It is wrong to try out EFI, if this fails. TPM2 ACPI table was already
> > detected.
>
> The next branch tries out EFI if the eventlog it found is empty, as it created
> an empty file. This branch would produce no eventlog if we fail to map the
> memory. I don't understand why there would be a difference between these two
> branches?
>
> This seems like an oversight after 3dcd15665aca80197333500a4be3900948afccc1
>
> > >
> > > memcpy_fromio(log->bios_event_log, virt, len);
> > >
> > > --
> > > 2.36.1
> >
> > What you are using this for? Without any actual bug report, this
> > is an obvious NAK.
>
> I have hardware with faulty ACPI values which prevents me from getting a
> eventlog. I can surely make a bugreport if it helps the case, but that seems
> like an arbiterary hurdle when I have already spent the time tracking down the
> issue and proposed a fix.
What is the hardware?
BR, Jarkko
