[tpmdd-devel] TPM_CHIP_FLAG_TPM2 ABI change in commit 9b774d5c
diff mbox

Message ID C5A28EF7B98F574C85C70238C8E9ECC04E682BF0A8@ABGEX74E.FSC.NET
State New
Headers show

Commit Message

Martin Wilck Nov. 6, 2015, 8:08 a.m. UTC
Hi Jarko,

tpm_tis driver under RHEL7.2 beta. The in-kernel parts of the TPM_Driver
were using BIT(2) while the module code had BIT(1), so my TPM was
misdetected as a 1.2 TPM. This is very hard-to-track ABI change which
may go unnoticed in future distro backports, too. Is there a good reason
to change the bit?

I know upstream doesn't care about ABI, but there are folks out there
who (have to) care.

Regards
Martin

------------------------------------------------------------------------------

Comments

Jarkko Sakkinen Nov. 6, 2015, 12:23 p.m. UTC | #1
On Fri, Nov 06, 2015 at 09:08:49AM +0100, Wilck, Martin wrote:
> Hi Jarko,
> 
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -158,8 +158,7 @@ struct tpm_vendor_specific {
>  
>  enum tpm_chip_flags {
>         TPM_CHIP_FLAG_REGISTERED        = BIT(0),
> -       TPM_CHIP_FLAG_PPI               = BIT(1),
> -       TPM_CHIP_FLAG_TPM2              = BIT(2),
> +       TPM_CHIP_FLAG_TPM2              = BIT(1),
>  };
>  
> This change has made me pull my hair out when I tried to run the latest
> tpm_tis driver under RHEL7.2 beta. The in-kernel parts of the TPM_Driver
> were using BIT(2) while the module code had BIT(1), so my TPM was
> misdetected as a 1.2 TPM. This is very hard-to-track ABI change which
> may go unnoticed in future distro backports, too. Is there a good reason
> to change the bit?

Frankly I don't understand why this would be a problem for distro backports
because tpm_tis is in the kernel tree. Usually distros update kernel
together with the driver modules for that kernel.

> I know upstream doesn't care about ABI, but there are folks out there
> who (have to) care.
> 
> Regards
> Martin

/Jarkko

------------------------------------------------------------------------------
Martin Wilck Nov. 6, 2015, 1:06 p.m. UTC | #2
> Frankly I don't understand why this would be a problem for distro backports
> because tpm_tis is in the kernel tree. Usually distros update kernel
> together with the driver modules for that kernel.

You are right. But out-of-tree TPM drivers could get in trouble.

The Infineon TPM2.0 that I've been working with isn't cleanly supported
by any current enterprise distribution. Thus the idea that OEMs would
try to solve this problem temporarily with an out-of-tree driver makes
some sense. I had considered that for Fujitsu, actually.

Martin

> 
> > I know upstream doesn't care about ABI, but there are folks out there
> > who (have to) care.
> > 
> > Regards
> > Martin
> 
> /Jarkko
------------------------------------------------------------------------------
Jarkko Sakkinen Nov. 6, 2015, 2:08 p.m. UTC | #3
On Fri, Nov 06, 2015 at 02:06:20PM +0100, Wilck, Martin wrote:
> > Frankly I don't understand why this would be a problem for distro backports
> > because tpm_tis is in the kernel tree. Usually distros update kernel
> > together with the driver modules for that kernel.
> 
> You are right. But out-of-tree TPM drivers could get in trouble.
> 
> The Infineon TPM2.0 that I've been working with isn't cleanly supported
> by any current enterprise distribution. Thus the idea that OEMs would
> try to solve this problem temporarily with an out-of-tree driver makes
> some sense. I had considered that for Fujitsu, actually.

Peter, what do you think about this?

> Martin

/Jarkko

------------------------------------------------------------------------------
Jarkko Sakkinen Nov. 6, 2015, 2:55 p.m. UTC | #4
On Fri, Nov 06, 2015 at 04:08:26PM +0200, Jarkko Sakkinen wrote:
> On Fri, Nov 06, 2015 at 02:06:20PM +0100, Wilck, Martin wrote:
> > > Frankly I don't understand why this would be a problem for distro backports
> > > because tpm_tis is in the kernel tree. Usually distros update kernel
> > > together with the driver modules for that kernel.
> > 
> > You are right. But out-of-tree TPM drivers could get in trouble.
> > 
> > The Infineon TPM2.0 that I've been working with isn't cleanly supported
> > by any current enterprise distribution. Thus the idea that OEMs would
> > try to solve this problem temporarily with an out-of-tree driver makes
> > some sense. I had considered that for Fujitsu, actually.
> 
> Peter, what do you think about this?

>From my side this is still NAK. This would encourage not to get that
driver into mainline or any other out-of-tree driver for that matter.
Distros are free to make the change you suggested. For upstream it does
not feel right at all.

> /Jarkko

/Jarkko

------------------------------------------------------------------------------
Peter Hüwe Nov. 6, 2015, 4:04 p.m. UTC | #5
Am 6. November 2015 06:08:26 PST, schrieb Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>:
>On Fri, Nov 06, 2015 at 02:06:20PM +0100, Wilck, Martin wrote:
>> > Frankly I don't understand why this would be a problem for distro
>backports
>> > because tpm_tis is in the kernel tree. Usually distros update
>kernel
>> > together with the driver modules for that kernel.
>> 
>> You are right. But out-of-tree TPM drivers could get in trouble.
>> 
>> The Infineon TPM2.0 that I've been working with isn't cleanly
>supported
>> by any current enterprise distribution. Thus the idea that OEMs would
>> try to solve this problem temporarily with an out-of-tree driver
>makes
>> some sense. I had considered that for Fujitsu, actually.
>
>Peter, what do you think about this?
>
I'll look through all that over the weekend.
Haven't got time to look at it yet.

Peter
Martin Wilck Nov. 9, 2015, 11:05 a.m. UTC | #6
Hello Jarkko,

> From my side this is still NAK. This would encourage not to get that
> driver into mainline or any other out-of-tree driver for that matter.
> Distros are free to make the change you suggested. For upstream it does
> not feel right at all.

I can't quite follow you. Why would simply keeping that bit in place
discourage anyone to push a driver into mainline?

I'd like to put it the other way round. There is plenty of space in this
bit field. You are only using 2 bits. What's the problem with using bits
0 and 2 rather than 0 and 1? It's just an aesthetic question in mainline
but it breaks things out of tree.

Anyway, it's your decision whether or not to revert this change. I just
wanted to raise a bit of awareness for the ABI issue.

Martin

------------------------------------------------------------------------------
Presto, an open source distributed SQL query engine for big data, initially
developed by Facebook, enables you to easily query your data on Hadoop in a 
more interactive manner. Teradata is also now providing full enterprise
support for Presto. Download a free open source copy now.
http://pubads.g.doubleclick.net/gampad/clk?id=250295911&iu=/4140
Jarkko Sakkinen Nov. 9, 2015, 2:29 p.m. UTC | #7
On Mon, Nov 09, 2015 at 12:05:39PM +0100, Wilck, Martin wrote:
> Hello Jarkko,
> 
> > From my side this is still NAK. This would encourage not to get that
> > driver into mainline or any other out-of-tree driver for that matter.
> > Distros are free to make the change you suggested. For upstream it does
> > not feel right at all.
> 
> I can't quite follow you. Why would simply keeping that bit in place
> discourage anyone to push a driver into mainline?
> 
> I'd like to put it the other way round. There is plenty of space in this
> bit field. You are only using 2 bits. What's the problem with using bits
> 0 and 2 rather than 0 and 1? It's just an aesthetic question in mainline
> but it breaks things out of tree.
> 
> Anyway, it's your decision whether or not to revert this change. I just
> wanted to raise a bit of awareness for the ABI issue.

I might be splitting hairs here but I'm still lookig for second opinion
here.

> Martin

/Jarkko

------------------------------------------------------------------------------
Presto, an open source distributed SQL query engine for big data, initially
developed by Facebook, enables you to easily query your data on Hadoop in a 
more interactive manner. Teradata is also now providing full enterprise
support for Presto. Download a free open source copy now.
http://pubads.g.doubleclick.net/gampad/clk?id=250295911&iu=/4140

Patch
diff mbox

--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -158,8 +158,7 @@  struct tpm_vendor_specific {
 
 enum tpm_chip_flags {
        TPM_CHIP_FLAG_REGISTERED        = BIT(0),
-       TPM_CHIP_FLAG_PPI               = BIT(1),
-       TPM_CHIP_FLAG_TPM2              = BIT(2),
+       TPM_CHIP_FLAG_TPM2              = BIT(1),
 };
 
This change has made me pull my hair out when I tried to run the latest