[PATCH 0/1] tpm_tis_i2c: Fix -Wdeclaration-after-statement

[PATCH 0/1] tpm_tis_i2c: Fix -Wdeclaration-after-statement

[Peter Delevoryas]

Hey Joel,

I've been trying to build fby35 from https://github.com/openbmc/openbmc, and I
noticed that the TPM TIS I2C file seems to emit a warning that causes a
compilation error:

../drivers/char/tpm/tpm_tis_i2c.c: In function ‘tpm_tis_i2c_write_bytes’:
../drivers/char/tpm/tpm_tis_i2c.c:114:17: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
  114 |                 struct i2c_msg msgs[] = {
      |                 ^~~~~~

I'm sending this patch as a fix for the warning, to avoid the compilation error.

Normally I think I would send this to the upstream kernel mailing list, but
this file doesn't actually seem to be in the upstream at any point:

    drivers/char/tpm/tpm_tis_i2c.c

It looks like it was added by Nuvoton and never upstreamed successfully? Perhaps
we should get rid of it at this point? fby35 doesn't actually use the TPM TIS
I2C interface, it uses the SPI one. Bletchley doesn't use the I2C one either.
Only older FB platforms did.

Actually, doing a quick search on lore.kernel.org: It looks like maybe there's
a new version submitted by Infineon in June:

    https://lore.kernel.org/all/20220608173113.9232-1-Alexander.Steffen@infineon.com/

If fby35 wants to avoid this error, should we fix the driver, remove it from the
kernel, pull in the new driver from upstream, or maybe just disable it in the
linux-aspeed tpm2 distro feature Kconfig?

Thanks,
Peter

Peter Delevoryas (1):
  tpm_tis_i2c: Fix -Wdeclaration-after-statement

 drivers/char/tpm/tpm_tis_i2c.c | 36 ++++++++++++----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

-- 
2.37.1

[PATCH 1/1] tpm_tis_i2c: Fix -Wdeclaration-after-statement

[Peter Delevoryas]

If I try to build with CONFIG_TCG_TIS_I2C=y, I get the following
warning:

../drivers/char/tpm/tpm_tis_i2c.c: In function ‘tpm_tis_i2c_write_bytes’:
../drivers/char/tpm/tpm_tis_i2c.c:114:17: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
  114 |                 struct i2c_msg msgs[] = {
      |                 ^~~~~~

This just refactors the code slightly to avoid the warning. It shouldn't
really be any different behavior. In fact, I think the first call to
i2c_transfer() is wrong: it's sending 2 messages but only populating the
first one? That doesn't seem right. But, I'm not testing this change, so
I'll leave the behavior as-is.

Signed-off-by: Peter Delevoryas <peter@pjd.dev>
---
 drivers/char/tpm/tpm_tis_i2c.c | 36 ++++++++++++----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
index 12984a3be327..0bc8a1cea554 100644
--- a/drivers/char/tpm/tpm_tis_i2c.c
+++ b/drivers/char/tpm/tpm_tis_i2c.c
@@ -101,6 +101,7 @@ static int tpm_tis_i2c_write_bytes(struct tpm_tis_data *data, u32 addr,
 				   u16 len, const u8 *value)
 {
 	struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
+	struct i2c_msg msgs[2];
 	int ret = 0;
 	int i = 0;
 
@@ -111,14 +112,10 @@ static int tpm_tis_i2c_write_bytes(struct tpm_tis_data *data, u32 addr,
 		phy->iobuf[0] = address_to_register(addr);
 		memcpy(phy->iobuf + 1, value, len);
 
-		struct i2c_msg msgs[] = {
-			{
-				.addr = phy->i2c_client->addr,
-				.len = len + 1,
-				.buf = phy->iobuf,
-			},
-		};
-
+		memset(msgs, 0, sizeof(msgs));
+		msgs[0].addr = phy->i2c_client->addr;
+		msgs[0].len = len + 1;
+		msgs[0].buf = phy->iobuf;
 		do {
 			ret = i2c_transfer(phy->i2c_client->adapter,
 					   msgs, ARRAY_SIZE(msgs));
@@ -127,21 +124,14 @@ static int tpm_tis_i2c_write_bytes(struct tpm_tis_data *data, u32 addr,
 		} while (ret < 0 && i++ < TPM_RETRY);
 	} else {
 		u8 reg = address_to_register(addr);
-
-		struct i2c_msg msgs[] = {
-			{
-				.addr = phy->i2c_client->addr,
-				.len = sizeof(reg),
-				.buf = &reg,
-			},
-			{
-				.addr = phy->i2c_client->addr,
-				.len = len,
-				.buf = (u8 *)value,
-				.flags = I2C_M_NOSTART,
-			},
-		};
-
+		memset(msgs, 0, sizeof(msgs));
+		msgs[0].addr = phy->i2c_client->addr;
+		msgs[0].len = sizeof(reg);
+		msgs[0].buf = &reg;
+		msgs[1].addr = phy->i2c_client->addr;
+		msgs[1].len = len;
+		msgs[1].buf = (u8 *)value;
+		msgs[1].flags = I2C_M_NOSTART;
 		do {
 			ret = i2c_transfer(phy->i2c_client->adapter, msgs,
 					   ARRAY_SIZE(msgs));
-- 
2.37.1

Re: [PATCH 0/1] tpm_tis_i2c: Fix -Wdeclaration-after-statement

[Joel Stanley]

Hi Peter,

On Mon, 25 Jul 2022 at 22:51, Peter Delevoryas <peter@pjd.dev> wrote:
>
> Hey Joel,
>
> I've been trying to build fby35 from https://github.com/openbmc/openbmc, and I
> noticed that the TPM TIS I2C file seems to emit a warning that causes a
> compilation error:
>
> ../drivers/char/tpm/tpm_tis_i2c.c: In function ‘tpm_tis_i2c_write_bytes’:
> ../drivers/char/tpm/tpm_tis_i2c.c:114:17: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
>   114 |                 struct i2c_msg msgs[] = {
>       |                 ^~~~~~
>
> I'm sending this patch as a fix for the warning, to avoid the compilation error.

Thanks for trying to fix this.

>
> Normally I think I would send this to the upstream kernel mailing list, but
> this file doesn't actually seem to be in the upstream at any point:
>
>     drivers/char/tpm/tpm_tis_i2c.c
>
> It looks like it was added by Nuvoton and never upstreamed successfully? Perhaps
> we should get rid of it at this point? fby35 doesn't actually use the TPM TIS
> I2C interface, it uses the SPI one. Bletchley doesn't use the I2C one either.
> Only older FB platforms did.

The i2c driver was added for the IBM P10 platforms.

> Actually, doing a quick search on lore.kernel.org: It looks like maybe there's
> a new version submitted by Infineon in June:
>
>     https://lore.kernel.org/all/20220608173113.9232-1-Alexander.Steffen@infineon.com/
>
> If fby35 wants to avoid this error, should we fix the driver, remove it from the
> kernel, pull in the new driver from upstream, or maybe just disable it in the
> linux-aspeed tpm2 distro feature Kconfig?

The upstream maintainer has been reluctant to merge this code. Nuvoton
told me off-list they had abandoned plans to upstream it. Since then
Infineon have picked it up and made some submissions. It looks like
it's been queued for merging in v5.20.

I'd be happy moving to the latest version of the patch in the openbmc
tree. Either as a revert+new driver, or as a diff, whichever you think
makes sense. They inexplicably removed the compatible strings in the
latest submission, so we would need to add them back:

 static const struct of_device_id of_tis_i2c_match[] = {
-       { .compatible = "nuvoton,npct75x", },
-       { .compatible = "tcg,tpm-tis-i2c", },
+       { .compatible = "infineon,slb9673", },


Cheers,

Joel

Re: [PATCH 0/1] tpm_tis_i2c: Fix -Wdeclaration-after-statement

[Peter Delevoryas]

On Fri, Jul 29, 2022 at 12:21:31AM +0000, Joel Stanley wrote:
> Hi Peter,
> 
> On Mon, 25 Jul 2022 at 22:51, Peter Delevoryas <peter@pjd.dev> wrote:
> >
> > Hey Joel,
> >
> > I've been trying to build fby35 from https://github.com/openbmc/openbmc, and I
> > noticed that the TPM TIS I2C file seems to emit a warning that causes a
> > compilation error:
> >
> > ../drivers/char/tpm/tpm_tis_i2c.c: In function ‘tpm_tis_i2c_write_bytes’:
> > ../drivers/char/tpm/tpm_tis_i2c.c:114:17: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
> >   114 |                 struct i2c_msg msgs[] = {
> >       |                 ^~~~~~
> >
> > I'm sending this patch as a fix for the warning, to avoid the compilation error.
> 
> Thanks for trying to fix this.
> 
> >
> > Normally I think I would send this to the upstream kernel mailing list, but
> > this file doesn't actually seem to be in the upstream at any point:
> >
> >     drivers/char/tpm/tpm_tis_i2c.c
> >
> > It looks like it was added by Nuvoton and never upstreamed successfully? Perhaps
> > we should get rid of it at this point? fby35 doesn't actually use the TPM TIS
> > I2C interface, it uses the SPI one. Bletchley doesn't use the I2C one either.
> > Only older FB platforms did.
> 
> The i2c driver was added for the IBM P10 platforms.

I see, thanks for pointing that out, I didn't do a proper survey.

> 
> > Actually, doing a quick search on lore.kernel.org: It looks like maybe there's
> > a new version submitted by Infineon in June:
> >
> >     https://lore.kernel.org/all/20220608173113.9232-1-Alexander.Steffen@infineon.com/
> >
> > If fby35 wants to avoid this error, should we fix the driver, remove it from the
> > kernel, pull in the new driver from upstream, or maybe just disable it in the
> > linux-aspeed tpm2 distro feature Kconfig?
> 
> The upstream maintainer has been reluctant to merge this code. Nuvoton
> told me off-list they had abandoned plans to upstream it. Since then
> Infineon have picked it up and made some submissions. It looks like
> it's been queued for merging in v5.20.

Oh great, nice that Infineon did that.

> 
> I'd be happy moving to the latest version of the patch in the openbmc
> tree. Either as a revert+new driver, or as a diff, whichever you think
> makes sense. They inexplicably removed the compatible strings in the
> latest submission, so we would need to add them back:

That's great! I think revert + new driver would make most sense to me,
are you planning on just pulling the diffs from that mailing list thread
and applying the compatible string change on top of that? Let me know if
I can help somehow.

Thanks,
Peter

> 
>  static const struct of_device_id of_tis_i2c_match[] = {
> -       { .compatible = "nuvoton,npct75x", },
> -       { .compatible = "tcg,tpm-tis-i2c", },
> +       { .compatible = "infineon,slb9673", },
> 
> 
> Cheers,
> 
> Joel