Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1731071 > unrolled thread

[PATCH 4.13 20/27] Revert "firmware: add sanity check on shutdown/suspend"

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4.13 20/27] Revert "firmware: add sanity check on shutdown/suspend" Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-09-12 19:10 +0200
    Re: [PATCH 4.13 20/27] Revert "firmware: add sanity check on  shutdown/suspend" "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-09-12 19:30 +0200
      Re: [PATCH 4.13 20/27] Revert "firmware: add sanity check on  shutdown/suspend" Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-09-13 02:50 +0200
        Re: [PATCH 4.13 20/27] Revert "firmware: add sanity check on  shutdown/suspend" "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-09-13 03:30 +0200
          Re: [PATCH 4.13 20/27] Revert "firmware: add sanity check on  shutdown/suspend" Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-09-13 03:50 +0200
        Re: [PATCH 4.13 20/27] Revert "firmware: add sanity check on shutdown/suspend" Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-13 06:20 +0200
          Re: [PATCH 4.13 20/27] Revert "firmware: add sanity check on  shutdown/suspend" "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-09-13 20:40 +0200
            Re: [PATCH 4.13 20/27] Revert "firmware: add sanity check on shutdown/suspend" Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-13 21:40 +0200
              Re: [PATCH 4.13 20/27] Revert "firmware: add sanity check on  shutdown/suspend" "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-09-13 23:50 +0200

#1731071 — [PATCH 4.13 20/27] Revert "firmware: add sanity check on shutdown/suspend"

4.13-stable review patch.  If anyone has any objections, please let me know.

------------------

From: Linus Torvalds <torvalds@linux-foundation.org>

commit f007cad159e99fa2acd3b2e9364fbb32ad28b971 upstream.

This reverts commit 81f95076281fdd3bc382e004ba1bce8e82fccbce.

It causes random failures of firmware loading at resume time (well,
random for me, it seems to be more reliable for others) because the
firmware disabling is not actually synchronous with any particular
resume event, and at least the btusb driver that uses a workqueue to
load the firmware at resume seems to occasionally hit the "firmware
loading is disabled" logic because the firmware loader hasn't gotten the
resume event yet.

Some kind of sanity check for not trying to load firmware when it's not
possible might be a good thing, but this commit was not it.

Greg seems to have silently suffered the same issue, and pointed to the
likely culprit, and Gabriel C verified the revert fixed it for him too.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Pointed-at-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Tested-by: Gabriel C <nix.or.die@gmail.com>
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
 Documentation/driver-api/firmware/request_firmware.rst |   11 -
 drivers/base/firmware_class.c                          |   99 -----------------
 2 files changed, 110 deletions(-)

--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -44,17 +44,6 @@ request_firmware_nowait
 .. kernel-doc:: drivers/base/firmware_class.c
    :functions: request_firmware_nowait
 
-Considerations for suspend and resume
-=====================================
-
-During suspend and resume only the built-in firmware and the firmware cache
-elements of the firmware API can be used. This is managed by fw_pm_notify().
-
-fw_pm_notify
-------------
-.. kernel-doc:: drivers/base/firmware_class.c
-   :functions: fw_pm_notify
-
 request firmware API expected driver use
 ========================================
 
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -256,38 +256,6 @@ static int fw_cache_piggyback_on_request
  * guarding for corner cases a global lock should be OK */
 static DEFINE_MUTEX(fw_lock);
 
-static bool __enable_firmware = false;
-
-static void enable_firmware(void)
-{
-	mutex_lock(&fw_lock);
-	__enable_firmware = true;
-	mutex_unlock(&fw_lock);
-}
-
-static void disable_firmware(void)
-{
-	mutex_lock(&fw_lock);
-	__enable_firmware = false;
-	mutex_unlock(&fw_lock);
-}
-
-/*
- * When disabled only the built-in firmware and the firmware cache will be
- * used to look for firmware.
- */
-static bool firmware_enabled(void)
-{
-	bool enabled = false;
-
-	mutex_lock(&fw_lock);
-	if (__enable_firmware)
-		enabled = true;
-	mutex_unlock(&fw_lock);
-
-	return enabled;
-}
-
 static struct firmware_cache fw_cache;
 
 static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
@@ -1239,12 +1207,6 @@ _request_firmware(const struct firmware
 	if (ret <= 0) /* error or already assigned */
 		goto out;
 
-	if (!firmware_enabled()) {
-		WARN(1, "firmware request while host is not available\n");
-		ret = -EHOSTDOWN;
-		goto out;
-	}
-
 	ret = fw_get_filesystem_firmware(device, fw->priv);
 	if (ret) {
 		if (!(opt_flags & FW_OPT_NO_WARN))
@@ -1755,62 +1717,6 @@ static void device_uncache_fw_images_del
 			   msecs_to_jiffies(delay));
 }
 
-/**
- * fw_pm_notify - notifier for suspend/resume
- * @notify_block: unused
- * @mode: mode we are switching to
- * @unused: unused
- *
- * Used to modify the firmware_class state as we move in between states.
- * The firmware_class implements a firmware cache to enable device driver
- * to fetch firmware upon resume before the root filesystem is ready. We
- * disable API calls which do not use the built-in firmware or the firmware
- * cache when we know these calls will not work.
- *
- * The inner logic behind all this is a bit complex so it is worth summarizing
- * the kernel's own suspend/resume process with context and focus on how this
- * can impact the firmware API.
- *
- * First a review on how we go to suspend::
- *
- *	pm_suspend() --> enter_state() -->
- *	sys_sync()
- *	suspend_prepare() -->
- *		__pm_notifier_call_chain(PM_SUSPEND_PREPARE, ...);
- *		suspend_freeze_processes() -->
- *			freeze_processes() -->
- *				__usermodehelper_set_disable_depth(UMH_DISABLED);
- *				freeze all tasks ...
- *			freeze_kernel_threads()
- *	suspend_devices_and_enter() -->
- *		dpm_suspend_start() -->
- *				dpm_prepare()
- *				dpm_suspend()
- *		suspend_enter()  -->
- *			platform_suspend_prepare()
- *			dpm_suspend_late()
- *			freeze_enter()
- *			syscore_suspend()
- *
- * When we resume we bail out of a loop from suspend_devices_and_enter() and
- * unwind back out to the caller enter_state() where we were before as follows::
- *
- * 	enter_state() -->
- *	suspend_devices_and_enter() --> (bail from loop)
- *		dpm_resume_end() -->
- *			dpm_resume()
- *			dpm_complete()
- *	suspend_finish() -->
- *		suspend_thaw_processes() -->
- *			thaw_processes() -->
- *				__usermodehelper_set_disable_depth(UMH_FREEZING);
- *				thaw_workqueues();
- *				thaw all processes ...
- *				usermodehelper_enable();
- *		pm_notifier_call_chain(PM_POST_SUSPEND);
- *
- * fw_pm_notify() works through pm_notifier_call_chain().
- */
 static int fw_pm_notify(struct notifier_block *notify_block,
 			unsigned long mode, void *unused)
 {
@@ -1824,7 +1730,6 @@ static int fw_pm_notify(struct notifier_
 		 */
 		kill_pending_fw_fallback_reqs(true);
 		device_cache_fw_images();
-		disable_firmware();
 		break;
 
 	case PM_POST_SUSPEND:
@@ -1837,7 +1742,6 @@ static int fw_pm_notify(struct notifier_
 		mutex_lock(&fw_lock);
 		fw_cache.state = FW_LOADER_NO_CACHE;
 		mutex_unlock(&fw_lock);
-		enable_firmware();
 
 		device_uncache_fw_images_delay(10 * MSEC_PER_SEC);
 		break;
@@ -1886,7 +1790,6 @@ static void __init fw_cache_init(void)
 static int fw_shutdown_notify(struct notifier_block *unused1,
 			      unsigned long unused2, void *unused3)
 {
-	disable_firmware();
 	/*
 	 * Kill all pending fallback requests to avoid both stalling shutdown,
 	 * and avoid a deadlock with the usermode_lock.
@@ -1902,7 +1805,6 @@ static struct notifier_block fw_shutdown
 
 static int __init firmware_class_init(void)
 {
-	enable_firmware();
 	fw_cache_init();
 	register_reboot_notifier(&fw_shutdown_nb);
 #ifdef CONFIG_FW_LOADER_USER_HELPER
@@ -1914,7 +1816,6 @@ static int __init firmware_class_init(vo
 
 static void __exit firmware_class_exit(void)
 {
-	disable_firmware();
 #ifdef CONFIG_PM_SLEEP
 	unregister_syscore_ops(&fw_syscore_ops);
 	unregister_pm_notifier(&fw_cache.pm_notify);

[toc] | [next] | [standalone]

#1731110 — Re: [PATCH 4.13 20/27] Revert "firmware: add sanity check on shutdown/suspend"

On Tue, Sep 12, 2017 at 10:00:00AM -0700, Greg Kroah-Hartman wrote:
> 4.13-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Linus Torvalds <torvalds@linux-foundation.org>
> 
> commit f007cad159e99fa2acd3b2e9364fbb32ad28b971 upstream.
> 
> This reverts commit 81f95076281fdd3bc382e004ba1bce8e82fccbce.

I'm not convinced reverting this commit is the right thing to do at
this point given it would seem other errors were happening prior to
v4.13 and this commit would rather just bring to light the core of
the issue which needs to be addressed.

If reverting this commit please consider reverting also commit
06a45a93e7d34a ("firmware: move umh try locks into the umh code").

> It causes random failures of firmware loading at resume time (well,
> random for me, it seems to be more reliable for others)

That randomness is expected, as well as the old issues without this
commit and commit 06a45a93e7d34a. By removing this commit though we
should also be introducing another vector of possible issues though,
and I'm afraid that it may be hard to predict when they were caused
by a race issue on resume.

> because the
> firmware disabling is not actually synchronous with any particular
> resume event, and at least the btusb driver that uses a workqueue to
> load the firmware at resume seems to occasionally hit the "firmware
> loading is disabled" logic because the firmware loader hasn't gotten the
> resume event yet.

It would seem that without these commits the issue was present also:

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1356076

These commits just bring forward the issues closer to attention. They
have my attention now and I am looking at a clean way to address it
with Marcel.

> Some kind of sanity check for not trying to load firmware when it's not
> possible might be a good thing, but this commit was not it.
> 
> Greg seems to have silently suffered the same issue, and pointed to the
> likely culprit, and Gabriel C verified the revert fixed it for him too.

If testing against old behaviour the right way would be to also revert
commit 06a45a93e7d34a ("firmware: move umh try locks into the umh code").

  Luis

> 
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Pointed-at-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Tested-by: Gabriel C <nix.or.die@gmail.com>
> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> ---
>  Documentation/driver-api/firmware/request_firmware.rst |   11 -
>  drivers/base/firmware_class.c                          |   99 -----------------
>  2 files changed, 110 deletions(-)
> 
> --- a/Documentation/driver-api/firmware/request_firmware.rst
> +++ b/Documentation/driver-api/firmware/request_firmware.rst
> @@ -44,17 +44,6 @@ request_firmware_nowait
>  .. kernel-doc:: drivers/base/firmware_class.c
>     :functions: request_firmware_nowait
>  
> -Considerations for suspend and resume
> -=====================================
> -
> -During suspend and resume only the built-in firmware and the firmware cache
> -elements of the firmware API can be used. This is managed by fw_pm_notify().
> -
> -fw_pm_notify
> -------------
> -.. kernel-doc:: drivers/base/firmware_class.c
> -   :functions: fw_pm_notify
> -
>  request firmware API expected driver use
>  ========================================
>  
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -256,38 +256,6 @@ static int fw_cache_piggyback_on_request
>   * guarding for corner cases a global lock should be OK */
>  static DEFINE_MUTEX(fw_lock);
>  
> -static bool __enable_firmware = false;
> -
> -static void enable_firmware(void)
> -{
> -	mutex_lock(&fw_lock);
> -	__enable_firmware = true;
> -	mutex_unlock(&fw_lock);
> -}
> -
> -static void disable_firmware(void)
> -{
> -	mutex_lock(&fw_lock);
> -	__enable_firmware = false;
> -	mutex_unlock(&fw_lock);
> -}
> -
> -/*
> - * When disabled only the built-in firmware and the firmware cache will be
> - * used to look for firmware.
> - */
> -static bool firmware_enabled(void)
> -{
> -	bool enabled = false;
> -
> -	mutex_lock(&fw_lock);
> -	if (__enable_firmware)
> -		enabled = true;
> -	mutex_unlock(&fw_lock);
> -
> -	return enabled;
> -}
> -
>  static struct firmware_cache fw_cache;
>  
>  static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
> @@ -1239,12 +1207,6 @@ _request_firmware(const struct firmware
>  	if (ret <= 0) /* error or already assigned */
>  		goto out;
>  
> -	if (!firmware_enabled()) {
> -		WARN(1, "firmware request while host is not available\n");
> -		ret = -EHOSTDOWN;
> -		goto out;
> -	}
> -
>  	ret = fw_get_filesystem_firmware(device, fw->priv);
>  	if (ret) {
>  		if (!(opt_flags & FW_OPT_NO_WARN))
> @@ -1755,62 +1717,6 @@ static void device_uncache_fw_images_del
>  			   msecs_to_jiffies(delay));
>  }
>  
> -/**
> - * fw_pm_notify - notifier for suspend/resume
> - * @notify_block: unused
> - * @mode: mode we are switching to
> - * @unused: unused
> - *
> - * Used to modify the firmware_class state as we move in between states.
> - * The firmware_class implements a firmware cache to enable device driver
> - * to fetch firmware upon resume before the root filesystem is ready. We
> - * disable API calls which do not use the built-in firmware or the firmware
> - * cache when we know these calls will not work.
> - *
> - * The inner logic behind all this is a bit complex so it is worth summarizing
> - * the kernel's own suspend/resume process with context and focus on how this
> - * can impact the firmware API.
> - *
> - * First a review on how we go to suspend::
> - *
> - *	pm_suspend() --> enter_state() -->
> - *	sys_sync()
> - *	suspend_prepare() -->
> - *		__pm_notifier_call_chain(PM_SUSPEND_PREPARE, ...);
> - *		suspend_freeze_processes() -->
> - *			freeze_processes() -->
> - *				__usermodehelper_set_disable_depth(UMH_DISABLED);
> - *				freeze all tasks ...
> - *			freeze_kernel_threads()
> - *	suspend_devices_and_enter() -->
> - *		dpm_suspend_start() -->
> - *				dpm_prepare()
> - *				dpm_suspend()
> - *		suspend_enter()  -->
> - *			platform_suspend_prepare()
> - *			dpm_suspend_late()
> - *			freeze_enter()
> - *			syscore_suspend()
> - *
> - * When we resume we bail out of a loop from suspend_devices_and_enter() and
> - * unwind back out to the caller enter_state() where we were before as follows::
> - *
> - * 	enter_state() -->
> - *	suspend_devices_and_enter() --> (bail from loop)
> - *		dpm_resume_end() -->
> - *			dpm_resume()
> - *			dpm_complete()
> - *	suspend_finish() -->
> - *		suspend_thaw_processes() -->
> - *			thaw_processes() -->
> - *				__usermodehelper_set_disable_depth(UMH_FREEZING);
> - *				thaw_workqueues();
> - *				thaw all processes ...
> - *				usermodehelper_enable();
> - *		pm_notifier_call_chain(PM_POST_SUSPEND);
> - *
> - * fw_pm_notify() works through pm_notifier_call_chain().
> - */
>  static int fw_pm_notify(struct notifier_block *notify_block,
>  			unsigned long mode, void *unused)
>  {
> @@ -1824,7 +1730,6 @@ static int fw_pm_notify(struct notifier_
>  		 */
>  		kill_pending_fw_fallback_reqs(true);
>  		device_cache_fw_images();
> -		disable_firmware();
>  		break;
>  
>  	case PM_POST_SUSPEND:
> @@ -1837,7 +1742,6 @@ static int fw_pm_notify(struct notifier_
>  		mutex_lock(&fw_lock);
>  		fw_cache.state = FW_LOADER_NO_CACHE;
>  		mutex_unlock(&fw_lock);
> -		enable_firmware();
>  
>  		device_uncache_fw_images_delay(10 * MSEC_PER_SEC);
>  		break;
> @@ -1886,7 +1790,6 @@ static void __init fw_cache_init(void)
>  static int fw_shutdown_notify(struct notifier_block *unused1,
>  			      unsigned long unused2, void *unused3)
>  {
> -	disable_firmware();
>  	/*
>  	 * Kill all pending fallback requests to avoid both stalling shutdown,
>  	 * and avoid a deadlock with the usermode_lock.
> @@ -1902,7 +1805,6 @@ static struct notifier_block fw_shutdown
>  
>  static int __init firmware_class_init(void)
>  {
> -	enable_firmware();
>  	fw_cache_init();
>  	register_reboot_notifier(&fw_shutdown_nb);
>  #ifdef CONFIG_FW_LOADER_USER_HELPER
> @@ -1914,7 +1816,6 @@ static int __init firmware_class_init(vo
>  
>  static void __exit firmware_class_exit(void)
>  {
> -	disable_firmware();
>  #ifdef CONFIG_PM_SLEEP
>  	unregister_syscore_ops(&fw_syscore_ops);
>  	unregister_pm_notifier(&fw_cache.pm_notify);
> 
> 
> 

-- 
Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg

[toc] | [prev] | [next] | [standalone]

#1731309 — Re: [PATCH 4.13 20/27] Revert "firmware: add sanity check on shutdown/suspend"

On Tue, Sep 12, 2017 at 07:20:08PM +0200, Luis R. Rodriguez wrote:
> On Tue, Sep 12, 2017 at 10:00:00AM -0700, Greg Kroah-Hartman wrote:
> > 4.13-stable review patch.  If anyone has any objections, please let me know.
> > 
> > ------------------
> > 
> > From: Linus Torvalds <torvalds@linux-foundation.org>
> > 
> > commit f007cad159e99fa2acd3b2e9364fbb32ad28b971 upstream.
> > 
> > This reverts commit 81f95076281fdd3bc382e004ba1bce8e82fccbce.
> 
> I'm not convinced reverting this commit is the right thing to do at
> this point given it would seem other errors were happening prior to
> v4.13 and this commit would rather just bring to light the core of
> the issue which needs to be addressed.

Given that this fixes a reported regression, yes, it is the right thing
to do.

> If reverting this commit please consider reverting also commit
> 06a45a93e7d34a ("firmware: move umh try locks into the umh code").

Ok, I can queue that revert up in my tree and will send it to Linus once
4.14-rc1 is out.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1731325 — Re: [PATCH 4.13 20/27] Revert "firmware: add sanity check on shutdown/suspend"

On Tue, Sep 12, 2017 at 05:47:58PM -0700, Greg Kroah-Hartman wrote:
> On Tue, Sep 12, 2017 at 07:20:08PM +0200, Luis R. Rodriguez wrote:
> > On Tue, Sep 12, 2017 at 10:00:00AM -0700, Greg Kroah-Hartman wrote:
> > > 4.13-stable review patch.  If anyone has any objections, please let me know.
> > > 
> > > ------------------
> > > 
> > > From: Linus Torvalds <torvalds@linux-foundation.org>
> > > 
> > > commit f007cad159e99fa2acd3b2e9364fbb32ad28b971 upstream.
> > > 
> > > This reverts commit 81f95076281fdd3bc382e004ba1bce8e82fccbce.
> > 
> > I'm not convinced reverting this commit is the right thing to do at
> > this point given it would seem other errors were happening prior to
> > v4.13 and this commit would rather just bring to light the core of
> > the issue which needs to be addressed.
> 
> Given that this fixes a reported regression, yes, it is the right thing
> to do.

Once 06a45a93e7d34a is also reverted I believe it may be revealed this was
not a regression after all.

> > If reverting this commit please consider reverting also commit
> > 06a45a93e7d34a ("firmware: move umh try locks into the umh code").
> 
> Ok, I can queue that revert up in my tree and will send it to Linus once
> 4.14-rc1 is out.

But lets find out.

  Luis

[toc] | [prev] | [next] | [standalone]

#1731326 — Re: [PATCH 4.13 20/27] Revert "firmware: add sanity check on shutdown/suspend"

On Wed, Sep 13, 2017 at 03:22:52AM +0200, Luis R. Rodriguez wrote:
> On Tue, Sep 12, 2017 at 05:47:58PM -0700, Greg Kroah-Hartman wrote:
> > On Tue, Sep 12, 2017 at 07:20:08PM +0200, Luis R. Rodriguez wrote:
> > > On Tue, Sep 12, 2017 at 10:00:00AM -0700, Greg Kroah-Hartman wrote:
> > > > 4.13-stable review patch.  If anyone has any objections, please let me know.
> > > > 
> > > > ------------------
> > > > 
> > > > From: Linus Torvalds <torvalds@linux-foundation.org>
> > > > 
> > > > commit f007cad159e99fa2acd3b2e9364fbb32ad28b971 upstream.
> > > > 
> > > > This reverts commit 81f95076281fdd3bc382e004ba1bce8e82fccbce.
> > > 
> > > I'm not convinced reverting this commit is the right thing to do at
> > > this point given it would seem other errors were happening prior to
> > > v4.13 and this commit would rather just bring to light the core of
> > > the issue which needs to be addressed.
> > 
> > Given that this fixes a reported regression, yes, it is the right thing
> > to do.
> 
> Once 06a45a93e7d34a is also reverted I believe it may be revealed this was
> not a regression after all.

Then we will put it back :)

[toc] | [prev] | [next] | [standalone]

#1731368

On Tue, Sep 12, 2017 at 5:47 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
>> If reverting this commit please consider reverting also commit
>> 06a45a93e7d34a ("firmware: move umh try locks into the umh code").
>
> Ok, I can queue that revert up in my tree and will send it to Linus once
> 4.14-rc1 is out.

I want to see a _reason_ for that revert. The two have absolutely
nothing to do with each other., Reverting one is *not* a reason for
reverting the other.

Commit 06a45a93e7d34a seems to be a cleanup. The arguments in
06a45a93e7d3 ("firmware: move umh try locks into the umh code") seem
valid, and there's no real reason to worry about that FW_OPT_NOWAIT
etc for the direct-from-filesystem loading. That's simply not
sensible.

That whole FW_OPT_NOWAIT thing only makes sense for the actual
user-mode helper, so that commit actually seems to move the testing
and the logic to a place that really does make sense.

So why would we revert a commit that makes SENSE?

In contrast, the commit that already got reverted, added random
locking state logic associated with a callback that didn't actually
make any sense in that context.

Honestly, what we need here is less "let's do random changes", and
more "have actual reasons for those changes".

For example, I'm the first to tell people that they shouldn't just try
to load firmware from random contexts. If you're bringing up a device,
loading firmware might depend on *another* device that hasn't been
brought up yet, and you might end up with essentially a deadlock
waiting for IO from another device that just isn't going to happen.

But we shouldn't fix that by then adding a flag that gets set
asynchronously by some random callback, and that then causes a
workqueue entry that *could* sleep to just fail. There's no "root
cause logic" there. That's the wrong kind of random change. There may
be a reason for it in some situation, but in another situation it just
fails for no good reason.

For example, what might be senseible is to add a warning that tries to
verify that people do *not* do firmware loads from the ->resume()
callbacks. But then it should literally check *that*: it could do
something like

         WARN_ON_ONCE(current == resume_thread, "Firmware loading
called synchronously during resume");

or whatever, exactly because it's obviously *not* ok to block the same
process that is going to resume all the other devices that might be
*needed* for the firmware loading.

But on the other hand, if somebody then does an independent thread to
resume their firmware, we could just block to wait for it - it
wouldn't be the same kind of chicken-and-egg issue with IO at resume
time.

So instead of "arbitrary rules", there should be things that actually
make sense.

The commit that Luis now argues for _also_ reverting makes a lot of
sense to me to keep. I'm not seeing why that should be reverted, when
the _only_ reason seems to be some spiteful "well, if you reverted one
commit, you should randomly revert another one too".

               Linus

[toc] | [prev] | [next] | [standalone]

#1731773 — Re: [PATCH 4.13 20/27] Revert "firmware: add sanity check on shutdown/suspend"

Rafeal a question for you below.

On Tue, Sep 12, 2017 at 09:11:46PM -0700, Linus Torvalds wrote:
> On Tue, Sep 12, 2017 at 5:47 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> >> If reverting this commit please consider reverting also commit
> >> 06a45a93e7d34a ("firmware: move umh try locks into the umh code").
> >
> > Ok, I can queue that revert up in my tree and will send it to Linus once
> > 4.14-rc1 is out.
> 
> I want to see a _reason_ for that revert. The two have absolutely
> nothing to do with each other., Reverting one is *not* a reason for
> reverting the other.

There is a dependency between both commits, the reason is not obvious though.
I'll explain below.

> Commit 06a45a93e7d34a seems to be a cleanup. The arguments in
> 06a45a93e7d3 ("firmware: move umh try locks into the umh code") seem
> valid, and there's no real reason to worry about that FW_OPT_NOWAIT
> etc for the direct-from-filesystem loading. That's simply not
> sensible.

Indeed! That stupid UMH lock *seems* wrong on the direct filesystem path!

Hence these changes.

The devil is in the details though. That UMH lock however carried an implicit
suspend guard, the "cleanup" actually then has a functional change. The commit
which was reverted provided the safe guard in generic form, in case we already
had become dependent on the suspend guard. This UMH lock on the direct FS path
then added an implicit "arbitrary rule", as you put it, on the firmware API.

Commit 81f95076281fd clarified and made it explicit just to be safe.

*Only* carrying 06a45a93e7d34a then was a long term goal, eventually I intended
to remove the code which you removed, but I would have preferred to have waited
at least a release.

By only having 06a45a93e7d34a we then loose the suspend guard from the direct
FS lookup path immediately. If we're happy to live with that right away then
great, this may be one of those *random arbitrary rules* inherited worth
removing, it was a long term goal to remove that code, however I just want
folks to be very well aware of the original goal: 06a45a93e7d34a without
81f95076281fd is not a cleanup, there is a hidden functional change there.

So to be clear: I'm happy with 81f95076281fd reverted as it was my long term
goal, however I would not be doing my job if I were not explaining that
06a45a93e7d34a *has* a functional change and that 81f95076281fd was an
attempt at a safe baby step forward.

> That whole FW_OPT_NOWAIT thing only makes sense for the actual
> user-mode helper, so that commit actually seems to move the testing
> and the logic to a place that really does make sense.

Indeed!

> So why would we revert a commit that makes SENSE?

Because although the UMH lock should only be used for code which needs the UMH
helpers, the UMH lock also had added a suspend guard which we grew to rely on.
Removing it could mean allowing races on resume, and there was no clear way
generically detecting this. The UMH lock was *also* never used on *any other*
UMH code, so from the UMH perspective it also begs the question if the other
UMH code is error prone as it lacks the safe UMH locking safe guards.

> For example, what might be senseible is to add a warning that tries to
> verify that people do *not* do firmware loads from the ->resume()
> callbacks.

The firmware API *allows* for resume() callbacks to use the firmware API in
ways which *should* not block much if any at all, however they must have first
at least called the firmware API once so that the device gets a devres entry
with a string associated; this is the firmware cache. Then upon suspend the
firmware API requests for each of these firmwares.  If a resume() callback then
calls the firmware API for any of these files it would work without issue as
the firmware is loaded in the cache, as simple pointer reference. Processing
and consuming the firmware on the driver though *can* take a while and block
longer.

Because of the firmware cache implementation then we needed a whitelist, so the
check which I implemented in 81f95076281fd *only* complains if we've passed the
firmware cache check, and we *know* then the incoming call is new.

> But then it should literally check *that*: it could do
> something like
> 
>          WARN_ON_ONCE(current == resume_thread, "Firmware loading
> called synchronously during resume");

Sure that would be *much* smaller code.

Rafael do you have helpers for this sort of thing or are OK with them being
added?

> or whatever, exactly because it's obviously *not* ok to block the same
> process that is going to resume all the other devices that might be
> *needed* for the firmware loading.

Your interest in this seems to be the blocking implications on synchronous
calls. That's certainly a new concern I had not heard raised yet and is
worthwhile addressing. Are you not concerned in any way though about the
filesystem being ready?

> But on the other hand, if somebody then does an independent thread to
> resume their firmware, we could just block to wait for it - it
> wouldn't be the same kind of chicken-and-egg issue with IO at resume
> time.
> 
> So instead of "arbitrary rules", there should be things that actually
> make sense.

The arbitrary rule here was hidden underneath code Rafael added years ago.
I'm pretty sure he did not intend to add the check on the direct FS path
as direct FS path came later as the code evolved, *but* we kept the lock
on the direct FS path so we have no option but to review carefully its
removal.

> The commit that Luis now argues for _also_ reverting makes a lot of
> sense to me to keep. I'm not seeing why that should be reverted, when
> the _only_ reason seems to be some spiteful "well, if you reverted one
> commit, you should randomly revert another one too".

I hope to have clarified there is no spite here, I'm simply being very
careful to avoid a regression with a real hidden functional change.

  Luis

[toc] | [prev] | [next] | [standalone]

#1731808

On Wed, Sep 13, 2017 at 11:38 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>> Commit 06a45a93e7d34a seems to be a cleanup. The arguments in
>> 06a45a93e7d3 ("firmware: move umh try locks into the umh code") seem
>> valid, and there's no real reason to worry about that FW_OPT_NOWAIT
>> etc for the direct-from-filesystem loading. That's simply not
>> sensible.
>
> Indeed! That stupid UMH lock *seems* wrong on the direct filesystem path!
>
> Hence these changes.
>
> The devil is in the details though. That UMH lock however carried an implicit
> suspend guard, the "cleanup" actually then has a functional change. The commit
> which was reverted provided the safe guard in generic form, in case we already
> had become dependent on the suspend guard. This UMH lock on the direct FS path
> then added an implicit "arbitrary rule", as you put it, on the firmware API.

I still refuse to revert a commit "just because".

It improved the code.

There is no actual sign that it causes problems.

Yes, moving the lock may change behavior, but nothing you say actually
makes me think it regressed anything.

I refuse to do this "let's just go back to what we used to have,
without an actual _reason_ to go back to it".

The old user-mode-helper crap was crap that had its own totally
separate issues. Even without the problems that we had with incredibly
bad maintainership of the user mode side, the usermode helper had
serious issues with just the fact that user mode itself is disabled by
the suspend/resume process.

So getting rid of the locking that was added for usermode helper ion
the direct file loading path makes a lot of sense.

Just saying that "we tried to re-introduce that locking in another
form, and that was a mistake, and got reverted" is *not* a reason to
then revert the movement.

Yes, the movement of the locking might need to be reverted too - but
only if it actually shows problems.

We should *not* go back to old code "just because".

            Linus

[toc] | [prev] | [next] | [standalone]

#1731945 — Re: [PATCH 4.13 20/27] Revert "firmware: add sanity check on shutdown/suspend"

On Wed, Sep 13, 2017 at 12:30:44PM -0700, Linus Torvalds wrote:
> Yes, the movement of the locking might need to be reverted too - but
> only if it actually shows problems.

This speeds up the cleanup of that crap UMH code so I'm happy to
wait to hear if just the move the UMH lock it creates an issue.

I suspect it wont.

  Luis

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web