From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matt Fleming Subject: Re: [PATCH v10 1/1] efi: a misc char interface for user to update efi firmware Date: Thu, 28 Jan 2016 12:16:02 +0000 Message-ID: <20160128121602.GD2571@codeblueprint.co.uk> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-efi-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: "Kweh, Hock Leong" Cc: Greg Kroah-Hartman , "Ong, Boon Leong" , LKML , "linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Sam Protsenko , Peter Jones , Andy Lutomirski , Roy Franz , Borislav Petkov , James Bottomley , Linux FS Devel , "Anvin, H Peter" List-Id: linux-efi@vger.kernel.org On Tue, 26 Jan, at 03:10:03AM, Kweh Hock Leong wrote: > > > > This mutex is not needed. It doesn't protect anything in your code. > > This is to synchronize/serializes one of the instant is calling efi_capsule_supported() > and the other one is calling efi_capsule_update() which they are exported symbol > from capsule.c. You don't need to synchronise them. Looking at my original capsule patches I can see why you might be doing this locking, but it's unnecessary. You don't need to serialise access to efi_capsule_supported() and efi_capsule_update(). Internally to the core EFI capsule code we need to ensure that we don't allow capsules with conflicting reset types to be sent to the firmware (how would we know the type of reset to perform?), but that should be entirely transparent to your driver. I'm planning on re-sending my capsule patches, so all this should become clearer. > > This function would be much more simple if you handled the above > > condition differently. > > > > Instead of having code in efi_capsule_setup_info() to allocate the > > initial ->pages memory, why not just allocate it in efi_capsule_open() > > at the same time as you allocate the private_data? You can then > > free it in efi_capsule_release() (you're leaking it at the moment). > > > > You are always going to need at least one element in ->pages for > > successful operation, so you might as well allocate it up front. > > Just want to double check I understand you correctly. Do you mean > I should free ->pages during close(2) but not free the ->pages[X] if > there is any successfully submitted? Yes, that's correct. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f54.google.com ([74.125.82.54]:38048 "EHLO mail-wm0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965390AbcA1MQH (ORCPT ); Thu, 28 Jan 2016 07:16:07 -0500 Received: by mail-wm0-f54.google.com with SMTP id p63so21816287wmp.1 for ; Thu, 28 Jan 2016 04:16:05 -0800 (PST) Date: Thu, 28 Jan 2016 12:16:02 +0000 From: Matt Fleming To: "Kweh, Hock Leong" Cc: Greg Kroah-Hartman , "Ong, Boon Leong" , LKML , "linux-efi@vger.kernel.org" , Sam Protsenko , Peter Jones , Andy Lutomirski , Roy Franz , Borislav Petkov , James Bottomley , Linux FS Devel , "Anvin, H Peter" Subject: Re: [PATCH v10 1/1] efi: a misc char interface for user to update efi firmware Message-ID: <20160128121602.GD2571@codeblueprint.co.uk> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue, 26 Jan, at 03:10:03AM, Kweh Hock Leong wrote: > > > > This mutex is not needed. It doesn't protect anything in your code. > > This is to synchronize/serializes one of the instant is calling efi_capsule_supported() > and the other one is calling efi_capsule_update() which they are exported symbol > from capsule.c. You don't need to synchronise them. Looking at my original capsule patches I can see why you might be doing this locking, but it's unnecessary. You don't need to serialise access to efi_capsule_supported() and efi_capsule_update(). Internally to the core EFI capsule code we need to ensure that we don't allow capsules with conflicting reset types to be sent to the firmware (how would we know the type of reset to perform?), but that should be entirely transparent to your driver. I'm planning on re-sending my capsule patches, so all this should become clearer. > > This function would be much more simple if you handled the above > > condition differently. > > > > Instead of having code in efi_capsule_setup_info() to allocate the > > initial ->pages memory, why not just allocate it in efi_capsule_open() > > at the same time as you allocate the private_data? You can then > > free it in efi_capsule_release() (you're leaking it at the moment). > > > > You are always going to need at least one element in ->pages for > > successful operation, so you might as well allocate it up front. > > Just want to double check I understand you correctly. Do you mean > I should free ->pages during close(2) but not free the ->pages[X] if > there is any successfully submitted? Yes, that's correct.