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

[David Wang]

Hi, 

Thanks for reviewing~
At 2024-09-02 14:31:35, "Greg KH" <gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxx>
>> ---
>>  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