Re: [PATCH 1/5] firmware: share fw fallback killing on reboot/suspend

[Coelho, Luciano]

Hi Luis,


On Wed, 2017-03-29 at 20:24 -0700, Luis R. Rodriguez wrote:
> We kill pending fallback requests on suspend and reboot,
> the only difference is that on suspend we only kill custom
> fallback requests. Provide a wrapper that lets us customize
> the request with a flag.
> 
> This also lets us simplify the #ifdef'ery over the calls.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
> ---
>  drivers/base/firmware_class.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index ac350c518e0c..d2e2d83aaf26 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -562,16 +562,15 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
>  
>  static LIST_HEAD(pending_fw_head);
>  
> -/* reboot notifier for avoid deadlock with usermode_lock */
>  static int fw_shutdown_notify(struct notifier_block *unused1,
>  			      unsigned long unused2, void *unused3)
>  {
> -	mutex_lock(&fw_lock);
> -	while (!list_empty(&pending_fw_head))
> -		__fw_load_abort(list_first_entry(&pending_fw_head,
> -					       struct firmware_buf,
> -					       pending_list));
> -	mutex_unlock(&fw_lock);
> +	/*
> +	 * Kill all pending fallback requests to avoid both stalling shutdown,
> +	 * and avoid a deadlock with the usermode_lock.
> +	 */
> +	kill_pending_fw_fallback_reqs(false);

You are calling this function before you declare it, this won't compile
when CONFIG_FW_LOADER_USER_HELPER is not set.



> +
>  	return NOTIFY_DONE;
>  }
>  
> @@ -1048,21 +1047,20 @@ static int fw_load_from_user_helper(struct firmware *firmware,
>  	return _request_firmware_load(fw_priv, opt_flags, timeout);
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
> -/* kill pending requests without uevent to avoid blocking suspend */
> -static void kill_requests_without_uevent(void)
> +static void kill_pending_fw_fallback_reqs(bool only_kill_custom)
>  {
>  	struct firmware_buf *buf;
>  	struct firmware_buf *next;
>  
>  	mutex_lock(&fw_lock);
>  	list_for_each_entry_safe(buf, next, &pending_fw_head, pending_list) {
> -		if (!buf->need_uevent)
> +		if (!only_kill_custom)
> +			 __fw_load_abort(buf);
> +		else if (!buf->need_uevent)
>  			 __fw_load_abort(buf);
Why not use this?

	if (!only_kill_custom || !buf->need_uevent)
		__fw_load_abort(buf);

--
Cheers,
Luca.