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

[Nayna <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>]

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