| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2024022023-anemic-icy-6f3d@gregkh>
Date: Tue, 20 Feb 2024 08:47:28 +0100
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: buckzhangwh@...il.com
Cc: linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drivers:usb:disable usb hub&port async suspend to avoid
block system PM
On Mon, Feb 19, 2024 at 11:24:13PM -0800, buckzhangwh@...il.com wrote:
> From: weihui zhang <buckzhangwh@...il.com>
>
> many phones are crashed and unable to wake up by power key.
> We analyzed more than ten kernel-dumps,
Odd trailing whitespace :(
> the common was that system were all blocked by usb.
> the phone doesn't crash again,after we disable usb hub&port async suspend.
That's not ok to disable, why is this needed?
>
> here is one kernel-dump analysis case:
> task 446 & 4511 &365 state are Uninterrupt
> task 446 dpm_wait_for_superior then schedule out.
>
> PID: 446 TASK: ffffff81f3e9cb00 CPU: 4 COMMAND: "charge"
>
> #0 [ffffffc015a0b9a0] __switch_to at ffffffc010088b54
>
> #1 [ffffffc015a0b9f0] __schedule at ffffffc010f7ef2c
>
> #2 [ffffffc015a0ba40] schedule at ffffffc010f7f3a4
>
> #3 [ffffffc015a0baa0] async_synchronize_cookie_domain at
>
> #4 [ffffffc015a0bb00] async_synchronize_full at ffffffc010131e0c
>
> #5 [ffffffc015a0bb10] dpm_resume at ffffffc0107ec408
>
> #6 [ffffffc015a0bb70] dpm_resume_end at ffffffc0107ed0e8
>
> #7 [ffffffc015a0bbb0] suspend_devices_and_enter at ffffffc0101776d8
>
> #8 [ffffffc015a0bbf0] enter_state at ffffffc010177e70
>
> #9 [ffffffc015a0bc20] pm_suspend at ffffffc010177d5c
>
> PID: 4511 TASK: ffffff8153b0e740 CPU: 6 COMMAND: "kworker/u16:11"
>
> #0 [ffffffc01bc9baa0] __switch_to at ffffffc01010293c
>
> #1 [ffffffc01bc9bb10] __schedule at ffffffc0116b0008
>
> #2 [ffffffc01bc9bb70] schedule at ffffffc0116b0794
>
> #3 [ffffffc01bc9bbe0] schedule_timeout at ffffffc0116b6c2c
>
> #4 [ffffffc01bc9bc40] wait_for_common at ffffffc0116b198c
>
> #5 [ffffffc01bc9bca0] dpm_wait_for_superior at ffffffc010b999e8
>
> #6 [ffffffc01bc9bce0] device_resume at ffffffc010b9d804
> //x0(struct device)= ffffff81f32a3808
>
> #7 [ffffffc01bc9bd10] async_resume at ffffffc010b9d6ec
>
> #8 [ffffffc01bc9bd40] async_run_entry_fn at ffffffc0101e238c
>
> #9 [ffffffc01bc9bdb0] process_one_work at ffffffc0101d00e0
>
> crash_arm64> struct device ffffff81f32a3808 -x
>
> struct device {
>
> kobj = {
>
> name = 0xffffff81f15c6d80 "usb1-port1",
>
> entry = {
>
> next = 0xffffff81f32a6018,
>
> prev = 0xffffff81e4103838
>
> },
> type = 0xffffffc0124102b8 <usb_port_device_type>,
>
> bus = 0x0,
>
> driver = 0xffffffc0124102e8 <usb_port_driver>,
>
> async_suspend = 0x1,
> ...........
All of the changelog text has extra lines? Well some of them do, why?
Please fix that up.
>
> PID: 365 TASK: ffffff81f3bf6900 CPU: 0 COMMAND: "kworker/u16:5"
>
> #0 [ffffffc0158dbac0] __switch_to at ffffffc010088b54
>
> #1 [ffffffc0158dbb10] __schedule at ffffffc010f7ef2c
>
> #2 [ffffffc0158dbb60] schedule at ffffffc010f7f3a4
>
> #3 [ffffffc0158dbbe0] schedule_timeout at ffffffc010f83fd8
>
> #4 [ffffffc0158dbc40] wait_for_common at ffffffc010f80600
>
> #5 [ffffffc0158dbc90] wait_for_completion at ffffffc010f8051c
>
> #6 [ffffffc0158dbcb0] dpm_wait_for_superior at ffffffc0107eee00
>
> #7 [ffffffc0158dbd00] device_resume at ffffffc0107ec6a8
> //x0(struct device) = 0xffffff81ee1b3030
>
> #8 [ffffffc0158dbd40] async_resume at ffffffc0107ec588
>
> #9 [ffffffc0158dbd60] async_run_entry_fn at ffffffc010131c60
>
> crash_arm64> struct device ffffff81ee1b3030 -x
>
> struct device {
>
> kobj = {
>
> name = 0xffffff81f1fc1500 "1-0:1.0",
>
> type = 0xffffffc011b62a38 <usb_if_device_type>,
> ............
I don't understand, why the kobject structure stuff here?
>
> Signed-off-by: weihui zhang <buckzhangwh@...il.com>
> ---
> drivers/usb/core/hub.c | 2 +-
> drivers/usb/core/message.c | 2 +-
> drivers/usb/core/port.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index e38a4124f..de74f70e5 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2602,7 +2602,7 @@ int usb_new_device(struct usb_device *udev)
> add_device_randomness(udev->manufacturer,
> strlen(udev->manufacturer));
>
> - device_enable_async_suspend(&udev->dev);
> + device_disable_async_suspend(&udev->dev);
Why? You just changed the way the whole system worked?
Are you sure that the device itself shouldn't be fixed instead of doing
this for ALL devices?
>
> /* check whether the hub or firmware marks this port as non-removable */
> set_usb_port_removable(udev);
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 077dfe48d..944f01aa7 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -2203,7 +2203,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
> "adding %s (config #%d, interface %d)\n",
> dev_name(&intf->dev), configuration,
> intf->cur_altsetting->desc.bInterfaceNumber);
> - device_enable_async_suspend(&intf->dev);
> + device_disable_async_suspend(&intf->dev);
> ret = device_add(&intf->dev);
> if (ret != 0) {
> dev_err(&dev->dev, "device_add(%s) --> %d\n",
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index c628c1abc..97696c415 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
> @@ -760,7 +760,7 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
> pm_runtime_set_active(&port_dev->dev);
> pm_runtime_get_noresume(&port_dev->dev);
> pm_runtime_enable(&port_dev->dev);
> - device_enable_async_suspend(&port_dev->dev);
> + device_disable_async_suspend(&port_dev->dev);
Again, I don't think this is a wise change, how did you test it?
thanks,
greg k-h
Powered by blists - more mailing lists