| Message ID | 1472532619-22170-5-git-send-email-nayna@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 ------------------------------------------------------------------------------
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 > ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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 --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__);
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(-)