From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936076AbcIHIFi (ORCPT ); Thu, 8 Sep 2016 04:05:38 -0400 Received: from cit-hm8-mail01.bmw-carit.de ([212.118.206.84]:55654 "EHLO cit-hm8-gw01.bmw-carit.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S935955AbcIHIFd (ORCPT ); Thu, 8 Sep 2016 04:05:33 -0400 X-CTCH-RefID: str=0001.0A0C0208.57D11BC0.00F1,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0 Subject: Re: [PATCH v4 2/4] firmware: encapsulate firmware loading status To: "Luis R. Rodriguez" , Daniel Wagner , Takashi Iwai References: <1473237908-20989-1-git-send-email-wagi@monom.org> <1473237908-20989-3-git-send-email-wagi@monom.org> <20160908013940.GA3296@wotan.suse.de> CC: , Ming Lei , Greg Kroah-Hartman From: Daniel Wagner Message-ID: <1aae9991-0fc2-3da7-b60b-001548f36daa@bmw-carit.de> Date: Thu, 8 Sep 2016 10:05:19 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20160908013940.GA3296@wotan.suse.de> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.10.50.50] X-ClientProxiedBy: CIT-HM8-EX01.bmw-carit.intra (10.40.100.13) To CIT-HM8-EX01.bmw-carit.intra (10.40.100.13) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/08/2016 03:39 AM, Luis R. Rodriguez wrote: > On Wed, Sep 07, 2016 at 10:45:06AM +0200, Daniel Wagner wrote: >> From: Daniel Wagner >> timeout = MAX_JIFFY_OFFSET; >> } >> >> - retval = wait_for_completion_interruptible_timeout(&buf->completion, >> - timeout); >> + retval = fw_status_wait_timeout(&buf->fw_st, timeout); > > Note this is one of the users for fw_status_wait_timeout(). This makes sense > for this fw_status_wait_timeout, as its umh related. > >> if (retval == -ERESTARTSYS || !retval) { >> mutex_lock(&fw_lock); >> fw_load_abort(fw_priv); >> @@ -965,7 +1022,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, >> retval = 0; >> } >> >> - if (is_fw_load_aborted(buf)) >> + if (fw_status_is_aborted(&buf->fw_st)) >> retval = -EAGAIN; >> else if (buf->is_paged_buf && !buf->data) >> retval = -ENOMEM; >> @@ -1040,29 +1097,25 @@ fw_load_from_user_helper(struct firmware *firmware, const char *name, >> return -ENOENT; >> } >> >> -/* No abort during direct loading */ >> -#define is_fw_load_aborted(buf) false >> - >> #ifdef CONFIG_PM_SLEEP >> static inline void kill_requests_without_uevent(void) { } >> #endif >> >> #endif /* CONFIG_FW_LOADER_USER_HELPER */ >> >> - >> /* 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_status_is_done(&buf->fw_st)) { >> + if (fw_status_is_aborted(&buf->fw_st)) { >> ret = -ENOENT; >> break; >> } >> mutex_unlock(&fw_lock); >> - ret = wait_for_completion_interruptible(&buf->completion); >> + ret = fw_status_wait_timeout(&buf->fw_st, 0); > > Now this one -- I am not sure of. That it, it is not clear why we would > need fw_status_wait_timeout() here, and the code history does not reveal > that either. Obviously this is a no-op for for non UMH kenrels *but* > even for UMH -- why do we need it? This while loop was originally a goto loop: 1f2b79599ee8 ("firmware loader: always let firmware_buf own the pages buffer") I don't think the code doesn't do what it was indented to do. The reason is that calling complete_all() the wait_for_completion() will not block again until it has 'eaten up' the done counter. That is around UMAX/2 loops. So there is an reinit_completion() missing in this case. Before the above commit, the completion was used to wait inside _request_firmware_load() till the UML had done its job. The answer for your question is probably in 1f2b79599ee8 ("firmware loader: always let firmware_buf own the pages buffer"). /me starts reading. cheers, daniel