Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751468AbdFFJEq (ORCPT <rfc822;w@1wt.eu>);
        Tue, 6 Jun 2017 05:04:46 -0400
Received: from mail-wm0-f43.google.com ([74.125.82.43]:34989 "EHLO
        mail-wm0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751437AbdFFJEm (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 6 Jun 2017 05:04:42 -0400
Subject: Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on
 fallback
To: "Luis R. Rodriguez" <mcgrof@kernel.org>,
        Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        Peter Zijlstra <peterz@infradead.org>, Alan Cox <alan@linux.intel.com>,
        Jonathan Corbet <corbet@lwn.net>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>,
        Andy Lutomirski <luto@kernel.org>,
        Greg KH <gregkh@linuxfoundation.org>
References: <20170524214027.7775-1-mcgrof@kernel.org>
 <CALCETrXUrirO-vg3M+MGhn=0gZTwx0phsRDS4TCwWWgNYC6RsA@mail.gmail.com>
 <CAB=NE6VENGmTEZ3_8EWmv6bhT81k=szg0_POWJm2SP+V21CeBw@mail.gmail.com>
 <CALCETrU4__YUGk36PN=FbuEf0SBaTrxQQqm4sWs2NrZ+6WN7jA@mail.gmail.com>
 <CANh8QzwPb_+RKs5QVt7mdFk8h_rOMVS3j9m0OADgvzBtNqBBLg@mail.gmail.com>
 <87fufr3mdy.fsf@xmission.com> <20170526194640.GS8951@wotan.suse.de>
 <CAKdAkRTrcTVOAP5GK-R=Au_tL5WqSn5UkQEzNe5NcCWXS8mbtA@mail.gmail.com>
 <CAB=NE6Wh9inuA-R9-zegnvJV=s_6o_doMNDOhBVhYn4ZQ_Ku5Q@mail.gmail.com>
 <20170526215518.GB40877@dtor-ws> <20170605202410.GQ8951@wotan.suse.de>
Cc: Linux API <linux-api@vger.kernel.org>, Daniel Wagner <wagi@monom.org>,
        David Woodhouse <dwmw2@infradead.org>, jewalt@lgsinnovations.com,
        rafal@milecki.pl, Arend Van Spriel <arend.vanspriel@broadcom.com>,
        "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        "Li, Yi" <yi1.li@linux.intel.com>, atull@opensource.altera.com,
        Moritz Fischer <moritz.fischer@ettus.com>,
        Petr Mladek <pmladek@suse.com>,
        Johannes Berg <johannes.berg@intel.com>,
        Emmanuel Grumbach <emmanuel.grumbach@intel.com>,
        Luca Coelho <luciano.coelho@intel.com>,
        Kalle Valo <kvalo@codeaurora.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Kees Cook <keescook@chromium.org>,
        AKASHI Takahiro <takahiro.akashi@linaro.org>,
        David Howells <dhowells@redhat.com>, Peter Jones <pjones@redhat.com>,
        Hans de G oede <hdegoede@redhat.com>, "Ted Ts'o" <tytso@mit.edu>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
From: Martin Fuzzey <mfuzzey@parkeon.com>
Message-ID: <59367025.3020901@parkeon.com>
Date: Tue, 6 Jun 2017 11:04:37 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.3.0
MIME-Version: 1.0
In-Reply-To: <20170605202410.GQ8951@wotan.suse.de>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3513
Lines: 107

On 05/06/17 22:24, Luis R. Rodriguez wrote:
>
>
> For these two reasons then it would seem best we do two things actually:
>
> 1) return -EINTR instead of -EAGAIN when we detect swait_event_interruptible_timeout()
> got interrupted by a signal (it returns -ERESTARTSYS)


I disagree. That would force userspace to handle the signal rather than 
having the kernel retry.

 From Documentation/DocBook/kernel-hacking.tmpl:

    After you slept you should check if a signal occurred: the
    Unix/Linux way of handling signals is to temporarily exit the
    system call with the <constant>-ERESTARTSYS</constant> error.  The
    system call entry code will switch back to user context, process
    the signal handler and then your system call will be restarted
    (unless the user disabled that).  So you should be prepared to
    process the restart, e.g. if you're in the middle of manipulating
    some data structure.



> 2) Do as you note below and add wait_event_killable_timeout()

Hum,
I do think that would be better but, (please correct me if I'm wrong) 
the _killable_ variants only allow
SIGKILL  (and not SIGINT).

0cb64249ca "firmware_loader: abort request if wait_for_completion is 
interrupted"

specifically mentrions ctrl-c (SIGINT) in the commit message so that 
would no longer work.

Myself I think having to use kill -9 to interrupt firmware loading by a 
usespace helper is OK but others may disagree.

> I do not see why we could not introduce wait_event_killable_timeout()
> and swait_event_killable_timeout() into -stables.
> After seeing how simple it is to do so I tend to agree. Greg, Peter,
> what are your thoughts ?
>
> Martin Fuzzey can you test this patch as an alternative to your issue ?
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index b9f907eedbf7..70fc42e5e0da 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -131,7 +131,7 @@ static int __fw_state_wait_common(struct fw_state *fw_st, long timeout)
>   {
>   	long ret;
>   
> -	ret = swait_event_interruptible_timeout(fw_st->wq,
> +	ret = swait_event_killable_timeout(fw_st->wq,
>   				__fw_state_is_done(READ_ONCE(fw_st->status)),
>   				timeout);
>   	if (ret != 0 && fw_st->status == FW_STATUS_ABORTED)
> diff --git a/include/linux/swait.h b/include/linux/swait.h
> index c1f9c62a8a50..9c5ca2898b2f 100644
> --- a/include/linux/swait.h
> +++ b/include/linux/swait.h
> @@ -169,4 +169,29 @@ do {									\
>   	__ret;								\
>   })
>   
> +#define __swait_event_killable(wq, condition)				\
> +	(void)___swait_event(wq, condition, TASK_KILLABLE, 0, schedule())
> +
> +#define swait_event_killable(wq, condition)				\
> +({									\
> +	int __ret = 0;							\
> +	if (!(condition))						\
> +		__ret = __swait_event_killable(wq, condition);		\
> +	__ret;								\
> +})
> +
> +#define __swait_event_killable_timeout(wq, condition, timeout)		\
> +	___swait_event(wq, ___wait_cond_timeout(condition),		\
> +		      TASK_INTERRUPTIBLE, timeout,			\
> +		      __ret = schedule_timeout(__ret))
> +

Should be TASK_KILLABLE above

> +#define swait_event_killable_timeout(wq, condition, timeout)		\
> +({									\
> +	long __ret = timeout;						\
> +	if (!___wait_cond_timeout(condition))				\
> +		__ret = __swait_event_killable_timeout(wq,		\
> +						condition, timeout);	\
> +	__ret;								\
> +})
> +
>   #endif /* _LINUX_SWAIT_H */
>
>    Luis

After replacing TASK_INTERRUPTIBLE with TASK_KILLABLE above it works for me.


Regards,

Martin