From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nayna Subject: Re: [PATCH 1/2] TPM2.0: Refactor eventlog init functions for TPM1.2 and Date: Mon, 1 Aug 2016 22:17:02 +0530 Message-ID: <579F7D06.7080507@linux.vnet.ibm.com> References: <1469774679-25232-1-git-send-email-nayna@linux.vnet.ibm.com> <1469774679-25232-2-git-send-email-nayna@linux.vnet.ibm.com> <20160729165708.GA6331@obsidianresearch.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20160729165708.GA6331-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: tpmdd-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org To: Jason Gunthorpe Cc: David Heller , tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: tpmdd-devel@lists.sourceforge.net 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 > ------------------------------------------------------------------------------