[PATCH v5 0/7] tpm: cleanup/fixes in existing event log support

[PATCH v5 0/7] tpm: cleanup/fixes in existing event log support

[Nayna Jain]

This patch set includes the cleanup and bug fixes patches, previously
part of the "tpm: add the securityfs pseudo files support for TPM 2.0
firmware event log" patch set, in order to upstream them more quickly.

Changelog History

v5:

- Moved cleanup/fixes patches into this patch set.
- Patch "fix the race condition between event log access and chip
getting unregistered"
  - updated subject line and commit description.
  - modified fops code to use chip kref.
  - modified fops to lock inode before accessing inode private data.
  - renamed tpm_securityfs_data to tpm_chip_seqops, as it no more
    holds bios log, but associates seqops with respective chip. For
    the same reason, moved it to tpm.h
- Patch "replace or remove printk error messages"
  - cleaned up dev_dbg and used dev_info as applicable.

v4:

- Includes feedbacks from Jarkko and Jason.
- Patch "tpm: define a generic open() method for ascii & bios
measurements".
  - Fix indentation issue.
- Patch "tpm: replace the dynamically allocated bios_dir as
struct dentry array".
  - Continue to use bios_dir_count variable to use is_bad() checks and
    to maintain correct order for securityfs_remove() during teardown.
  - Reset chip->bios_dir_count in teardown() function.
- Patch "tpm: validate the event log access before tpm_bios_log_setup".
  - Retain TPM2 check which was removed in previous patch.
  - Add tpm_bios_log_setup failure handling.
  - Remove use of private data from v3 version of patch. Add a new
  member to struct tpm_chip to achieve the same purpose.
- Patch "tpm: redefine the read_log method to check for ACPI/OF 
properties sequentially".
  - Move replacement of CONFIG_TCG_IBMVTPM with CONFIG_OF to
  this patch from patch 3.
  - Replace -1 error code with -ENODEV.
- Patch "tpm: replace the of_find_node_by_name() with dev of_node
property".
  - Uses chip->dev.parent->of_node.
  - Created separate patch for cleanup of pr_err 
  messages.
- Patch "tpm: remove printk error messages".
  - New Patch.
- Patch "tpm: add the securityfs file support for TPM 2.0 event log".
  - Parses event digests using event alg_id rather than event log header
    alg_id.
  - Uses of_property_match_string to differentiate tpm/vtpm compatible
    property.
  - Adds the comment for difference in tpm/vtpm endianness.

v3:

- Includes the review feedbacks as suggested by Jason.
- Split of patches into one patch per idea.
- Generic open() method for ascii/bios measurements.
- Replacement of of **bios_dir with *bios_dir[3].
- Verifying readlog() is successful before creating securityfs entries.
- Generic readlog() to check for ACPI/OF in sequence.
- read_log_of() method now uses of_node propertry rather than
  calling find_device_by_name.
- read_log differentiates vtpm/tpm using its compatible property.
- Cleans pr_err with dev_dbg.
- Commit msgs subject line prefixed with tpm.

v2:

- Fixes issues as given in feedback by Jason.
- Adds documentation for device tree.

Jarkko Sakkinen (2):
  tpm: replace dynamically allocated bios_dir with a static array
  tpm: drop tpm1_chip_register(/unregister)

Nayna Jain (5):
  tpm: define a generic open() method for ascii & bios measurements
  tpm: fix the race condition between event log access and chip getting
    unregistered
  tpm: redefine read_log() to handle ACPI/OF at runtime
  tpm: replace of_find_node_by_name() with dev of_node property
  tpm: replace or remove printk error messages

 drivers/char/tpm/Makefile       |  14 +--
 drivers/char/tpm/tpm-chip.c     |  31 ++----
 drivers/char/tpm/tpm-sysfs.c    |   3 +
 drivers/char/tpm/tpm.h          |  14 ++-
 drivers/char/tpm/tpm_acpi.c     |  36 +++----
 drivers/char/tpm/tpm_eventlog.c | 208 ++++++++++++++++++++--------------------
 drivers/char/tpm/tpm_eventlog.h |  22 +++--
 drivers/char/tpm/tpm_of.c       |  46 ++++-----
 8 files changed, 176 insertions(+), 198 deletions(-)

-- 
2.5.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

[PATCH v5 1/7] tpm: define a generic open() method for ascii & bios measurements

[Nayna Jain]

open() method for event log ascii and binary bios measurements file
operations are very similar. This patch refactors the code into a
single open() call by passing seq_operations as i_node->private data.

Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Tested-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/char/tpm/tpm_eventlog.c | 59 +++++++++--------------------------------
 1 file changed, 13 insertions(+), 46 deletions(-)

diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index e722886..75e6644 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -7,6 +7,7 @@
  *	Stefan Berger <stefanb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
  *	Reiner Sailer <sailer-aZOuKsOsJu3MbYB6QlFGEg@public.gmane.org>
  *	Kylene Hall <kjhall-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
+ *	Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
  *
  * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
  *
@@ -318,12 +319,14 @@ static const struct seq_operations tpm_binary_b_measurments_seqops = {
 	.show = tpm_binary_bios_measurements_show,
 };
 
-static int tpm_ascii_bios_measurements_open(struct inode *inode,
+static int tpm_bios_measurements_open(struct inode *inode,
 					    struct file *file)
 {
 	int err;
 	struct tpm_bios_log *log;
 	struct seq_file *seq;
+	const struct seq_operations *seqops =
+		(const struct seq_operations *)inode->i_private;
 
 	log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL);
 	if (!log)
@@ -333,7 +336,7 @@ static int tpm_ascii_bios_measurements_open(struct inode *inode,
 		goto out_free;
 
 	/* now register seq file */
-	err = seq_open(file, &tpm_ascii_b_measurments_seqops);
+	err = seq_open(file, seqops);
 	if (!err) {
 		seq = file->private_data;
 		seq->private = log;
@@ -349,46 +352,8 @@ out_free:
 	goto out;
 }
 
-static const struct file_operations tpm_ascii_bios_measurements_ops = {
-	.open = tpm_ascii_bios_measurements_open,
-	.read = seq_read,
-	.llseek = seq_lseek,
-	.release = tpm_bios_measurements_release,
-};
-
-static int tpm_binary_bios_measurements_open(struct inode *inode,
-					     struct file *file)
-{
-	int err;
-	struct tpm_bios_log *log;
-	struct seq_file *seq;
-
-	log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL);
-	if (!log)
-		return -ENOMEM;
-
-	if ((err = read_log(log)))
-		goto out_free;
-
-	/* now register seq file */
-	err = seq_open(file, &tpm_binary_b_measurments_seqops);
-	if (!err) {
-		seq = file->private_data;
-		seq->private = log;
-	} else {
-		goto out_free;
-	}
-
-out:
-	return err;
-out_free:
-	kfree(log->bios_event_log);
-	kfree(log);
-	goto out;
-}
-
-static const struct file_operations tpm_binary_bios_measurements_ops = {
-	.open = tpm_binary_bios_measurements_open,
+static const struct file_operations tpm_bios_measurements_ops = {
+	.open = tpm_bios_measurements_open,
 	.read = seq_read,
 	.llseek = seq_lseek,
 	.release = tpm_bios_measurements_release,
@@ -413,15 +378,17 @@ struct dentry **tpm_bios_log_setup(const char *name)
 
 	bin_file =
 	    securityfs_create_file("binary_bios_measurements",
-				   S_IRUSR | S_IRGRP, tpm_dir, NULL,
-				   &tpm_binary_bios_measurements_ops);
+				   S_IRUSR | S_IRGRP, tpm_dir,
+				   (void *)&tpm_binary_b_measurments_seqops,
+				   &tpm_bios_measurements_ops);
 	if (is_bad(bin_file))
 		goto out_tpm;
 
 	ascii_file =
 	    securityfs_create_file("ascii_bios_measurements",
-				   S_IRUSR | S_IRGRP, tpm_dir, NULL,
-				   &tpm_ascii_bios_measurements_ops);
+				   S_IRUSR | S_IRGRP, tpm_dir,
+				   (void *)&tpm_ascii_b_measurments_seqops,
+				   &tpm_bios_measurements_ops);
 	if (is_bad(ascii_file))
 		goto out_bin;
 
-- 
2.5.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH v5 1/7] tpm: define a generic open() method for ascii & bios measurements

[Jarkko Sakkinen]

On Tue, Oct 18, 2016 at 08:49:39PM -0400, Nayna Jain wrote:
> open() method for event log ascii and binary bios measurements file
> operations are very similar. This patch refactors the code into a
> single open() call by passing seq_operations as i_node->private data.
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

I applied this to the master branch. It really does not make sense to
cycle this as part of this patch set anymore.

/Jarkko

> ---
>  drivers/char/tpm/tpm_eventlog.c | 59 +++++++++--------------------------------
>  1 file changed, 13 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
> index e722886..75e6644 100644
> --- a/drivers/char/tpm/tpm_eventlog.c
> +++ b/drivers/char/tpm/tpm_eventlog.c
> @@ -7,6 +7,7 @@
>   *	Stefan Berger <stefanb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>   *	Reiner Sailer <sailer-aZOuKsOsJu3MbYB6QlFGEg@public.gmane.org>
>   *	Kylene Hall <kjhall-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> + *	Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>   *
>   * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
>   *
> @@ -318,12 +319,14 @@ static const struct seq_operations tpm_binary_b_measurments_seqops = {
>  	.show = tpm_binary_bios_measurements_show,
>  };
>  
> -static int tpm_ascii_bios_measurements_open(struct inode *inode,
> +static int tpm_bios_measurements_open(struct inode *inode,
>  					    struct file *file)
>  {
>  	int err;
>  	struct tpm_bios_log *log;
>  	struct seq_file *seq;
> +	const struct seq_operations *seqops =
> +		(const struct seq_operations *)inode->i_private;
>  
>  	log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL);
>  	if (!log)
> @@ -333,7 +336,7 @@ static int tpm_ascii_bios_measurements_open(struct inode *inode,
>  		goto out_free;
>  
>  	/* now register seq file */
> -	err = seq_open(file, &tpm_ascii_b_measurments_seqops);
> +	err = seq_open(file, seqops);
>  	if (!err) {
>  		seq = file->private_data;
>  		seq->private = log;
> @@ -349,46 +352,8 @@ out_free:
>  	goto out;
>  }
>  
> -static const struct file_operations tpm_ascii_bios_measurements_ops = {
> -	.open = tpm_ascii_bios_measurements_open,
> -	.read = seq_read,
> -	.llseek = seq_lseek,
> -	.release = tpm_bios_measurements_release,
> -};
> -
> -static int tpm_binary_bios_measurements_open(struct inode *inode,
> -					     struct file *file)
> -{
> -	int err;
> -	struct tpm_bios_log *log;
> -	struct seq_file *seq;
> -
> -	log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL);
> -	if (!log)
> -		return -ENOMEM;
> -
> -	if ((err = read_log(log)))
> -		goto out_free;
> -
> -	/* now register seq file */
> -	err = seq_open(file, &tpm_binary_b_measurments_seqops);
> -	if (!err) {
> -		seq = file->private_data;
> -		seq->private = log;
> -	} else {
> -		goto out_free;
> -	}
> -
> -out:
> -	return err;
> -out_free:
> -	kfree(log->bios_event_log);
> -	kfree(log);
> -	goto out;
> -}
> -
> -static const struct file_operations tpm_binary_bios_measurements_ops = {
> -	.open = tpm_binary_bios_measurements_open,
> +static const struct file_operations tpm_bios_measurements_ops = {
> +	.open = tpm_bios_measurements_open,
>  	.read = seq_read,
>  	.llseek = seq_lseek,
>  	.release = tpm_bios_measurements_release,
> @@ -413,15 +378,17 @@ struct dentry **tpm_bios_log_setup(const char *name)
>  
>  	bin_file =
>  	    securityfs_create_file("binary_bios_measurements",
> -				   S_IRUSR | S_IRGRP, tpm_dir, NULL,
> -				   &tpm_binary_bios_measurements_ops);
> +				   S_IRUSR | S_IRGRP, tpm_dir,
> +				   (void *)&tpm_binary_b_measurments_seqops,
> +				   &tpm_bios_measurements_ops);
>  	if (is_bad(bin_file))
>  		goto out_tpm;
>  
>  	ascii_file =
>  	    securityfs_create_file("ascii_bios_measurements",
> -				   S_IRUSR | S_IRGRP, tpm_dir, NULL,
> -				   &tpm_ascii_bios_measurements_ops);
> +				   S_IRUSR | S_IRGRP, tpm_dir,
> +				   (void *)&tpm_ascii_b_measurments_seqops,
> +				   &tpm_bios_measurements_ops);
>  	if (is_bad(ascii_file))
>  		goto out_bin;
>  
> -- 
> 2.5.0
> 

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi

[PATCH v5 2/7] tpm: replace dynamically allocated bios_dir with a static array

[Nayna Jain]

From: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

This commit is based on a commit by Nayna Jain. Replaced dynamically
allocated bios_dir with a static array as the size is always constant.

Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/char/tpm/tpm-chip.c     |  9 ++++---
 drivers/char/tpm/tpm.h          |  3 ++-
 drivers/char/tpm/tpm_eventlog.c | 60 +++++++++++++++++++----------------------
 drivers/char/tpm/tpm_eventlog.h | 10 +++----
 4 files changed, 39 insertions(+), 43 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index e595013..a56b609 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -278,14 +278,16 @@ static void tpm_del_char_device(struct tpm_chip *chip)
 
 static int tpm1_chip_register(struct tpm_chip *chip)
 {
+	int rc;
+
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		return 0;
 
 	tpm_sysfs_add_device(chip);
 
-	chip->bios_dir = tpm_bios_log_setup(dev_name(&chip->dev));
+	rc = tpm_bios_log_setup(chip);
 
-	return 0;
+	return rc;
 }
 
 static void tpm1_chip_unregister(struct tpm_chip *chip)
@@ -293,8 +295,7 @@ static void tpm1_chip_unregister(struct tpm_chip *chip)
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		return;
 
-	if (chip->bios_dir)
-		tpm_bios_log_teardown(chip->bios_dir);
+	tpm_bios_log_teardown(chip);
 }
 
 static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 4d183c9..4c118a4 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -40,6 +40,7 @@ enum tpm_const {
 	TPM_BUFSIZE = 4096,
 	TPM_NUM_DEVICES = 65536,
 	TPM_RETRY = 50,		/* 5 seconds */
+	TPM_NUM_EVENT_LOG_FILES = 3,
 };
 
 enum tpm_timeout {
@@ -171,7 +172,7 @@ struct tpm_chip {
 	unsigned long duration[3]; /* jiffies */
 	bool duration_adjusted;
 
-	struct dentry **bios_dir;
+	struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES];
 
 	const struct attribute_group *groups[3];
 	unsigned int groups_cnt;
diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index 75e6644..c1c92d9 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -368,54 +368,48 @@ static int is_bad(void *p)
 	return 0;
 }
 
-struct dentry **tpm_bios_log_setup(const char *name)
+int tpm_bios_log_setup(struct tpm_chip *chip)
 {
-	struct dentry **ret = NULL, *tpm_dir, *bin_file, *ascii_file;
+	const char *name = dev_name(&chip->dev);
+	unsigned int cnt;
 
-	tpm_dir = securityfs_create_dir(name, NULL);
-	if (is_bad(tpm_dir))
-		goto out;
+	cnt = 0;
+	chip->bios_dir[cnt] =
+		securityfs_create_dir(name, NULL);
+	if (is_bad(chip->bios_dir[cnt]))
+		goto err;
+	cnt++;
 
-	bin_file =
+	chip->bios_dir[cnt] =
 	    securityfs_create_file("binary_bios_measurements",
-				   S_IRUSR | S_IRGRP, tpm_dir,
+				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
 				   (void *)&tpm_binary_b_measurments_seqops,
 				   &tpm_bios_measurements_ops);
-	if (is_bad(bin_file))
-		goto out_tpm;
+	if (is_bad(chip->bios_dir[cnt]))
+		goto err;
+	cnt++;
 
-	ascii_file =
+	chip->bios_dir[cnt] =
 	    securityfs_create_file("ascii_bios_measurements",
-				   S_IRUSR | S_IRGRP, tpm_dir,
+				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
 				   (void *)&tpm_ascii_b_measurments_seqops,
 				   &tpm_bios_measurements_ops);
-	if (is_bad(ascii_file))
-		goto out_bin;
+	if (is_bad(chip->bios_dir[cnt]))
+		goto err;
+	cnt++;
 
-	ret = kmalloc(3 * sizeof(struct dentry *), GFP_KERNEL);
-	if (!ret)
-		goto out_ascii;
-
-	ret[0] = ascii_file;
-	ret[1] = bin_file;
-	ret[2] = tpm_dir;
-
-	return ret;
+	return 0;
 
-out_ascii:
-	securityfs_remove(ascii_file);
-out_bin:
-	securityfs_remove(bin_file);
-out_tpm:
-	securityfs_remove(tpm_dir);
-out:
-	return NULL;
+err:
+	chip->bios_dir[cnt] = NULL;
+	tpm_bios_log_teardown(chip);
+	return -EIO;
 }
 
-void tpm_bios_log_teardown(struct dentry **lst)
+void tpm_bios_log_teardown(struct tpm_chip *chip)
 {
 	int i;
 
-	for (i = 0; i < 3; i++)
-		securityfs_remove(lst[i]);
+	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--)
+		securityfs_remove(chip->bios_dir[i]);
 }
diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
index 8de62b0..fd3357e 100644
--- a/drivers/char/tpm/tpm_eventlog.h
+++ b/drivers/char/tpm/tpm_eventlog.h
@@ -77,14 +77,14 @@ int read_log(struct tpm_bios_log *log);
 
 #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
 	defined(CONFIG_ACPI)
-extern struct dentry **tpm_bios_log_setup(const char *);
-extern void tpm_bios_log_teardown(struct dentry **);
+extern int tpm_bios_log_setup(struct tpm_chip *chip);
+extern void tpm_bios_log_teardown(struct tpm_chip *chip);
 #else
-static inline struct dentry **tpm_bios_log_setup(const char *name)
+static inline int tpm_bios_log_setup(struct tpm_chip *chip)
 {
-	return NULL;
+	return 0;
 }
-static inline void tpm_bios_log_teardown(struct dentry **dir)
+static inline void tpm_bios_log_teardown(struct tpm_chip *chip)
 {
 }
 #endif
-- 
2.5.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

[PATCH v5 3/7] tpm: drop tpm1_chip_register(/unregister)

[Nayna Jain]

From: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Check for TPM2 chip in tpm_sysfs_add_device, tpm_bios_log_setup and
tpm_bios_log_teardown in order to make code flow cleaner and to enable
to implement TPM 2.0 support later on. This is partially derived from
the commit by Nayna Jain with the extension that also tpm1_chip_register
is dropped.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/char/tpm/tpm-chip.c     | 31 +++++--------------------------
 drivers/char/tpm/tpm-sysfs.c    |  3 +++
 drivers/char/tpm/tpm_eventlog.c |  3 +++
 3 files changed, 11 insertions(+), 26 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index a56b609..eac1f10 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -276,28 +276,6 @@ static void tpm_del_char_device(struct tpm_chip *chip)
 	up_write(&chip->ops_sem);
 }
 
-static int tpm1_chip_register(struct tpm_chip *chip)
-{
-	int rc;
-
-	if (chip->flags & TPM_CHIP_FLAG_TPM2)
-		return 0;
-
-	tpm_sysfs_add_device(chip);
-
-	rc = tpm_bios_log_setup(chip);
-
-	return rc;
-}
-
-static void tpm1_chip_unregister(struct tpm_chip *chip)
-{
-	if (chip->flags & TPM_CHIP_FLAG_TPM2)
-		return;
-
-	tpm_bios_log_teardown(chip);
-}
-
 static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
 {
 	struct attribute **i;
@@ -364,7 +342,9 @@ int tpm_chip_register(struct tpm_chip *chip)
 			return rc;
 	}
 
-	rc = tpm1_chip_register(chip);
+	tpm_sysfs_add_device(chip);
+
+	rc = tpm_bios_log_setup(chip);
 	if (rc)
 		return rc;
 
@@ -372,7 +352,7 @@ int tpm_chip_register(struct tpm_chip *chip)
 
 	rc = tpm_add_char_device(chip);
 	if (rc) {
-		tpm1_chip_unregister(chip);
+		tpm_bios_log_teardown(chip);
 		return rc;
 	}
 
@@ -407,8 +387,7 @@ void tpm_chip_unregister(struct tpm_chip *chip)
 		return;
 
 	tpm_del_legacy_sysfs(chip);
-
-	tpm1_chip_unregister(chip);
+	tpm_bios_log_teardown(chip);
 	tpm_del_char_device(chip);
 }
 EXPORT_SYMBOL_GPL(tpm_chip_unregister);
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index a76ab4a..1eca5ec 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -284,6 +284,9 @@ static const struct attribute_group tpm_dev_group = {
 
 void tpm_sysfs_add_device(struct tpm_chip *chip)
 {
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		return;
+
 	/* The sysfs routines rely on an implicit tpm_try_get_ops, device_del
 	 * is called before ops is null'd and the sysfs core synchronizes this
 	 * removal so that no callbacks are running or can run again
diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index c1c92d9..753e92d 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -373,6 +373,9 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
 	const char *name = dev_name(&chip->dev);
 	unsigned int cnt;
 
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		return 0;
+
 	cnt = 0;
 	chip->bios_dir[cnt] =
 		securityfs_create_dir(name, NULL);
-- 
2.5.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

[PATCH v5 4/7] tpm: fix the race condition between event log access and chip getting unregistered

[Nayna Jain]

Currently, the event log file operations are not serialized with
tpm_chip_unregister(), which can possibly cause a race condition.

This patch fixes the race condition by:
 - moving read_log() from fops to chip register.
 - disallowing event log file operations when chip unregister is in
   progress.
 - guarding event log memory using chip krefs.

Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 drivers/char/tpm/tpm-chip.c     |  1 +
 drivers/char/tpm/tpm.h          | 11 ++++++
 drivers/char/tpm/tpm_acpi.c     | 12 +++++--
 drivers/char/tpm/tpm_eventlog.c | 80 ++++++++++++++++++++++++++---------------
 drivers/char/tpm/tpm_eventlog.h |  2 +-
 drivers/char/tpm/tpm_of.c       |  4 ++-
 6 files changed, 76 insertions(+), 34 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index eac1f10..813e0d7 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -127,6 +127,7 @@ static void tpm_dev_release(struct device *dev)
 	idr_remove(&dev_nums_idr, chip->dev_num);
 	mutex_unlock(&idr_lock);
 
+	kfree(chip->log.bios_event_log);
 	kfree(chip);
 }
 
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 4c118a4..bfe93e6 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -35,6 +35,8 @@
 #include <linux/cdev.h>
 #include <linux/highmem.h>
 
+#include "tpm_eventlog.h"
+
 enum tpm_const {
 	TPM_MINOR = 224,	/* officially assigned */
 	TPM_BUFSIZE = 4096,
@@ -146,6 +148,11 @@ enum tpm_chip_flags {
 	TPM_CHIP_FLAG_VIRTUAL		= BIT(3),
 };
 
+struct tpm_chip_seqops {
+	struct tpm_chip *chip;
+	const struct seq_operations *seqops;
+};
+
 struct tpm_chip {
 	struct device dev;
 	struct cdev cdev;
@@ -157,6 +164,10 @@ struct tpm_chip {
 	struct rw_semaphore ops_sem;
 	const struct tpm_class_ops *ops;
 
+	struct tpm_bios_log log;
+	struct tpm_chip_seqops bin_log_seqops;
+	struct tpm_chip_seqops ascii_log_seqops;
+
 	unsigned int flags;
 
 	int dev_num;		/* /dev/tpm# */
diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
index 565a947..4d6c2d7 100644
--- a/drivers/char/tpm/tpm_acpi.c
+++ b/drivers/char/tpm/tpm_acpi.c
@@ -45,13 +45,15 @@ struct acpi_tcpa {
 };
 
 /* read binary bios log */
-int read_log(struct tpm_bios_log *log)
+int read_log(struct tpm_chip *chip)
 {
 	struct acpi_tcpa *buff;
 	acpi_status status;
 	void __iomem *virt;
 	u64 len, start;
+	struct tpm_bios_log *log;
 
+	log = &chip->log;
 	if (log->bios_event_log != NULL) {
 		printk(KERN_ERR
 		       "%s: ERROR - Eventlog already initialized\n",
@@ -97,13 +99,17 @@ int read_log(struct tpm_bios_log *log)
 
 	virt = acpi_os_map_iomem(start, len);
 	if (!virt) {
-		kfree(log->bios_event_log);
 		printk("%s: ERROR - Unable to map memory\n", __func__);
-		return -EIO;
+		goto err;
 	}
 
 	memcpy_fromio(log->bios_event_log, virt, len);
 
 	acpi_os_unmap_iomem(virt, len);
 	return 0;
+
+err:
+	kfree(log->bios_event_log);
+	return -EIO;
+
 }
diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index 753e92d..bb142f2 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -73,7 +73,8 @@ static const char* tcpa_pc_event_id_strings[] = {
 static void *tpm_bios_measurements_start(struct seq_file *m, loff_t *pos)
 {
 	loff_t i;
-	struct tpm_bios_log *log = m->private;
+	struct tpm_chip *chip = m->private;
+	struct tpm_bios_log *log = &chip->log;
 	void *addr = log->bios_event_log;
 	void *limit = log->bios_event_log_end;
 	struct tcpa_event *event;
@@ -120,7 +121,8 @@ static void *tpm_bios_measurements_next(struct seq_file *m, void *v,
 					loff_t *pos)
 {
 	struct tcpa_event *event = v;
-	struct tpm_bios_log *log = m->private;
+	struct tpm_chip *chip = m->private;
+	struct tpm_bios_log *log = &chip->log;
 	void *limit = log->bios_event_log_end;
 	u32 converted_event_size;
 	u32 converted_event_type;
@@ -261,13 +263,10 @@ static int tpm_binary_bios_measurements_show(struct seq_file *m, void *v)
 static int tpm_bios_measurements_release(struct inode *inode,
 					 struct file *file)
 {
-	struct seq_file *seq = file->private_data;
-	struct tpm_bios_log *log = seq->private;
+	struct seq_file *seq = (struct seq_file *)file->private_data;
+	struct tpm_chip *chip = (struct tpm_chip *)seq->private;
 
-	if (log) {
-		kfree(log->bios_event_log);
-		kfree(log);
-	}
+	put_device(&chip->dev);
 
 	return seq_release(inode, file);
 }
@@ -323,36 +322,34 @@ static int tpm_bios_measurements_open(struct inode *inode,
 					    struct file *file)
 {
 	int err;
-	struct tpm_bios_log *log;
 	struct seq_file *seq;
-	const struct seq_operations *seqops =
-		(const struct seq_operations *)inode->i_private;
-
-	log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL);
-	if (!log)
-		return -ENOMEM;
-
-	if ((err = read_log(log)))
-		goto out_free;
+	struct tpm_chip_seqops *chip_seqops;
+	const struct seq_operations *seqops;
+	struct tpm_chip *chip;
+
+	inode_lock(inode);
+	if (!inode->i_private) {
+		inode_unlock(inode);
+		return -ENODEV;
+	}
+	chip_seqops = (struct tpm_chip_seqops *)inode->i_private;
+	seqops = chip_seqops->seqops;
+	chip = chip_seqops->chip;
+	get_device(&chip->dev);
 
 	/* now register seq file */
 	err = seq_open(file, seqops);
 	if (!err) {
 		seq = file->private_data;
-		seq->private = log;
-	} else {
-		goto out_free;
+		seq->private = chip;
 	}
 
-out:
+	inode_unlock(inode);
 	return err;
-out_free:
-	kfree(log->bios_event_log);
-	kfree(log);
-	goto out;
 }
 
 static const struct file_operations tpm_bios_measurements_ops = {
+	.owner = THIS_MODULE,
 	.open = tpm_bios_measurements_open,
 	.read = seq_read,
 	.llseek = seq_lseek,
@@ -372,10 +369,22 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
 {
 	const char *name = dev_name(&chip->dev);
 	unsigned int cnt;
+	int rc = 0;
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		return 0;
 
+	rc = read_log(chip);
+	/*
+	 * read_log failure means event log is not supported except for ENOMEM
+	 */
+	if (rc < 0) {
+		if (rc == -ENOMEM)
+			return rc;
+		else
+			return 0;
+	}
+
 	cnt = 0;
 	chip->bios_dir[cnt] =
 		securityfs_create_dir(name, NULL);
@@ -383,19 +392,25 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
 		goto err;
 	cnt++;
 
+	chip->bin_log_seqops.chip = chip;
+	chip->bin_log_seqops.seqops = &tpm_binary_b_measurments_seqops;
+
 	chip->bios_dir[cnt] =
 	    securityfs_create_file("binary_bios_measurements",
 				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
-				   (void *)&tpm_binary_b_measurments_seqops,
+				   (void *)&chip->bin_log_seqops,
 				   &tpm_bios_measurements_ops);
 	if (is_bad(chip->bios_dir[cnt]))
 		goto err;
 	cnt++;
 
+	chip->ascii_log_seqops.chip = chip;
+	chip->ascii_log_seqops.seqops = &tpm_ascii_b_measurments_seqops;
+
 	chip->bios_dir[cnt] =
 	    securityfs_create_file("ascii_bios_measurements",
 				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
-				   (void *)&tpm_ascii_b_measurments_seqops,
+				   (void *)&chip->ascii_log_seqops,
 				   &tpm_bios_measurements_ops);
 	if (is_bad(chip->bios_dir[cnt]))
 		goto err;
@@ -412,7 +427,14 @@ err:
 void tpm_bios_log_teardown(struct tpm_chip *chip)
 {
 	int i;
+	struct inode *inode;
 
-	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--)
+	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) {
+		inode = d_inode(chip->bios_dir[i]);
+		inode_lock(inode);
+		inode->i_private = NULL;
+		inode_unlock(inode);
 		securityfs_remove(chip->bios_dir[i]);
+	}
+
 }
diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
index fd3357e..6df2f8e 100644
--- a/drivers/char/tpm/tpm_eventlog.h
+++ b/drivers/char/tpm/tpm_eventlog.h
@@ -73,7 +73,7 @@ enum tcpa_pc_event_ids {
 	HOST_TABLE_OF_DEVICES,
 };
 
-int read_log(struct tpm_bios_log *log);
+int read_log(struct tpm_chip *chip);
 
 #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
 	defined(CONFIG_ACPI)
diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
index 570f30c..68d891a 100644
--- a/drivers/char/tpm/tpm_of.c
+++ b/drivers/char/tpm/tpm_of.c
@@ -20,12 +20,14 @@
 #include "tpm.h"
 #include "tpm_eventlog.h"
 
-int read_log(struct tpm_bios_log *log)
+int read_log(struct tpm_chip *chip)
 {
 	struct device_node *np;
 	const u32 *sizep;
 	const u64 *basep;
+	struct tpm_bios_log *log;
 
+	log = &chip->log;
 	if (log->bios_event_log != NULL) {
 		pr_err("%s: ERROR - Eventlog already initialized\n", __func__);
 		return -EFAULT;
-- 
2.5.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH v5 4/7] tpm: fix the race condition between event log access and chip getting unregistered

[Jarkko Sakkinen]

On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote:
> Currently, the event log file operations are not serialized with
> tpm_chip_unregister(), which can possibly cause a race condition.
> 
> This patch fixes the race condition by:
>  - moving read_log() from fops to chip register.
>  - disallowing event log file operations when chip unregister is in
>    progress.
>  - guarding event log memory using chip krefs.
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

I cannot promise yet that I have time to properly review this before
LPC but I'll try to find time to test this and apply to the eventlog
branch.

/Jarkko

> ---
>  drivers/char/tpm/tpm-chip.c     |  1 +
>  drivers/char/tpm/tpm.h          | 11 ++++++
>  drivers/char/tpm/tpm_acpi.c     | 12 +++++--
>  drivers/char/tpm/tpm_eventlog.c | 80 ++++++++++++++++++++++++++---------------
>  drivers/char/tpm/tpm_eventlog.h |  2 +-
>  drivers/char/tpm/tpm_of.c       |  4 ++-
>  6 files changed, 76 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index eac1f10..813e0d7 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -127,6 +127,7 @@ static void tpm_dev_release(struct device *dev)
>  	idr_remove(&dev_nums_idr, chip->dev_num);
>  	mutex_unlock(&idr_lock);
>  
> +	kfree(chip->log.bios_event_log);
>  	kfree(chip);
>  }
>  
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 4c118a4..bfe93e6 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -35,6 +35,8 @@
>  #include <linux/cdev.h>
>  #include <linux/highmem.h>
>  
> +#include "tpm_eventlog.h"
> +
>  enum tpm_const {
>  	TPM_MINOR = 224,	/* officially assigned */
>  	TPM_BUFSIZE = 4096,
> @@ -146,6 +148,11 @@ enum tpm_chip_flags {
>  	TPM_CHIP_FLAG_VIRTUAL		= BIT(3),
>  };
>  
> +struct tpm_chip_seqops {
> +	struct tpm_chip *chip;
> +	const struct seq_operations *seqops;
> +};
> +
>  struct tpm_chip {
>  	struct device dev;
>  	struct cdev cdev;
> @@ -157,6 +164,10 @@ struct tpm_chip {
>  	struct rw_semaphore ops_sem;
>  	const struct tpm_class_ops *ops;
>  
> +	struct tpm_bios_log log;
> +	struct tpm_chip_seqops bin_log_seqops;
> +	struct tpm_chip_seqops ascii_log_seqops;
> +
>  	unsigned int flags;
>  
>  	int dev_num;		/* /dev/tpm# */
> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> index 565a947..4d6c2d7 100644
> --- a/drivers/char/tpm/tpm_acpi.c
> +++ b/drivers/char/tpm/tpm_acpi.c
> @@ -45,13 +45,15 @@ struct acpi_tcpa {
>  };
>  
>  /* read binary bios log */
> -int read_log(struct tpm_bios_log *log)
> +int read_log(struct tpm_chip *chip)
>  {
>  	struct acpi_tcpa *buff;
>  	acpi_status status;
>  	void __iomem *virt;
>  	u64 len, start;
> +	struct tpm_bios_log *log;
>  
> +	log = &chip->log;
>  	if (log->bios_event_log != NULL) {
>  		printk(KERN_ERR
>  		       "%s: ERROR - Eventlog already initialized\n",
> @@ -97,13 +99,17 @@ int read_log(struct tpm_bios_log *log)
>  
>  	virt = acpi_os_map_iomem(start, len);
>  	if (!virt) {
> -		kfree(log->bios_event_log);
>  		printk("%s: ERROR - Unable to map memory\n", __func__);
> -		return -EIO;
> +		goto err;
>  	}
>  
>  	memcpy_fromio(log->bios_event_log, virt, len);
>  
>  	acpi_os_unmap_iomem(virt, len);
>  	return 0;
> +
> +err:
> +	kfree(log->bios_event_log);
> +	return -EIO;
> +
>  }
> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
> index 753e92d..bb142f2 100644
> --- a/drivers/char/tpm/tpm_eventlog.c
> +++ b/drivers/char/tpm/tpm_eventlog.c
> @@ -73,7 +73,8 @@ static const char* tcpa_pc_event_id_strings[] = {
>  static void *tpm_bios_measurements_start(struct seq_file *m, loff_t *pos)
>  {
>  	loff_t i;
> -	struct tpm_bios_log *log = m->private;
> +	struct tpm_chip *chip = m->private;
> +	struct tpm_bios_log *log = &chip->log;
>  	void *addr = log->bios_event_log;
>  	void *limit = log->bios_event_log_end;
>  	struct tcpa_event *event;
> @@ -120,7 +121,8 @@ static void *tpm_bios_measurements_next(struct seq_file *m, void *v,
>  					loff_t *pos)
>  {
>  	struct tcpa_event *event = v;
> -	struct tpm_bios_log *log = m->private;
> +	struct tpm_chip *chip = m->private;
> +	struct tpm_bios_log *log = &chip->log;
>  	void *limit = log->bios_event_log_end;
>  	u32 converted_event_size;
>  	u32 converted_event_type;
> @@ -261,13 +263,10 @@ static int tpm_binary_bios_measurements_show(struct seq_file *m, void *v)
>  static int tpm_bios_measurements_release(struct inode *inode,
>  					 struct file *file)
>  {
> -	struct seq_file *seq = file->private_data;
> -	struct tpm_bios_log *log = seq->private;
> +	struct seq_file *seq = (struct seq_file *)file->private_data;
> +	struct tpm_chip *chip = (struct tpm_chip *)seq->private;
>  
> -	if (log) {
> -		kfree(log->bios_event_log);
> -		kfree(log);
> -	}
> +	put_device(&chip->dev);
>  
>  	return seq_release(inode, file);
>  }
> @@ -323,36 +322,34 @@ static int tpm_bios_measurements_open(struct inode *inode,
>  					    struct file *file)
>  {
>  	int err;
> -	struct tpm_bios_log *log;
>  	struct seq_file *seq;
> -	const struct seq_operations *seqops =
> -		(const struct seq_operations *)inode->i_private;
> -
> -	log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL);
> -	if (!log)
> -		return -ENOMEM;
> -
> -	if ((err = read_log(log)))
> -		goto out_free;
> +	struct tpm_chip_seqops *chip_seqops;
> +	const struct seq_operations *seqops;
> +	struct tpm_chip *chip;
> +
> +	inode_lock(inode);
> +	if (!inode->i_private) {
> +		inode_unlock(inode);
> +		return -ENODEV;
> +	}
> +	chip_seqops = (struct tpm_chip_seqops *)inode->i_private;
> +	seqops = chip_seqops->seqops;
> +	chip = chip_seqops->chip;
> +	get_device(&chip->dev);
>  
>  	/* now register seq file */
>  	err = seq_open(file, seqops);
>  	if (!err) {
>  		seq = file->private_data;
> -		seq->private = log;
> -	} else {
> -		goto out_free;
> +		seq->private = chip;
>  	}
>  
> -out:
> +	inode_unlock(inode);
>  	return err;
> -out_free:
> -	kfree(log->bios_event_log);
> -	kfree(log);
> -	goto out;
>  }
>  
>  static const struct file_operations tpm_bios_measurements_ops = {
> +	.owner = THIS_MODULE,
>  	.open = tpm_bios_measurements_open,
>  	.read = seq_read,
>  	.llseek = seq_lseek,
> @@ -372,10 +369,22 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
>  {
>  	const char *name = dev_name(&chip->dev);
>  	unsigned int cnt;
> +	int rc = 0;
>  
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>  		return 0;
>  
> +	rc = read_log(chip);
> +	/*
> +	 * read_log failure means event log is not supported except for ENOMEM
> +	 */
> +	if (rc < 0) {
> +		if (rc == -ENOMEM)
> +			return rc;
> +		else
> +			return 0;
> +	}
> +
>  	cnt = 0;
>  	chip->bios_dir[cnt] =
>  		securityfs_create_dir(name, NULL);
> @@ -383,19 +392,25 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
>  		goto err;
>  	cnt++;
>  
> +	chip->bin_log_seqops.chip = chip;
> +	chip->bin_log_seqops.seqops = &tpm_binary_b_measurments_seqops;
> +
>  	chip->bios_dir[cnt] =
>  	    securityfs_create_file("binary_bios_measurements",
>  				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
> -				   (void *)&tpm_binary_b_measurments_seqops,
> +				   (void *)&chip->bin_log_seqops,
>  				   &tpm_bios_measurements_ops);
>  	if (is_bad(chip->bios_dir[cnt]))
>  		goto err;
>  	cnt++;
>  
> +	chip->ascii_log_seqops.chip = chip;
> +	chip->ascii_log_seqops.seqops = &tpm_ascii_b_measurments_seqops;
> +
>  	chip->bios_dir[cnt] =
>  	    securityfs_create_file("ascii_bios_measurements",
>  				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
> -				   (void *)&tpm_ascii_b_measurments_seqops,
> +				   (void *)&chip->ascii_log_seqops,
>  				   &tpm_bios_measurements_ops);
>  	if (is_bad(chip->bios_dir[cnt]))
>  		goto err;
> @@ -412,7 +427,14 @@ err:
>  void tpm_bios_log_teardown(struct tpm_chip *chip)
>  {
>  	int i;
> +	struct inode *inode;
>  
> -	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--)
> +	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) {
> +		inode = d_inode(chip->bios_dir[i]);
> +		inode_lock(inode);
> +		inode->i_private = NULL;
> +		inode_unlock(inode);
>  		securityfs_remove(chip->bios_dir[i]);
> +	}
> +
>  }
> diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
> index fd3357e..6df2f8e 100644
> --- a/drivers/char/tpm/tpm_eventlog.h
> +++ b/drivers/char/tpm/tpm_eventlog.h
> @@ -73,7 +73,7 @@ enum tcpa_pc_event_ids {
>  	HOST_TABLE_OF_DEVICES,
>  };
>  
> -int read_log(struct tpm_bios_log *log);
> +int read_log(struct tpm_chip *chip);
>  
>  #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
>  	defined(CONFIG_ACPI)
> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> index 570f30c..68d891a 100644
> --- a/drivers/char/tpm/tpm_of.c
> +++ b/drivers/char/tpm/tpm_of.c
> @@ -20,12 +20,14 @@
>  #include "tpm.h"
>  #include "tpm_eventlog.h"
>  
> -int read_log(struct tpm_bios_log *log)
> +int read_log(struct tpm_chip *chip)
>  {
>  	struct device_node *np;
>  	const u32 *sizep;
>  	const u64 *basep;
> +	struct tpm_bios_log *log;
>  
> +	log = &chip->log;
>  	if (log->bios_event_log != NULL) {
>  		pr_err("%s: ERROR - Eventlog already initialized\n", __func__);
>  		return -EFAULT;
> -- 
> 2.5.0
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH v5 4/7] tpm: fix the race condition between event log access and chip getting unregistered

[Jason Gunthorpe]

On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote:
> +	struct tpm_chip_seqops *chip_seqops;
> +	const struct seq_operations *seqops;
> +	struct tpm_chip *chip;
> +
> +	inode_lock(inode);
> +	if (!inode->i_private) {
> +		inode_unlock(inode);
> +		return -ENODEV;
> +	}
> +	chip_seqops = (struct tpm_chip_seqops *)inode->i_private;
> +	seqops = chip_seqops->seqops;
> +	chip = chip_seqops->chip;
> +	get_device(&chip->dev);

The inode_unlock should be here, not further below - reduces the risk of
deadlocking.

> +	rc = read_log(chip);
> +	/*
> +	 * read_log failure means event log is not supported except for ENOMEM
> +	 */
> +	if (rc < 0) {
> +		if (rc == -ENOMEM)

It would be more consistent to use -ENODEV as indicating no support,
and everything else is pass as an error

Rest looks right to me

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH v5 4/7] tpm: fix the race condition between event log access and chip getting unregistered

[Jarkko Sakkinen]

On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote:
> Currently, the event log file operations are not serialized with
> tpm_chip_unregister(), which can possibly cause a race condition.
> 
> This patch fixes the race condition by:
>  - moving read_log() from fops to chip register.

What is "chip register"? Please use exact names.

>  - disallowing event log file operations when chip unregister is in
>    progress.

Could you elaborate this sentence?

>  - guarding event log memory using chip krefs.

Could you elaborate this sentence?

Please describe how the race condition could happen and provide the
"Fixes:" line for the commit ID that caused it. Otherwise, your commit
message won't make any sense. I cannot apply this commit with this
commit message.

The commit message does not make much sense...

> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/char/tpm/tpm-chip.c     |  1 +
>  drivers/char/tpm/tpm.h          | 11 ++++++
>  drivers/char/tpm/tpm_acpi.c     | 12 +++++--
>  drivers/char/tpm/tpm_eventlog.c | 80 ++++++++++++++++++++++++++---------------
>  drivers/char/tpm/tpm_eventlog.h |  2 +-
>  drivers/char/tpm/tpm_of.c       |  4 ++-
>  6 files changed, 76 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index eac1f10..813e0d7 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -127,6 +127,7 @@ static void tpm_dev_release(struct device *dev)
>  	idr_remove(&dev_nums_idr, chip->dev_num);
>  	mutex_unlock(&idr_lock);
>  
> +	kfree(chip->log.bios_event_log);
>  	kfree(chip);
>  }
>  
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 4c118a4..bfe93e6 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -35,6 +35,8 @@
>  #include <linux/cdev.h>
>  #include <linux/highmem.h>
>  
> +#include "tpm_eventlog.h"
> +
>  enum tpm_const {
>  	TPM_MINOR = 224,	/* officially assigned */
>  	TPM_BUFSIZE = 4096,
> @@ -146,6 +148,11 @@ enum tpm_chip_flags {
>  	TPM_CHIP_FLAG_VIRTUAL		= BIT(3),
>  };
>  
> +struct tpm_chip_seqops {
> +	struct tpm_chip *chip;
> +	const struct seq_operations *seqops;
> +};
> +
>  struct tpm_chip {
>  	struct device dev;
>  	struct cdev cdev;
> @@ -157,6 +164,10 @@ struct tpm_chip {
>  	struct rw_semaphore ops_sem;
>  	const struct tpm_class_ops *ops;
>  
> +	struct tpm_bios_log log;
> +	struct tpm_chip_seqops bin_log_seqops;
> +	struct tpm_chip_seqops ascii_log_seqops;
> +
>  	unsigned int flags;
>  
>  	int dev_num;		/* /dev/tpm# */
> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> index 565a947..4d6c2d7 100644
> --- a/drivers/char/tpm/tpm_acpi.c
> +++ b/drivers/char/tpm/tpm_acpi.c
> @@ -45,13 +45,15 @@ struct acpi_tcpa {
>  };
>  
>  /* read binary bios log */
> -int read_log(struct tpm_bios_log *log)
> +int read_log(struct tpm_chip *chip)
>  {
>  	struct acpi_tcpa *buff;
>  	acpi_status status;
>  	void __iomem *virt;
>  	u64 len, start;
> +	struct tpm_bios_log *log;
>  
> +	log = &chip->log;
>  	if (log->bios_event_log != NULL) {
>  		printk(KERN_ERR
>  		       "%s: ERROR - Eventlog already initialized\n",
> @@ -97,13 +99,17 @@ int read_log(struct tpm_bios_log *log)
>  
>  	virt = acpi_os_map_iomem(start, len);
>  	if (!virt) {
> -		kfree(log->bios_event_log);
>  		printk("%s: ERROR - Unable to map memory\n", __func__);
> -		return -EIO;
> +		goto err;
>  	}
>  
>  	memcpy_fromio(log->bios_event_log, virt, len);
>  
>  	acpi_os_unmap_iomem(virt, len);
>  	return 0;
> +
> +err:
> +	kfree(log->bios_event_log);

Is this kfree() necessary?

> +	return -EIO;
> +
>  }
> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
> index 753e92d..bb142f2 100644
> --- a/drivers/char/tpm/tpm_eventlog.c
> +++ b/drivers/char/tpm/tpm_eventlog.c
> @@ -73,7 +73,8 @@ static const char* tcpa_pc_event_id_strings[] = {
>  static void *tpm_bios_measurements_start(struct seq_file *m, loff_t *pos)
>  {
>  	loff_t i;
> -	struct tpm_bios_log *log = m->private;
> +	struct tpm_chip *chip = m->private;
> +	struct tpm_bios_log *log = &chip->log;
>  	void *addr = log->bios_event_log;
>  	void *limit = log->bios_event_log_end;
>  	struct tcpa_event *event;
> @@ -120,7 +121,8 @@ static void *tpm_bios_measurements_next(struct seq_file *m, void *v,
>  					loff_t *pos)
>  {
>  	struct tcpa_event *event = v;
> -	struct tpm_bios_log *log = m->private;
> +	struct tpm_chip *chip = m->private;
> +	struct tpm_bios_log *log = &chip->log;
>  	void *limit = log->bios_event_log_end;
>  	u32 converted_event_size;
>  	u32 converted_event_type;
> @@ -261,13 +263,10 @@ static int tpm_binary_bios_measurements_show(struct seq_file *m, void *v)
>  static int tpm_bios_measurements_release(struct inode *inode,
>  					 struct file *file)
>  {
> -	struct seq_file *seq = file->private_data;
> -	struct tpm_bios_log *log = seq->private;
> +	struct seq_file *seq = (struct seq_file *)file->private_data;
> +	struct tpm_chip *chip = (struct tpm_chip *)seq->private;
>  
> -	if (log) {
> -		kfree(log->bios_event_log);
> -		kfree(log);
> -	}
> +	put_device(&chip->dev);
>  
>  	return seq_release(inode, file);
>  }
> @@ -323,36 +322,34 @@ static int tpm_bios_measurements_open(struct inode *inode,
>  					    struct file *file)
>  {
>  	int err;
> -	struct tpm_bios_log *log;
>  	struct seq_file *seq;
> -	const struct seq_operations *seqops =
> -		(const struct seq_operations *)inode->i_private;
> -
> -	log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL);
> -	if (!log)
> -		return -ENOMEM;
> -
> -	if ((err = read_log(log)))
> -		goto out_free;
> +	struct tpm_chip_seqops *chip_seqops;
> +	const struct seq_operations *seqops;
> +	struct tpm_chip *chip;
> +
> +	inode_lock(inode);
> +	if (!inode->i_private) {
> +		inode_unlock(inode);
> +		return -ENODEV;
> +	}

Why is this is done (inode_lock + setting to NULL), and if so, why
it wasn't done in 1/7?

> +	chip_seqops = (struct tpm_chip_seqops *)inode->i_private;
> +	seqops = chip_seqops->seqops;
> +	chip = chip_seqops->chip;
> +	get_device(&chip->dev);

If I recall the last time I looked at this patch (which was few weeks
ago) this was needed because securityfs did not have proper fencing.

tpm_bios_log_teardown() is called before tpm_del_char_device() but this
won't help because callbacks could be in action after
securityfs_remove().

Please put a comment here explaining why this get_device() is done so
that it is easy to backtrack when changing this part of the driver later
on.

>  	/* now register seq file */
>  	err = seq_open(file, seqops);
>  	if (!err) {
>  		seq = file->private_data;
> -		seq->private = log;
> -	} else {
> -		goto out_free;
> +		seq->private = chip;
>  	}
>  
> -out:
> +	inode_unlock(inode);
>  	return err;
> -out_free:
> -	kfree(log->bios_event_log);
> -	kfree(log);
> -	goto out;
>  }
>  
>  static const struct file_operations tpm_bios_measurements_ops = {
> +	.owner = THIS_MODULE,

Should be a separate commit event though it is a one line change.
Please split the commit into two.

>  	.open = tpm_bios_measurements_open,
>  	.read = seq_read,
>  	.llseek = seq_lseek,
> @@ -372,10 +369,22 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
>  {
>  	const char *name = dev_name(&chip->dev);
>  	unsigned int cnt;
> +	int rc = 0;
>  
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>  		return 0;
>  
> +	rc = read_log(chip);
> +	/*
> +	 * read_log failure means event log is not supported except for ENOMEM
> +	 */
> +	if (rc < 0) {
> +		if (rc == -ENOMEM)
> +			return rc;
> +		else
> +			return 0;
> +	}
> +
>  	cnt = 0;
>  	chip->bios_dir[cnt] =
>  		securityfs_create_dir(name, NULL);
> @@ -383,19 +392,25 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
>  		goto err;
>  	cnt++;
>  
> +	chip->bin_log_seqops.chip = chip;
> +	chip->bin_log_seqops.seqops = &tpm_binary_b_measurments_seqops;
> +
>  	chip->bios_dir[cnt] =
>  	    securityfs_create_file("binary_bios_measurements",
>  				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
> -				   (void *)&tpm_binary_b_measurments_seqops,
> +				   (void *)&chip->bin_log_seqops,
>  				   &tpm_bios_measurements_ops);
>  	if (is_bad(chip->bios_dir[cnt]))
>  		goto err;
>  	cnt++;
>  
> +	chip->ascii_log_seqops.chip = chip;
> +	chip->ascii_log_seqops.seqops = &tpm_ascii_b_measurments_seqops;
> +
>  	chip->bios_dir[cnt] =
>  	    securityfs_create_file("ascii_bios_measurements",
>  				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
> -				   (void *)&tpm_ascii_b_measurments_seqops,
> +				   (void *)&chip->ascii_log_seqops,
>  				   &tpm_bios_measurements_ops);
>  	if (is_bad(chip->bios_dir[cnt]))
>  		goto err;
> @@ -412,7 +427,14 @@ err:
>  void tpm_bios_log_teardown(struct tpm_chip *chip)
>  {
>  	int i;
> +	struct inode *inode;
>  
> -	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--)
> +	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) {
> +		inode = d_inode(chip->bios_dir[i]);
> +		inode_lock(inode);
> +		inode->i_private = NULL;
> +		inode_unlock(inode);

Why is this is done (inode_lock + setting to NULL), and if so, why
it wasn't done in 1/7?

>  		securityfs_remove(chip->bios_dir[i]);
> +	}
> +
>  }
> diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
> index fd3357e..6df2f8e 100644
> --- a/drivers/char/tpm/tpm_eventlog.h
> +++ b/drivers/char/tpm/tpm_eventlog.h
> @@ -73,7 +73,7 @@ enum tcpa_pc_event_ids {
>  	HOST_TABLE_OF_DEVICES,
>  };
>  
> -int read_log(struct tpm_bios_log *log);
> +int read_log(struct tpm_chip *chip);
>  
>  #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
>  	defined(CONFIG_ACPI)
> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> index 570f30c..68d891a 100644
> --- a/drivers/char/tpm/tpm_of.c
> +++ b/drivers/char/tpm/tpm_of.c
> @@ -20,12 +20,14 @@
>  #include "tpm.h"
>  #include "tpm_eventlog.h"
>  
> -int read_log(struct tpm_bios_log *log)
> +int read_log(struct tpm_chip *chip)
>  {
>  	struct device_node *np;
>  	const u32 *sizep;
>  	const u64 *basep;
> +	struct tpm_bios_log *log;
>  
> +	log = &chip->log;
>  	if (log->bios_event_log != NULL) {
>  		pr_err("%s: ERROR - Eventlog already initialized\n", __func__);
>  		return -EFAULT;
> -- 
> 2.5.0
> 

/Jarkko

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi

Re: [PATCH v5 4/7] tpm: fix the race condition between event log access and chip getting unregistered

[Jason Gunthorpe]

On Thu, Nov 03, 2016 at 11:14:10PM -0600, Jarkko Sakkinen wrote:
> On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote:
> > Currently, the event log file operations are not serialized with
> > tpm_chip_unregister(), which can possibly cause a race condition.
> > 
> > This patch fixes the race condition by:
> >  - moving read_log() from fops to chip register.
> 
> What is "chip register"? Please use exact names.
> 
> >  - disallowing event log file operations when chip unregister is in
> >    progress.
> 
> Could you elaborate this sentence?
> 
> >  - guarding event log memory using chip krefs.
> 
> Could you elaborate this sentence?
> 
> Please describe how the race condition could happen and provide the
> "Fixes:" line for the commit ID that caused it. Otherwise, your commit
> message won't make any sense. I cannot apply this commit with this
> commit message.
> 
> The commit message does not make much sense...

Lets get this moving along, it is hard to keep everything straight
over months..

Nayna: This commit message should work:

tpm: Have eventlog use the tpm_chip

Move the backing memory for the eventlog into tpm_chip and push
the tpm_chip into read_log. This optimizes read_log processing by
only doing it once and prepares things for the next patches in the
series which require the tpm_chip to locate the event log via
ACPI and OF handles instead of searching.

This is straightfoward except for the issue of passing a kref through
i_private with securityfs. Since securityfs_remove does not have any
removal fencing like sysfs we use the inode lock to safely get a
kref on the tpm_chip.


> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > index eac1f10..813e0d7 100644
> > +++ b/drivers/char/tpm/tpm-chip.c
> > @@ -127,6 +127,7 @@ static void tpm_dev_release(struct device *dev)
> >  	idr_remove(&dev_nums_idr, chip->dev_num);
> >  	mutex_unlock(&idr_lock);
> >  
> > +	kfree(chip->log.bios_event_log);
> >  	kfree(chip);
> >  }
[..]
> >  /* read binary bios log */
> > -int read_log(struct tpm_bios_log *log)
> > +int read_log(struct tpm_chip *chip)
[..]
> > +err:
> > +	kfree(log->bios_event_log);
> 
> Is this kfree() necessary?

Yes, if the log is not loaded then bios_event_log log should be null.

Nayna - there is a bug here, you have to null bios_event_log after
kfree'ing it so that tpm_dev_release does not attempt to kfree a
free'd pointer.

> > +	inode_lock(inode);
> > +	if (!inode->i_private) {
> > +		inode_unlock(inode);
> > +		return -ENODEV;
> > +	}
> 
> Why is this is done (inode_lock + setting to NULL), and if so, why
> it wasn't done in 1/7?

Patch 1 does not use memory backed by tpm_chip, this is the first
patch in the series that introduces that concept, so it is the first
place that needs to do this.

> > -	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--)
> > +	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) {
> > +		inode = d_inode(chip->bios_dir[i]);
> > +		inode_lock(inode);
> > +		inode->i_private = NULL;
> > +		inode_unlock(inode);
> 
> Why is this is done (inode_lock + setting to NULL), and if so, why
> it wasn't done in 1/7?

I'm sure I've explained this before, but again, the inode lock is
a work around for lack of removal fencing in securityfs. We null the
i_private here during remove and test for null during open under this
lock.

This design ensures that open either safely gets a kref or fails.

Holding the kref as long as the securityfs file is open by userspace
ensures there are no use-after-frees in this code.

A comment to that effect would be good.

Jason

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi

Re: [PATCH v5 4/7] tpm: fix the race condition between event log access and chip getting unregistered

[Jarkko Sakkinen]

On Mon, Nov 07, 2016 at 04:48:39PM -0700, Jason Gunthorpe wrote:
> On Thu, Nov 03, 2016 at 11:14:10PM -0600, Jarkko Sakkinen wrote:
> > On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote:
> > > Currently, the event log file operations are not serialized with
> > > tpm_chip_unregister(), which can possibly cause a race condition.
> > > 
> > > This patch fixes the race condition by:
> > >  - moving read_log() from fops to chip register.
> > 
> > What is "chip register"? Please use exact names.
> > 
> > >  - disallowing event log file operations when chip unregister is in
> > >    progress.
> > 
> > Could you elaborate this sentence?
> > 
> > >  - guarding event log memory using chip krefs.
> > 
> > Could you elaborate this sentence?
> > 
> > Please describe how the race condition could happen and provide the
> > "Fixes:" line for the commit ID that caused it. Otherwise, your commit
> > message won't make any sense. I cannot apply this commit with this
> > commit message.
> > 
> > The commit message does not make much sense...
> 
> Lets get this moving along, it is hard to keep everything straight
> over months..
> 
> Nayna: This commit message should work:
> 
> tpm: Have eventlog use the tpm_chip
> 
> Move the backing memory for the eventlog into tpm_chip and push
> the tpm_chip into read_log. This optimizes read_log processing by
> only doing it once and prepares things for the next patches in the
> series which require the tpm_chip to locate the event log via
> ACPI and OF handles instead of searching.
> 
> This is straightfoward except for the issue of passing a kref through
> i_private with securityfs. Since securityfs_remove does not have any
> removal fencing like sysfs we use the inode lock to safely get a
> kref on the tpm_chip.

Perfect. Thank you.

With this

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

No need for resend.

/Jarkko

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi

Re: [PATCH v5 4/7] tpm: fix the race condition between event log access and chip getting unregistered

[Jason Gunthorpe]

On Mon, Nov 07, 2016 at 04:26:43PM -0800, Jarkko Sakkinen wrote:
> On Mon, Nov 07, 2016 at 04:48:39PM -0700, Jason Gunthorpe wrote:
> > On Thu, Nov 03, 2016 at 11:14:10PM -0600, Jarkko Sakkinen wrote:
> > > On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote:
> > > > Currently, the event log file operations are not serialized with
> > > > tpm_chip_unregister(), which can possibly cause a race condition.
> > > > 
> > > > This patch fixes the race condition by:
> > > >  - moving read_log() from fops to chip register.
> > > 
> > > What is "chip register"? Please use exact names.
> > > 
> > > >  - disallowing event log file operations when chip unregister is in
> > > >    progress.
> > > 
> > > Could you elaborate this sentence?
> > > 
> > > >  - guarding event log memory using chip krefs.
> > > 
> > > Could you elaborate this sentence?
> > > 
> > > Please describe how the race condition could happen and provide the
> > > "Fixes:" line for the commit ID that caused it. Otherwise, your commit
> > > message won't make any sense. I cannot apply this commit with this
> > > commit message.
> > > 
> > > The commit message does not make much sense...
> > 
> > Lets get this moving along, it is hard to keep everything straight
> > over months..
> > 
> > Nayna: This commit message should work:
> > 
> > tpm: Have eventlog use the tpm_chip
> > 
> > Move the backing memory for the eventlog into tpm_chip and push
> > the tpm_chip into read_log. This optimizes read_log processing by
> > only doing it once and prepares things for the next patches in the
> > series which require the tpm_chip to locate the event log via
> > ACPI and OF handles instead of searching.
> > 
> > This is straightfoward except for the issue of passing a kref through
> > i_private with securityfs. Since securityfs_remove does not have any
> > removal fencing like sysfs we use the inode lock to safely get a
> > kref on the tpm_chip.
> 
> Perfect. Thank you.
> 
> With this
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> 
> No need for resend.

The missing null still needs to be fixed, and the inode lock needs to
be held only over get_device in open()

Jason

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi

Re: [PATCH v5 4/7] tpm: fix the race condition between event log access and chip getting unregistered

[Jarkko Sakkinen]

On Mon, Nov 07, 2016 at 05:29:43PM -0700, Jason Gunthorpe wrote:
> On Mon, Nov 07, 2016 at 04:26:43PM -0800, Jarkko Sakkinen wrote:
> > On Mon, Nov 07, 2016 at 04:48:39PM -0700, Jason Gunthorpe wrote:
> > > On Thu, Nov 03, 2016 at 11:14:10PM -0600, Jarkko Sakkinen wrote:
> > > > On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote:
> > > > > Currently, the event log file operations are not serialized with
> > > > > tpm_chip_unregister(), which can possibly cause a race condition.
> > > > > 
> > > > > This patch fixes the race condition by:
> > > > >  - moving read_log() from fops to chip register.
> > > > 
> > > > What is "chip register"? Please use exact names.
> > > > 
> > > > >  - disallowing event log file operations when chip unregister is in
> > > > >    progress.
> > > > 
> > > > Could you elaborate this sentence?
> > > > 
> > > > >  - guarding event log memory using chip krefs.
> > > > 
> > > > Could you elaborate this sentence?
> > > > 
> > > > Please describe how the race condition could happen and provide the
> > > > "Fixes:" line for the commit ID that caused it. Otherwise, your commit
> > > > message won't make any sense. I cannot apply this commit with this
> > > > commit message.
> > > > 
> > > > The commit message does not make much sense...
> > > 
> > > Lets get this moving along, it is hard to keep everything straight
> > > over months..
> > > 
> > > Nayna: This commit message should work:
> > > 
> > > tpm: Have eventlog use the tpm_chip
> > > 
> > > Move the backing memory for the eventlog into tpm_chip and push
> > > the tpm_chip into read_log. This optimizes read_log processing by
> > > only doing it once and prepares things for the next patches in the
> > > series which require the tpm_chip to locate the event log via
> > > ACPI and OF handles instead of searching.
> > > 
> > > This is straightfoward except for the issue of passing a kref through
> > > i_private with securityfs. Since securityfs_remove does not have any
> > > removal fencing like sysfs we use the inode lock to safely get a
> > > kref on the tpm_chip.
> > 
> > Perfect. Thank you.
> > 
> > With this
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > 
> > No need for resend.
> 
> The missing null still needs to be fixed, and the inode lock needs to
> be held only over get_device in open()

Right, I'm sorry I forgot that... Another reason for one more resend.

/Jarkko

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi

Re: [PATCH v5 4/7] tpm: fix the race condition between event log access and chip getting unregistered

[Jarkko Sakkinen]

On Mon, Nov 07, 2016 at 04:26:42PM -0800, Jarkko Sakkinen wrote:
> On Mon, Nov 07, 2016 at 04:48:39PM -0700, Jason Gunthorpe wrote:
> > On Thu, Nov 03, 2016 at 11:14:10PM -0600, Jarkko Sakkinen wrote:
> > > On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote:
> > > > Currently, the event log file operations are not serialized with
> > > > tpm_chip_unregister(), which can possibly cause a race condition.
> > > > 
> > > > This patch fixes the race condition by:
> > > >  - moving read_log() from fops to chip register.
> > > 
> > > What is "chip register"? Please use exact names.
> > > 
> > > >  - disallowing event log file operations when chip unregister is in
> > > >    progress.
> > > 
> > > Could you elaborate this sentence?
> > > 
> > > >  - guarding event log memory using chip krefs.
> > > 
> > > Could you elaborate this sentence?
> > > 
> > > Please describe how the race condition could happen and provide the
> > > "Fixes:" line for the commit ID that caused it. Otherwise, your commit
> > > message won't make any sense. I cannot apply this commit with this
> > > commit message.
> > > 
> > > The commit message does not make much sense...
> > 
> > Lets get this moving along, it is hard to keep everything straight
> > over months..
> > 
> > Nayna: This commit message should work:
> > 
> > tpm: Have eventlog use the tpm_chip
> > 
> > Move the backing memory for the eventlog into tpm_chip and push
> > the tpm_chip into read_log. This optimizes read_log processing by
> > only doing it once and prepares things for the next patches in the
> > series which require the tpm_chip to locate the event log via
> > ACPI and OF handles instead of searching.
> > 
> > This is straightfoward except for the issue of passing a kref through
> > i_private with securityfs. Since securityfs_remove does not have any
> > removal fencing like sysfs we use the inode lock to safely get a
> > kref on the tpm_chip.
> 
> Perfect. Thank you.
> 
> With this
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> 
> No need for resend.

There actually *is* need for one more resend because the patces were not
CC'd to linux-security-module. I discussed about this at LPC with James
Morris and his take was that *major* changes should be CC'd to
linux-security-module. This is such a major set of changes.

If this series is sent one more time with CC to linux-kernel and
linux-security-module and the commit message is changed for this
commit from my part it is ready for to be applied. You can add my
Reviewed-by to this patch.

/Jarkko

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi

[PATCH v5 5/7] tpm: redefine read_log() to handle ACPI/OF at runtime

[Nayna Jain]

Currently, read_log() has two implementations: one for ACPI platforms
and the other for device tree(OF) based platforms. The proper one is
selected at compile time using Kconfig and #ifdef in the Makefile,
which is not the recommended approach.

This patch removes the #ifdef in the Makefile by defining a single
read_log() method, which checks for ACPI/OF event log properties at
runtime.

Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 drivers/char/tpm/Makefile       | 14 ++++----------
 drivers/char/tpm/tpm_acpi.c     |  9 ++-------
 drivers/char/tpm/tpm_eventlog.c | 18 ++++++++++++++++++
 drivers/char/tpm/tpm_eventlog.h | 22 +++++++++++++---------
 drivers/char/tpm/tpm_of.c       |  8 ++------
 5 files changed, 39 insertions(+), 32 deletions(-)

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index a385fb8..a05b1eb 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -2,16 +2,10 @@
 # Makefile for the kernel tpm device drivers.
 #
 obj-$(CONFIG_TCG_TPM) += tpm.o
-tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o
-tpm-$(CONFIG_ACPI) += tpm_ppi.o
-
-ifdef CONFIG_ACPI
-	tpm-y += tpm_eventlog.o tpm_acpi.o
-else
-ifdef CONFIG_TCG_IBMVTPM
-	tpm-y += tpm_eventlog.o tpm_of.o
-endif
-endif
+tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \
+		tpm_eventlog.o
+tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_acpi.o
+tpm-$(CONFIG_OF) += tpm_of.o
 obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
 obj-$(CONFIG_TCG_TIS) += tpm_tis.o
 obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
index 4d6c2d7..859bdba 100644
--- a/drivers/char/tpm/tpm_acpi.c
+++ b/drivers/char/tpm/tpm_acpi.c
@@ -6,6 +6,7 @@
  *	Stefan Berger <stefanb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
  *	Reiner Sailer <sailer-aZOuKsOsJu3MbYB6QlFGEg@public.gmane.org>
  *	Kylene Hall <kjhall-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
+ *	Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
  *
  * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
  *
@@ -45,7 +46,7 @@ struct acpi_tcpa {
 };
 
 /* read binary bios log */
-int read_log(struct tpm_chip *chip)
+int read_log_acpi(struct tpm_chip *chip)
 {
 	struct acpi_tcpa *buff;
 	acpi_status status;
@@ -54,12 +55,6 @@ int read_log(struct tpm_chip *chip)
 	struct tpm_bios_log *log;
 
 	log = &chip->log;
-	if (log->bios_event_log != NULL) {
-		printk(KERN_ERR
-		       "%s: ERROR - Eventlog already initialized\n",
-		       __func__);
-		return -EFAULT;
-	}
 
 	/* Find TCPA entry in RSDT (ACPI_LOGICAL_ADDRESSING) */
 	status = acpi_get_table(ACPI_SIG_TCPA, 1,
diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index bb142f2..b60c028 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -365,6 +365,24 @@ static int is_bad(void *p)
 	return 0;
 }
 
+int read_log(struct tpm_chip *chip)
+{
+	int rc;
+
+	if (chip->log.bios_event_log != NULL) {
+		dev_dbg(&chip->dev, "%s: ERROR - Eventlog already initialized\n",
+			__func__);
+		return -EFAULT;
+	}
+
+	rc = read_log_acpi(chip);
+	if ((rc == 0) || (rc == -ENOMEM))
+		return rc;
+	rc = read_log_of(chip);
+	return rc;
+
+}
+
 int tpm_bios_log_setup(struct tpm_chip *chip)
 {
 	const char *name = dev_name(&chip->dev);
diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
index 6df2f8e..be529ad 100644
--- a/drivers/char/tpm/tpm_eventlog.h
+++ b/drivers/char/tpm/tpm_eventlog.h
@@ -73,20 +73,24 @@ enum tcpa_pc_event_ids {
 	HOST_TABLE_OF_DEVICES,
 };
 
-int read_log(struct tpm_chip *chip);
-
-#if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
-	defined(CONFIG_ACPI)
-extern int tpm_bios_log_setup(struct tpm_chip *chip);
-extern void tpm_bios_log_teardown(struct tpm_chip *chip);
+#if defined(CONFIG_ACPI)
+int read_log_acpi(struct tpm_chip *chip);
 #else
-static inline int tpm_bios_log_setup(struct tpm_chip *chip)
+static inline int read_log_acpi(struct tpm_chip *chip)
 {
-	return 0;
+	return -ENODEV;
 }
-static inline void tpm_bios_log_teardown(struct tpm_chip *chip)
+#endif
+#if defined(CONFIG_OF)
+int read_log_of(struct tpm_chip *chip);
+#else
+static inline int read_log_of(struct tpm_chip *chip)
 {
+	return -ENODEV;
 }
 #endif
 
+int tpm_bios_log_setup(struct tpm_chip *chip);
+void tpm_bios_log_teardown(struct tpm_chip *chip);
+
 #endif
diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
index 68d891a..7c30752 100644
--- a/drivers/char/tpm/tpm_of.c
+++ b/drivers/char/tpm/tpm_of.c
@@ -2,6 +2,7 @@
  * Copyright 2012 IBM Corporation
  *
  * Author: Ashley Lai <ashleydlai-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ *         Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
  *
  * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
  *
@@ -20,7 +21,7 @@
 #include "tpm.h"
 #include "tpm_eventlog.h"
 
-int read_log(struct tpm_chip *chip)
+int read_log_of(struct tpm_chip *chip)
 {
 	struct device_node *np;
 	const u32 *sizep;
@@ -28,11 +29,6 @@ int read_log(struct tpm_chip *chip)
 	struct tpm_bios_log *log;
 
 	log = &chip->log;
-	if (log->bios_event_log != NULL) {
-		pr_err("%s: ERROR - Eventlog already initialized\n", __func__);
-		return -EFAULT;
-	}
-
 	np = of_find_node_by_name(NULL, "vtpm");
 	if (!np) {
 		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
-- 
2.5.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH v5 5/7] tpm: redefine read_log() to handle ACPI/OF at runtime

[Jarkko Sakkinen]

On Tue, Oct 18, 2016 at 08:49:43PM -0400, Nayna Jain wrote:
> Currently, read_log() has two implementations: one for ACPI platforms
> and the other for device tree(OF) based platforms. The proper one is
> selected at compile time using Kconfig and #ifdef in the Makefile,
> which is not the recommended approach.
> 
> This patch removes the #ifdef in the Makefile by defining a single
> read_log() method, which checks for ACPI/OF event log properties at
> runtime.
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>

Do we have situation where we need a compiled kernel image that is
both DT and ACPI enabled?

/Jarkko

> ---
>  drivers/char/tpm/Makefile       | 14 ++++----------
>  drivers/char/tpm/tpm_acpi.c     |  9 ++-------
>  drivers/char/tpm/tpm_eventlog.c | 18 ++++++++++++++++++
>  drivers/char/tpm/tpm_eventlog.h | 22 +++++++++++++---------
>  drivers/char/tpm/tpm_of.c       |  8 ++------
>  5 files changed, 39 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index a385fb8..a05b1eb 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -2,16 +2,10 @@
>  # Makefile for the kernel tpm device drivers.
>  #
>  obj-$(CONFIG_TCG_TPM) += tpm.o
> -tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o
> -tpm-$(CONFIG_ACPI) += tpm_ppi.o
> -
> -ifdef CONFIG_ACPI
> -	tpm-y += tpm_eventlog.o tpm_acpi.o
> -else
> -ifdef CONFIG_TCG_IBMVTPM
> -	tpm-y += tpm_eventlog.o tpm_of.o
> -endif
> -endif
> +tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \
> +		tpm_eventlog.o
> +tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_acpi.o
> +tpm-$(CONFIG_OF) += tpm_of.o
>  obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
>  obj-$(CONFIG_TCG_TIS) += tpm_tis.o
>  obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> index 4d6c2d7..859bdba 100644
> --- a/drivers/char/tpm/tpm_acpi.c
> +++ b/drivers/char/tpm/tpm_acpi.c
> @@ -6,6 +6,7 @@
>   *	Stefan Berger <stefanb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>   *	Reiner Sailer <sailer-aZOuKsOsJu3MbYB6QlFGEg@public.gmane.org>
>   *	Kylene Hall <kjhall-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> + *	Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>   *
>   * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
>   *
> @@ -45,7 +46,7 @@ struct acpi_tcpa {
>  };
>  
>  /* read binary bios log */
> -int read_log(struct tpm_chip *chip)
> +int read_log_acpi(struct tpm_chip *chip)
>  {
>  	struct acpi_tcpa *buff;
>  	acpi_status status;
> @@ -54,12 +55,6 @@ int read_log(struct tpm_chip *chip)
>  	struct tpm_bios_log *log;
>  
>  	log = &chip->log;
> -	if (log->bios_event_log != NULL) {
> -		printk(KERN_ERR
> -		       "%s: ERROR - Eventlog already initialized\n",
> -		       __func__);
> -		return -EFAULT;
> -	}
>  
>  	/* Find TCPA entry in RSDT (ACPI_LOGICAL_ADDRESSING) */
>  	status = acpi_get_table(ACPI_SIG_TCPA, 1,
> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
> index bb142f2..b60c028 100644
> --- a/drivers/char/tpm/tpm_eventlog.c
> +++ b/drivers/char/tpm/tpm_eventlog.c
> @@ -365,6 +365,24 @@ static int is_bad(void *p)
>  	return 0;
>  }
>  
> +int read_log(struct tpm_chip *chip)
> +{
> +	int rc;
> +
> +	if (chip->log.bios_event_log != NULL) {
> +		dev_dbg(&chip->dev, "%s: ERROR - Eventlog already initialized\n",
> +			__func__);
> +		return -EFAULT;
> +	}
> +
> +	rc = read_log_acpi(chip);
> +	if ((rc == 0) || (rc == -ENOMEM))
> +		return rc;
> +	rc = read_log_of(chip);
> +	return rc;
> +
> +}
> +
>  int tpm_bios_log_setup(struct tpm_chip *chip)
>  {
>  	const char *name = dev_name(&chip->dev);
> diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
> index 6df2f8e..be529ad 100644
> --- a/drivers/char/tpm/tpm_eventlog.h
> +++ b/drivers/char/tpm/tpm_eventlog.h
> @@ -73,20 +73,24 @@ enum tcpa_pc_event_ids {
>  	HOST_TABLE_OF_DEVICES,
>  };
>  
> -int read_log(struct tpm_chip *chip);
> -
> -#if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
> -	defined(CONFIG_ACPI)
> -extern int tpm_bios_log_setup(struct tpm_chip *chip);
> -extern void tpm_bios_log_teardown(struct tpm_chip *chip);
> +#if defined(CONFIG_ACPI)
> +int read_log_acpi(struct tpm_chip *chip);
>  #else
> -static inline int tpm_bios_log_setup(struct tpm_chip *chip)
> +static inline int read_log_acpi(struct tpm_chip *chip)
>  {
> -	return 0;
> +	return -ENODEV;
>  }
> -static inline void tpm_bios_log_teardown(struct tpm_chip *chip)
> +#endif
> +#if defined(CONFIG_OF)
> +int read_log_of(struct tpm_chip *chip);
> +#else
> +static inline int read_log_of(struct tpm_chip *chip)
>  {
> +	return -ENODEV;
>  }
>  #endif
>  
> +int tpm_bios_log_setup(struct tpm_chip *chip);
> +void tpm_bios_log_teardown(struct tpm_chip *chip);
> +
>  #endif
> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> index 68d891a..7c30752 100644
> --- a/drivers/char/tpm/tpm_of.c
> +++ b/drivers/char/tpm/tpm_of.c
> @@ -2,6 +2,7 @@
>   * Copyright 2012 IBM Corporation
>   *
>   * Author: Ashley Lai <ashleydlai-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + *         Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>   *
>   * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
>   *
> @@ -20,7 +21,7 @@
>  #include "tpm.h"
>  #include "tpm_eventlog.h"
>  
> -int read_log(struct tpm_chip *chip)
> +int read_log_of(struct tpm_chip *chip)
>  {
>  	struct device_node *np;
>  	const u32 *sizep;
> @@ -28,11 +29,6 @@ int read_log(struct tpm_chip *chip)
>  	struct tpm_bios_log *log;
>  
>  	log = &chip->log;
> -	if (log->bios_event_log != NULL) {
> -		pr_err("%s: ERROR - Eventlog already initialized\n", __func__);
> -		return -EFAULT;
> -	}
> -
>  	np = of_find_node_by_name(NULL, "vtpm");
>  	if (!np) {
>  		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
> -- 
> 2.5.0
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH v5 5/7] tpm: redefine read_log() to handle ACPI/OF at runtime

[Jason Gunthorpe]

On Wed, Oct 19, 2016 at 07:17:40PM +0300, Jarkko Sakkinen wrote:
> > Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> > Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> 
> Do we have situation where we need a compiled kernel image that is
> both DT and ACPI enabled?

It is possible to enable DT on x86, and apparently ARM64 has some
combination too, there is nothing in kconfig that prevents both from
being enabled, it has been a longstanding issue in this code that they
are #ifdef'd to be exclusive.

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH v5 5/7] tpm: redefine read_log() to handle ACPI/OF at runtime

[Jarkko Sakkinen]

On Wed, Oct 19, 2016 at 11:26:25AM -0600, Jason Gunthorpe wrote:
> On Wed, Oct 19, 2016 at 07:17:40PM +0300, Jarkko Sakkinen wrote:
> > > Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > > Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> > > Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > 
> > Do we have situation where we need a compiled kernel image that is
> > both DT and ACPI enabled?
> 
> It is possible to enable DT on x86, and apparently ARM64 has some
> combination too, there is nothing in kconfig that prevents both from
> being enabled, it has been a longstanding issue in this code that they
> are #ifdef'd to be exclusive.

OK, makes sense then.

I just got Minnowboard Turbot. I have to do some research if I could use
it to do a DT-based boot.

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH v5 5/7] tpm: redefine read_log() to handle ACPI/OF at runtime

[Jarkko Sakkinen]

On Tue, Oct 18, 2016 at 08:49:43PM -0400, Nayna Jain wrote:
> Currently, read_log() has two implementations: one for ACPI platforms
> and the other for device tree(OF) based platforms. The proper one is
> selected at compile time using Kconfig and #ifdef in the Makefile,
> which is not the recommended approach.
> 
> This patch removes the #ifdef in the Makefile by defining a single
> read_log() method, which checks for ACPI/OF event log properties at
> runtime.
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

/Jarkko

> ---
>  drivers/char/tpm/Makefile       | 14 ++++----------
>  drivers/char/tpm/tpm_acpi.c     |  9 ++-------
>  drivers/char/tpm/tpm_eventlog.c | 18 ++++++++++++++++++
>  drivers/char/tpm/tpm_eventlog.h | 22 +++++++++++++---------
>  drivers/char/tpm/tpm_of.c       |  8 ++------
>  5 files changed, 39 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index a385fb8..a05b1eb 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -2,16 +2,10 @@
>  # Makefile for the kernel tpm device drivers.
>  #
>  obj-$(CONFIG_TCG_TPM) += tpm.o
> -tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o
> -tpm-$(CONFIG_ACPI) += tpm_ppi.o
> -
> -ifdef CONFIG_ACPI
> -	tpm-y += tpm_eventlog.o tpm_acpi.o
> -else
> -ifdef CONFIG_TCG_IBMVTPM
> -	tpm-y += tpm_eventlog.o tpm_of.o
> -endif
> -endif
> +tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \
> +		tpm_eventlog.o
> +tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_acpi.o
> +tpm-$(CONFIG_OF) += tpm_of.o
>  obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
>  obj-$(CONFIG_TCG_TIS) += tpm_tis.o
>  obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> index 4d6c2d7..859bdba 100644
> --- a/drivers/char/tpm/tpm_acpi.c
> +++ b/drivers/char/tpm/tpm_acpi.c
> @@ -6,6 +6,7 @@
>   *	Stefan Berger <stefanb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>   *	Reiner Sailer <sailer-aZOuKsOsJu3MbYB6QlFGEg@public.gmane.org>
>   *	Kylene Hall <kjhall-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> + *	Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>   *
>   * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
>   *
> @@ -45,7 +46,7 @@ struct acpi_tcpa {
>  };
>  
>  /* read binary bios log */
> -int read_log(struct tpm_chip *chip)
> +int read_log_acpi(struct tpm_chip *chip)
>  {
>  	struct acpi_tcpa *buff;
>  	acpi_status status;
> @@ -54,12 +55,6 @@ int read_log(struct tpm_chip *chip)
>  	struct tpm_bios_log *log;
>  
>  	log = &chip->log;
> -	if (log->bios_event_log != NULL) {
> -		printk(KERN_ERR
> -		       "%s: ERROR - Eventlog already initialized\n",
> -		       __func__);
> -		return -EFAULT;
> -	}
>  
>  	/* Find TCPA entry in RSDT (ACPI_LOGICAL_ADDRESSING) */
>  	status = acpi_get_table(ACPI_SIG_TCPA, 1,
> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
> index bb142f2..b60c028 100644
> --- a/drivers/char/tpm/tpm_eventlog.c
> +++ b/drivers/char/tpm/tpm_eventlog.c
> @@ -365,6 +365,24 @@ static int is_bad(void *p)
>  	return 0;
>  }
>  
> +int read_log(struct tpm_chip *chip)
> +{
> +	int rc;
> +
> +	if (chip->log.bios_event_log != NULL) {
> +		dev_dbg(&chip->dev, "%s: ERROR - Eventlog already initialized\n",
> +			__func__);
> +		return -EFAULT;
> +	}
> +
> +	rc = read_log_acpi(chip);
> +	if ((rc == 0) || (rc == -ENOMEM))
> +		return rc;
> +	rc = read_log_of(chip);
> +	return rc;
> +
> +}
> +
>  int tpm_bios_log_setup(struct tpm_chip *chip)
>  {
>  	const char *name = dev_name(&chip->dev);
> diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
> index 6df2f8e..be529ad 100644
> --- a/drivers/char/tpm/tpm_eventlog.h
> +++ b/drivers/char/tpm/tpm_eventlog.h
> @@ -73,20 +73,24 @@ enum tcpa_pc_event_ids {
>  	HOST_TABLE_OF_DEVICES,
>  };
>  
> -int read_log(struct tpm_chip *chip);
> -
> -#if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
> -	defined(CONFIG_ACPI)
> -extern int tpm_bios_log_setup(struct tpm_chip *chip);
> -extern void tpm_bios_log_teardown(struct tpm_chip *chip);
> +#if defined(CONFIG_ACPI)
> +int read_log_acpi(struct tpm_chip *chip);
>  #else
> -static inline int tpm_bios_log_setup(struct tpm_chip *chip)
> +static inline int read_log_acpi(struct tpm_chip *chip)
>  {
> -	return 0;
> +	return -ENODEV;
>  }
> -static inline void tpm_bios_log_teardown(struct tpm_chip *chip)
> +#endif
> +#if defined(CONFIG_OF)
> +int read_log_of(struct tpm_chip *chip);
> +#else
> +static inline int read_log_of(struct tpm_chip *chip)
>  {
> +	return -ENODEV;
>  }
>  #endif
>  
> +int tpm_bios_log_setup(struct tpm_chip *chip);
> +void tpm_bios_log_teardown(struct tpm_chip *chip);
> +
>  #endif
> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> index 68d891a..7c30752 100644
> --- a/drivers/char/tpm/tpm_of.c
> +++ b/drivers/char/tpm/tpm_of.c
> @@ -2,6 +2,7 @@
>   * Copyright 2012 IBM Corporation
>   *
>   * Author: Ashley Lai <ashleydlai-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + *         Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>   *
>   * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
>   *
> @@ -20,7 +21,7 @@
>  #include "tpm.h"
>  #include "tpm_eventlog.h"
>  
> -int read_log(struct tpm_chip *chip)
> +int read_log_of(struct tpm_chip *chip)
>  {
>  	struct device_node *np;
>  	const u32 *sizep;
> @@ -28,11 +29,6 @@ int read_log(struct tpm_chip *chip)
>  	struct tpm_bios_log *log;
>  
>  	log = &chip->log;
> -	if (log->bios_event_log != NULL) {
> -		pr_err("%s: ERROR - Eventlog already initialized\n", __func__);
> -		return -EFAULT;
> -	}
> -
>  	np = of_find_node_by_name(NULL, "vtpm");
>  	if (!np) {
>  		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
> -- 
> 2.5.0
> 

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi

[PATCH v5 6/7] tpm: replace of_find_node_by_name() with dev of_node property

[Nayna Jain]

Using the device of_node property is a better way to refer to the
device tree node rather than of_find_node_by_name().

Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/char/tpm/tpm_of.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
index 7c30752..22b8f81 100644
--- a/drivers/char/tpm/tpm_of.c
+++ b/drivers/char/tpm/tpm_of.c
@@ -29,7 +29,8 @@ int read_log_of(struct tpm_chip *chip)
 	struct tpm_bios_log *log;
 
 	log = &chip->log;
-	np = of_find_node_by_name(NULL, "vtpm");
+	if (chip->dev.parent->of_node)
+		np = chip->dev.parent->of_node;
 	if (!np) {
 		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
 		return -ENODEV;
@@ -55,18 +56,15 @@ int read_log_of(struct tpm_chip *chip)
 	if (!log->bios_event_log) {
 		pr_err("%s: ERROR - Not enough memory for BIOS measurements\n",
 		       __func__);
-		of_node_put(np);
 		return -ENOMEM;
 	}
 
 	log->bios_event_log_end = log->bios_event_log + *sizep;
 
 	memcpy(log->bios_event_log, __va(*basep), *sizep);
-	of_node_put(np);
 
 	return 0;
 
 cleanup_eio:
-	of_node_put(np);
 	return -EIO;
 }
-- 
2.5.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

[PATCH v5 7/7] tpm: replace or remove printk error messages

[Nayna Jain]

This patch removes the unnecessary error messages on failing to
allocate memory and replaces pr_err/printk with dev_dbg/dev_info
as applicable.

Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 drivers/char/tpm/tpm_acpi.c | 17 +++++------------
 drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
 2 files changed, 15 insertions(+), 32 deletions(-)

diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
index 859bdba..22e42da 100644
--- a/drivers/char/tpm/tpm_acpi.c
+++ b/drivers/char/tpm/tpm_acpi.c
@@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
 	status = acpi_get_table(ACPI_SIG_TCPA, 1,
 				(struct acpi_table_header **)&buff);
 
-	if (ACPI_FAILURE(status)) {
-		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
-		       __func__);
+	if (ACPI_FAILURE(status))
 		return -EIO;
-	}
 
 	switch(buff->platform_class) {
 	case BIOS_SERVER:
@@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
 		break;
 	}
 	if (!len) {
-		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n", __func__);
+		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
+			__func__);
 		return -EIO;
 	}
 
 	/* malloc EventLog space */
 	log->bios_event_log = kmalloc(len, GFP_KERNEL);
-	if (!log->bios_event_log) {
-		printk("%s: ERROR - Not enough  Memory for BIOS measurements\n",
-			__func__);
+	if (!log->bios_event_log)
 		return -ENOMEM;
-	}
 
 	log->bios_event_log_end = log->bios_event_log + len;
 
 	virt = acpi_os_map_iomem(start, len);
-	if (!virt) {
-		printk("%s: ERROR - Unable to map memory\n", __func__);
+	if (!virt)
 		goto err;
-	}
 
 	memcpy_fromio(log->bios_event_log, virt, len);
 
diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
index 22b8f81..8b7e677 100644
--- a/drivers/char/tpm/tpm_of.c
+++ b/drivers/char/tpm/tpm_of.c
@@ -31,40 +31,30 @@ int read_log_of(struct tpm_chip *chip)
 	log = &chip->log;
 	if (chip->dev.parent->of_node)
 		np = chip->dev.parent->of_node;
-	if (!np) {
-		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
+	if (!np)
 		return -ENODEV;
-	}
 
 	sizep = of_get_property(np, "linux,sml-size", NULL);
-	if (sizep == NULL) {
-		pr_err("%s: ERROR - SML size not found\n", __func__);
-		goto cleanup_eio;
-	}
+	if (sizep == NULL)
+		return -EIO;
+
 	if (*sizep == 0) {
-		pr_err("%s: ERROR - event log area empty\n", __func__);
-		goto cleanup_eio;
+		dev_info(&chip->dev, "%s: ERROR - event log area empty\n",
+			__func__);
+		return -EIO;
 	}
 
 	basep = of_get_property(np, "linux,sml-base", NULL);
-	if (basep == NULL) {
-		pr_err("%s: ERROR - SML not found\n", __func__);
-		goto cleanup_eio;
-	}
+	if (basep == NULL)
+		return -EIO;
 
 	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
-	if (!log->bios_event_log) {
-		pr_err("%s: ERROR - Not enough memory for BIOS measurements\n",
-		       __func__);
+	if (!log->bios_event_log)
 		return -ENOMEM;
-	}
 
 	log->bios_event_log_end = log->bios_event_log + *sizep;
 
 	memcpy(log->bios_event_log, __va(*basep), *sizep);
 
 	return 0;
-
-cleanup_eio:
-	return -EIO;
 }
-- 
2.5.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH v5 7/7] tpm: replace or remove printk error messages

[Jarkko Sakkinen]

On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
> This patch removes the unnecessary error messages on failing to
> allocate memory and replaces pr_err/printk with dev_dbg/dev_info
> as applicable.
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

/Jarkko

> ---
>  drivers/char/tpm/tpm_acpi.c | 17 +++++------------
>  drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
>  2 files changed, 15 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> index 859bdba..22e42da 100644
> --- a/drivers/char/tpm/tpm_acpi.c
> +++ b/drivers/char/tpm/tpm_acpi.c
> @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
>  	status = acpi_get_table(ACPI_SIG_TCPA, 1,
>  				(struct acpi_table_header **)&buff);
>  
> -	if (ACPI_FAILURE(status)) {
> -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
> -		       __func__);
> +	if (ACPI_FAILURE(status))
>  		return -EIO;
> -	}
>  
>  	switch(buff->platform_class) {
>  	case BIOS_SERVER:
> @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
>  		break;
>  	}
>  	if (!len) {
> -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n", __func__);
> +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
> +			__func__);
>  		return -EIO;
>  	}
>  
>  	/* malloc EventLog space */
>  	log->bios_event_log = kmalloc(len, GFP_KERNEL);
> -	if (!log->bios_event_log) {
> -		printk("%s: ERROR - Not enough  Memory for BIOS measurements\n",
> -			__func__);
> +	if (!log->bios_event_log)
>  		return -ENOMEM;
> -	}
>  
>  	log->bios_event_log_end = log->bios_event_log + len;
>  
>  	virt = acpi_os_map_iomem(start, len);
> -	if (!virt) {
> -		printk("%s: ERROR - Unable to map memory\n", __func__);
> +	if (!virt)
>  		goto err;
> -	}
>  
>  	memcpy_fromio(log->bios_event_log, virt, len);
>  
> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> index 22b8f81..8b7e677 100644
> --- a/drivers/char/tpm/tpm_of.c
> +++ b/drivers/char/tpm/tpm_of.c
> @@ -31,40 +31,30 @@ int read_log_of(struct tpm_chip *chip)
>  	log = &chip->log;
>  	if (chip->dev.parent->of_node)
>  		np = chip->dev.parent->of_node;
> -	if (!np) {
> -		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
> +	if (!np)
>  		return -ENODEV;
> -	}
>  
>  	sizep = of_get_property(np, "linux,sml-size", NULL);
> -	if (sizep == NULL) {
> -		pr_err("%s: ERROR - SML size not found\n", __func__);
> -		goto cleanup_eio;
> -	}
> +	if (sizep == NULL)
> +		return -EIO;
> +
>  	if (*sizep == 0) {
> -		pr_err("%s: ERROR - event log area empty\n", __func__);
> -		goto cleanup_eio;
> +		dev_info(&chip->dev, "%s: ERROR - event log area empty\n",
> +			__func__);
> +		return -EIO;
>  	}
>  
>  	basep = of_get_property(np, "linux,sml-base", NULL);
> -	if (basep == NULL) {
> -		pr_err("%s: ERROR - SML not found\n", __func__);
> -		goto cleanup_eio;
> -	}
> +	if (basep == NULL)
> +		return -EIO;
>  
>  	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
> -	if (!log->bios_event_log) {
> -		pr_err("%s: ERROR - Not enough memory for BIOS measurements\n",
> -		       __func__);
> +	if (!log->bios_event_log)
>  		return -ENOMEM;
> -	}
>  
>  	log->bios_event_log_end = log->bios_event_log + *sizep;
>  
>  	memcpy(log->bios_event_log, __va(*basep), *sizep);
>  
>  	return 0;
> -
> -cleanup_eio:
> -	return -EIO;
>  }
> -- 
> 2.5.0
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH v5 7/7] tpm: replace or remove printk error messages

[Winkler, Tomas]

> On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
> > This patch removes the unnecessary error messages on failing to
> > allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
> > applicable.
> >
> > Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> 
> /Jarkko
> 
> > ---
> >  drivers/char/tpm/tpm_acpi.c | 17 +++++------------
> >  drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
> >  2 files changed, 15 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> > index 859bdba..22e42da 100644
> > --- a/drivers/char/tpm/tpm_acpi.c
> > +++ b/drivers/char/tpm/tpm_acpi.c
> > @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
> >  	status = acpi_get_table(ACPI_SIG_TCPA, 1,
> >  				(struct acpi_table_header **)&buff);
> >
> > -	if (ACPI_FAILURE(status)) {
> > -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
> > -		       __func__);
> > +	if (ACPI_FAILURE(status))
> >  		return -EIO;
> > -	}
> >
> >  	switch(buff->platform_class) {
> >  	case BIOS_SERVER:
> > @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
> >  		break;
> >  	}
> >  	if (!len) {
> > -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
> __func__);
> > +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
> > +			__func__);


Why to keep __func__ here, dev_dbg already does it for you. 

> >  		return -EIO;
> >  	}
> >
> >  	/* malloc EventLog space */
> >  	log->bios_event_log = kmalloc(len, GFP_KERNEL);
> > -	if (!log->bios_event_log) {
> > -		printk("%s: ERROR - Not enough  Memory for BIOS
> measurements\n",
> > -			__func__);
> > +	if (!log->bios_event_log)
> >  		return -ENOMEM;
> > -	}
> >
> >  	log->bios_event_log_end = log->bios_event_log + len;
> >
> >  	virt = acpi_os_map_iomem(start, len);
> > -	if (!virt) {
> > -		printk("%s: ERROR - Unable to map memory\n", __func__);
> > +	if (!virt)
> >  		goto err;
> > -	}
> >
> >  	memcpy_fromio(log->bios_event_log, virt, len);
> >
> > diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> > index 22b8f81..8b7e677 100644
> > --- a/drivers/char/tpm/tpm_of.c
> > +++ b/drivers/char/tpm/tpm_of.c
> > @@ -31,40 +31,30 @@ int read_log_of(struct tpm_chip *chip)
> >  	log = &chip->log;
> >  	if (chip->dev.parent->of_node)
> >  		np = chip->dev.parent->of_node;
> > -	if (!np) {
> > -		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
> > +	if (!np)
> >  		return -ENODEV;
> > -	}
> >
> >  	sizep = of_get_property(np, "linux,sml-size", NULL);
> > -	if (sizep == NULL) {
> > -		pr_err("%s: ERROR - SML size not found\n", __func__);
> > -		goto cleanup_eio;
> > -	}
> > +	if (sizep == NULL)
> > +		return -EIO;
> > +
> >  	if (*sizep == 0) {
> > -		pr_err("%s: ERROR - event log area empty\n", __func__);
> > -		goto cleanup_eio;
> > +		dev_info(&chip->dev, "%s: ERROR - event log area empty\n",
> > +			__func__);
> > +		return -EIO;
> >  	}
> >
> >  	basep = of_get_property(np, "linux,sml-base", NULL);
> > -	if (basep == NULL) {
> > -		pr_err("%s: ERROR - SML not found\n", __func__);
> > -		goto cleanup_eio;
> > -	}
> > +	if (basep == NULL)
> > +		return -EIO;
> >
> >  	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
> > -	if (!log->bios_event_log) {
> > -		pr_err("%s: ERROR - Not enough memory for BIOS
> measurements\n",
> > -		       __func__);
> > +	if (!log->bios_event_log)
> >  		return -ENOMEM;
> > -	}
> >
> >  	log->bios_event_log_end = log->bios_event_log + *sizep;
> >
> >  	memcpy(log->bios_event_log, __va(*basep), *sizep);
> >
> >  	return 0;
> > -
> > -cleanup_eio:
> > -	return -EIO;
> >  }
> > --
> > 2.5.0
> >
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH v5 7/7] tpm: replace or remove printk error messages

[Jarkko Sakkinen]

On Thu, Oct 20, 2016 at 07:34:37AM +0000, Winkler, Tomas wrote:
> > On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
> > > This patch removes the unnecessary error messages on failing to
> > > allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
> > > applicable.
> > >
> > > Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > > Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > 
> > /Jarkko
> > 
> > > ---
> > >  drivers/char/tpm/tpm_acpi.c | 17 +++++------------
> > >  drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
> > >  2 files changed, 15 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> > > index 859bdba..22e42da 100644
> > > --- a/drivers/char/tpm/tpm_acpi.c
> > > +++ b/drivers/char/tpm/tpm_acpi.c
> > > @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
> > >  	status = acpi_get_table(ACPI_SIG_TCPA, 1,
> > >  				(struct acpi_table_header **)&buff);
> > >
> > > -	if (ACPI_FAILURE(status)) {
> > > -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
> > > -		       __func__);
> > > +	if (ACPI_FAILURE(status))
> > >  		return -EIO;
> > > -	}
> > >
> > >  	switch(buff->platform_class) {
> > >  	case BIOS_SERVER:
> > > @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
> > >  		break;
> > >  	}
> > >  	if (!len) {
> > > -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
> > __func__);
> > > +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
> > > +			__func__);
> 
> 
> Why to keep __func__ here, dev_dbg already does it for you. 

Good catch. I would actually consider also changing this to

dev_err(dev, FW_BUG "TCPA log area empty\n");

If TCPA exists but it's empty, that's most likely a FW bug.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH v5 7/7] tpm: replace or remove printk error messages

[Nayna]

On 10/20/2016 04:54 PM, Jarkko Sakkinen wrote:
> On Thu, Oct 20, 2016 at 07:34:37AM +0000, Winkler, Tomas wrote:
>>> On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
>>>> This patch removes the unnecessary error messages on failing to
>>>> allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
>>>> applicable.
>>>>
>>>> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>>>> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>>>
>>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>>
>>> /Jarkko
>>>
>>>> ---
>>>>   drivers/char/tpm/tpm_acpi.c | 17 +++++------------
>>>>   drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
>>>>   2 files changed, 15 insertions(+), 32 deletions(-)
>>>>
>>>> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
>>>> index 859bdba..22e42da 100644
>>>> --- a/drivers/char/tpm/tpm_acpi.c
>>>> +++ b/drivers/char/tpm/tpm_acpi.c
>>>> @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
>>>>   	status = acpi_get_table(ACPI_SIG_TCPA, 1,
>>>>   				(struct acpi_table_header **)&buff);
>>>>
>>>> -	if (ACPI_FAILURE(status)) {
>>>> -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
>>>> -		       __func__);
>>>> +	if (ACPI_FAILURE(status))
>>>>   		return -EIO;
>>>> -	}
>>>>
>>>>   	switch(buff->platform_class) {
>>>>   	case BIOS_SERVER:
>>>> @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
>>>>   		break;
>>>>   	}
>>>>   	if (!len) {
>>>> -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
>>> __func__);
>>>> +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
>>>> +			__func__);
>>
>>
>> Why to keep __func__ here, dev_dbg already does it for you.
>
> Good catch. I would actually consider also changing this to
>
> dev_err(dev, FW_BUG "TCPA log area empty\n");
>
> If TCPA exists but it's empty, that's most likely a FW bug.

If it can be possibly a FW bug, should it fail the probe also just like 
-ENOMEM error ?

Thanks & Regards,
     - Nayna

>
> /Jarkko
>


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH v5 7/7] tpm: replace or remove printk error messages

[Jarkko Sakkinen]

On Fri, Oct 21, 2016 at 08:52:14AM +0530, Nayna wrote:
> 
> 
> On 10/20/2016 04:54 PM, Jarkko Sakkinen wrote:
> > On Thu, Oct 20, 2016 at 07:34:37AM +0000, Winkler, Tomas wrote:
> > > > On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
> > > > > This patch removes the unnecessary error messages on failing to
> > > > > allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
> > > > > applicable.
> > > > > 
> > > > > Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > > > > Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> > > > 
> > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > > > 
> > > > /Jarkko
> > > > 
> > > > > ---
> > > > >   drivers/char/tpm/tpm_acpi.c | 17 +++++------------
> > > > >   drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
> > > > >   2 files changed, 15 insertions(+), 32 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> > > > > index 859bdba..22e42da 100644
> > > > > --- a/drivers/char/tpm/tpm_acpi.c
> > > > > +++ b/drivers/char/tpm/tpm_acpi.c
> > > > > @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
> > > > >   	status = acpi_get_table(ACPI_SIG_TCPA, 1,
> > > > >   				(struct acpi_table_header **)&buff);
> > > > > 
> > > > > -	if (ACPI_FAILURE(status)) {
> > > > > -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
> > > > > -		       __func__);
> > > > > +	if (ACPI_FAILURE(status))
> > > > >   		return -EIO;
> > > > > -	}
> > > > > 
> > > > >   	switch(buff->platform_class) {
> > > > >   	case BIOS_SERVER:
> > > > > @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
> > > > >   		break;
> > > > >   	}
> > > > >   	if (!len) {
> > > > > -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
> > > > __func__);
> > > > > +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
> > > > > +			__func__);
> > > 
> > > 
> > > Why to keep __func__ here, dev_dbg already does it for you.
> > 
> > Good catch. I would actually consider also changing this to
> > 
> > dev_err(dev, FW_BUG "TCPA log area empty\n");
> > 
> > If TCPA exists but it's empty, that's most likely a FW bug.
> 
> If it can be possibly a FW bug, should it fail the probe also just like
> -ENOMEM error ?

I think so but I hold for second opinion on this. I mean wouldn't
it seem like a bit broken situation where TCPA tabe would exist but
would also be empty?

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH v5 7/7] tpm: replace or remove printk error messages

[Nayna]

On 10/21/2016 08:32 PM, Jarkko Sakkinen wrote:
> On Fri, Oct 21, 2016 at 08:52:14AM +0530, Nayna wrote:
>>
>>
>> On 10/20/2016 04:54 PM, Jarkko Sakkinen wrote:
>>> On Thu, Oct 20, 2016 at 07:34:37AM +0000, Winkler, Tomas wrote:
>>>>> On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
>>>>>> This patch removes the unnecessary error messages on failing to
>>>>>> allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
>>>>>> applicable.
>>>>>>
>>>>>> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>>>>>> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>>>>>
>>>>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>>>>
>>>>> /Jarkko
>>>>>
>>>>>> ---
>>>>>>    drivers/char/tpm/tpm_acpi.c | 17 +++++------------
>>>>>>    drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
>>>>>>    2 files changed, 15 insertions(+), 32 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
>>>>>> index 859bdba..22e42da 100644
>>>>>> --- a/drivers/char/tpm/tpm_acpi.c
>>>>>> +++ b/drivers/char/tpm/tpm_acpi.c
>>>>>> @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
>>>>>>    	status = acpi_get_table(ACPI_SIG_TCPA, 1,
>>>>>>    				(struct acpi_table_header **)&buff);
>>>>>>
>>>>>> -	if (ACPI_FAILURE(status)) {
>>>>>> -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
>>>>>> -		       __func__);
>>>>>> +	if (ACPI_FAILURE(status))
>>>>>>    		return -EIO;
>>>>>> -	}
>>>>>>
>>>>>>    	switch(buff->platform_class) {
>>>>>>    	case BIOS_SERVER:
>>>>>> @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
>>>>>>    		break;
>>>>>>    	}
>>>>>>    	if (!len) {
>>>>>> -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
>>>>> __func__);
>>>>>> +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
>>>>>> +			__func__);
>>>>
>>>>
>>>> Why to keep __func__ here, dev_dbg already does it for you.
>>>
>>> Good catch. I would actually consider also changing this to
>>>
>>> dev_err(dev, FW_BUG "TCPA log area empty\n");
>>>
>>> If TCPA exists but it's empty, that's most likely a FW bug.
>>
>> If it can be possibly a FW bug, should it fail the probe also just like
>> -ENOMEM error ?
>
> I think so but I hold for second opinion on this. I mean wouldn't
> it seem like a bit broken situation where TCPA tabe would exist but
> would also be empty?

Hmm, is it possible for this to be firmware implementation dependent ?

Thanks & Regards,
    - Nayna
>
> /Jarkko
>


------------------------------------------------------------------------------
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik

Re: [PATCH v5 7/7] tpm: replace or remove printk error messages

[Jarkko Sakkinen]

On Wed, Oct 26, 2016 at 07:52:53AM +0530, Nayna wrote:
> 
> 
> On 10/21/2016 08:32 PM, Jarkko Sakkinen wrote:
> > On Fri, Oct 21, 2016 at 08:52:14AM +0530, Nayna wrote:
> > > 
> > > 
> > > On 10/20/2016 04:54 PM, Jarkko Sakkinen wrote:
> > > > On Thu, Oct 20, 2016 at 07:34:37AM +0000, Winkler, Tomas wrote:
> > > > > > On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
> > > > > > > This patch removes the unnecessary error messages on failing to
> > > > > > > allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
> > > > > > > applicable.
> > > > > > > 
> > > > > > > Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > > > > > > Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> > > > > > 
> > > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > > > > > 
> > > > > > /Jarkko
> > > > > > 
> > > > > > > ---
> > > > > > >    drivers/char/tpm/tpm_acpi.c | 17 +++++------------
> > > > > > >    drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
> > > > > > >    2 files changed, 15 insertions(+), 32 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> > > > > > > index 859bdba..22e42da 100644
> > > > > > > --- a/drivers/char/tpm/tpm_acpi.c
> > > > > > > +++ b/drivers/char/tpm/tpm_acpi.c
> > > > > > > @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
> > > > > > >    	status = acpi_get_table(ACPI_SIG_TCPA, 1,
> > > > > > >    				(struct acpi_table_header **)&buff);
> > > > > > > 
> > > > > > > -	if (ACPI_FAILURE(status)) {
> > > > > > > -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
> > > > > > > -		       __func__);
> > > > > > > +	if (ACPI_FAILURE(status))
> > > > > > >    		return -EIO;
> > > > > > > -	}
> > > > > > > 
> > > > > > >    	switch(buff->platform_class) {
> > > > > > >    	case BIOS_SERVER:
> > > > > > > @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
> > > > > > >    		break;
> > > > > > >    	}
> > > > > > >    	if (!len) {
> > > > > > > -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
> > > > > > __func__);
> > > > > > > +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
> > > > > > > +			__func__);
> > > > > 
> > > > > 
> > > > > Why to keep __func__ here, dev_dbg already does it for you.
> > > > 
> > > > Good catch. I would actually consider also changing this to
> > > > 
> > > > dev_err(dev, FW_BUG "TCPA log area empty\n");
> > > > 
> > > > If TCPA exists but it's empty, that's most likely a FW bug.
> > > 
> > > If it can be possibly a FW bug, should it fail the probe also just like
> > > -ENOMEM error ?
> > 
> > I think so but I hold for second opinion on this. I mean wouldn't
> > it seem like a bit broken situation where TCPA tabe would exist but
> > would also be empty?
> 
> Hmm, is it possible for this to be firmware implementation dependent ?

Let me put it this way. Why would anyone expose TCPA to the user space
that is empty? What would be the point?

/Jarkko

------------------------------------------------------------------------------
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik

Re: [PATCH v5 7/7] tpm: replace or remove printk error messages

[Nayna]

On 10/26/2016 04:26 PM, Jarkko Sakkinen wrote:
> On Wed, Oct 26, 2016 at 07:52:53AM +0530, Nayna wrote:
>>
>>
>> On 10/21/2016 08:32 PM, Jarkko Sakkinen wrote:
>>> On Fri, Oct 21, 2016 at 08:52:14AM +0530, Nayna wrote:
>>>>
>>>>
>>>> On 10/20/2016 04:54 PM, Jarkko Sakkinen wrote:
>>>>> On Thu, Oct 20, 2016 at 07:34:37AM +0000, Winkler, Tomas wrote:
>>>>>>> On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
>>>>>>>> This patch removes the unnecessary error messages on failing to
>>>>>>>> allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
>>>>>>>> applicable.
>>>>>>>>
>>>>>>>> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>>>>>>>> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>>>>>>>
>>>>>>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>>>>>>
>>>>>>> /Jarkko
>>>>>>>
>>>>>>>> ---
>>>>>>>>     drivers/char/tpm/tpm_acpi.c | 17 +++++------------
>>>>>>>>     drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
>>>>>>>>     2 files changed, 15 insertions(+), 32 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
>>>>>>>> index 859bdba..22e42da 100644
>>>>>>>> --- a/drivers/char/tpm/tpm_acpi.c
>>>>>>>> +++ b/drivers/char/tpm/tpm_acpi.c
>>>>>>>> @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
>>>>>>>>     	status = acpi_get_table(ACPI_SIG_TCPA, 1,
>>>>>>>>     				(struct acpi_table_header **)&buff);
>>>>>>>>
>>>>>>>> -	if (ACPI_FAILURE(status)) {
>>>>>>>> -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
>>>>>>>> -		       __func__);
>>>>>>>> +	if (ACPI_FAILURE(status))
>>>>>>>>     		return -EIO;
>>>>>>>> -	}
>>>>>>>>
>>>>>>>>     	switch(buff->platform_class) {
>>>>>>>>     	case BIOS_SERVER:
>>>>>>>> @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
>>>>>>>>     		break;
>>>>>>>>     	}
>>>>>>>>     	if (!len) {
>>>>>>>> -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
>>>>>>> __func__);
>>>>>>>> +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
>>>>>>>> +			__func__);
>>>>>>
>>>>>>
>>>>>> Why to keep __func__ here, dev_dbg already does it for you.
>>>>>
>>>>> Good catch. I would actually consider also changing this to
>>>>>
>>>>> dev_err(dev, FW_BUG "TCPA log area empty\n");
>>>>>
>>>>> If TCPA exists but it's empty, that's most likely a FW bug.
>>>>
>>>> If it can be possibly a FW bug, should it fail the probe also just like
>>>> -ENOMEM error ?
>>>
>>> I think so but I hold for second opinion on this. I mean wouldn't
>>> it seem like a bit broken situation where TCPA tabe would exist but
>>> would also be empty?
>>
>> Hmm, is it possible for this to be firmware implementation dependent ?
>
> Let me put it this way. Why would anyone expose TCPA to the user space
> that is empty? What would be the point?

If I understand correctly,  the reserved memory for event log would be 
allocated much earlier and firmware would directly write to this 
allocated memory. So, if there is a firmware bug, and events are not 
recorded, it will get exposed to userspace as empty event log and this 
might also help applications to know that it is broken.  Is there any 
wrong assumption here ?

Thanks & Regards,
- Nayna

>
> /Jarkko
>


------------------------------------------------------------------------------
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik

Re: [PATCH v5 7/7] tpm: replace or remove printk error messages

[Jarkko Sakkinen]

On Wed, Oct 26, 2016 at 11:01:00PM +0530, Nayna wrote:
> 
> 
> On 10/26/2016 04:26 PM, Jarkko Sakkinen wrote:
> > On Wed, Oct 26, 2016 at 07:52:53AM +0530, Nayna wrote:
> > > 
> > > 
> > > On 10/21/2016 08:32 PM, Jarkko Sakkinen wrote:
> > > > On Fri, Oct 21, 2016 at 08:52:14AM +0530, Nayna wrote:
> > > > > 
> > > > > 
> > > > > On 10/20/2016 04:54 PM, Jarkko Sakkinen wrote:
> > > > > > On Thu, Oct 20, 2016 at 07:34:37AM +0000, Winkler, Tomas wrote:
> > > > > > > > On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
> > > > > > > > > This patch removes the unnecessary error messages on failing to
> > > > > > > > > allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
> > > > > > > > > applicable.
> > > > > > > > > 
> > > > > > > > > Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > > > > > > > > Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> > > > > > > > 
> > > > > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > > > > > > > 
> > > > > > > > /Jarkko
> > > > > > > > 
> > > > > > > > > ---
> > > > > > > > >     drivers/char/tpm/tpm_acpi.c | 17 +++++------------
> > > > > > > > >     drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
> > > > > > > > >     2 files changed, 15 insertions(+), 32 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> > > > > > > > > index 859bdba..22e42da 100644
> > > > > > > > > --- a/drivers/char/tpm/tpm_acpi.c
> > > > > > > > > +++ b/drivers/char/tpm/tpm_acpi.c
> > > > > > > > > @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
> > > > > > > > >     	status = acpi_get_table(ACPI_SIG_TCPA, 1,
> > > > > > > > >     				(struct acpi_table_header **)&buff);
> > > > > > > > > 
> > > > > > > > > -	if (ACPI_FAILURE(status)) {
> > > > > > > > > -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
> > > > > > > > > -		       __func__);
> > > > > > > > > +	if (ACPI_FAILURE(status))
> > > > > > > > >     		return -EIO;
> > > > > > > > > -	}
> > > > > > > > > 
> > > > > > > > >     	switch(buff->platform_class) {
> > > > > > > > >     	case BIOS_SERVER:
> > > > > > > > > @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
> > > > > > > > >     		break;
> > > > > > > > >     	}
> > > > > > > > >     	if (!len) {
> > > > > > > > > -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
> > > > > > > > __func__);
> > > > > > > > > +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
> > > > > > > > > +			__func__);
> > > > > > > 
> > > > > > > 
> > > > > > > Why to keep __func__ here, dev_dbg already does it for you.
> > > > > > 
> > > > > > Good catch. I would actually consider also changing this to
> > > > > > 
> > > > > > dev_err(dev, FW_BUG "TCPA log area empty\n");
> > > > > > 
> > > > > > If TCPA exists but it's empty, that's most likely a FW bug.
> > > > > 
> > > > > If it can be possibly a FW bug, should it fail the probe also just like
> > > > > -ENOMEM error ?
> > > > 
> > > > I think so but I hold for second opinion on this. I mean wouldn't
> > > > it seem like a bit broken situation where TCPA tabe would exist but
> > > > would also be empty?
> > > 
> > > Hmm, is it possible for this to be firmware implementation dependent ?
> > 
> > Let me put it this way. Why would anyone expose TCPA to the user space
> > that is empty? What would be the point?
> 
> If I understand correctly,  the reserved memory for event log would be
> allocated much earlier and firmware would directly write to this allocated
> memory. So, if there is a firmware bug, and events are not recorded, it will
> get exposed to userspace as empty event log and this might also help
> applications to know that it is broken.  Is there any wrong assumption here
> ?

I think the right question to ask is can event log be legally empty?
If not, then FW_BUG should be used here.

/Jarkko

------------------------------------------------------------------------------
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik

Re: [PATCH v5 7/7] tpm: replace or remove printk error messages

[Nayna]

On 10/27/2016 07:23 PM, Jarkko Sakkinen wrote:
> On Wed, Oct 26, 2016 at 11:01:00PM +0530, Nayna wrote:
>>
>>
>> On 10/26/2016 04:26 PM, Jarkko Sakkinen wrote:
>>> On Wed, Oct 26, 2016 at 07:52:53AM +0530, Nayna wrote:
>>>>
>>>>
>>>> On 10/21/2016 08:32 PM, Jarkko Sakkinen wrote:
>>>>> On Fri, Oct 21, 2016 at 08:52:14AM +0530, Nayna wrote:
>>>>>>
>>>>>>
>>>>>> On 10/20/2016 04:54 PM, Jarkko Sakkinen wrote:
>>>>>>> On Thu, Oct 20, 2016 at 07:34:37AM +0000, Winkler, Tomas wrote:
>>>>>>>>> On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
>>>>>>>>>> This patch removes the unnecessary error messages on failing to
>>>>>>>>>> allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
>>>>>>>>>> applicable.
>>>>>>>>>>
>>>>>>>>>> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>>>>>>>>>> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>>>>>>>>>
>>>>>>>>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>>>>>>>>
>>>>>>>>> /Jarkko
>>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>>      drivers/char/tpm/tpm_acpi.c | 17 +++++------------
>>>>>>>>>>      drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
>>>>>>>>>>      2 files changed, 15 insertions(+), 32 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
>>>>>>>>>> index 859bdba..22e42da 100644
>>>>>>>>>> --- a/drivers/char/tpm/tpm_acpi.c
>>>>>>>>>> +++ b/drivers/char/tpm/tpm_acpi.c
>>>>>>>>>> @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
>>>>>>>>>>      	status = acpi_get_table(ACPI_SIG_TCPA, 1,
>>>>>>>>>>      				(struct acpi_table_header **)&buff);
>>>>>>>>>>
>>>>>>>>>> -	if (ACPI_FAILURE(status)) {
>>>>>>>>>> -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
>>>>>>>>>> -		       __func__);
>>>>>>>>>> +	if (ACPI_FAILURE(status))
>>>>>>>>>>      		return -EIO;
>>>>>>>>>> -	}
>>>>>>>>>>
>>>>>>>>>>      	switch(buff->platform_class) {
>>>>>>>>>>      	case BIOS_SERVER:
>>>>>>>>>> @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
>>>>>>>>>>      		break;
>>>>>>>>>>      	}
>>>>>>>>>>      	if (!len) {
>>>>>>>>>> -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
>>>>>>>>> __func__);
>>>>>>>>>> +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
>>>>>>>>>> +			__func__);
>>>>>>>>
>>>>>>>>
>>>>>>>> Why to keep __func__ here, dev_dbg already does it for you.
>>>>>>>
>>>>>>> Good catch. I would actually consider also changing this to
>>>>>>>
>>>>>>> dev_err(dev, FW_BUG "TCPA log area empty\n");
>>>>>>>
>>>>>>> If TCPA exists but it's empty, that's most likely a FW bug.
>>>>>>
>>>>>> If it can be possibly a FW bug, should it fail the probe also just like
>>>>>> -ENOMEM error ?
>>>>>
>>>>> I think so but I hold for second opinion on this. I mean wouldn't
>>>>> it seem like a bit broken situation where TCPA tabe would exist but
>>>>> would also be empty?
>>>>
>>>> Hmm, is it possible for this to be firmware implementation dependent ?
>>>
>>> Let me put it this way. Why would anyone expose TCPA to the user space
>>> that is empty? What would be the point?
>>
>> If I understand correctly,  the reserved memory for event log would be
>> allocated much earlier and firmware would directly write to this allocated
>> memory. So, if there is a firmware bug, and events are not recorded, it will
>> get exposed to userspace as empty event log and this might also help
>> applications to know that it is broken.  Is there any wrong assumption here
>> ?
>
> I think the right question to ask is can event log be legally empty?

No

> If not, then FW_BUG should be used here.
Ok. yeah. and then I think we would want it to fail the probe ?

Thanks & Regards,
    - Nayna

>
> /Jarkko
>


------------------------------------------------------------------------------
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik

Re: [PATCH v5 7/7] tpm: replace or remove printk error messages

[Nayna]

On 10/20/2016 01:04 PM, Winkler, Tomas wrote:
>> On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
>>> This patch removes the unnecessary error messages on failing to
>>> allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
>>> applicable.
>>>
>>> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>>> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>>
>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>
>> /Jarkko
>>
>>> ---
>>>   drivers/char/tpm/tpm_acpi.c | 17 +++++------------
>>>   drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
>>>   2 files changed, 15 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
>>> index 859bdba..22e42da 100644
>>> --- a/drivers/char/tpm/tpm_acpi.c
>>> +++ b/drivers/char/tpm/tpm_acpi.c
>>> @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
>>>   	status = acpi_get_table(ACPI_SIG_TCPA, 1,
>>>   				(struct acpi_table_header **)&buff);
>>>
>>> -	if (ACPI_FAILURE(status)) {
>>> -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
>>> -		       __func__);
>>> +	if (ACPI_FAILURE(status))
>>>   		return -EIO;
>>> -	}
>>>
>>>   	switch(buff->platform_class) {
>>>   	case BIOS_SERVER:
>>> @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
>>>   		break;
>>>   	}
>>>   	if (!len) {
>>> -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
>> __func__);
>>> +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
>>> +			__func__);
>
>
> Why to keep __func__ here, dev_dbg already does it for you.

I tried to trace through dev_dbg
- http://lxr.free-electrons.com/source/include/linux/device.h#L1195
which further calls dev_printk from 
http://lxr.free-electrons.com/source/drivers/base/core.c#L2199

I couldn't find it printing __func__ anywhere.. I just see it printing 
"driver" and "dev" as tpm : tpm0.

Please let me know if I am missing something

Thanks & Regards,
- Nayna
>
>>>   		return -EIO;
>>>   	}
>>>
>>>   	/* malloc EventLog space */
>>>   	log->bios_event_log = kmalloc(len, GFP_KERNEL);
>>> -	if (!log->bios_event_log) {
>>> -		printk("%s: ERROR - Not enough  Memory for BIOS
>> measurements\n",
>>> -			__func__);
>>> +	if (!log->bios_event_log)
>>>   		return -ENOMEM;
>>> -	}
>>>
>>>   	log->bios_event_log_end = log->bios_event_log + len;
>>>
>>>   	virt = acpi_os_map_iomem(start, len);
>>> -	if (!virt) {
>>> -		printk("%s: ERROR - Unable to map memory\n", __func__);
>>> +	if (!virt)
>>>   		goto err;
>>> -	}
>>>
>>>   	memcpy_fromio(log->bios_event_log, virt, len);
>>>
>>> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
>>> index 22b8f81..8b7e677 100644
>>> --- a/drivers/char/tpm/tpm_of.c
>>> +++ b/drivers/char/tpm/tpm_of.c
>>> @@ -31,40 +31,30 @@ int read_log_of(struct tpm_chip *chip)
>>>   	log = &chip->log;
>>>   	if (chip->dev.parent->of_node)
>>>   		np = chip->dev.parent->of_node;
>>> -	if (!np) {
>>> -		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
>>> +	if (!np)
>>>   		return -ENODEV;
>>> -	}
>>>
>>>   	sizep = of_get_property(np, "linux,sml-size", NULL);
>>> -	if (sizep == NULL) {
>>> -		pr_err("%s: ERROR - SML size not found\n", __func__);
>>> -		goto cleanup_eio;
>>> -	}
>>> +	if (sizep == NULL)
>>> +		return -EIO;
>>> +
>>>   	if (*sizep == 0) {
>>> -		pr_err("%s: ERROR - event log area empty\n", __func__);
>>> -		goto cleanup_eio;
>>> +		dev_info(&chip->dev, "%s: ERROR - event log area empty\n",
>>> +			__func__);
>>> +		return -EIO;
>>>   	}
>>>
>>>   	basep = of_get_property(np, "linux,sml-base", NULL);
>>> -	if (basep == NULL) {
>>> -		pr_err("%s: ERROR - SML not found\n", __func__);
>>> -		goto cleanup_eio;
>>> -	}
>>> +	if (basep == NULL)
>>> +		return -EIO;
>>>
>>>   	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
>>> -	if (!log->bios_event_log) {
>>> -		pr_err("%s: ERROR - Not enough memory for BIOS
>> measurements\n",
>>> -		       __func__);
>>> +	if (!log->bios_event_log)
>>>   		return -ENOMEM;
>>> -	}
>>>
>>>   	log->bios_event_log_end = log->bios_event_log + *sizep;
>>>
>>>   	memcpy(log->bios_event_log, __va(*basep), *sizep);
>>>
>>>   	return 0;
>>> -
>>> -cleanup_eio:
>>> -	return -EIO;
>>>   }
>>> --
>>> 2.5.0
>>>
>>
>


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH v5 7/7] tpm: replace or remove printk error messages

[Jarkko Sakkinen]

On Thu, Oct 20, 2016 at 07:01:51PM +0530, Nayna wrote:
> 
> 
> On 10/20/2016 01:04 PM, Winkler, Tomas wrote:
> > > On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
> > > > This patch removes the unnecessary error messages on failing to
> > > > allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
> > > > applicable.
> > > > 
> > > > Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > > > Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> > > 
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > > 
> > > /Jarkko
> > > 
> > > > ---
> > > >   drivers/char/tpm/tpm_acpi.c | 17 +++++------------
> > > >   drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
> > > >   2 files changed, 15 insertions(+), 32 deletions(-)
> > > > 
> > > > diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> > > > index 859bdba..22e42da 100644
> > > > --- a/drivers/char/tpm/tpm_acpi.c
> > > > +++ b/drivers/char/tpm/tpm_acpi.c
> > > > @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
> > > >   	status = acpi_get_table(ACPI_SIG_TCPA, 1,
> > > >   				(struct acpi_table_header **)&buff);
> > > > 
> > > > -	if (ACPI_FAILURE(status)) {
> > > > -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
> > > > -		       __func__);
> > > > +	if (ACPI_FAILURE(status))
> > > >   		return -EIO;
> > > > -	}
> > > > 
> > > >   	switch(buff->platform_class) {
> > > >   	case BIOS_SERVER:
> > > > @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
> > > >   		break;
> > > >   	}
> > > >   	if (!len) {
> > > > -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
> > > __func__);
> > > > +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
> > > > +			__func__);
> > 
> > 
> > Why to keep __func__ here, dev_dbg already does it for you.
> 
> I tried to trace through dev_dbg
> - http://lxr.free-electrons.com/source/include/linux/device.h#L1195
> which further calls dev_printk from
> http://lxr.free-electrons.com/source/drivers/base/core.c#L2199
> 
> I couldn't find it printing __func__ anywhere.. I just see it printing
> "driver" and "dev" as tpm : tpm0.
> 
> Please let me know if I am missing something
> 
> Thanks & Regards,
> - Nayna

Sorry I mixed up in my last response. You can filter in dynamic
debug with a function name but doesn't print function name.

Anyway I would recommend to change this to dev_err in a way that
I explained in my response.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH v5 7/7] tpm: replace or remove printk error messages

[Jarkko Sakkinen]

On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
> This patch removes the unnecessary error messages on failing to
> allocate memory and replaces pr_err/printk with dev_dbg/dev_info
> as applicable.
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

Why subject line is "replace or remove"? What does that mean?
If you have both, maybe using the phrase "clean up" would be
better.

/Jarkko

> ---
>  drivers/char/tpm/tpm_acpi.c | 17 +++++------------
>  drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
>  2 files changed, 15 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> index 859bdba..22e42da 100644
> --- a/drivers/char/tpm/tpm_acpi.c
> +++ b/drivers/char/tpm/tpm_acpi.c
> @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
>  	status = acpi_get_table(ACPI_SIG_TCPA, 1,
>  				(struct acpi_table_header **)&buff);
>  
> -	if (ACPI_FAILURE(status)) {
> -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
> -		       __func__);
> +	if (ACPI_FAILURE(status))
>  		return -EIO;
> -	}
>  
>  	switch(buff->platform_class) {
>  	case BIOS_SERVER:
> @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
>  		break;
>  	}
>  	if (!len) {
> -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n", __func__);
> +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
> +			__func__);
>  		return -EIO;
>  	}
>  
>  	/* malloc EventLog space */
>  	log->bios_event_log = kmalloc(len, GFP_KERNEL);
> -	if (!log->bios_event_log) {
> -		printk("%s: ERROR - Not enough  Memory for BIOS measurements\n",
> -			__func__);
> +	if (!log->bios_event_log)
>  		return -ENOMEM;
> -	}
>  
>  	log->bios_event_log_end = log->bios_event_log + len;
>  
>  	virt = acpi_os_map_iomem(start, len);
> -	if (!virt) {
> -		printk("%s: ERROR - Unable to map memory\n", __func__);
> +	if (!virt)
>  		goto err;
> -	}
>  
>  	memcpy_fromio(log->bios_event_log, virt, len);
>  
> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> index 22b8f81..8b7e677 100644
> --- a/drivers/char/tpm/tpm_of.c
> +++ b/drivers/char/tpm/tpm_of.c
> @@ -31,40 +31,30 @@ int read_log_of(struct tpm_chip *chip)
>  	log = &chip->log;
>  	if (chip->dev.parent->of_node)
>  		np = chip->dev.parent->of_node;
> -	if (!np) {
> -		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
> +	if (!np)
>  		return -ENODEV;
> -	}
>  
>  	sizep = of_get_property(np, "linux,sml-size", NULL);
> -	if (sizep == NULL) {
> -		pr_err("%s: ERROR - SML size not found\n", __func__);
> -		goto cleanup_eio;
> -	}
> +	if (sizep == NULL)
> +		return -EIO;
> +
>  	if (*sizep == 0) {
> -		pr_err("%s: ERROR - event log area empty\n", __func__);
> -		goto cleanup_eio;
> +		dev_info(&chip->dev, "%s: ERROR - event log area empty\n",
> +			__func__);
> +		return -EIO;
>  	}
>  
>  	basep = of_get_property(np, "linux,sml-base", NULL);
> -	if (basep == NULL) {
> -		pr_err("%s: ERROR - SML not found\n", __func__);
> -		goto cleanup_eio;
> -	}
> +	if (basep == NULL)
> +		return -EIO;
>  
>  	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
> -	if (!log->bios_event_log) {
> -		pr_err("%s: ERROR - Not enough memory for BIOS measurements\n",
> -		       __func__);
> +	if (!log->bios_event_log)
>  		return -ENOMEM;
> -	}
>  
>  	log->bios_event_log_end = log->bios_event_log + *sizep;
>  
>  	memcpy(log->bios_event_log, __va(*basep), *sizep);
>  
>  	return 0;
> -
> -cleanup_eio:
> -	return -EIO;
>  }
> -- 
> 2.5.0
> 

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi

Re: [PATCH v5 0/7] tpm: cleanup/fixes in existing event log support

[Jarkko Sakkinen]

On Tue, Oct 18, 2016 at 08:49:38PM -0400, Nayna Jain wrote:
> This patch set includes the cleanup and bug fixes patches, previously
> part of the "tpm: add the securityfs pseudo files support for TPM 2.0
> firmware event log" patch set, in order to upstream them more quickly.

I realized something that is obviously wrong in this patch set. You
should CC this to linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org and probably also
linux-security-module-u79uwXL29TaiAVqoAR/hOA@public.gmane.org It's only sent to this mailing
list. It works for minor changes and RFC but not for this. Do this
for the next version.

Include all seven patches, or more, because it really seems that 4/7
needs to be split into multiple patches.

I have to drop the first patch too because of this reason.

/Jarkko

> Changelog History
> 
> v5:
> 
> - Moved cleanup/fixes patches into this patch set.
> - Patch "fix the race condition between event log access and chip
> getting unregistered"
>   - updated subject line and commit description.
>   - modified fops code to use chip kref.
>   - modified fops to lock inode before accessing inode private data.
>   - renamed tpm_securityfs_data to tpm_chip_seqops, as it no more
>     holds bios log, but associates seqops with respective chip. For
>     the same reason, moved it to tpm.h
> - Patch "replace or remove printk error messages"
>   - cleaned up dev_dbg and used dev_info as applicable.
> 
> v4:
> 
> - Includes feedbacks from Jarkko and Jason.
> - Patch "tpm: define a generic open() method for ascii & bios
> measurements".
>   - Fix indentation issue.
> - Patch "tpm: replace the dynamically allocated bios_dir as
> struct dentry array".
>   - Continue to use bios_dir_count variable to use is_bad() checks and
>     to maintain correct order for securityfs_remove() during teardown.
>   - Reset chip->bios_dir_count in teardown() function.
> - Patch "tpm: validate the event log access before tpm_bios_log_setup".
>   - Retain TPM2 check which was removed in previous patch.
>   - Add tpm_bios_log_setup failure handling.
>   - Remove use of private data from v3 version of patch. Add a new
>   member to struct tpm_chip to achieve the same purpose.
> - Patch "tpm: redefine the read_log method to check for ACPI/OF 
> properties sequentially".
>   - Move replacement of CONFIG_TCG_IBMVTPM with CONFIG_OF to
>   this patch from patch 3.
>   - Replace -1 error code with -ENODEV.
> - Patch "tpm: replace the of_find_node_by_name() with dev of_node
> property".
>   - Uses chip->dev.parent->of_node.
>   - Created separate patch for cleanup of pr_err 
>   messages.
> - Patch "tpm: remove printk error messages".
>   - New Patch.
> - Patch "tpm: add the securityfs file support for TPM 2.0 event log".
>   - Parses event digests using event alg_id rather than event log header
>     alg_id.
>   - Uses of_property_match_string to differentiate tpm/vtpm compatible
>     property.
>   - Adds the comment for difference in tpm/vtpm endianness.
> 
> v3:
> 
> - Includes the review feedbacks as suggested by Jason.
> - Split of patches into one patch per idea.
> - Generic open() method for ascii/bios measurements.
> - Replacement of of **bios_dir with *bios_dir[3].
> - Verifying readlog() is successful before creating securityfs entries.
> - Generic readlog() to check for ACPI/OF in sequence.
> - read_log_of() method now uses of_node propertry rather than
>   calling find_device_by_name.
> - read_log differentiates vtpm/tpm using its compatible property.
> - Cleans pr_err with dev_dbg.
> - Commit msgs subject line prefixed with tpm.
> 
> v2:
> 
> - Fixes issues as given in feedback by Jason.
> - Adds documentation for device tree.
> 
> Jarkko Sakkinen (2):
>   tpm: replace dynamically allocated bios_dir with a static array
>   tpm: drop tpm1_chip_register(/unregister)
> 
> Nayna Jain (5):
>   tpm: define a generic open() method for ascii & bios measurements
>   tpm: fix the race condition between event log access and chip getting
>     unregistered
>   tpm: redefine read_log() to handle ACPI/OF at runtime
>   tpm: replace of_find_node_by_name() with dev of_node property
>   tpm: replace or remove printk error messages
> 
>  drivers/char/tpm/Makefile       |  14 +--
>  drivers/char/tpm/tpm-chip.c     |  31 ++----
>  drivers/char/tpm/tpm-sysfs.c    |   3 +
>  drivers/char/tpm/tpm.h          |  14 ++-
>  drivers/char/tpm/tpm_acpi.c     |  36 +++----
>  drivers/char/tpm/tpm_eventlog.c | 208 ++++++++++++++++++++--------------------
>  drivers/char/tpm/tpm_eventlog.h |  22 +++--
>  drivers/char/tpm/tpm_of.c       |  46 ++++-----
>  8 files changed, 176 insertions(+), 198 deletions(-)
> 
> -- 
> 2.5.0
> 

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi