lkml.org 
[lkml]   [2022]   [May]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v3 1/2] tpm: Add tpm_tis_i2c backend for tpm_tis_core
From
On 23.05.22 11:44, Krzysztof Kozlowski wrote:
> On 23/05/2022 11:32, Krzysztof Kozlowski wrote:
>> On 23/05/2022 10:55, Alexander Steffen wrote:
>>> On 22.05.22 10:30, Krzysztof Kozlowski wrote:
>>>> Without bindings, new compatibles and properties cannot be accepted, so NAK.
>>>
>>> Could you be more specific as to what the correct solution is here?
>>> Usually, I'd just look at what the existing code does, but that is a
>>> little messy:
>>>
>>>
>>>
>>> * socionext,synquacer-tpm-mmio is documented only in
>>> Documentation/devicetree/bindings/trivial-devices.yaml
>>>
>>> * nuvoton,npct601 is documented in trivial-devices.yaml and is also
>>> mentioned in Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt
>>>
>>> * nuvoton,npct650 is only mentioned in tpm-i2c.txt, but appears nowhere
>>> in the code
>>>
>>> * infineon,tpm_i2c_infineon appears only in tpm_i2c_infineon.c, but is
>>> documented nowhere
>>>
>>> * tpm_tis_spi_main.c has all its compatibles documented in
>>> tpm_tis_spi.txt, except google,cr50, which is documented in
>>> google,cr50.txt, even though it has the same properties
>>>
>>> * tpm_tis_i2c_cr50.c uses the exact same google,cr50, even though that
>>> is explicitly documented as a device "on SPI Bus" and lists
>>> spi-max-frequency as one of its required properties, which does not make
>>> any sense for an I2C device
>>>
>>> * According to the feedback in
>>> https://patchwork.kernel.org/project/linux-integrity/patch/20220404081835.495-4-johannes.holland@infineon.com/#24801807,
>>> the text format, that is currently used everywhere in
>>> Documentation/devicetree/bindings/security/tpm/, is deprecated anyway
>>> and should be replaced by YAML
>>>
>>>
>>>
>>> So would you be okay with just adding the compatibles from tpm_tis_i2c.c
>>> to trivial-devices.yaml, so that checkpatch does not complain anymore,
>>> and leave the cleanup of the mess above for later?
>>
> To trivial-devices you should add only bindings of really trivial
> devices, which do not have any other properties, even when the bindings
> are finished. This means you describe fully the hardware and still have
> only reg+compatible.
>
> If this device fits such case - no other hardware properties than reg -
> then, feel free to document it in trivial-devices. However I am not sure
> that TPM devices are that trivial... For example tpm-i2c.txt defines
> also interrupts and label.
>
> If the device is not trivial, it should be documented in bindings,
> either dedicated or some existing ones.

Ok, let's see whether I understood that correctly:



I think, in general, TPMs are trivial devices: They sit on the I2C or
SPI bus waiting for commands, but don't do much else. They might have
TPM-specific properties, like whether they implement the 1.2 or 2.0
command set, but we don't encode those in the device tree, since the
driver tries to detect the available functionality dynamically (which
makes sense, since some TPMs can be upgraded from 1.2 to 2.0, so that is
not a static property of the device). Other properties, such as the
maximum SPI frequency, are not TPM-specific, but apply to every SPI
device and might be limited by either the SPI device itself or the SPI
controller (or the user, wishing to run at lower frequencies, for
whatever reason).



Looking at the code, there are some TPM-specific properties in use
though: There is the powered-while-suspended flag, which only the TPM
driver looks at (in drivers/char/tpm/eventlog/of.c). It is not specific
to a single TPM (a single compatible string), but can be set for all the
TPMs. Also, the linux,sml-* properties might be TPM-specific, though
they get set in arch/powerpc/kernel/prom_init.c to communicate some
information to the TPM driver. And there is lpcpd-gpios, which is only
used by st33zp24.



Now the purpose of trivial-devices.yaml is to specify a schema of valid
device definitions. It only allows the properties reg, interrupts and
spi-max-frequency in addition to the compatible strings. So, strictly
speaking, none of the TPMs should be listed there, since all the TPMs
can, in theory, use the powered-while-suspended flag, which is not
allowed by the schema. With other properties the schema does not seem to
be too strict, since it applies to both I2C and SPI devices, but allows
the spi-max-frequency property, even though that does not make sense for
I2C devices.



So the correct solution could be this: Replace all the text files in
Documentation/devicetree/bindings/security/tpm/ with a single
trivial-tpms.yaml (similar to trivial-devices.yaml) and also move all
the TPMs from trivial-devices.yaml there. This allows to properly
document the powered-while-suspended flag and other generic TPM
properties. st33zp24 should get its own YAML, to document lpcpd-gpios,
that is only used by this driver. I'm not quite sure what to do with
ibmvtpm.txt, since that seems to document several properties, that are
not referenced in the TPM driver at all but instead get used by some
scsi driver (e.g. ibm,loc-code), so I'd probably ignore it for now. What
do you think?



As for tpm_tis_i2c, if there are no other objections to its current
state, I'd add its compatible strings to trivial-devices.yaml for now,
then do the cleanup as described above later. It does not make the code
more wrong, since trivial-devices.yaml already contains some TPM
devices, that are very similar to what tpm_tis_i2c now supports (i.e.
that also don't have special properties), but allows for more time to do
the cleanup properly, without holding up tpm_tis_i2c.



Kind regards

Alexander

\
 
 \ /
  Last update: 2022-05-23 18:22    [W:0.082 / U:0.068 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site