| Message ID | C5A28EF7B98F574C85C70238C8E9ECC04E682BF0A8@ABGEX74E.FSC.NET |
|---|---|
| State | New |
| Headers | show |
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 ------------------------------------------------------------------------------
> 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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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
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
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
--- 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