Re: [PATCH 1/2] TPM2.0: Refactor eventlog init functions for TPM1.2 and

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

Hi Jason,

Thanks for the review.

On 07/29/2016 10:27 PM, Jason Gunthorpe wrote:
> On Fri, Jul 29, 2016 at 02:44:38AM -0400, Nayna Jain wrote:
>> Refactored eventlog.c file into tpm_eventlog.c and tpm_eventlog_init.c
>
> If you are going to work on this stuff (and have the ability to test
> it) can you fix some of the generic pre-existing problems too?

Can you please help me to understand what in particular are you 
referring to by "some of the generic pre-existing problems" ?
>
>> +static int tpm_ascii_bios_measurements_open(struct inode *inode,
>> +					    struct file *file)
>
>> +static int tpm_binary_bios_measurements_open(struct inode *inode,
>> +					     struct file *file)
>
> We don't need two (or more!) identical versions of this function, and it
> is easy to fix:

Sure, will fix it.
>
>> +	bin_file =
>> +	    securityfs_create_file("binary_bios_measurements",
>> +				   S_IRUSR | S_IRGRP, tpm_dir, NULL,
>> +				   &tpm_binary_bios_measurements_ops);
>
> Replace NULL with &tpm_binary_b_measurments_seqops and recover it  in
> the generic open using the inode->i_private pointer.

Sure, will fix it.

>
>> +	ret = kmalloc(3 * sizeof(struct dentry *), GFP_KERNEL);
>> +	if (!ret)
>> +		goto out_ascii;
>
> I can't find a kfree for this memory, looks like it is leaking, please
> fix it.
>
> Do not allocate memory for this, just include the dentry array
> directly in the tpm_chip as the sysfs does today.

Do you mean here to define it as struct dentry *bios_dir[3] as member of 
struct tpm_chip ?

>
> You can change the signatures to accept tpm_chip in a cleanup patch as
> well, moving from the tpm2 patch.
>

Ok. Will address these comments in the next version of the patch I will 
be posting.

Thanks & Regards,
    - Nayna

> Jason
>


------------------------------------------------------------------------------