linux-kernel - Re: [PATCH v3 1/2] tpm: Add tpm_tis_i2c backend for tpm_tis

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8f0d2098-8c7f-2347-3004-bf3e422de3a3@infineon.com>
Date:   Mon, 23 May 2022 10:55:45 +0200
From:   Alexander Steffen <Alexander.Steffen@...ineon.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        <jarkko@...nel.org>, <linux-kernel@...r.kernel.org>,
        <linux-integrity@...r.kernel.org>
CC:     <peterhuewe@....de>, <jgg@...pe.ca>,
        <krzysztof.kozlowski+dt@...aro.org>,
        Johannes Holland <johannes.holland@...ineon.com>,
        Amir Mizinski <amirmizi6@...il.com>
Subject: Re: [PATCH v3 1/2] tpm: Add tpm_tis_i2c backend for tpm_tis_core

On 22.05.22 10:30, Krzysztof Kozlowski wrote:
> On 20/05/2022 19:24, Alexander Steffen wrote:
>>
>> +MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_id);
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id of_tis_i2c_match[] = {
>> +     { .compatible = "infineon,slb9673", },
>> +     { .compatible = "tcg,tpm_tis-i2c", },
> 
> Please run checkpatch on your patches. You add undocumented compatibles.

Sorry, the old infrastructure I had to do that automatically is not in 
place at the moment, so it slipped through.

> 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?



Kind regards

Alexander