linux-kernel - Re: [PATCH] PM: add: move warn message out of mutex lock.

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <128c29b9.67bf.191b17d5148.Coremail.00107082@163.com>
Date: Mon, 2 Sep 2024 14:47:45 +0800 (CST)
From: "David Wang" <00107082@....com>
To: "Greg KH" <gregkh@...uxfoundation.org>
Cc: rafael@...nel.org, len.brown@...el.com, pavel@....cz, 
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] PM: add: move warn message out of mutex lock.

Hi, 

Thanks for reviewing~
At 2024-09-02 14:31:35, "Greg KH" <gregkh@...uxfoundation.org> wrote:
>On Mon, Sep 02, 2024 at 01:49:59PM +0800, David Wang wrote:
>> dpm_list_mtx does not protect any data used by
>> dev_warn for checking parent's power, move
>> dev_warn out of mutex lock block make the
>> lock more efficient, especially when the warn
>> is triggered. This can happen on some HW when
>> resume from suspend with USB camera opened:
>
>Please wrap changelog lines at 72 columns if possible.
>
>>  >usb 3-1.1: reset high-speed USB device number 4 using xhci_hcd
>>  >..
>>  >ep_81: PM: parent 3-1.1:1.1 should not be sleeping
>> 
>> Signed-off-by: David Wang <00107082@....com>
>> ---
>>  drivers/base/power/main.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>> index 4a67e83300e1..934e5bb61f13 100644
>> --- a/drivers/base/power/main.c
>> +++ b/drivers/base/power/main.c
>> @@ -134,10 +134,10 @@ void device_pm_add(struct device *dev)
>>  	pr_debug("Adding info for %s:%s\n",
>>  		 dev->bus ? dev->bus->name : "No Bus", dev_name(dev));
>>  	device_pm_check_callbacks(dev);
>> -	mutex_lock(&dpm_list_mtx);
>>  	if (dev->parent && dev->parent->power.is_prepared)
>>  		dev_warn(dev, "parent %s should not be sleeping\n",
>>  			dev_name(dev->parent));
>> +	mutex_lock(&dpm_list_mtx);
>
>I do not understand how this change will remove the offending log
>message.  It should be safe to hold the lock while the check happens and
>the message is printed out, you should not see any functional change at
>all.
>
>So are you sure this is needed?

This patch does not fix anything, the warning is still there and indeed no functional change at all.
It is more of a code refactor: when I follow the kernel warn on my system and check the code, I 
feel its better to move dev_warn out of the lock section since the lock is not meant to protect it, right?
And I mention the warning message in the commit log because I think it would make the lock-holding unnecessarily
longer when the warning do happen. 

>
>thanks,
>
>greg k-h