[v2,0/2] char: tpm: add new driver for tpm i2c ptp
mbox series

Message ID 20190628151327.206818-1-oshrialkoby85@gmail.com
Headers show
Series
  • char: tpm: add new driver for tpm i2c ptp
Related show

Message

Oshri Alkobi June 28, 2019, 3:13 p.m. UTC
This patch set adds support for TPM devices that implement the I2C
interface defined by TCG PTP specification:
https://trustedcomputinggroup.org/wp-content/uploads/TCG_PC_Client_Platform_TPM_Profile_PTP_2.0_r1.03_v22.pdf

The driver was tested on Raspberry-Pie 3, using Nuvoton NPCT75X TPM.

changes
-------

v2:
- Extended device tree binding description

v1:
- Initial version

Oshri Alkoby (2):
  dt-bindings: tpm: add the TPM I2C PTP device tree binding
    documentation
  char: tpm: add new driver for tpm i2c ptp

 .../bindings/security/tpm/tpm-i2c-ptp.txt     |   24 +
 drivers/char/tpm/Kconfig                      |   22 +
 drivers/char/tpm/Makefile                     |    1 +
 drivers/char/tpm/tpm_i2c_ptp.c                | 1099 +++++++++++++++++
 4 files changed, 1146 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-i2c-ptp.txt
 create mode 100644 drivers/char/tpm/tpm_i2c_ptp.c

Comments

Jarkko Sakkinen July 4, 2019, 8:43 a.m. UTC | #1
On Fri, 2019-06-28 at 18:13 +0300, Oshri Alkoby wrote:

The long descriptions are still missing. Please take the time and write
a proper commit messages that clearly tell what the patch does.

Check out tpm_tis_core.c and tpm_tis_spi.c. TPM TIS driver implements
that spec so you should only implement a new physical layer.

/Jarkko
Alexander Steffen July 4, 2019, 11:29 a.m. UTC | #2
On 04.07.2019 10:43, Jarkko Sakkinen wrote:
> Check out tpm_tis_core.c and tpm_tis_spi.c. TPM TIS driver implements
> that spec so you should only implement a new physical layer.

I had the same thought. Unfortunately, the I2C-TIS specification 
introduces two relevant changes compared to tpm_tis/tpm_tis_spi:

1. Locality is not encoded into register addresses anymore, but stored 
in a separate register.
2. Several register addresses have changed (but still contain compatible 
contents).

I'd still prefer not to duplicate all the high-level logic from 
tpm_tis_core. But this will probably mean to introduce some new 
interfaces between tpm_tis_core and the physical layers.

Also, shouldn't the new driver be called tpm_tis_i2c, to group it with 
all the other (TIS) drivers, that implement a vendor-independent 
protocol? With tpm_i2c_ptp users might assume that ptp is just another 
vendor.

Alexander
Jarkko Sakkinen July 5, 2019, 11:15 a.m. UTC | #3
On Thu, 2019-07-04 at 13:29 +0200, Alexander Steffen wrote:
> On 04.07.2019 10:43, Jarkko Sakkinen wrote:
> > Check out tpm_tis_core.c and tpm_tis_spi.c. TPM TIS driver implements
> > that spec so you should only implement a new physical layer.
> 
> I had the same thought. Unfortunately, the I2C-TIS specification 
> introduces two relevant changes compared to tpm_tis/tpm_tis_spi:

I doubt that there was any comparison made.

> 1. Locality is not encoded into register addresses anymore, but stored 
> in a separate register.
> 2. Several register addresses have changed (but still contain compatible 
> contents).
> 
> I'd still prefer not to duplicate all the high-level logic from 
> tpm_tis_core. But this will probably mean to introduce some new 
> interfaces between tpm_tis_core and the physical layers.

Agreed. Some plumbing needs to be done in tpm_tis_core to make it work
for this. We definitely do not want to duplicate code that has been
field tested for years.

> Also, shouldn't the new driver be called tpm_tis_i2c, to group it with 
> all the other (TIS) drivers, that implement a vendor-independent 
> protocol? With tpm_i2c_ptp users might assume that ptp is just another 
> vendor.

Yes, absolutely. I guess the driver has been done without looking at
what already exist in the TPM kernel stack.

/Jarkko
Jarkko Sakkinen July 5, 2019, 11:28 a.m. UTC | #4
On Thu, 2019-07-04 at 12:48 -0500, Oshri Alkobi wrote:
> Alex, Jarkko, thank you very much for your feedbacks!

Please configure your email client to use plain text.

> I totally agree, there are some duplications that can be common, indeed it
> will require some work in tpm_tis_core.
> Since I believe it is not going to happen soon, I would suggest to examine
> what duplications can currently be dropped from the new driver, so the kernel
> will support the PTP I2C interface in the meantime.
> I will appreciate getting ideas about any tpm_tis_core logic that currently
> can be used as is by the new drive.

I rather wait for a solution that integrates with our mature stack for
TIS (or these days FIFO) than integrate something half-baked. If you
want something in, please do right things right.

What you are proposing would mean maintaining duplicate stacks forever.

> Since the TIS is an old specification that mostly defines FIFO for TPM1.2 I
> would say the name tpm_tis_i2c does not completely reflect its goal. However
> we really don't have any problem with any name that the group will agree on.
> Does tpm_ptp_i2c sound better than the current name?

Absolutely not going to use that name. The naming convention is what
it is for other drivers that are adapt tpm_tis_core to different HW
interfaces.

/Jarkko
Jarkko Sakkinen July 15, 2019, 9:45 a.m. UTC | #5
On Mon, Jul 15, 2019 at 11:08:47AM +0300, Tomer Maimon wrote:
>    Thanks for your feedback and sorry for the late response.
>
>    Due to the amount of work required to handle this technical feedback and
>    project constraints we need to put this task on hold for the near future.
>
>    In the meantime, anyone from the community is welcome to take over this
>    code and handle the re-design for the benefit of the entire TPM community.

Ok, so there is already driver for this called tpm_tis_core.

So you go and create a new module, whose name given the framework of
things that we already have deployed, is destined to be tpm_tis_i2c.

Then you roughly implement a new physical layer by using  a callback
interface provided to you by tpm_tis_core.

The so called re-design was already addressed by Alexander [1].

How hard can it be seriously?

[1] https://lkml.org/lkml/2019/7/4/331

/Jarkko
Alexander Steffen July 17, 2019, 7:48 a.m. UTC | #6
On 15.07.2019 10:08, Tomer Maimon wrote:
> Hi Jarkko and All,
> 
> Thanks for your feedback and sorry for the late response.
> 
> 
> Due to the amount of work required to handle this technical feedback and 
> project constraints we need to put this task on hold for the near future.
> 
> In the meantime, anyone from the community is welcome to take over this 
> code and handle the re-design for the benefit of the entire TPM community.

Too bad you lack the time to finish this.

If somebody else were to pick it up, would it be possible for you to 
provide a couple of TPMs that implement this protocol, so that it can be 
properly tested?

Alexander
Eyal.Cohen@nuvoton.com July 18, 2019, 12:51 p.m. UTC | #7
Hi Jarkko and Alexander,

We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer.
However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core.
At a minimum we thought of:
1. Handling TPM Localities in I2C is different
2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core
3. Handling Chip specific issues, since I2C implementation might be slightly different across the various TPM vendors
4. Modify tpm_tis_send_data and tpm_tis_recv_data to work according the TCG Device Driver Guide (optimization on TPM_STS access and send/recv retry)
Besides this, during development we might encounter additional differences between SPI and I2C.

We currently target to allocate an eng. to work on this on the second half of August with a goal to have the driver ready for the next kernel merge window.

Regards,
Eyal.

-----Original Message-----
From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Sent: Monday, July 15, 2019 12:46 PM
To: Tomer Maimon <tmaimon77@gmail.com>
Cc: Oshri Alkobi <oshrialkoby85@gmail.com>; Alexander Steffen <Alexander.Steffen@infineon.com>; Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; peterhuewe@gmx.de; jgg@ziepe.ca; Arnd Bergmann <arnd@arndb.de>; Greg KH <gregkh@linuxfoundation.org>; IS20 Oshri Alkoby <oshri.alkoby@nuvoton.com>; devicetree <devicetree@vger.kernel.org>; AP MS30 Linux Kernel community <linux-kernel@vger.kernel.org>; linux-integrity@vger.kernel.org; gcwilson@us.ibm.com; kgoldman@us.ibm.com; nayna@linux.vnet.ibm.com; IS30 Dan Morav <Dan.Morav@nuvoton.com>; IS20 Eyal Cohen <Eyal.Cohen@nuvoton.com>
Subject: Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

On Mon, Jul 15, 2019 at 11:08:47AM +0300, Tomer Maimon wrote:
>    Thanks for your feedback and sorry for the late response.
>
>    Due to the amount of work required to handle this technical feedback and
>    project constraints we need to put this task on hold for the near future.
>
>    In the meantime, anyone from the community is welcome to take over this
>    code and handle the re-design for the benefit of the entire TPM community.

Ok, so there is already driver for this called tpm_tis_core.

So you go and create a new module, whose name given the framework of things that we already have deployed, is destined to be tpm_tis_i2c.

Then you roughly implement a new physical layer by using  a callback interface provided to you by tpm_tis_core.

The so called re-design was already addressed by Alexander [1].

How hard can it be seriously?

[1] https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2019_7_4_331&d=DwIBAg&c=ue8mO8zgC4VZ4q_aNVKt8G9MC01UFDmisvMR1k-EoDM&r=hjzIxEsS8wf3Ezr__r0ZOjw3kki8jIlQK-SQ5pWhXaM&m=aYs86sTFmxqvlI5rE2AWLGRl0lb9XRuiRKVtsCaXdRM&s=_3MMoq5UISFwMfK4OfgLbA2kMfZVgEfgkaWKqDEUXGw&e=

/Jarkko


===========================================================================================
The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Nuvoton is strictly prohibited; and any information in this email irrelevant to the official business of Nuvoton shall be deemed as neither given nor endorsed by Nuvoton.
Alexander Steffen July 18, 2019, 5:10 p.m. UTC | #8
On 18.07.2019 14:51, Eyal.Cohen@nuvoton.com wrote:
> Hi Jarkko and Alexander,
> 
> We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer.

Great :) In the meantime, I've done some experiments creating an I2C 
driver based on tpm_tis_core, see 
https://patchwork.kernel.org/patch/11049363/ Please have a look at that 
and provide your feedback (and/or use it as a basis for further 
implementations).

> However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core.
> At a minimum we thought of:
> 1. Handling TPM Localities in I2C is different

It turned out not to be that different in the end, see the code 
mentioned above and my comment here: 
https://patchwork.kernel.org/cover/11049365/

> 2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core

That is completely optional, so there is no need to implement it in the 
beginning. Also, do you expect a huge benefit from that functionality? 
Are bit flips that much more likely on I2C compared to SPI, which has no 
CRC at all, but still works fine?

> 3. Handling Chip specific issues, since I2C implementation might be slightly different across the various TPM vendors

Right, that seems similar to the cr50 issues 
(https://lkml.org/lkml/2019/7/17/677), so there should probably be a 
similar way to do it.

> 4. Modify tpm_tis_send_data and tpm_tis_recv_data to work according the TCG Device Driver Guide (optimization on TPM_STS access and send/recv retry)

Optimizations are always welcome, but I'd expect basic communication to 
work already with the current code (though maybe not as efficiently as 
possible).

> Besides this, during development we might encounter additional differences between SPI and I2C.
> 
> We currently target to allocate an eng. to work on this on the second half of August with a goal to have the driver ready for the next kernel merge window.
> 
> Regards,
> Eyal.
Benoit HOUYERE July 30, 2019, 8:39 a.m. UTC | #9
Hi Alexander, Jarkko and Eyal,

A first I2C TCG patch (tpm_tis_i2c.c) has been proposed in the same time as tpm_tis_spi.c by Christophe 3 years ago.

https://patchwork.kernel.org/patch/8628681/

At the time, we have had two concerns :
	1) I2C TPM component number, in the market, compliant with new I2C TCG specification to validate new I2C driver.
	2) Lots changing  was already provided by tpm_tis_spi.c on 4.8.

That's why Tpm_tis_i2c.c has been postponed.

Tpm_tis_spi Linux driver is now robust, if we have several different I2C TPM solutions today to validate a tpm_tis_i2c driver, I 'm ready to contribute to it for validation (STmicro TPM) or propose a solution compatible on 5.1 linux driver if needed under timeframe proposed (second half of august).

Best Regards,

Benoit



-----Original Message-----
From: linux-integrity-owner@vger.kernel.org <linux-integrity-owner@vger.kernel.org> On Behalf Of Alexander Steffen
Sent: jeudi 18 juillet 2019 19:10
To: Eyal.Cohen@nuvoton.com; jarkko.sakkinen@linux.intel.com; tmaimon77@gmail.com
Cc: oshrialkoby85@gmail.com; robh+dt@kernel.org; mark.rutland@arm.com; peterhuewe@gmx.de; jgg@ziepe.ca; arnd@arndb.de; gregkh@linuxfoundation.org; oshri.alkoby@nuvoton.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-integrity@vger.kernel.org; gcwilson@us.ibm.com; kgoldman@us.ibm.com; nayna@linux.vnet.ibm.com; Dan.Morav@nuvoton.com; oren.tanami@nuvoton.com
Subject: Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

On 18.07.2019 14:51, Eyal.Cohen@nuvoton.com wrote:
> Hi Jarkko and Alexander,
> 
> We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer.

Great :) In the meantime, I've done some experiments creating an I2C driver based on tpm_tis_core, see https://patchwork.kernel.org/patch/11049363/ Please have a look at that and provide your feedback (and/or use it as a basis for further implementations).

> However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core.
> At a minimum we thought of:
> 1. Handling TPM Localities in I2C is different

It turned out not to be that different in the end, see the code mentioned above and my comment here: 
https://patchwork.kernel.org/cover/11049365/

> 2. Handling I2C CRC - relevant only to I2C bus hence not supported 
> today by TIS core

That is completely optional, so there is no need to implement it in the beginning. Also, do you expect a huge benefit from that functionality? 
Are bit flips that much more likely on I2C compared to SPI, which has no CRC at all, but still works fine?

> 3. Handling Chip specific issues, since I2C implementation might be 
> slightly different across the various TPM vendors

Right, that seems similar to the cr50 issues (https://lkml.org/lkml/2019/7/17/677), so there should probably be a similar way to do it.

> 4. Modify tpm_tis_send_data and tpm_tis_recv_data to work according 
> the TCG Device Driver Guide (optimization on TPM_STS access and 
> send/recv retry)

Optimizations are always welcome, but I'd expect basic communication to work already with the current code (though maybe not as efficiently as possible).

> Besides this, during development we might encounter additional differences between SPI and I2C.
> 
> We currently target to allocate an eng. to work on this on the second half of August with a goal to have the driver ready for the next kernel merge window.
> 
> Regards,
> Eyal.
Alexander Steffen July 30, 2019, 5:42 p.m. UTC | #10
Hi Benoit,

good to see you're still around.

On 30.07.2019 10:39, Benoit HOUYERE wrote:
> Hi Alexander, Jarkko and Eyal,
> 
> A first I2C TCG patch (tpm_tis_i2c.c) has been proposed in the same time as tpm_tis_spi.c by Christophe 3 years ago.
> 
> https://patchwork.kernel.org/patch/8628681/

Thanks for mentioning this. I forgot it exists, since it was still on 
the old mailing list.

> At the time, we have had two concerns :
> 	1) I2C TPM component number, in the market, compliant with new I2C TCG specification to validate new I2C driver.
> 	2) Lots changing  was already provided by tpm_tis_spi.c on 4.8.
> 
> That's why Tpm_tis_i2c.c has been postponed.
> 
> Tpm_tis_spi Linux driver is now robust, if we have several different I2C TPM solutions today to validate a tpm_tis_i2c driver, I 'm ready to contribute to it for validation (STmicro TPM) or propose a solution compatible on 5.1 linux driver if needed under timeframe proposed (second half of august).

Could you run your tests against the simple implementation that I posted 
a while ago (https://patchwork.kernel.org/cover/11049365/) and provide 
your feedback? Since it is already based on the current tpm_tis_core, it 
is probably easier to integrate necessary changes there.

By the way, has it gotten any easier in the meantime to get hold of your 
TPMs to use them for kernel tests?

Alexander
Oshri Alkobi Aug. 15, 2019, 5:03 p.m. UTC | #11
On Thu, Jul 18, 2019 at 8:10 PM Alexander Steffen
<Alexander.Steffen@infineon.com> wrote:
>
> On 18.07.2019 14:51, Eyal.Cohen@nuvoton.com wrote:
> > Hi Jarkko and Alexander,
> >
> > We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer.
>
> Great :) In the meantime, I've done some experiments creating an I2C
> driver based on tpm_tis_core, see
> https://patchwork.kernel.org/patch/11049363/ Please have a look at that
> and provide your feedback (and/or use it as a basis for further
> implementations).
>

Sorry for the late response.

Thanks Alexander, indeed it looks much simpler.
I've checked it with Nuvoton's TPM - basic TPM commands work, I only
had to remove the first msg from the read/write I2C transmitting
(from/to TPM_LOC_SEL), the TPM couldn't handle two register writes in
a sequence.
Actually it is more efficient to set TPM_LOC_SEL only before locality
check/request/relinquish - it is sticky.
I still didn't manage to work with interrupts, will debug it.

We weren't aware to the implementation of Christophe/ST which looks
good and can be complement to yours.
If no one is currently working on that, we can prepare a new patch
that is based on both.
Please let us know.

> > However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core.
> > At a minimum we thought of:
> > 1. Handling TPM Localities in I2C is different
>
> It turned out not to be that different in the end, see the code
> mentioned above and my comment here:
> https://patchwork.kernel.org/cover/11049365/
>
> > 2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core
>
> That is completely optional, so there is no need to implement it in the
> beginning. Also, do you expect a huge benefit from that functionality?
> Are bit flips that much more likely on I2C compared to SPI, which has no
> CRC at all, but still works fine?
>

I2C is noisy bus with potentially more devices with larger variety
than SPI. I2C may have more than one master and may have collisions
and/or arbitration.
Still we can start w/o CRC for the first stage and add it later.
BTW, Christophe already did most of the work
(https://patchwork.kernel.org/patch/8628661/)

> > 3. Handling Chip specific issues, since I2C implementation might be slightly different across the various TPM vendors
>
> Right, that seems similar to the cr50 issues
> (https://lkml.org/lkml/2019/7/17/677), so there should probably be a
> similar way to do it.

Got it. We hope things will work for us without having another driver,
but it's an option.

>
> > 4. Modify tpm_tis_send_data and tpm_tis_recv_data to work according the TCG Device Driver Guide (optimization on TPM_STS access and send/recv retry)
>
> Optimizations are always welcome, but I'd expect basic communication to
> work already with the current code (though maybe not as efficiently as
> possible).
>
> > Besides this, during development we might encounter additional differences between SPI and I2C.
> >
> > We currently target to allocate an eng. to work on this on the second half of August with a goal to have the driver ready for the next kernel merge window.
> >
> > Regards,
> > Eyal.
Alexander Steffen Aug. 16, 2019, 4:12 p.m. UTC | #12
On 15.08.2019 19:03, Oshri Alkobi wrote:
> On Thu, Jul 18, 2019 at 8:10 PM Alexander Steffen
> <Alexander.Steffen@infineon.com> wrote:
>>
>> On 18.07.2019 14:51, Eyal.Cohen@nuvoton.com wrote:
>>> Hi Jarkko and Alexander,
>>>
>>> We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer.
>>
>> Great :) In the meantime, I've done some experiments creating an I2C
>> driver based on tpm_tis_core, see
>> https://patchwork.kernel.org/patch/11049363/ Please have a look at that
>> and provide your feedback (and/or use it as a basis for further
>> implementations).
>>
> 
> Sorry for the late response.
> 
> Thanks Alexander, indeed it looks much simpler.
> I've checked it with Nuvoton's TPM - basic TPM commands work

Nice :)

> I only
> had to remove the first msg from the read/write I2C transmitting
> (from/to TPM_LOC_SEL), the TPM couldn't handle two register writes in
> a sequence.
> Actually it is more efficient to set TPM_LOC_SEL only before locality
> check/request/relinquish - it is sticky.

There is one problem though: Do we assume that only the kernel driver 
will communicate with the TPM or might there be something else that also 
talks to the TPM?

If it is only the kernel driver, we could probably skip setting 
TPM_LOC_SEL at all, since it defaults to 0 and the driver will never use 
anything else. If something else does its own I2C communication with the 
TPM, it might write different values to TPM_LOC_SEL at any time, causing 
the kernel driver to use a different locality than intended. This was 
the reason I always set TPM_LOC_SEL within the same transaction.

> I still didn't manage to work with interrupts, will debug it.

Interrupt support might be broken in general at the moment, see this 
thread: https://www.spinics.net/lists/linux-integrity/msg08663.html

> We weren't aware to the implementation of Christophe/ST which looks
> good and can be complement to yours.
> If no one is currently working on that, we can prepare a new patch
> that is based on both.
> Please let us know.

I won't have the time to do anything, at least for the next two weeks, 
so feel free to pick it up.

>>> However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core.
>>> At a minimum we thought of:
>>> 1. Handling TPM Localities in I2C is different
>>
>> It turned out not to be that different in the end, see the code
>> mentioned above and my comment here:
>> https://patchwork.kernel.org/cover/11049365/
>>
>>> 2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core
>>
>> That is completely optional, so there is no need to implement it in the
>> beginning. Also, do you expect a huge benefit from that functionality?
>> Are bit flips that much more likely on I2C compared to SPI, which has no
>> CRC at all, but still works fine?
>>
> 
> I2C is noisy bus with potentially more devices with larger variety
> than SPI. I2C may have more than one master and may have collisions
> and/or arbitration.

If multi-master usage is a concern, there are probably a lot more places 
in the driver that need to be adapted to deal with concurrent 
access/data corruption. For now, I'd assume a single master, similar to SPI.

Alexander
Oshri Alkobi Aug. 25, 2019, 11:25 a.m. UTC | #13
On Fri, Aug 16, 2019 at 7:12 PM Alexander Steffen
<Alexander.Steffen@infineon.com> wrote:
>
> On 15.08.2019 19:03, Oshri Alkobi wrote:
> > On Thu, Jul 18, 2019 at 8:10 PM Alexander Steffen
> > <Alexander.Steffen@infineon.com> wrote:
> >>
> >> On 18.07.2019 14:51, Eyal.Cohen@nuvoton.com wrote:
> >>> Hi Jarkko and Alexander,
> >>>
> >>> We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer.
> >>
> >> Great :) In the meantime, I've done some experiments creating an I2C
> >> driver based on tpm_tis_core, see
> >> https://patchwork.kernel.org/patch/11049363/ Please have a look at that
> >> and provide your feedback (and/or use it as a basis for further
> >> implementations).
> >>
> >
> > Sorry for the late response.
> >
> > Thanks Alexander, indeed it looks much simpler.
> > I've checked it with Nuvoton's TPM - basic TPM commands work
>
> Nice :)
>
> > I only
> > had to remove the first msg from the read/write I2C transmitting
> > (from/to TPM_LOC_SEL), the TPM couldn't handle two register writes in
> > a sequence.
> > Actually it is more efficient to set TPM_LOC_SEL only before locality
> > check/request/relinquish - it is sticky.
>
> There is one problem though: Do we assume that only the kernel driver
> will communicate with the TPM or might there be something else that also
> talks to the TPM?
>
> If it is only the kernel driver, we could probably skip setting
> TPM_LOC_SEL at all, since it defaults to 0 and the driver will never use
> anything else. If something else does its own I2C communication with the
> TPM, it might write different values to TPM_LOC_SEL at any time, causing
> the kernel driver to use a different locality than intended. This was
> the reason I always set TPM_LOC_SEL within the same transaction.
>
> > I still didn't manage to work with interrupts, will debug it.
>
> Interrupt support might be broken in general at the moment, see this
> thread: https://www.spinics.net/lists/linux-integrity/msg08663.html
>
> > We weren't aware to the implementation of Christophe/ST which looks
> > good and can be complement to yours.
> > If no one is currently working on that, we can prepare a new patch
> > that is based on both.
> > Please let us know.
>
> I won't have the time to do anything, at least for the next two weeks,
> so feel free to pick it up.
>

Great, we will start working on it.

> >>> However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core.
> >>> At a minimum we thought of:
> >>> 1. Handling TPM Localities in I2C is different
> >>
> >> It turned out not to be that different in the end, see the code
> >> mentioned above and my comment here:
> >> https://patchwork.kernel.org/cover/11049365/
> >>
> >>> 2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core
> >>
> >> That is completely optional, so there is no need to implement it in the
> >> beginning. Also, do you expect a huge benefit from that functionality?
> >> Are bit flips that much more likely on I2C compared to SPI, which has no
> >> CRC at all, but still works fine?
> >>
> >
> > I2C is noisy bus with potentially more devices with larger variety
> > than SPI. I2C may have more than one master and may have collisions
> > and/or arbitration.
>
> If multi-master usage is a concern, there are probably a lot more places
> in the driver that need to be adapted to deal with concurrent
> access/data corruption. For now, I'd assume a single master, similar to SPI.
>
> Alexander

Oshri
Benoit HOUYERE Sept. 6, 2019, 12:16 p.m. UTC | #14
Hi Steffen,

We have performed test against your simple implementation. For basic test it was ok, however for stress test, your implementation does not take in account NACK and it failed.

In PTP document, in chapter7.2.2.1.2 (Register write with address NACK), it indicates : "it's a good practice to repeat the current cycle using the correct I2C device address".

In other implementation, 
https://patchwork.kernel.org/patch/8628681/

tpm_tis_i2c_read_bytes and tpm_tis_i2c_write_bytes handle NACK with a for loop.

+	for (i = 0; i < TPM_RETRY && ret < 0; i++) {
+		tpm_tis_i2c_sleep_guard_time(phy, TPM_I2C_SEND);
+		ret = i2c_master_send(phy->client, &i2c_reg, 1);
+		mod_timer(&phy->guard_timer, phy->guard_time);
+	}
+
+	if (ret < 0)
+		goto exit;
+
+	ret = -1;
+	for (i = 0; i < TPM_RETRY && ret < 0; i++) {
+		tpm_tis_i2c_sleep_guard_time(phy, TPM_I2C_RECV);
+		ret = i2c_master_recv(phy->client, result, len * size);
+		mod_timer(&phy->guard_timer, phy->guard_time);
+	}

I think that we should implement it before to include tpm_tis_i2c.c officially.

Thanks in advance,

Best Regards,

Benoit

-----Original Message-----
From: Alexander Steffen <Alexander.Steffen@infineon.com> 
Sent: mardi 30 juillet 2019 19:42
To: Benoit HOUYERE <benoit.houyere@st.com>; Eyal.Cohen@nuvoton.com; jarkko.sakkinen@linux.intel.com; tmaimon77@gmail.com
Cc: oshrialkoby85@gmail.com; robh+dt@kernel.org; mark.rutland@arm.com; peterhuewe@gmx.de; jgg@ziepe.ca; arnd@arndb.de; gregkh@linuxfoundation.org; oshri.alkoby@nuvoton.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-integrity@vger.kernel.org; gcwilson@us.ibm.com; kgoldman@us.ibm.com; nayna@linux.vnet.ibm.com; Dan.Morav@nuvoton.com; oren.tanami@nuvoton.com; Christophe Ricard (christophe.ricard@gmail.com) <christophe.ricard@gmail.com>; Elena WILLIS <elena.willis@st.com>; Olivier COLLART <olivier.collart@st.com>
Subject: Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

Hi Benoit,

good to see you're still around.

On 30.07.2019 10:39, Benoit HOUYERE wrote:
> Hi Alexander, Jarkko and Eyal,
> 
> A first I2C TCG patch (tpm_tis_i2c.c) has been proposed in the same time as tpm_tis_spi.c by Christophe 3 years ago.
> 
> https://patchwork.kernel.org/patch/8628681/

Thanks for mentioning this. I forgot it exists, since it was still on the old mailing list.

> At the time, we have had two concerns :
> 	1) I2C TPM component number, in the market, compliant with new I2C TCG specification to validate new I2C driver.
> 	2) Lots changing  was already provided by tpm_tis_spi.c on 4.8.
> 
> That's why Tpm_tis_i2c.c has been postponed.
> 
> Tpm_tis_spi Linux driver is now robust, if we have several different I2C TPM solutions today to validate a tpm_tis_i2c driver, I 'm ready to contribute to it for validation (STmicro TPM) or propose a solution compatible on 5.1 linux driver if needed under timeframe proposed (second half of august).

Could you run your tests against the simple implementation that I posted a while ago (https://patchwork.kernel.org/cover/11049365/) and provide your feedback? Since it is already based on the current tpm_tis_core, it is probably easier to integrate necessary changes there.

By the way, has it gotten any easier in the meantime to get hold of your TPMs to use them for kernel tests?

Alexander