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

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

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(&chip->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(&chip->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(&chip->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(&chip->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