Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S261767AbTH2WRu (ORCPT ); Fri, 29 Aug 2003 18:17:50 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S261682AbTH2WRu (ORCPT ); Fri, 29 Aug 2003 18:17:50 -0400 Received: from smtp2.us.dell.com ([143.166.85.133]:15592 "EHLO smtp2.us.dell.com") by vger.kernel.org with ESMTP id S261336AbTH2WRq (ORCPT ); Fri, 29 Aug 2003 18:17:46 -0400 Date: Fri, 29 Aug 2003 17:17:17 -0500 (CDT) From: Matt Domsch X-X-Sender: mdomsch@humbolt.us.dell.com To: Matt Tolentino cc: matthew.e.tolentino@intel.com, , Subject: Re: [PATCH] efivars update In-Reply-To: <200308292124.h7TLOCAZ000785@snoqualmie.dp.intel.com> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2119 Lines: 68 > The patch is against 2.6.0-test4.? Please apply.? Thanks for your work on this! One bug, 3 nits... > +static ssize_t > +efivar_edit(struct efivar_entry *entry, const char *buf, size_t count) ... > +?????? spin_lock(&efivars_lock); > +?????? status = efi.set_variable(new_var->VariableName, > +?????? ??????? ??????? ??????? ? &new_var->VendorGuid, > +?????? ??????? ??????? ??????? ? new_var->Attributes, > +?????? ??????? ??????? ??????? ? new_var->DataSize, > +?????? ??????? ??????? ??????? ? new_var->Data); > + > +?????? if (status != EFI_SUCCESS) { > +?????? ??????? printk(KERN_WARNING "set_variable() failed: status=%lx\n", > +?????? ??????? ??????? ??????? status); > +?????? ??????? return -EIO; > +?????? } > +?????? spin_unlock(&efivars_lock); Move the unlock up above the status test, else you return while holding the lock on failure. > +static ssize_t > +efivar_create(struct subsystem *sub, const char *buf, size_t count) ... > +?????? if (found) { > +?????? ??????? printk("EFI variable already exists!\n"); level on printk please. > +efivar_delete(struct subsystem *sub, const char *buf, size_t count) ... > +?????? status = efi.set_variable(del_var->VariableName, > +?????? ??????? ??????? ??????? ? &del_var->VendorGuid, > +?????? ??????? ??????? ??????? ? del_var->Attributes, > +?????? ??????? ??????? ??????? ? del_var->DataSize, > +?????? ??????? ??????? ??????? ? del_var->Data); Might as well just force the deletion by setting Attributes=0 or DataSize=0 or both. Less chance for userspace error. > + * Overrides for Emacs so that we follow Linus's tabbing style. Please kill this chunk. I shouldn't have had it there in the first place. :-) Looks great to me! Thanks again. -- Matt Domsch Sr. Software Engineer, Lead Engineer Dell Linux Solutions www.dell.com/linux Linux on Dell mailing lists @ http://lists.us.dell.com - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/