Hi
>Betreff: [PATCH] tpm, tpm_crb: migrate to struct acpi_table_tpm2 and acpi_tpm2_control
> Migrate to struct acpi_table_tpm2 and struct acpi_tpm2_control defined
> in include/acpi/actbl3.h from the internal structures.
I definitely do like the idea! Thanks for spotting this!
However one small remark
> -struct crb_control_area {
> - u32 req;
> - u32 sts;
> - u32 cancel;
> - u32 start;
> - u32 int_enable;
> - u32 int_sts;
> - u32 cmd_size;
> - u64 cmd_pa;
> - u32 rsp_size;
> - u64 rsp_pa;
> -} __packed;
> -
>
> - if (le32_to_cpu(ioread32(&priv->cca->sts)) & CRB_CA_STS_ERROR)
> + if (le32_to_cpu(ioread32(&priv->ctl->error)) & CRB_CA_STS_ERROR)
> return -EIO;
I know the fields are described in include/acpi/actbl3.h as
+struct acpi_tpm2_control {
+ u32 reserved;
+ u32 error;
+ u32 cancel;
+ u32 start;
+ u64 interrupt_control;
+ u32 command_size;
+ u64 command_address;
+ u32 response_size;
+ u64 response_address;
+};
but are the names there still correct? Isn't this information outdated?
The acpi spec refers to the MS spec which is not present anymore, and MS refers to the TCG -- and in the PTP your names are used.
---> We should update the ACPI header?
At least the naming for reserved and error.
What do you think?
Thanks,
Peter
Hi
I somehow missed your reply to this last week.
On Tue, Jun 02, 2015 at 04:00:37PM +0200, Peter Huewe wrote:
> Hi
> >Betreff: [PATCH] tpm, tpm_crb: migrate to struct acpi_table_tpm2 and acpi_tpm2_control
> > Migrate to struct acpi_table_tpm2 and struct acpi_tpm2_control defined
> > in include/acpi/actbl3.h from the internal structures.
>
> I definitely do like the idea! Thanks for spotting this!
>
> However one small remark
> > -struct crb_control_area {
> > - u32 req;
> > - u32 sts;
> > - u32 cancel;
> > - u32 start;
> > - u32 int_enable;
> > - u32 int_sts;
> > - u32 cmd_size;
> > - u64 cmd_pa;
> > - u32 rsp_size;
> > - u64 rsp_pa;
> > -} __packed;
> > -
> >
> > - if (le32_to_cpu(ioread32(&priv->cca->sts)) & CRB_CA_STS_ERROR)
> > + if (le32_to_cpu(ioread32(&priv->ctl->error)) & CRB_CA_STS_ERROR)
> > return -EIO;
>
> I know the fields are described in include/acpi/actbl3.h as
> +struct acpi_tpm2_control {
> + u32 reserved;
> + u32 error;
> + u32 cancel;
> + u32 start;
> + u64 interrupt_control;
> + u32 command_size;
> + u64 command_address;
> + u32 response_size;
> + u64 response_address;
> +};
>
> but are the names there still correct? Isn't this information outdated?
> The acpi spec refers to the MS spec which is not present anymore, and MS refers to the TCG -- and in the PTP your names are used.
>
> ---> We should update the ACPI header?
> At least the naming for reserved and error.
> What do you think?
I think you are right. It does not make sense to degrade here. I'll
prepare "CRB fixes" patch set and also include a workaround for this
bug:
https://bugzilla.kernel.org/show_bug.cgi?id=98181
See my last comment.
> Thanks,
> Peter
/Jarkko
On Mon, Jun 08, 2015 at 02:54:35PM +0300, Jarkko Sakkinen wrote:
> Hi
>
> I somehow missed your reply to this last week.
>
> On Tue, Jun 02, 2015 at 04:00:37PM +0200, Peter Huewe wrote:
> > Hi
> > >Betreff: [PATCH] tpm, tpm_crb: migrate to struct acpi_table_tpm2 and acpi_tpm2_control
> > > Migrate to struct acpi_table_tpm2 and struct acpi_tpm2_control defined
> > > in include/acpi/actbl3.h from the internal structures.
> >
> > I definitely do like the idea! Thanks for spotting this!
> >
> > However one small remark
> > > -struct crb_control_area {
> > > - u32 req;
> > > - u32 sts;
> > > - u32 cancel;
> > > - u32 start;
> > > - u32 int_enable;
> > > - u32 int_sts;
> > > - u32 cmd_size;
> > > - u64 cmd_pa;
> > > - u32 rsp_size;
> > > - u64 rsp_pa;
> > > -} __packed;
> > > -
> > >
> > > - if (le32_to_cpu(ioread32(&priv->cca->sts)) & CRB_CA_STS_ERROR)
> > > + if (le32_to_cpu(ioread32(&priv->ctl->error)) & CRB_CA_STS_ERROR)
> > > return -EIO;
> >
> > I know the fields are described in include/acpi/actbl3.h as
> > +struct acpi_tpm2_control {
> > + u32 reserved;
> > + u32 error;
> > + u32 cancel;
> > + u32 start;
> > + u64 interrupt_control;
> > + u32 command_size;
> > + u64 command_address;
> > + u32 response_size;
> > + u64 response_address;
> > +};
> >
> > but are the names there still correct? Isn't this information outdated?
> > The acpi spec refers to the MS spec which is not present anymore, and MS refers to the TCG -- and in the PTP your names are used.
> >
> > ---> We should update the ACPI header?
> > At least the naming for reserved and error.
> > What do you think?
>
> I think you are right. It does not make sense to degrade here. I'll
> prepare "CRB fixes" patch set and also include a workaround for this
> bug:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=98181
>
> See my last comment.
Some rethinking after sending this email.
I implement and submit the bug as a separate item as these are
unrelated issues.
I think the control area should not be in the ACPI headers in the
first place as it is not defined TCG ACPI Specification.
/Jarkko
Hi Jarkko
> > > >Betreff: [PATCH] tpm, tpm_crb: migrate to struct acpi_table_tpm2 and
> > > >acpi_tpm2_control
> > > but are the names there still correct? Isn't this information outdated?
> > > The acpi spec refers to the MS spec which is not present anymore, and
> > > MS refers to the TCG -- and in the PTP your names are used.
> > >
> > > ---> We should update the ACPI header?
> > > At least the naming for reserved and error.
> > > What do you think?
> >
> > I think you are right. It does not make sense to degrade here.
so I'm waiting on the new version depending on the updated acpi header?
Thanks,
Peter
On Tue, Jun 16, 2015 at 10:46:50PM +0200, Peter H?we wrote:
> Hi Jarkko
>
> > > > >Betreff: [PATCH] tpm, tpm_crb: migrate to struct acpi_table_tpm2 and
> > > > >acpi_tpm2_control
> > > > but are the names there still correct? Isn't this information outdated?
> > > > The acpi spec refers to the MS spec which is not present anymore, and
> > > > MS refers to the TCG -- and in the PTP your names are used.
> > > >
> > > > ---> We should update the ACPI header?
> > > > At least the naming for reserved and error.
> > > > What do you think?
> > >
> > > I think you are right. It does not make sense to degrade here.
>
> so I'm waiting on the new version depending on the updated acpi header?
Yes. I'll submit a new patch later on when new arrives come from ACPICA.
You can ignore this until then.
> Thanks,
> Peter
/Jarkko