Re: [PATCH v1] Revert "Bluetooth: core: Fix missing power_on work cancel on HCI close"

From: Francesco Dolcini <francesco.dolcini@toradex.com>
To: Marcel Holtmann <marcel@holtmann.org>,
	Vasyl Vavrychuk <vasyl.vavrychuk@opensynergy.com>
Cc: Max Krummenacher <max.oss.09@gmail.com>,
	max.krummenacher@toradex.com,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>,
	Johan Hedberg <johan.hedberg@gmail.com>,
	Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
	Paolo Abeni <pabeni@redhat.com>,
	linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org,
	netdev@vger.kernel.org
Subject: Re: [PATCH v1] Revert "Bluetooth: core: Fix missing power_on work cancel on HCI close"
Date: Tue, 21 Jun 2022 13:55:14 +0200	[thread overview]
Message-ID: <20220621115514.GA75773@francesco-nb.int.toradex.com> (raw)
In-Reply-To: <20220614181706.26513-1-max.oss.09@gmail.com>

Marcel, Vasyl,
any comment on this?

Francesco

On Tue, Jun 14, 2022 at 08:17:06PM +0200, Max Krummenacher wrote:
> From: Max Krummenacher <max.krummenacher@toradex.com>
> 
> This reverts commit ff7f2926114d3a50f5ffe461a9bce8d761748da5.
> 
> The commit ff7f2926114d ("Bluetooth: core: Fix missing power_on work
> cancel on HCI close") introduced between v5.18 and v5.19-rc1 makes
> going to suspend freeze. v5.19-rc2 is equally affected.
> 
> This has been seen on a Colibri iMX6ULL WB which has a Marvell 8997
> based WiFi / Bluetooth module connected over SDIO.
> 
> With 'v5.18' or 'v5.19-rc1 with said commit reverted' a suspend/resume
> cycle looks as follows and the device is functional after the resume:
> 
> root@imx6ull:~# rfkill
> ID TYPE      DEVICE    SOFT      HARD
>  0 bluetooth hci0   blocked unblocked
>  1 wlan      phy0   blocked unblocked
> root@imx6ull:~# echo enabled > /sys/class/tty/ttymxc0/power/wakeup
> root@imx6ull:~# date;echo mem > /sys/power/state;date
> Tue Jun 14 14:43:03 UTC 2022
> [ 6393.464497] PM: suspend entry (deep)
> [ 6393.529398] Filesystems sync: 0.064 seconds
> [ 6393.594006] Freezing user space processes ... (elapsed 0.015 seconds) done.
> [ 6393.610266] OOM killer disabled.
> [ 6393.610285] Freezing remaining freezable tasks ... (elapsed 0.013 seconds) done.
> [ 6393.623727] printk: Suspending console(s) (use no_console_suspend to debug)
> 
> ~~ suspended until console initiates the resume
> 
> [ 6394.023552] fec 20b4000.ethernet eth0: Link is Down
> [ 6394.049902] PM: suspend devices took 0.300 seconds
> [ 6394.091654] Disabling non-boot CPUs ...
> [ 6394.565896] PM: resume devices took 0.440 seconds
> [ 6394.681350] OOM killer enabled.
> [ 6394.681369] Restarting tasks ... done.
> [ 6394.741157] random: crng reseeded on system resumption
> [ 6394.813135] PM: suspend exit
> Tue Jun 14 14:43:11 UTC 2022
> [ 6396.403873] fec 20b4000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
> [ 6396.404347] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> root@imx6ull:~#
> 
> With 'v5.19-rc1' suspend freezes in the suspend phase, i.e. power
> consumption is not lowered and no wakeup source initiates a wakup.
> 
> root@imx6ull:~# rfkill
> ID TYPE      DEVICE    SOFT      HARD
>  0 bluetooth hci0   blocked unblocked
>  1 wlan      phy0   blocked unblocked
> root@imx6ull:~# echo enabled > /sys/class/tty/ttymxc0/power/wakeup
> root@imx6ull:~# date;echo mem > /sys/power/state;date
> Tue Jun 14 12:40:38 UTC 2022
> [  122.476333] PM: suspend entry (deep)
> [  122.556012] Filesystems sync: 0.079 seconds
> 
> ~~ no further kernel output
> 
> If one first unbinds the bluetooth device driver, suspend / resume works
> as expected also with 'v5.19-rc1':
> 
> root@imx6ull:~# echo mmc1:0001:2 > /sys/bus/sdio/drivers/btmrvl_sdio/unbind
> root@imx6ull:~# rfkill
> ID TYPE DEVICE    SOFT      HARD
>  1 wlan phy0   blocked unblocked
> root@imx6ull:~# echo enabled > /sys/class/tty/ttymxc0/power/wakeup
> root@imx6ull:~# date;echo mem > /sys/power/state;date
> Tue Jun 14 14:59:26 UTC 2022
> [  123.530310] PM: suspend entry (deep)
> [  123.595432] Filesystems sync: 0.064 seconds
> [  123.672478] Freezing user space processes ... (elapsed 0.028 seconds) done.
> [  123.701848] OOM killer disabled.
> [  123.701869] Freezing remaining freezable tasks ... (elapsed 0.007 seconds) done.
> [  123.709993] printk: Suspending console(s) (use no_console_suspend to debug)
> [  124.097772] fec 20b4000.ethernet eth0: Link is Down
> [  124.124795] PM: suspend devices took 0.280 seconds
> [  124.165893] Disabling non-boot CPUs ...
> [  124.632959] PM: resume devices took 0.430 seconds
> [  124.750164] OOM killer enabled.
> [  124.750187] Restarting tasks ... done.
> [  124.827899] random: crng reseeded on system resumption
> [  124.923183] PM: suspend exit
> Tue Jun 14 14:59:31 UTC 2022
> [  127.520321] fec 20b4000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
> [  127.520514] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> root@imx6ull:~#
> 
> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> 
> ---
> 
>  net/bluetooth/hci_core.c | 2 ++
>  net/bluetooth/hci_sync.c | 1 -
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 59a5c1341c26..19df3905c5f8 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2675,6 +2675,8 @@ void hci_unregister_dev(struct hci_dev *hdev)
>  	list_del(&hdev->list);
>  	write_unlock(&hci_dev_list_lock);
>  
> +	cancel_work_sync(&hdev->power_on);
> +
>  	hci_cmd_sync_clear(hdev);
>  
>  	if (!test_bit(HCI_QUIRK_NO_SUSPEND_NOTIFIER, &hdev->quirks))
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 286d6767f017..1739e8cb3291 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -4088,7 +4088,6 @@ int hci_dev_close_sync(struct hci_dev *hdev)
>  
>  	bt_dev_dbg(hdev, "");
>  
> -	cancel_work_sync(&hdev->power_on);
>  	cancel_delayed_work(&hdev->power_off);
>  	cancel_delayed_work(&hdev->ncmd_timer);
>  
> -- 
> 2.20.1
> 