Re: [tpmdd-devel] [PATCH v2 1/3] TPM2.0: Refactored eventlog init functions.

2016-08-16 Thread Nayna 
Sure Jason.. Taking care of split and other fixes in my V3 version of patch.

Thanks & Regards,
   - Nayna

On 08/13/2016 08:15 AM, Jason Gunthorpe wrote:
> On Wed, Aug 10, 2016 at 04:42:20PM +0530, Nayna wrote:
>> Thanks for reviewing.
>>
>> Sure, I will post next version with split as two patches and other fixes as
>> suggested. Below is the breakdown of two patches, let me know if this
>> doesn't sound ok.
>>
>> 1. First patch to clean up the code related to tpm_eventlog_init.c - generic
>> open() and bios_dir as dentry array.
>
> Split the patches further. One idea per patch.
>
>> 2. Second patch to have changes related to using of_node property and struct
>> tpm_chip in tpm_of.c. Includes adding CONFIG_OF.
>
> Yes
>
>> And one feedback which I didn't understand and so need your help with that
>> is
>>
 -extern struct dentry **tpm_bios_log_setup(const char *);
 -extern void tpm_bios_log_teardown(struct dentry **);
 +extern void tpm_bios_log_setup(struct tpm_chip *chip);
 +extern void tpm_bios_log_teardown(struct tpm_chip *chip);
>>>
>>> We are trying to get rid of these extra externs..
>>
>> This is currently called by tpm_chip_register to setup the bios log.
>> So, what did it meant by getting rid of these ?
>
> The word extern in this context is unnecessary, just drop it when you
> edit the line.
>
> Jason
>


--
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH v2 1/3] TPM2.0: Refactored eventlog init functions.

2016-08-12 Thread Jason Gunthorpe 
On Wed, Aug 10, 2016 at 04:42:20PM +0530, Nayna wrote:
> Thanks for reviewing.
> 
> Sure, I will post next version with split as two patches and other fixes as
> suggested. Below is the breakdown of two patches, let me know if this
> doesn't sound ok.
> 
> 1. First patch to clean up the code related to tpm_eventlog_init.c - generic
> open() and bios_dir as dentry array.

Split the patches further. One idea per patch.

> 2. Second patch to have changes related to using of_node property and struct
> tpm_chip in tpm_of.c. Includes adding CONFIG_OF.

Yes

> And one feedback which I didn't understand and so need your help with that
> is
> 
> >> -extern struct dentry **tpm_bios_log_setup(const char *);
> >> -extern void tpm_bios_log_teardown(struct dentry **);
> >> +extern void tpm_bios_log_setup(struct tpm_chip *chip);
> >> +extern void tpm_bios_log_teardown(struct tpm_chip *chip);
> >
> > We are trying to get rid of these extra externs..
> 
> This is currently called by tpm_chip_register to setup the bios log.
> So, what did it meant by getting rid of these ?

The word extern in this context is unnecessary, just drop it when you
edit the line.

Jason

--
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH v2 1/3] TPM2.0: Refactored eventlog init functions.

2016-08-10 Thread Jarkko Sakkinen 
On Tue, Aug 09, 2016 at 03:34:53PM -0400, Nayna Jain wrote:
> Refactored eventlog.c file into tpm_eventlog.c and tpm_eventlog_init.c
> 
> Breakdown is:
> 
> * tpm_eventlog_init.c : Moved eventlog initialization methods like
> to setup securityfs, to open and release seqfile from tpm_eventlog.c
> to this file. This is to keep the logic of initialization for TPM1.2
> and TPM2.0 in common file.
> 
> * tpm_eventlog.c : This file now has only methods specific to parsing
> and iterate TPM1.2 entry log formats. It can understand only TPM1.2
> and is called by methods in tpm_eventlog_init if identified TPM device
> is TPM1.2.

I'm not going to review this in the current form. I would rather see a
clean one paragraph "why" and "how" description and the commit do only
one thing.

That said I NAK the split. It adds more complexity than brings value in
this case.

/Jarkko

> Changelog v2:
> 
> * Using of_node property of device rather than direct reading
> the device node.
> * Cleaned up the code to have generic open() for ascii and bios
> measurements
> * Removed dyncamic allocation for bios_dir and having dentry array
>   directly into tpm-chip.
> * Using dev_dbg instead of pr_err in tpm_of.c
> * readlog(...) now accepts struct tpm_chip * as parameter.
> 
> 
> Signed-off-by: Nayna Jain 
> ---
>  drivers/char/tpm/Makefile|   4 +-
>  drivers/char/tpm/tpm-chip.c  |   6 +-
>  drivers/char/tpm/tpm.h   |   2 +-
>  drivers/char/tpm/tpm_acpi.c  |   2 +-
>  drivers/char/tpm/tpm_eventlog.c  | 156 
> +--
>  drivers/char/tpm/tpm_eventlog.h  |  16 ++--
>  drivers/char/tpm/tpm_eventlog_init.c | 155 ++
>  drivers/char/tpm/tpm_of.c|  22 +++--
>  8 files changed, 189 insertions(+), 174 deletions(-)
>  create mode 100644 drivers/char/tpm/tpm_eventlog_init.c
> 
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index a385fb8..9136762 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -6,10 +6,10 @@ 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
> + tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_acpi.o
>  else
>  ifdef CONFIG_TCG_IBMVTPM
> - tpm-y += tpm_eventlog.o tpm_of.o
> + tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_of.o
>  endif
>  endif
>  obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index e595013..7f6cdab 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -171,6 +171,8 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
>   chip->dev.release = tpm_dev_release;
>   chip->dev.parent = dev;
>   chip->dev.groups = chip->groups;
> + if (dev->of_node)
> + chip->dev.of_node = dev->of_node;
>  
>   if (chip->dev_num == 0)
>   chip->dev.devt = MKDEV(MISC_MAJOR, TPM_MINOR);
> @@ -283,7 +285,7 @@ static int tpm1_chip_register(struct tpm_chip *chip)
>  
>   tpm_sysfs_add_device(chip);
>  
> - chip->bios_dir = tpm_bios_log_setup(dev_name(>dev));
> + tpm_bios_log_setup(chip);
>  
>   return 0;
>  }
> @@ -294,7 +296,7 @@ static void tpm1_chip_unregister(struct tpm_chip *chip)
>   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 6e002c4..cfa408f 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -171,7 +171,7 @@ struct tpm_chip {
>   unsigned long duration[3]; /* jiffies */
>   bool duration_adjusted;
>  
> - struct dentry **bios_dir;
> + struct dentry *bios_dir[3];
>  
>   const struct attribute_group *groups[3];
>   unsigned int groups_cnt;
> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> index 565a947..c2a122a 100644
> --- a/drivers/char/tpm/tpm_acpi.c
> +++ b/drivers/char/tpm/tpm_acpi.c
> @@ -45,7 +45,7 @@ struct acpi_tcpa {
>  };
>  
>  /* read binary bios log */
> -int read_log(struct tpm_bios_log *log)
> +int read_log(struct tpm_bios_log *log, struct tpm_chip *chip)
>  {
>   struct acpi_tcpa *buff;
>   acpi_status status;
> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
> index e722886..b8f22ec 100644
> --- a/drivers/char/tpm/tpm_eventlog.c
> +++ b/drivers/char/tpm/tpm_eventlog.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (C) 2005, 2012 IBM Corporation
> + * Copyright (C) 2005, 2012, 2016 IBM Corporation
>   *
>   * Authors:
>   *   Kent Yoder 
> @@ -11,6 +11,7 @@
>   * Maintained by:

Re: [tpmdd-devel] [PATCH v2 1/3] TPM2.0: Refactored eventlog init functions.

2016-08-10 Thread Nayna 
Thanks for reviewing.

Sure, I will post next version with split as two patches and other fixes 
as suggested. Below is the breakdown of two patches, let me know if this 
doesn't sound ok.

1. First patch to clean up the code related to tpm_eventlog_init.c - 
generic open() and bios_dir as dentry array.

2. Second patch to have changes related to using of_node property and 
struct tpm_chip in tpm_of.c. Includes adding CONFIG_OF.

And one feedback which I didn't understand and so need your help with 
that is

 >> -extern struct dentry **tpm_bios_log_setup(const char *);
 >> -extern void tpm_bios_log_teardown(struct dentry **);
 >> +extern void tpm_bios_log_setup(struct tpm_chip *chip);
 >> +extern void tpm_bios_log_teardown(struct tpm_chip *chip);
 >
 > We are trying to get rid of these extra externs..

This is currently called by tpm_chip_register to setup the bios log.
So, what did it meant by getting rid of these ?

Thanks & Regards,
- Nayna

On 08/10/2016 03:57 AM, Jason Gunthorpe wrote:
> On Tue, Aug 09, 2016 at 03:34:53PM -0400, Nayna Jain wrote:
>> Refactored eventlog.c file into tpm_eventlog.c and tpm_eventlog_init.c
>>
>> Breakdown is:
>>
>> * tpm_eventlog_init.c : Moved eventlog initialization methods like
>> to setup securityfs, to open and release seqfile from tpm_eventlog.c
>> to this file. This is to keep the logic of initialization for TPM1.2
>> and TPM2.0 in common file.
>>
>> * tpm_eventlog.c : This file now has only methods specific to parsing
>> and iterate TPM1.2 entry log formats. It can understand only TPM1.2
>> and is called by methods in tpm_eventlog_init if identified TPM device
>> is TPM1.2.
>>
>> Changelog v2:
>>
>>  * Using of_node property of device rather than direct reading
>>  the device node.
>>  * Cleaned up the code to have generic open() for ascii and bios
>>  measurements
>>  * Removed dyncamic allocation for bios_dir and having dentry array
>>  directly into tpm-chip.
>>  * Using dev_dbg instead of pr_err in tpm_of.c
>>  * readlog(...) now accepts struct tpm_chip * as parameter.
>
> This patch needs to be split.
>
> One patch per idea please.
>
>>   ifdef CONFIG_ACPI
>> -tpm-y += tpm_eventlog.o tpm_acpi.o
>> +tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_acpi.o
>>   else
>>   ifdef CONFIG_TCG_IBMVTPM
>> -tpm-y += tpm_eventlog.o tpm_of.o
>> +tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_of.o
>>   endif
>
> Still need to fix this, more like
>
> tpm_of should be included if CONFIG_OF is set,
> and tpm_acpi if CONFIG_ACPI is set, do not do this based on random
> other configus..
>
>
>>   endif
>>   obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index e595013..7f6cdab 100644
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -171,6 +171,8 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
>>  chip->dev.release = tpm_dev_release;
>>  chip->dev.parent = dev;
>>  chip->dev.groups = chip->groups;
>> +if (dev->of_node)
>> +chip->dev.of_node = dev->of_node;
>
> No. chip->dev.parent->of_node.
>
>> -extern struct dentry **tpm_bios_log_setup(const char *);
>> -extern void tpm_bios_log_teardown(struct dentry **);
>> +extern void tpm_bios_log_setup(struct tpm_chip *chip);
>> +extern void tpm_bios_log_teardown(struct tpm_chip *chip);
>
> We are trying to get rid of these extra externs..
>
>> +
>> +bin_file =
>> +securityfs_create_file("binary_bios_measurements",
>
> No, do
>
> chip->bios_dir_count = 0;
> chip->bios_dir[chip->bios_dir_count] = [..]
> if (is_bad(chip->bios_dir[chip->bios_dir_count])
>goto err
> chip->bios_dir_count++
>
> err:
> for (I = 0; I != chip->bios_dir_count; ++I)
> securityfs_remove(chip->bios_dir[I]);
>
>> +for (i = 0; i < 3; i++) {
>> +if (chip->bios_dir[i])
>> +securityfs_remove(chip->bios_dir[i]);
>
> Same logic as err: example above
>>
>> -np = of_find_node_by_name(NULL, "vtpm");
>> +if (chip->dev.of_node)
>> +np = chip->dev.of_node;
>>  if (!np) {
>> -pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
>> +dev_dbg(>dev, "%s: ERROR - IBMVTPM not supported\n",
>> +__func__);
>>  return -ENODEV;
>>  }
>
> Where you able to test this on IBM's 'vtpm' stuff as well?
>
>>
>>  sizep = of_get_property(np, "linux,sml-size", NULL);
>>  if (sizep == NULL) {
>> -pr_err("%s: ERROR - SML size not found\n", __func__);
>> +dev_dbg(>dev, "%s: ERROR - SML size not found\n",
>> +__func__);
>>  goto cleanup_eio;
>>  }
>>  if (*sizep == 0) {
>> -pr_err("%s: ERROR - event log area empty\n", __func__);
>> +dev_dbg(>dev, "%s: ERROR - event log area empty\n",
>> +__func__);
>>  goto cleanup_eio;
>>  }
>>
>>  basep = of_get_property(np,

Re: [tpmdd-devel] [PATCH v2 1/3] TPM2.0: Refactored eventlog init functions.

2016-08-10 Thread Jarkko Sakkinen 
On Tue, Aug 09, 2016 at 04:27:40PM -0600, Jason Gunthorpe wrote:
> On Tue, Aug 09, 2016 at 03:34:53PM -0400, Nayna Jain wrote:
> > Refactored eventlog.c file into tpm_eventlog.c and tpm_eventlog_init.c
> > 
> > Breakdown is:
> > 
> > * tpm_eventlog_init.c : Moved eventlog initialization methods like
> > to setup securityfs, to open and release seqfile from tpm_eventlog.c
> > to this file. This is to keep the logic of initialization for TPM1.2
> > and TPM2.0 in common file.
> > 
> > * tpm_eventlog.c : This file now has only methods specific to parsing
> > and iterate TPM1.2 entry log formats. It can understand only TPM1.2
> > and is called by methods in tpm_eventlog_init if identified TPM device
> > is TPM1.2.
> > 
> > Changelog v2:
> > 
> > * Using of_node property of device rather than direct reading
> > the device node.
> > * Cleaned up the code to have generic open() for ascii and bios
> > measurements
> > * Removed dyncamic allocation for bios_dir and having dentry array
> > directly into tpm-chip.
> > * Using dev_dbg instead of pr_err in tpm_of.c
> > * readlog(...) now accepts struct tpm_chip * as parameter.
> 
> This patch needs to be split.
> 
> One patch per idea please.

And changelog per patch *set*.

/Jarkko

> >  ifdef CONFIG_ACPI
> > -   tpm-y += tpm_eventlog.o tpm_acpi.o
> > +   tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_acpi.o
> >  else
> >  ifdef CONFIG_TCG_IBMVTPM
> > -   tpm-y += tpm_eventlog.o tpm_of.o
> > +   tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_of.o
> >  endif
> 
> Still need to fix this, more like
> 
> tpm_of should be included if CONFIG_OF is set,
> and tpm_acpi if CONFIG_ACPI is set, do not do this based on random
> other configus..
> 
> 
> >  endif
> >  obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > index e595013..7f6cdab 100644
> > +++ b/drivers/char/tpm/tpm-chip.c
> > @@ -171,6 +171,8 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
> > chip->dev.release = tpm_dev_release;
> > chip->dev.parent = dev;
> > chip->dev.groups = chip->groups;
> > +   if (dev->of_node)
> > +   chip->dev.of_node = dev->of_node;
> 
> No. chip->dev.parent->of_node.
> 
> > -extern struct dentry **tpm_bios_log_setup(const char *);
> > -extern void tpm_bios_log_teardown(struct dentry **);
> > +extern void tpm_bios_log_setup(struct tpm_chip *chip);
> > +extern void tpm_bios_log_teardown(struct tpm_chip *chip);
> 
> We are trying to get rid of these extra externs..
> 
> > +
> > +   bin_file =
> > +   securityfs_create_file("binary_bios_measurements",
> 
> No, do
> 
> chip->bios_dir_count = 0;
> chip->bios_dir[chip->bios_dir_count] = [..]
> if (is_bad(chip->bios_dir[chip->bios_dir_count])
>   goto err
> chip->bios_dir_count++
> 
> err:
> for (I = 0; I != chip->bios_dir_count; ++I)
>securityfs_remove(chip->bios_dir[I]);
> 
> > +   for (i = 0; i < 3; i++) {
> > +   if (chip->bios_dir[i])
> > +   securityfs_remove(chip->bios_dir[i]);
> 
> Same logic as err: example above
> >  
> > -   np = of_find_node_by_name(NULL, "vtpm");
> > +   if (chip->dev.of_node)
> > +   np = chip->dev.of_node;
> > if (!np) {
> > -   pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
> > +   dev_dbg(>dev, "%s: ERROR - IBMVTPM not supported\n",
> > +   __func__);
> > return -ENODEV;
> > }
> 
> Where you able to test this on IBM's 'vtpm' stuff as well?
> 
> >  
> > sizep = of_get_property(np, "linux,sml-size", NULL);
> > if (sizep == NULL) {
> > -   pr_err("%s: ERROR - SML size not found\n", __func__);
> > +   dev_dbg(>dev, "%s: ERROR - SML size not found\n",
> > +   __func__);
> > goto cleanup_eio;
> > }
> > if (*sizep == 0) {
> > -   pr_err("%s: ERROR - event log area empty\n", __func__);
> > +   dev_dbg(>dev, "%s: ERROR - event log area empty\n",
> > +   __func__);
> > goto cleanup_eio;
> > }
> >  
> > basep = of_get_property(np, "linux,sml-base", NULL);
> > if (basep == NULL) {
> > -   pr_err("%s: ERROR - SML not found\n", __func__);
> > +   dev_dbg(>dev, "%s: ERROR - SML not found\n",
> > +   __func__);
> > goto cleanup_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__);
> > of_node_put(np);
> 
> Why is there an of_node_put here but not in other error paths? Where is
> the get this is putting?
> 
> Jason
> 
> --
> What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
> patterns at an interface-level. Reveals which users, apps, and protocols are 
>

Re: [tpmdd-devel] [PATCH v2 1/3] TPM2.0: Refactored eventlog init functions.

2016-08-09 Thread Jason Gunthorpe 
On Tue, Aug 09, 2016 at 03:34:53PM -0400, Nayna Jain wrote:
> Refactored eventlog.c file into tpm_eventlog.c and tpm_eventlog_init.c
> 
> Breakdown is:
> 
> * tpm_eventlog_init.c : Moved eventlog initialization methods like
> to setup securityfs, to open and release seqfile from tpm_eventlog.c
> to this file. This is to keep the logic of initialization for TPM1.2
> and TPM2.0 in common file.
> 
> * tpm_eventlog.c : This file now has only methods specific to parsing
> and iterate TPM1.2 entry log formats. It can understand only TPM1.2
> and is called by methods in tpm_eventlog_init if identified TPM device
> is TPM1.2.
> 
> Changelog v2:
> 
> * Using of_node property of device rather than direct reading
> the device node.
> * Cleaned up the code to have generic open() for ascii and bios
> measurements
> * Removed dyncamic allocation for bios_dir and having dentry array
>   directly into tpm-chip.
> * Using dev_dbg instead of pr_err in tpm_of.c
> * readlog(...) now accepts struct tpm_chip * as parameter.

This patch needs to be split.

One patch per idea please.

>  ifdef CONFIG_ACPI
> - tpm-y += tpm_eventlog.o tpm_acpi.o
> + tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_acpi.o
>  else
>  ifdef CONFIG_TCG_IBMVTPM
> - tpm-y += tpm_eventlog.o tpm_of.o
> + tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_of.o
>  endif

Still need to fix this, more like

tpm_of should be included if CONFIG_OF is set,
and tpm_acpi if CONFIG_ACPI is set, do not do this based on random
other configus..


>  endif
>  obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index e595013..7f6cdab 100644
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -171,6 +171,8 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
>   chip->dev.release = tpm_dev_release;
>   chip->dev.parent = dev;
>   chip->dev.groups = chip->groups;
> + if (dev->of_node)
> + chip->dev.of_node = dev->of_node;

No. chip->dev.parent->of_node.

> -extern struct dentry **tpm_bios_log_setup(const char *);
> -extern void tpm_bios_log_teardown(struct dentry **);
> +extern void tpm_bios_log_setup(struct tpm_chip *chip);
> +extern void tpm_bios_log_teardown(struct tpm_chip *chip);

We are trying to get rid of these extra externs..

> +
> + bin_file =
> + securityfs_create_file("binary_bios_measurements",

No, do

chip->bios_dir_count = 0;
chip->bios_dir[chip->bios_dir_count] = [..]
if (is_bad(chip->bios_dir[chip->bios_dir_count])
  goto err
chip->bios_dir_count++

err:
for (I = 0; I != chip->bios_dir_count; ++I)
   securityfs_remove(chip->bios_dir[I]);

> + for (i = 0; i < 3; i++) {
> + if (chip->bios_dir[i])
> + securityfs_remove(chip->bios_dir[i]);

Same logic as err: example above
>  
> - np = of_find_node_by_name(NULL, "vtpm");
> + if (chip->dev.of_node)
> + np = chip->dev.of_node;
>   if (!np) {
> - pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
> + dev_dbg(>dev, "%s: ERROR - IBMVTPM not supported\n",
> + __func__);
>   return -ENODEV;
>   }

Where you able to test this on IBM's 'vtpm' stuff as well?

>  
>   sizep = of_get_property(np, "linux,sml-size", NULL);
>   if (sizep == NULL) {
> - pr_err("%s: ERROR - SML size not found\n", __func__);
> + dev_dbg(>dev, "%s: ERROR - SML size not found\n",
> + __func__);
>   goto cleanup_eio;
>   }
>   if (*sizep == 0) {
> - pr_err("%s: ERROR - event log area empty\n", __func__);
> + dev_dbg(>dev, "%s: ERROR - event log area empty\n",
> + __func__);
>   goto cleanup_eio;
>   }
>  
>   basep = of_get_property(np, "linux,sml-base", NULL);
>   if (basep == NULL) {
> - pr_err("%s: ERROR - SML not found\n", __func__);
> + dev_dbg(>dev, "%s: ERROR - SML not found\n",
> + __func__);
>   goto cleanup_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__);
>   of_node_put(np);

Why is there an of_node_put here but not in other error paths? Where is
the get this is putting?

Jason

--
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev
___
tpmdd-devel mailing list

[tpmdd-devel] [PATCH v2 1/3] TPM2.0: Refactored eventlog init functions.

2016-08-09 Thread Nayna Jain 
Refactored eventlog.c file into tpm_eventlog.c and tpm_eventlog_init.c

Breakdown is:

* tpm_eventlog_init.c : Moved eventlog initialization methods like
to setup securityfs, to open and release seqfile from tpm_eventlog.c
to this file. This is to keep the logic of initialization for TPM1.2
and TPM2.0 in common file.

* tpm_eventlog.c : This file now has only methods specific to parsing
and iterate TPM1.2 entry log formats. It can understand only TPM1.2
and is called by methods in tpm_eventlog_init if identified TPM device
is TPM1.2.

Changelog v2:

* Using of_node property of device rather than direct reading
the device node.
* Cleaned up the code to have generic open() for ascii and bios
measurements
* Removed dyncamic allocation for bios_dir and having dentry array
directly into tpm-chip.
* Using dev_dbg instead of pr_err in tpm_of.c
* readlog(...) now accepts struct tpm_chip * as parameter.


Signed-off-by: Nayna Jain 
---
 drivers/char/tpm/Makefile|   4 +-
 drivers/char/tpm/tpm-chip.c  |   6 +-
 drivers/char/tpm/tpm.h   |   2 +-
 drivers/char/tpm/tpm_acpi.c  |   2 +-
 drivers/char/tpm/tpm_eventlog.c  | 156 +--
 drivers/char/tpm/tpm_eventlog.h  |  16 ++--
 drivers/char/tpm/tpm_eventlog_init.c | 155 ++
 drivers/char/tpm/tpm_of.c|  22 +++--
 8 files changed, 189 insertions(+), 174 deletions(-)
 create mode 100644 drivers/char/tpm/tpm_eventlog_init.c

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index a385fb8..9136762 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -6,10 +6,10 @@ 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
+   tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_acpi.o
 else
 ifdef CONFIG_TCG_IBMVTPM
-   tpm-y += tpm_eventlog.o tpm_of.o
+   tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_of.o
 endif
 endif
 obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index e595013..7f6cdab 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -171,6 +171,8 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
chip->dev.release = tpm_dev_release;
chip->dev.parent = dev;
chip->dev.groups = chip->groups;
+   if (dev->of_node)
+   chip->dev.of_node = dev->of_node;
 
if (chip->dev_num == 0)
chip->dev.devt = MKDEV(MISC_MAJOR, TPM_MINOR);
@@ -283,7 +285,7 @@ static int tpm1_chip_register(struct tpm_chip *chip)
 
tpm_sysfs_add_device(chip);
 
-   chip->bios_dir = tpm_bios_log_setup(dev_name(>dev));
+   tpm_bios_log_setup(chip);
 
return 0;
 }
@@ -294,7 +296,7 @@ static void tpm1_chip_unregister(struct tpm_chip *chip)
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 6e002c4..cfa408f 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -171,7 +171,7 @@ struct tpm_chip {
unsigned long duration[3]; /* jiffies */
bool duration_adjusted;
 
-   struct dentry **bios_dir;
+   struct dentry *bios_dir[3];
 
const struct attribute_group *groups[3];
unsigned int groups_cnt;
diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
index 565a947..c2a122a 100644
--- a/drivers/char/tpm/tpm_acpi.c
+++ b/drivers/char/tpm/tpm_acpi.c
@@ -45,7 +45,7 @@ struct acpi_tcpa {
 };
 
 /* read binary bios log */
-int read_log(struct tpm_bios_log *log)
+int read_log(struct tpm_bios_log *log, struct tpm_chip *chip)
 {
struct acpi_tcpa *buff;
acpi_status status;
diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index e722886..b8f22ec 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2005, 2012 IBM Corporation
+ * Copyright (C) 2005, 2012, 2016 IBM Corporation
  *
  * Authors:
  * Kent Yoder 
@@ -11,6 +11,7 @@
  * Maintained by: 
  *
  * Access to the eventlog created by a system's firmware / BIOS
+ * specific to TPM 1.2.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -257,20 +258,6 @@ 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