| Message ID | 20210913120521.18572-1-fe@dev.tdt.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/1] tpm/tpm_i2c_infineon: Fix init endian vendor check | expand |
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
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
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
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
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
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;
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(-)