[PATCH v2 1/1] tpm/tpm_i2c_infineon: Fix init endian vendor check

[PATCH v2 1/1] tpm/tpm_i2c_infineon: Fix init endian vendor check

[Florian Eckert]

On my embedded system I use this tpm infineon chip via i2c bus.
The system is a MIPS architecture and therefore works in big endian mode.

The problem is, that the chip type is not correctly recognized,
because the vendor ID is wrongly aligned in the memory.

By declaring the vendor ID variable as a `__le32` type, the TPM chip is
then correctly recognized by the driver and feels then responsible.

The device works than as expected.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
v2:
* use variable type instead of le32_to_cpus function call
 drivers/char/tpm/tpm_i2c_infineon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c
index a19d32cb4e94..30c320ea57fd 100644
--- a/drivers/char/tpm/tpm_i2c_infineon.c
+++ b/drivers/char/tpm/tpm_i2c_infineon.c
@@ -611,7 +611,7 @@ static const struct tpm_class_ops tpm_tis_i2c = {
 
 static int tpm_tis_i2c_init(struct device *dev)
 {
-	u32 vendor;
+	__le32 vendor;
 	int rc = 0;
 	struct tpm_chip *chip;
 
-- 
2.20.1

Re: [PATCH v2 1/1] tpm/tpm_i2c_infineon: Fix init endian vendor check

[Jason Gunthorpe]

On Mon, Sep 13, 2021 at 02:05:21PM +0200, Florian Eckert wrote:
> On my embedded system I use this tpm infineon chip via i2c bus.
> The system is a MIPS architecture and therefore works in big endian mode.
> 
> The problem is, that the chip type is not correctly recognized,
> because the vendor ID is wrongly aligned in the memory.
> 
> By declaring the vendor ID variable as a `__le32` type, the TPM chip is
> then correctly recognized by the driver and feels then responsible.
> 
> The device works than as expected.
> 
> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> ---
> v2:
> * use variable type instead of le32_to_cpus function call
>  drivers/char/tpm/tpm_i2c_infineon.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

And if you do this it need to be made sparse clean/etc

Jason

Re: [PATCH v2 1/1] tpm/tpm_i2c_infineon: Fix init endian vendor check

[Florian Eckert]

Hello Jason,

>> The device works than as expected.
>> 
>> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
>> ---
>> v2:
>> * use variable type instead of le32_to_cpus function call
>>  drivers/char/tpm/tpm_i2c_infineon.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> And if you do this it need to be made sparse clean/etc

Sorry for the stupid question, but what exactly do you mean?

-- Florian

Re: [PATCH v2 1/1] tpm/tpm_i2c_infineon: Fix init endian vendor check

[Jason Gunthorpe]

On Mon, Sep 13, 2021 at 03:46:48PM +0200, Florian Eckert wrote:
> Hello Jason,
> 
> > > The device works than as expected.
> > > 
> > > Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> > > v2:
> > > * use variable type instead of le32_to_cpus function call
> > >  drivers/char/tpm/tpm_i2c_infineon.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > And if you do this it need to be made sparse clean/etc
> 
> Sorry for the stupid question, but what exactly do you mean?

There is a tool called sparse that checks the endia notations and
verfies correctness

It will complain if you do

__le32 x

x = le32tocpu(x)

you neeed another variable to store the cpu version

Jason 

Re: [PATCH v2 1/1] tpm/tpm_i2c_infineon: Fix init endian vendor check

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5782 bytes --]

Hi Florian,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on v5.15-rc1 next-20210913]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Florian-Eckert/tpm-tpm_i2c_infineon-Fix-init-endian-vendor-check/20210913-201852
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f
config: i386-randconfig-s001-20210913 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/0day-ci/linux/commit/454ecd483731a2a7c88ae1fa6e428f3c00c1669f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Florian-Eckert/tpm-tpm_i2c_infineon-Fix-init-endian-vendor-check/20210913-201852
        git checkout 454ecd483731a2a7c88ae1fa6e428f3c00c1669f
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/char/tpm/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/char/tpm/tpm_i2c_infineon.c:641:13: sparse: sparse: restricted __le32 degrades to integer
   drivers/char/tpm/tpm_i2c_infineon.c:643:20: sparse: sparse: restricted __le32 degrades to integer
   drivers/char/tpm/tpm_i2c_infineon.c:651:9: sparse: sparse: restricted __le32 degrades to integer

vim +641 drivers/char/tpm/tpm_i2c_infineon.c

aad628c1d91a6d Peter Huewe       2012-08-07  611  
afc6d36912f3f3 Bill Pemberton    2012-11-19  612  static int tpm_tis_i2c_init(struct device *dev)
aad628c1d91a6d Peter Huewe       2012-08-07  613  {
454ecd483731a2 Florian Eckert    2021-09-13  614  	__le32 vendor;
aad628c1d91a6d Peter Huewe       2012-08-07  615  	int rc = 0;
aad628c1d91a6d Peter Huewe       2012-08-07  616  	struct tpm_chip *chip;
aad628c1d91a6d Peter Huewe       2012-08-07  617  
afb5abc262e962 Jarkko Sakkinen   2014-12-12  618  	chip = tpmm_chip_alloc(dev, &tpm_tis_i2c);
afb5abc262e962 Jarkko Sakkinen   2014-12-12  619  	if (IS_ERR(chip))
afb5abc262e962 Jarkko Sakkinen   2014-12-12  620  		return PTR_ERR(chip);
aad628c1d91a6d Peter Huewe       2012-08-07  621  
aad628c1d91a6d Peter Huewe       2012-08-07  622  	/* Default timeouts */
af782f339a5d6e Christophe Ricard 2016-03-31  623  	chip->timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
af782f339a5d6e Christophe Ricard 2016-03-31  624  	chip->timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
af782f339a5d6e Christophe Ricard 2016-03-31  625  	chip->timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
af782f339a5d6e Christophe Ricard 2016-03-31  626  	chip->timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
aad628c1d91a6d Peter Huewe       2012-08-07  627  
aad628c1d91a6d Peter Huewe       2012-08-07  628  	if (request_locality(chip, 0) != 0) {
c61c86dd6e0a80 Peter Huewe       2013-03-04  629  		dev_err(dev, "could not request locality\n");
aad628c1d91a6d Peter Huewe       2012-08-07  630  		rc = -ENODEV;
afb5abc262e962 Jarkko Sakkinen   2014-12-12  631  		goto out_err;
aad628c1d91a6d Peter Huewe       2012-08-07  632  	}
aad628c1d91a6d Peter Huewe       2012-08-07  633  
aad628c1d91a6d Peter Huewe       2012-08-07  634  	/* read four bytes from DID_VID register */
aad628c1d91a6d Peter Huewe       2012-08-07  635  	if (iic_tpm_read(TPM_DID_VID(0), (u8 *)&vendor, 4) < 0) {
c61c86dd6e0a80 Peter Huewe       2013-03-04  636  		dev_err(dev, "could not read vendor id\n");
aad628c1d91a6d Peter Huewe       2012-08-07  637  		rc = -EIO;
aad628c1d91a6d Peter Huewe       2012-08-07  638  		goto out_release;
aad628c1d91a6d Peter Huewe       2012-08-07  639  	}
aad628c1d91a6d Peter Huewe       2012-08-07  640  
c61c86dd6e0a80 Peter Huewe       2013-03-04 @641  	if (vendor == TPM_TIS_I2C_DID_VID_9645) {
c61c86dd6e0a80 Peter Huewe       2013-03-04  642  		tpm_dev.chip_type = SLB9645;
c61c86dd6e0a80 Peter Huewe       2013-03-04  643  	} else if (vendor == TPM_TIS_I2C_DID_VID_9635) {
c61c86dd6e0a80 Peter Huewe       2013-03-04  644  		tpm_dev.chip_type = SLB9635;
c61c86dd6e0a80 Peter Huewe       2013-03-04  645  	} else {
c61c86dd6e0a80 Peter Huewe       2013-03-04  646  		dev_err(dev, "vendor id did not match! ID was %08x\n", vendor);
aad628c1d91a6d Peter Huewe       2012-08-07  647  		rc = -ENODEV;
aad628c1d91a6d Peter Huewe       2012-08-07  648  		goto out_release;
aad628c1d91a6d Peter Huewe       2012-08-07  649  	}
aad628c1d91a6d Peter Huewe       2012-08-07  650  
aad628c1d91a6d Peter Huewe       2012-08-07  651  	dev_info(dev, "1.2 TPM (device-id 0x%X)\n", vendor >> 16);
aad628c1d91a6d Peter Huewe       2012-08-07  652  
aad628c1d91a6d Peter Huewe       2012-08-07  653  	tpm_dev.chip = chip;
aad628c1d91a6d Peter Huewe       2012-08-07  654  
afb5abc262e962 Jarkko Sakkinen   2014-12-12  655  	return tpm_chip_register(chip);
aad628c1d91a6d Peter Huewe       2012-08-07  656  out_release:
56671c893e0e3e Christophe Ricard 2016-03-31  657  	release_locality(chip, tpm_dev.locality, 1);
aad628c1d91a6d Peter Huewe       2012-08-07  658  	tpm_dev.client = NULL;
aad628c1d91a6d Peter Huewe       2012-08-07  659  out_err:
aad628c1d91a6d Peter Huewe       2012-08-07  660  	return rc;
aad628c1d91a6d Peter Huewe       2012-08-07  661  }
aad628c1d91a6d Peter Huewe       2012-08-07  662  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31850 bytes --]

Re: [PATCH v2 1/1] tpm/tpm_i2c_infineon: Fix init endian vendor check

[Jarkko Sakkinen]

On Mon, 2021-09-13 at 14:05 +0200, Florian Eckert wrote:
> On my embedded system I use this tpm infineon chip via i2c bus.
> The system is a MIPS architecture and therefore works in big endian mode.
> 
> The problem is, that the chip type is not correctly recognized,
> because the vendor ID is wrongly aligned in the memory.
> 
> By declaring the vendor ID variable as a `__le32` type, the TPM chip is
> then correctly recognized by the driver and feels then responsible.

Please no hyphens just normal single quotes.

You should have always in a commit message some explanation what
the patch does in imperative form, e.g. "Change type of xxx ...
because ...".

I cannot from find a variable named "vendor ID" from
tpm_tis_i2c_init(). Maybe you are referring to the variable,
of which name is "vendor"?

Finally, the commit message lacks explanation what is changed, i.e.
tpm2_tis_i2c_init() in this case.

Did you find the commit ID where this regression was introduceD?

/Jarkko