diff mbox

[v3,4/7] tpm: Redefine the read_log method to check for ACPI/OF properties sequentially

Message ID 1472532619-22170-5-git-send-email-nayna@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Nayna Aug. 30, 2016, 4:50 a.m. UTC
Currently, the difference in read_log method for ACPI/OF based platforms
is handled by defining respective read_log method and handing
them using CONFIG based #ifdef condition in Makefile which is not
the recommended approach.

This patch cleans up the ifdef condition in Makefile by defining
single read_log method which checks for ACPI/OF event log memory in
sequence.

Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
---
 drivers/char/tpm/Makefile       | 11 ++---------
 drivers/char/tpm/tpm_acpi.c     |  9 +--------
 drivers/char/tpm/tpm_eventlog.c | 17 +++++++++++++++++
 drivers/char/tpm/tpm_eventlog.h | 18 +++++++++++++++++-
 drivers/char/tpm/tpm_of.c       | 11 +++++------
 5 files changed, 42 insertions(+), 24 deletions(-)

Comments

Jason Gunthorpe Aug. 30, 2016, 5:54 p.m. UTC | #1
On Tue, Aug 30, 2016 at 12:50:16AM -0400, Nayna Jain wrote:
> Currently, the difference in read_log method for ACPI/OF based platforms
> is handled by defining respective read_log method and handing
> them using CONFIG based #ifdef condition in Makefile which is not
> the recommended approach.
> 
> This patch cleans up the ifdef condition in Makefile by defining
> single read_log method which checks for ACPI/OF event log memory in
> sequence.
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>

Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

Yep, this is what I was looking to see..

> +#if defined(CONFIG_ACPI)
> +int read_log_acpi(struct tpm_chip *chip);
> +#else
> +static inline int read_log_acpi(struct tpm_chip *chip)
> +{
> +	return -1;
> +}
> +#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 -1;
> +}
> +#endif

Though shouldn't these two be ERRNOs of some kind? -ENODEV?

Jason

------------------------------------------------------------------------------
Nayna Aug. 31, 2016, 7:09 p.m. UTC | #2
On 08/30/2016 11:24 PM, Jason Gunthorpe wrote:
> On Tue, Aug 30, 2016 at 12:50:16AM -0400, Nayna Jain wrote:
>> Currently, the difference in read_log method for ACPI/OF based platforms
>> is handled by defining respective read_log method and handing
>> them using CONFIG based #ifdef condition in Makefile which is not
>> the recommended approach.
>>
>> This patch cleans up the ifdef condition in Makefile by defining
>> single read_log method which checks for ACPI/OF event log memory in
>> sequence.
>>
>> Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
>
> Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>
> Yep, this is what I was looking to see..
>
>> +#if defined(CONFIG_ACPI)
>> +int read_log_acpi(struct tpm_chip *chip);
>> +#else
>> +static inline int read_log_acpi(struct tpm_chip *chip)
>> +{
>> +	return -1;
>> +}
>> +#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 -1;
>> +}
>> +#endif
>
> Though shouldn't these two be ERRNOs of some kind? -ENODEV?

Sure..
Was just trying to see possible errno. And here are some thoughts.


#define EPERM            1      /* Operation not permitted */
#define ENODEV          19      /* No such device */

Was thinking that since tpm device will still be present and its either 
ACPI or OF way of accessing its properties, and one of them will return 
this errno. So, assuming it is ACPI, that means no OF functions 
permitted. So, how about using EPERM ?

Please let me know if it doesn't sound correct.

Thanks & Regards,
Nayna

>
> Jason
>


------------------------------------------------------------------------------
Jason Gunthorpe Sept. 6, 2016, 7:47 p.m. UTC | #3
On Thu, Sep 01, 2016 at 12:39:46AM +0530, Nayna wrote:
> >>+int read_log_of(struct tpm_chip *chip);
> >>+#else
> >>+static inline int read_log_of(struct tpm_chip *chip)
> >>+{
> >>+	return -1;
> >>+}
> >>+#endif
> >
> >Though shouldn't these two be ERRNOs of some kind? -ENODEV?
> 
> Sure..
> Was just trying to see possible errno. And here are some thoughts.
> 
> 
> #define EPERM            1      /* Operation not permitted */
> #define ENODEV          19      /* No such device */

> Was thinking that since tpm device will still be present and its either ACPI
> or OF way of accessing its properties, and one of them will return this
> errno. So, assuming it is ACPI, that means no OF functions permitted. So,
> how about using EPERM ?

I'd choose ENODEV over EPERM, that is the usual way in the kernel to
signal 'probe failed'

Remember, which ever it is, it should not cause any messages to be printed.

Jason

------------------------------------------------------------------------------
Peter Huewe Sept. 6, 2016, 8:08 p.m. UTC | #4
Am 6. September 2016 12:47:37 GMT-07:00, schrieb Jason Gunthorpe <jgunthorpe@obsidianresearch.com>:
>On Thu, Sep 01, 2016 at 12:39:46AM +0530, Nayna wrote:
>> >>+int read_log_of(struct tpm_chip *chip);
>> >>+#else
>> >>+static inline int read_log_of(struct tpm_chip *chip)
>> >>+{
>> >>+	return -1;
>> >>+}
>> >>+#endif
>> >
>> >Though shouldn't these two be ERRNOs of some kind? -ENODEV?
>> 
>> Sure..
>> Was just trying to see possible errno. And here are some thoughts.
>> 
>> 
>> #define EPERM            1      /* Operation not permitted */
>> #define ENODEV          19      /* No such device */
>
>> Was thinking that since tpm device will still be present and its
>either ACPI
>> or OF way of accessing its properties, and one of them will return
>this
>> errno. So, assuming it is ACPI, that means no OF functions permitted.
>So,
>> how about using EPERM ?
>
>I'd choose ENODEV over EPERM, that is the usual way in the kernel to
>signal 'probe failed'

Me too, EPERM sounds more like the caller is lacking priviledge to do so.

>
>Remember, which ever it is, it should not cause any messages to be
>printed.
>
>Jason
>
>------------------------------------------------------------------------------
>_______________________________________________
>tpmdd-devel mailing list
>tpmdd-devel@lists.sourceforge.net
>https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
diff mbox

Patch

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 00e48e4..e8c7b4d 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -5,15 +5,8 @@  obj-$(CONFIG_TCG_TPM) += tpm.o
 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
-
-ifdef CONFIG_ACPI
-	tpm-y += tpm_acpi.o
-else
-ifdef CONFIG_OF
-	tpm-y += tpm_of.o
-endif
-endif
+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 05b4e8a..a670c4f 100644
--- a/drivers/char/tpm/tpm_acpi.c
+++ b/drivers/char/tpm/tpm_acpi.c
@@ -45,20 +45,13 @@  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;
 	void __iomem *virt;
 	u64 len, start;
 
-	if (chip->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,
 				(struct acpi_table_header **)&buff);
diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index d6f2477..f84ce71 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -349,6 +349,23 @@  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)
+		return rc;
+	rc = read_log_of(chip);
+	return rc;
+}
+
 void 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 6a01d43..0e599ab 100644
--- a/drivers/char/tpm/tpm_eventlog.h
+++ b/drivers/char/tpm/tpm_eventlog.h
@@ -73,7 +73,23 @@  enum tcpa_pc_event_ids {
 	HOST_TABLE_OF_DEVICES,
 };
 
-int read_log(struct tpm_chip *chip);
+#if defined(CONFIG_ACPI)
+int read_log_acpi(struct tpm_chip *chip);
+#else
+static inline int read_log_acpi(struct tpm_chip *chip)
+{
+	return -1;
+}
+#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 -1;
+}
+#endif
 
 void tpm_bios_log_setup(struct tpm_chip *chip);
 void tpm_bios_log_teardown(struct tpm_chip *chip);
diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
index 8e77976..5067a86 100644
--- a/drivers/char/tpm/tpm_of.c
+++ b/drivers/char/tpm/tpm_of.c
@@ -14,23 +14,22 @@ 
  *
  */
 
+#include <linux/seq_file.h>
+#include <linux/fs.h>
+#include <linux/security.h>
+#include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/of.h>
 
 #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;
 	const u64 *basep;
 
-	if (chip->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__);