Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752008AbcJJUi1 (ORCPT ); Mon, 10 Oct 2016 16:38:27 -0400 Received: from mx2.suse.de ([195.135.220.15]:39791 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750881AbcJJUiY (ORCPT ); Mon, 10 Oct 2016 16:38:24 -0400 Date: Mon, 10 Oct 2016 22:37:52 +0200 From: "Luis R. Rodriguez" To: Daniel Wagner Cc: "Luis R. Rodriguez" , Daniel Wagner , linux-kernel@vger.kernel.org, Ming Lei , Greg Kroah-Hartman , "Srivatsa S . Bhat" , "Rafael J . Wysocki" , Daniel Vetter , Takashi Iwai , Bjorn Andersson , Arend van Spriel Subject: Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status Message-ID: <20161010203752.GB8651@wotan.suse.de> References: <1473423144-21734-1-git-send-email-wagi@monom.org> <1473423144-21734-3-git-send-email-wagi@monom.org> <3484559b-bc77-d056-0775-481233d51fc0@bmw-carit.de> <20161005202750.GF3296@wotan.suse.de> <1bbbe0a7-8dc5-3efe-9422-7c52a4f6cc3a@bmw-carit.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1bbbe0a7-8dc5-3efe-9422-7c52a4f6cc3a@bmw-carit.de> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4470 Lines: 127 On Fri, Oct 07, 2016 at 01:41:21PM +0200, Daniel Wagner wrote: > Hi Luis, > > On 10/05/2016 10:27 PM, Luis R. Rodriguez wrote: > > On Tue, Sep 13, 2016 at 11:47:08AM +0200, Daniel Wagner wrote: > > > On 09/09/2016 02:12 PM, Daniel Wagner wrote: > > > > The firmware user helper code tracks the current state of the loading > > > > process via unsigned long status and a completion in struct > > > > firmware_buf. We only need this for the usermode helper as such we can > > > > encapsulate all this data into its own data structure. > > > > > > I don't think we are able to move the completion code into a > > > CONFIG_FW_LOADER_HELPER section. The direct loading path uses > > > completion as well. > > > > Where? > > If you look at the current code (not these patches) you have dependency via > the firmware_buf for two concurrent _request_firmware() calls: > > > 1nd request (waker context) > > _request_firmware() > _request_firmware_prepare() > fw_lookup_and_allocate_buf() # no pendending request > # returns 0 -> load firmware "no pending request" is an invalid association with what fw_lookup_and_allocate_buf() does, its also why I have asked for this to be renamed, it looks for the firmware in the fw cache, if it finds it it returns 1. Otherwise it creates a new buf entry and adds it to the fw cache, and returns 0. > > fw_get_fileystem_firmware() > fw_finish_direct_load() > complete_all() > > > 2nd request (waiter context) > > _request_firmware() > _request_firmware_prepare() > fw_lookup_allocate_buf() # finds previously allocated buf > # returns 1 -> wait for loading > sync_cached_firmware_buf() > wait_for_completion_interruptible_timeout() No, that's wait_for_completion_interruptible() not wait_for_completion_interruptible_timeout() Also note that we only call sync_cached_firmware_buf() *iff* fw_lookup_and_allocate_buf() returned the 1 -- I mentioned when this happens above. That happens only if we already had the entry on the fw cache. As it stands -- concurrent calls against the same fw name could cause a clash here, as such, the wait_for_completion_interruptible() is indeed still needed. Further optimizations can be considered later but for indeed, agreed that completion is needed even for the direct fw load case. The timeout though, I don't see a reason for it. > > > > +#else /* CONFIG_FW_LOADER_USER_HELPER */ > > > > + > > > > +#define fw_umh_wait_timeout(fw_st, long) 0 > > > > + > > > > +#define fw_umh_done(fw_st) > > > > +#define fw_umh_is_done(fw_st) true > > > > +#define fw_umh_is_aborted(fw_st) false > > > > > > We still need the implementation for fw_umh_wait_timeout() and > > > fw_umh_start(), fw_umh_done() etc. > > > > Why? > > See above. Sure, but note how the timeout is not used. > > > > @@ -309,8 +373,7 @@ static void fw_finish_direct_load(struct device *device, > > > > struct firmware_buf *buf) > > > > { > > > > mutex_lock(&fw_lock); > > > > - set_bit(FW_STATUS_DONE, &buf->status); > > > > - complete_all(&buf->completion); > > > > + fw_umh_done(&buf->fw_umh); > > > > mutex_unlock(&fw_lock); > > > > } > > > > > > Here we signal that we have loaded the firmware > > > > The struct firmware_buf is only used for the sysfs stuff no? > > I don't know, I was looking at the code in firmware_class.c not any users. > Why is that important? Sorry I meant struct firmware_priv is used by sysfs stuff only, the sysfs stuff is only used for the FW UMH. > > > > /* wait until the shared firmware_buf becomes ready (or error) */ > > > > static int sync_cached_firmware_buf(struct firmware_buf *buf) > > > > { > > > > int ret = 0; > > > > > > > > mutex_lock(&fw_lock); > > > > - while (!test_bit(FW_STATUS_DONE, &buf->status)) { > > > > - if (is_fw_load_aborted(buf)) { > > > > + while (!fw_umh_is_done(&buf->fw_umh)) { > > > > + if (fw_umh_is_aborted(&buf->fw_umh)) { > > > > ret = -ENOENT; > > > > break; > > > > } > > > > mutex_unlock(&fw_lock); > > > > - ret = wait_for_completion_interruptible(&buf->completion); > > > > + ret = fw_umh_wait_timeout(&buf->fw_umh, 0); > > > > mutex_lock(&fw_lock); > > > > } > > > > > > and here we here we wait for it. > > > > Likewise. > > As I tried to explain above the buffering code is depending on completion. OK sure agreed. The timeout, no though, unless I missed something? Luis