Re: [PATCH 1/3 v3] tpm: Remove duplicate code from caps_show() in tpm-sysfs.c

2019-08-30 Thread Jerry Snitselaar

On Thu Aug 29 19, Jerry Snitselaar wrote:

From: Jarkko Sakkinen 

Replace existing TPM 1.x version structs with new structs that consolidate
the common parts into a single struct so that code duplication is no longer
needed in caps_show().

Cc: Peter Huewe 
Cc: Jason Gunthorpe 
Cc: Alexey Klimov 
Signed-off-by: Jarkko Sakkinen 
Reviewed-by: Jerry Snitselaar 
Tested-by: Jerry Snitselaar 
---
drivers/char/tpm/tpm-sysfs.c | 44 ++--
drivers/char/tpm/tpm.h   | 23 ---
2 files changed, 32 insertions(+), 35 deletions(-)

diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index d9caedda075b..e0550f0cfd8f 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -217,6 +217,7 @@ static ssize_t caps_show(struct device *dev, struct 
device_attribute *attr,
 char *buf)
{
struct tpm_chip *chip = to_tpm_chip(dev);
+   struct tpm1_version *version;
ssize_t rc = 0;
char *str = buf;
cap_t cap;
@@ -232,31 +233,30 @@ static ssize_t caps_show(struct device *dev, struct 
device_attribute *attr,
str += sprintf(str, "Manufacturer: 0x%x\n",
   be32_to_cpu(cap.manufacturer_id));

-   /* Try to get a TPM version 1.2 TPM_CAP_VERSION_INFO */
-   rc = tpm1_getcap(chip, TPM_CAP_VERSION_1_2, ,
+   /* TPM 1.2 */
+   if (!tpm1_getcap(chip, TPM_CAP_VERSION_1_2, ,
 "attempting to determine the 1.2 version",
-sizeof(cap.tpm_version_1_2));
-   if (!rc) {
-   str += sprintf(str,
-  "TCG version: %d.%d\nFirmware version: %d.%d\n",
-  cap.tpm_version_1_2.Major,
-  cap.tpm_version_1_2.Minor,
-  cap.tpm_version_1_2.revMajor,
-  cap.tpm_version_1_2.revMinor);
-   } else {
-   /* Otherwise just use TPM_STRUCT_VER */
-   if (tpm1_getcap(chip, TPM_CAP_VERSION_1_1, ,
-   "attempting to determine the 1.1 version",
-   sizeof(cap.tpm_version)))
-   goto out_ops;
-   str += sprintf(str,
-  "TCG version: %d.%d\nFirmware version: %d.%d\n",
-  cap.tpm_version.Major,
-  cap.tpm_version.Minor,
-  cap.tpm_version.revMajor,
-  cap.tpm_version.revMinor);
+sizeof(cap.version2))) {
+   version = 
+   goto out_print;
}
+
+   /* TPM 1.1 */
+   if (tpm1_getcap(chip, TPM_CAP_VERSION_1_1, ,
+   "attempting to determine the 1.1 version",
+   sizeof(cap.version1))) {
+   version = 
+   goto out_ops;
+   }


Jarkko, does the following change to the above block look good? I'll
submit a v4. With the current patch it is setting version when the
tpm1_getcap fails for 1.1 instead of when it succeeds. I only have
1.2 and 2.0 devices, so I didn't hit it when testing your patch.

+   /* TPM 1.1 */
+   if (tpm1_getcap(chip, TPM_CAP_VERSION_1_1, ,
+   "attempting to determine the 1.1 version",
+   sizeof(cap.version1)))
+   goto out_ops;
+
+   version = 



+
+out_print:
+   str += sprintf(str,
+  "TCG version: %d.%d\nFirmware version: %d.%d\n",
+  version->major, version->minor,
+  version->rev_major, version->rev_minor);
+
rc = str - buf;
+
out_ops:
tpm_put_ops(chip);
return rc;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a7fea3e0ca86..a4f74dd02a35 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -186,19 +186,16 @@ structstclear_flags_t {
u8  bGlobalLock;
} __packed;

-struct tpm_version_t {
-   u8  Major;
-   u8  Minor;
-   u8  revMajor;
-   u8  revMinor;
+struct tpm1_version {
+   u8 major;
+   u8 minor;
+   u8 rev_major;
+   u8 rev_minor;
} __packed;

-struct tpm_version_1_2_t {
-   __be16  tag;
-   u8  Major;
-   u8  Minor;
-   u8  revMajor;
-   u8  revMinor;
+struct tpm1_version2 {
+   __be16 tag;
+   struct tpm1_version version;
} __packed;

struct  timeout_t {
@@ -243,8 +240,8 @@ typedef union {
struct  stclear_flags_t stclear_flags;
__u8owned;
__be32  num_pcrs;
-   struct  tpm_version_t   tpm_version;
-   struct  tpm_version_1_2_t tpm_version_1_2;
+   struct tpm1_version version1;
+   struct tpm1_version2 version2;
__be32  manufacturer_id;
struct timeout_t  timeout;
struct duration_t duration;
--
2.21.0



[PATCH 1/3 v3] tpm: Remove duplicate code from caps_show() in tpm-sysfs.c

2019-08-29 Thread Jerry Snitselaar
From: Jarkko Sakkinen 

Replace existing TPM 1.x version structs with new structs that consolidate
the common parts into a single struct so that code duplication is no longer
needed in caps_show().

Cc: Peter Huewe 
Cc: Jason Gunthorpe 
Cc: Alexey Klimov 
Signed-off-by: Jarkko Sakkinen 
Reviewed-by: Jerry Snitselaar 
Tested-by: Jerry Snitselaar 
---
 drivers/char/tpm/tpm-sysfs.c | 44 ++--
 drivers/char/tpm/tpm.h   | 23 ---
 2 files changed, 32 insertions(+), 35 deletions(-)

diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index d9caedda075b..e0550f0cfd8f 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -217,6 +217,7 @@ static ssize_t caps_show(struct device *dev, struct 
device_attribute *attr,
 char *buf)
 {
struct tpm_chip *chip = to_tpm_chip(dev);
+   struct tpm1_version *version;
ssize_t rc = 0;
char *str = buf;
cap_t cap;
@@ -232,31 +233,30 @@ static ssize_t caps_show(struct device *dev, struct 
device_attribute *attr,
str += sprintf(str, "Manufacturer: 0x%x\n",
   be32_to_cpu(cap.manufacturer_id));
 
-   /* Try to get a TPM version 1.2 TPM_CAP_VERSION_INFO */
-   rc = tpm1_getcap(chip, TPM_CAP_VERSION_1_2, ,
+   /* TPM 1.2 */
+   if (!tpm1_getcap(chip, TPM_CAP_VERSION_1_2, ,
 "attempting to determine the 1.2 version",
-sizeof(cap.tpm_version_1_2));
-   if (!rc) {
-   str += sprintf(str,
-  "TCG version: %d.%d\nFirmware version: %d.%d\n",
-  cap.tpm_version_1_2.Major,
-  cap.tpm_version_1_2.Minor,
-  cap.tpm_version_1_2.revMajor,
-  cap.tpm_version_1_2.revMinor);
-   } else {
-   /* Otherwise just use TPM_STRUCT_VER */
-   if (tpm1_getcap(chip, TPM_CAP_VERSION_1_1, ,
-   "attempting to determine the 1.1 version",
-   sizeof(cap.tpm_version)))
-   goto out_ops;
-   str += sprintf(str,
-  "TCG version: %d.%d\nFirmware version: %d.%d\n",
-  cap.tpm_version.Major,
-  cap.tpm_version.Minor,
-  cap.tpm_version.revMajor,
-  cap.tpm_version.revMinor);
+sizeof(cap.version2))) {
+   version = 
+   goto out_print;
}
+
+   /* TPM 1.1 */
+   if (tpm1_getcap(chip, TPM_CAP_VERSION_1_1, ,
+   "attempting to determine the 1.1 version",
+   sizeof(cap.version1))) {
+   version = 
+   goto out_ops;
+   }
+
+out_print:
+   str += sprintf(str,
+  "TCG version: %d.%d\nFirmware version: %d.%d\n",
+  version->major, version->minor,
+  version->rev_major, version->rev_minor);
+
rc = str - buf;
+
 out_ops:
tpm_put_ops(chip);
return rc;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a7fea3e0ca86..a4f74dd02a35 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -186,19 +186,16 @@ structstclear_flags_t {
u8  bGlobalLock;
 } __packed;
 
-struct tpm_version_t {
-   u8  Major;
-   u8  Minor;
-   u8  revMajor;
-   u8  revMinor;
+struct tpm1_version {
+   u8 major;
+   u8 minor;
+   u8 rev_major;
+   u8 rev_minor;
 } __packed;
 
-struct tpm_version_1_2_t {
-   __be16  tag;
-   u8  Major;
-   u8  Minor;
-   u8  revMajor;
-   u8  revMinor;
+struct tpm1_version2 {
+   __be16 tag;
+   struct tpm1_version version;
 } __packed;
 
 struct timeout_t {
@@ -243,8 +240,8 @@ typedef union {
struct  stclear_flags_t stclear_flags;
__u8owned;
__be32  num_pcrs;
-   struct  tpm_version_t   tpm_version;
-   struct  tpm_version_1_2_t tpm_version_1_2;
+   struct tpm1_version version1;
+   struct tpm1_version2 version2;
__be32  manufacturer_id;
struct timeout_t  timeout;
struct duration_t duration;
-- 
2.21.0