From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751012AbdAPS3T (ORCPT ); Mon, 16 Jan 2017 13:29:19 -0500 Received: from mx2.suse.de ([195.135.220.15]:33095 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750788AbdAPS3R (ORCPT ); Mon, 16 Jan 2017 13:29:17 -0500 Date: Mon, 16 Jan 2017 19:29:12 +0100 From: "Luis R. Rodriguez" To: Jakub Kicinski Cc: gregkh@linuxfoundation.org, Bjorn Andersson , Daniel Wagner , "Luis R . Rodriguez" , Ming Lei , linux-kernel@vger.kernel.org, oss-drivers@netronome.com Subject: Re: [PATCH driver-core/master] firmware: Correct handling of fw_state_wait_timeout() return value Message-ID: <20170116182912.GA13946@wotan.suse.de> References: <20170116145706.19198-1-jakub.kicinski@netronome.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170116145706.19198-1-jakub.kicinski@netronome.com> 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 On Mon, Jan 16, 2017 at 02:57:06PM +0000, Jakub Kicinski wrote: > Commit 5d47ec02c37e ("firmware: Correct handling of fw_state_wait() > return value") made the assumption that any error returned from > fw_state_wait_timeout() means FW load has to be aborted. This is > incorrect FW load only has to be aborted when load timed out or You want a comma before FW -- but also: > has been interrupted, __fw_state_wait_common() returns -ENOENT when: if (ret != 0 && fw_st->status == FW_STATUS_ABORTED) return -ENOENT; Why not for when -ENOENT is returned ? > otherwise the waking thread had already > cleaned up for us. What code in what waking thread would have done precisely what cleanup? And why can't fw_load_abort() handle being called twice and why not just instead allow for that? > Fixes: 5d47ec02c37e ("firmware: Correct handling of fw_state_wait() return value") What does this fix exactly? A fix should describe the impact, what issues are in place without the fix. What also happens after the fix and why. In this commit log none of this is clear. > Signed-off-by: Jakub Kicinski > --- > drivers/base/firmware_class.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > index 4497d263209f..ce142e6b2c72 100644 > --- a/drivers/base/firmware_class.c > +++ b/drivers/base/firmware_class.c > @@ -1020,7 +1020,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, > } > > retval = fw_state_wait_timeout(&buf->fw_st, timeout); > - if (retval < 0) { > + if (retval == -ETIMEDOUT || retval == -ERESTARTSYS) { Also, if your change is correct I will also note fw_state_wait_timeout() is just a wrapper for __fw_state_wait_common(), but we also have another wrapper for __fw_state_wait_common() now: #define fw_state_wait(fw_st) \ __fw_state_wait_common(fw_st, MAX_SCHEDULE_TIMEOUT) Do we need to fix anything for fw_state_wait() ? Clarifying all this would help review your proposed changes. If you consider them a fix please be very clear as to the exact issue and what is fixed with your patch. Luis