| Message ID | ae40859b82494d75e9ad7bf616b3264138ad1f6a.1695754856.git.lukas@wunner.de |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/2] dt-bindings: reset: Add Infineon SLB9670 TPM reset driver | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
On Tue, 26 Sep 2023 21:09:35 +0200, Lukas Wunner wrote: > A new reset driver is about to be added to perform the reset sequence of > the Infineon SLB9670 Trusted Platform Module. > > Document its device tree bindings. > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > --- > .../bindings/reset/infineon,slb9670-reset.yaml | 68 ++++++++++++++++++++++ > 1 file changed, 68 insertions(+) > create mode 100644 Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml:29:111: [warning] line too long (124 > 110 characters) (line-length) dtschema/dtc warnings/errors: Documentation/devicetree/bindings/reset/infineon,slb9670-reset.example.dtb: /example-0/spi/tpm@0: failed to match any schema with compatible: ['infineon,slb9670'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/ae40859b82494d75e9ad7bf616b3264138ad1f6a.1695754856.git.lukas@wunner.de The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Tue, Sep 26, 2023 at 03:37:07PM -0500, Rob Herring wrote: > On Tue, 26 Sep 2023 21:09:35 +0200, Lukas Wunner wrote: > > A new reset driver is about to be added to perform the reset sequence of > > the Infineon SLB9670 Trusted Platform Module. > > > > Document its device tree bindings. > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' > on your patch (DT_CHECKER_FLAGS is new in v5.13): > > yamllint warnings/errors: > ./Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml:29:111: [warning] line too long (124 > 110 characters) (line-length) That's the following line: [1] https://www.infineon.com/dgdl/Infineon-SLB%209670VQ2.0-DataSheet-v01_04-EN.pdf?fileId=5546d4626fc1ce0b016fc78270350cd6 I'm not sure what to do about it. Use an URL shortener? I did see the warning when running static checks before submission, but it seemed like a false positive to me. > dtschema/dtc warnings/errors: > Documentation/devicetree/bindings/reset/infineon,slb9670-reset.example.dtb: /example-0/spi/tpm@0: failed to match any schema with compatible: ['infineon,slb9670'] The TPM DT bindings in Documentation/devicetree/bindings/security/tpm/ haven't been converted to YAML yet, hence the warning/error. Is it a prerequisite that I consolidate and convert them before this patch is acceptable? Thanks, Lukas
On 26/09/2023 21:09, Lukas Wunner wrote: > A new reset driver is about to be added to perform the reset sequence of > the Infineon SLB9670 Trusted Platform Module. > > Document its device tree bindings. > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > --- > .../bindings/reset/infineon,slb9670-reset.yaml | 68 ++++++++++++++++++++++ > 1 file changed, 68 insertions(+) > create mode 100644 Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml > > diff --git a/Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml b/Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml > new file mode 100644 > index 00000000..b1e23d47 > --- /dev/null > +++ b/Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml > @@ -0,0 +1,68 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/reset/infineon,slb9670-reset.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Infineon SLB9670 TPM Reset Driver Sorry, bindings are for hardware, not for drivers. > + > +maintainers: > + - Lukas Wunner <lukas@wunner.de> > + > +description: | > + The Infineon SLB9670 Trusted Platform Module requires a specific reset > + sequence on its RST# pin which is documented in sections 5.4 and 5.5 of > + the datasheet [1]. This driver performs the reset sequence using a GPIO. Sorry, bindings are for hardware, not for drivers. I don't see the point for this binding, especially that you refer to drivers. Why it cannot be just part of other device? > + > + The sequence with minimum wait intervals is as follows: > + deassert RST# > + wait at least 60 ms > + assert RST# > + wait at least 2 usecs > + deassert RST# > + wait at least 60 ms > + assert RST# > + wait at least 2 usecs > + deassert RST# > + wait at least 60 ms before issuing the first TPM command > + > + [1] https://www.infineon.com/dgdl/Infineon-SLB%209670VQ2.0-DataSheet-v01_04-EN.pdf?fileId=5546d4626fc1ce0b016fc78270350cd6 > + > +properties: > + compatible: > + enum: > + - infineon,slb9670-reset > + > + reset-gpios: > + maxItems: 1 > + description: Reference to the GPIO connected to the RST# pin. > + > + "#reset-cells": > + const: 0 > + > +required: > + - compatible > + - reset-gpios > + - "#reset-cells" > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + tpm_reset: reset-controller { No need for label. > + compatible = "infineon,slb9670-reset"; > + #reset-cells = <0>; > + reset-gpios = <&gpio 0 GPIO_ACTIVE_LOW>; > + }; > + > + spi { > + #address-cells = <1>; > + #size-cells = <0>; > + > + tpm@0 { > + compatible = "infineon,slb9670"; Drop node, not related to the binding. We never keep consumers in binding providers. > + reg = <0>; > + resets = <&tpm_reset>; > + }; > + }; Best regards, Krzysztof
On Wed, Sep 27, 2023 at 08:31:16AM +0200, Lukas Wunner wrote: > On Tue, Sep 26, 2023 at 03:37:07PM -0500, Rob Herring wrote: > > On Tue, 26 Sep 2023 21:09:35 +0200, Lukas Wunner wrote: > > > A new reset driver is about to be added to perform the reset sequence of > > > the Infineon SLB9670 Trusted Platform Module. > > > > > > Document its device tree bindings. > > > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' > > on your patch (DT_CHECKER_FLAGS is new in v5.13): > > > > yamllint warnings/errors: > > ./Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml:29:111: [warning] line too long (124 > 110 characters) (line-length) > > That's the following line: > > [1] https://www.infineon.com/dgdl/Infineon-SLB%209670VQ2.0-DataSheet-v01_04-EN.pdf?fileId=5546d4626fc1ce0b016fc78270350cd6 > > I'm not sure what to do about it. Use an URL shortener? The link doesn't even work for me. > I did see the warning when running static checks before submission, > but it seemed like a false positive to me. I suppose we could bump the limit by 1 as I think it is set to 110. > > > dtschema/dtc warnings/errors: > > Documentation/devicetree/bindings/reset/infineon,slb9670-reset.example.dtb: /example-0/spi/tpm@0: failed to match any schema with compatible: ['infineon,slb9670'] > > The TPM DT bindings in Documentation/devicetree/bindings/security/tpm/ > haven't been converted to YAML yet, hence the warning/error. Yes, there's been multiple attempts. Everyone disappears when I say work together and consolidate the efforts. > Is it a prerequisite that I consolidate and convert them before > this patch is acceptable? Yes. Can't have warnings. Rob
Hello Lino, hello Lukas, On Tue, Sep 26, 2023 at 09:09:36PM +0200, Lukas Wunner wrote: > From: Lino Sanfilippo <l.sanfilippo@kunbus.com> > > Normally the platform firmware is responsible for taking a Trusted > Platform Module out of reset on boot and storing measurements into it. > > However if the platform firmware is incapable of doing that -- as is the > case on the Raspberry Pi -- then the onus is on the kernel to take the > TPM out of reset before trying to attach a driver to it. > > Provide a reset driver for such platforms. > > The Infineon SLB9670 TPM requires a specific reset sequence on its RST# > pin which is documented in sections 5.4 and 5.5 of the datasheet: > > https://www.infineon.com/dgdl/Infineon-SLB%209670VQ2.0-DataSheet-v01_04-EN.pdf?fileId=5546d4626fc1ce0b016fc78270350cd6 > > The sequence with minimum wait intervals is as follows: > > deassert RST# > wait at least 60 ms > assert RST# > wait at least 2 usecs > deassert RST# > wait at least 60 ms > assert RST# > wait at least 2 usecs > deassert RST# > wait at least 60 ms before issuing the first TPM command Are you really sure that this change is required? I have seen the RST# Timing diagram in the datasheet, however I wonder if a reset is required at all during power-up, for example. Not to mention that I would have expected some firmware to implement such reset timing and I was not able to find any (I looked at arm/arm64), if this is really required I the driver can work at all? Which platform firmware implements such reset sequence? Not to mention that I was able to see the driver probe succeed in a similar setup to the one you are describing in the commit message (different board, arm64, but nothing done by the platform firmware). What am I missing? Thanks, Francesco
On Wed, Nov 22, 2023 at 12:34:09AM +0100, Francesco Dolcini wrote: > Not to mention that I would have expected some firmware to implement > such reset timing and I was not able to find any (I looked at > arm/arm64), if this is really required I the driver can work at all? ^^^ ...really required how the driver... Francesco
On Wed, Nov 22, 2023 at 12:33:58AM +0100, Francesco Dolcini wrote: > On Tue, Sep 26, 2023 at 09:09:36PM +0200, Lukas Wunner wrote: > > Normally the platform firmware is responsible for taking a Trusted > > Platform Module out of reset on boot and storing measurements into it. > > > > However if the platform firmware is incapable of doing that -- as is the > > case on the Raspberry Pi -- then the onus is on the kernel to take the > > TPM out of reset before trying to attach a driver to it. > > > > Provide a reset driver for such platforms. > > > > The Infineon SLB9670 TPM requires a specific reset sequence on its RST# > > pin which is documented in sections 5.4 and 5.5 of the datasheet: > > Are you really sure that this change is required? > I have seen the RST# Timing diagram in the datasheet, however I wonder > if a reset is required at all during power-up, for example. If the RST# pin is not toggled at all upon a warm reset (reboot), the TPM will remain in whatever state it was during the previous boot. Also, the pin controller connected to RST# might be reset upon a reboot (think of a SoC internal pin controller setting all its registers to 0) and RST# might be asserted as a result. It is then necessary to take the TPM out of reset. > Not to mention that I would have expected some firmware to implement > such reset timing and I was not able to find any (I looked at > arm/arm64), if this is really required I the driver can work at all? > Which platform firmware implements such reset sequence? I can't answer how a TPM is reset by firmware on arm/arm64, you'd have to ask an FAE at ARM. Normally I'd expect firmware in ROM do that so all subsequently executed code which is mutable (EFI, bootloader, kernel) can be measured. Again, on simple platforms such as the Raspberry Pi there's no support to reset a TPM in ROM. > Not to mention that I was able to see the driver probe succeed in a > similar setup to the one you are describing in the commit message > (different board, arm64, but nothing done by the platform firmware). Hm, is the RST# pin even connected on that board? Thanks, Lukas
Hello Lukas, On Wed, Nov 22, 2023 at 12:29:49PM +0100, Lukas Wunner wrote: > On Wed, Nov 22, 2023 at 12:33:58AM +0100, Francesco Dolcini wrote: > > On Tue, Sep 26, 2023 at 09:09:36PM +0200, Lukas Wunner wrote: > > > Normally the platform firmware is responsible for taking a Trusted > > > Platform Module out of reset on boot and storing measurements into it. > > > > > > However if the platform firmware is incapable of doing that -- as is the > > > case on the Raspberry Pi -- then the onus is on the kernel to take the > > > TPM out of reset before trying to attach a driver to it. > > > > > > Provide a reset driver for such platforms. > > > > > > The Infineon SLB9670 TPM requires a specific reset sequence on its RST# > > > pin which is documented in sections 5.4 and 5.5 of the datasheet: > > > > Are you really sure that this change is required? > > I have seen the RST# Timing diagram in the datasheet, however I wonder > > if a reset is required at all during power-up, for example. > > If the RST# pin is not toggled at all upon a warm reset (reboot), > the TPM will remain in whatever state it was during the previous boot. ... > Also, the pin controller connected to RST# might be reset upon a reboot > (think of a SoC internal pin controller setting all its registers to 0) > and RST# might be asserted as a result. It is then necessary to take > the TPM out of reset. Toggled at boot is different from what you are doing here. > > Not to mention that I was able to see the driver probe succeed in a > > similar setup to the one you are describing in the commit message > > (different board, arm64, but nothing done by the platform firmware). > > Hm, is the RST# pin even connected on that board? Yes, it's connected and it is asserted/de-asserted (aka toggled) during startup from the HW reset circuit. However this is not implementing the reset sequence you are implementing here. Francesco
On Wed, Nov 22, 2023 at 04:15:18PM +0100, Francesco Dolcini wrote: > On Wed, Nov 22, 2023 at 12:29:49PM +0100, Lukas Wunner wrote: > > On Wed, Nov 22, 2023 at 12:33:58AM +0100, Francesco Dolcini wrote: > > > Not to mention that I was able to see the driver probe succeed in a > > > similar setup to the one you are describing in the commit message > > > (different board, arm64, but nothing done by the platform firmware). > > > > Hm, is the RST# pin even connected on that board? > > Yes, it's connected and it is asserted/de-asserted (aka toggled) during > startup from the HW reset circuit. However this is not implementing the > reset sequence you are implementing here. Section 4.5 of the datasheet seems to indicate that unless the sequence in Figure 3 is observed, the TPM may enter a defense mode against dictionary attacks "from which a recovery is very complex or even not possible." Simply toggling the RST# pin might therefore not be sufficient to ensure the TPM is operable. Here's the relevant section in the datasheet: "The OPTIGA TPM SLB 9670 features a sophisticated protection mechanism against dictionary attacks on TPM-based authorization data. Basically, the device counts the number of failed authorization attempts in a counter which is located in the non-volatile memory. An attacker who has physical access to the device could try to cirumvent that mechanism by resetting the device after the authorization attempt but before the updated failure counter has been written into the NVM. Certain countermeasures have been added to the OPTIGA TPM SLB 9670. In certain time windows during power-on or warm boot of the device, such reset events might influence the dictionary attack counters and trigger other security mechanisms as well. In worst case, this might trigger special security defense modes from which a recovery is very complex or even not possible. To avoid that the OPTIGA TPM SLB 9670 reaches such a security defense state, the RST# signal must not be asserted in certain time windows. After the deassertion of the RST# signal, the system should wait for a minimum time of tRSTIN before asserting RST# again (see Figure 3 and Table 11). TPM commands should only be started after tRSTIN has expired (see Figure 3 again). If a TPM command is running, RST# should not be asserted; otherwise, this might also trigger some security functions. When the TPM shall be reset, the command TPM2_Shutdown should be issued before the assertion of the RST# signal. https://www.infineon.com/dgdl/?fileId=5546d4626fc1ce0b016fc78270350cd6 Thanks, Lukas
Hello Lukas, On Thu, Nov 23, 2023 at 09:59:43AM +0100, Lukas Wunner wrote: > On Wed, Nov 22, 2023 at 04:15:18PM +0100, Francesco Dolcini wrote: > > On Wed, Nov 22, 2023 at 12:29:49PM +0100, Lukas Wunner wrote: > > > On Wed, Nov 22, 2023 at 12:33:58AM +0100, Francesco Dolcini wrote: > > > > Not to mention that I was able to see the driver probe succeed in a > > > > similar setup to the one you are describing in the commit message > > > > (different board, arm64, but nothing done by the platform firmware). > > > > > > Hm, is the RST# pin even connected on that board? > > > > Yes, it's connected and it is asserted/de-asserted (aka toggled) during > > startup from the HW reset circuit. However this is not implementing the > > reset sequence you are implementing here. > > Section 4.5 of the datasheet seems to indicate that unless the sequence > in Figure 3 is observed, the TPM may enter a defense mode against > dictionary attacks "from which a recovery is very complex or even not > possible." > > Simply toggling the RST# pin might therefore not be sufficient to ensure > the TPM is operable. I am trying to follow-up with infineon on this regard, do you already have any insight from them maybe? Maybe this procedure is relevant only when the device is in "security defense state"? Francesco
Hi Francesco, On Mon, Dec 18, 2023 at 06:34:00PM +0100, Francesco Dolcini wrote: > On Thu, Nov 23, 2023 at 09:59:43AM +0100, Lukas Wunner wrote: > > On Wed, Nov 22, 2023 at 04:15:18PM +0100, Francesco Dolcini wrote: > > > On Wed, Nov 22, 2023 at 12:29:49PM +0100, Lukas Wunner wrote: > > > > On Wed, Nov 22, 2023 at 12:33:58AM +0100, Francesco Dolcini wrote: > > > > > Not to mention that I was able to see the driver probe succeed in a > > > > > similar setup to the one you are describing in the commit message > > > > > (different board, arm64, but nothing done by the platform firmware). > > > > > > > > Hm, is the RST# pin even connected on that board? > > > > > > Yes, it's connected and it is asserted/de-asserted (aka toggled) during > > > startup from the HW reset circuit. However this is not implementing the > > > reset sequence you are implementing here. > > > > Section 4.5 of the datasheet seems to indicate that unless the sequence > > in Figure 3 is observed, the TPM may enter a defense mode against > > dictionary attacks "from which a recovery is very complex or even not > > possible." > > > > Simply toggling the RST# pin might therefore not be sufficient to ensure > > the TPM is operable. > > I am trying to follow-up with infineon on this regard, do you already > have any insight from them maybe? > > Maybe this procedure is relevant only when the device is in "security > defense state"? Sorry, I honestly don't know. A colleague has talked to an FAE at an Infineon reseller but they couldn't give a definitive answer either. I'm very interested to hear whatever you learn from Infineon. Thanks, Lukas
On 18.12.2023 18:51, Lukas Wunner wrote: > Hi Francesco, > > On Mon, Dec 18, 2023 at 06:34:00PM +0100, Francesco Dolcini wrote: >> On Thu, Nov 23, 2023 at 09:59:43AM +0100, Lukas Wunner wrote: >>> On Wed, Nov 22, 2023 at 04:15:18PM +0100, Francesco Dolcini wrote: >>>> On Wed, Nov 22, 2023 at 12:29:49PM +0100, Lukas Wunner wrote: >>>>> On Wed, Nov 22, 2023 at 12:33:58AM +0100, Francesco Dolcini wrote: >>>>>> Not to mention that I was able to see the driver probe succeed in a >>>>>> similar setup to the one you are describing in the commit message >>>>>> (different board, arm64, but nothing done by the platform firmware). >>>>> >>>>> Hm, is the RST# pin even connected on that board? >>>> >>>> Yes, it's connected and it is asserted/de-asserted (aka toggled) during >>>> startup from the HW reset circuit. However this is not implementing the >>>> reset sequence you are implementing here. >>> >>> Section 4.5 of the datasheet seems to indicate that unless the sequence >>> in Figure 3 is observed, the TPM may enter a defense mode against >>> dictionary attacks "from which a recovery is very complex or even not >>> possible." >>> >>> Simply toggling the RST# pin might therefore not be sufficient to ensure >>> the TPM is operable. >> >> I am trying to follow-up with infineon on this regard, do you already >> have any insight from them maybe? >> >> Maybe this procedure is relevant only when the device is in "security >> defense state"? > > Sorry, I honestly don't know. A colleague has talked to an FAE at an > Infineon reseller but they couldn't give a definitive answer either. > I'm very interested to hear whatever you learn from Infineon. Infineon is here :) I'm sorry, the document is a little confusing, we'll fix that in the future. What the document wants to say is this: Any time you assert RST#, the TPM will reset. But if you reset the TPM at certain points in time, you will trigger some security functions. In general, as long as it only happens occasionally, this is not a problem (you can't avoid all power outages). Only if you happen to frequently issue resets (e.g. if your reset pin is not a dedicated TPM reset pin but is also used for other things), then you should make sure to wait at least t_RSTIN between those resets (and avoid interrupting TPM command execution). So in your case, you probably don't need to do anything special: Just assert RST# once and the TPM will reset. This should work with basically any TPM, so there is no need for a dedicated SLB9670 reset driver. Alexander
diff --git a/Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml b/Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml new file mode 100644 index 00000000..b1e23d47 --- /dev/null +++ b/Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml @@ -0,0 +1,68 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/reset/infineon,slb9670-reset.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Infineon SLB9670 TPM Reset Driver + +maintainers: + - Lukas Wunner <lukas@wunner.de> + +description: | + The Infineon SLB9670 Trusted Platform Module requires a specific reset + sequence on its RST# pin which is documented in sections 5.4 and 5.5 of + the datasheet [1]. This driver performs the reset sequence using a GPIO. + + The sequence with minimum wait intervals is as follows: + deassert RST# + wait at least 60 ms + assert RST# + wait at least 2 usecs + deassert RST# + wait at least 60 ms + assert RST# + wait at least 2 usecs + deassert RST# + wait at least 60 ms before issuing the first TPM command + + [1] https://www.infineon.com/dgdl/Infineon-SLB%209670VQ2.0-DataSheet-v01_04-EN.pdf?fileId=5546d4626fc1ce0b016fc78270350cd6 + +properties: + compatible: + enum: + - infineon,slb9670-reset + + reset-gpios: + maxItems: 1 + description: Reference to the GPIO connected to the RST# pin. + + "#reset-cells": + const: 0 + +required: + - compatible + - reset-gpios + - "#reset-cells" + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + tpm_reset: reset-controller { + compatible = "infineon,slb9670-reset"; + #reset-cells = <0>; + reset-gpios = <&gpio 0 GPIO_ACTIVE_LOW>; + }; + + spi { + #address-cells = <1>; + #size-cells = <0>; + + tpm@0 { + compatible = "infineon,slb9670"; + reg = <0>; + resets = <&tpm_reset>; + }; + };
A new reset driver is about to be added to perform the reset sequence of the Infineon SLB9670 Trusted Platform Module. Document its device tree bindings. Signed-off-by: Lukas Wunner <lukas@wunner.de> --- .../bindings/reset/infineon,slb9670-reset.yaml | 68 ++++++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 Documentation/devicetree/bindings/reset/infineon,slb9670-reset.yaml