Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback

[Martin Fuzzey]

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