Message-ID: <ffc7e137-8c0a-2151-cfb6-b41ae53cadf2@intel.com>
Date:   Thu, 16 Dec 2021 19:24:13 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.4.0
Subject: Re: [PATCH] PM: sleep: Avoid calling put_device() under dpm_list_mtx
Content-Language: en-US
To:     =?UTF-8?Q?Thomas_Hellstr=c3=b6m?= 
        <thomas.hellstrom@linux.intel.com>
CC:     LKML <linux-kernel@vger.kernel.org>,
        Intel Graphics Development <intel-gfx@lists.freedesktop.org>
References: <77c95e21-747a-87d8-4145-f37f75e76e18@linux.intel.com>
From:   "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Organization: Intel Technology Poland Sp. z o. o., KRS 101882, ul. Slowackiego
 173, 80-298 Gdansk
In-Reply-To: <77c95e21-747a-87d8-4145-f37f75e76e18@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?OUtmN1BjNWM1Z053OVlGU29IajRTS0lNeXRHSUF6L1hFNWpONmYyazZxVEJM?=
 =?utf-8?B?YXhlOXU3Y0xWYjVURm1yUFdmN095cFIwMno4V216STNpS2M0TzNTZjgvUnBn?=
 =?utf-8?B?WnYzUk9LaVZBVkM1RFBTL3U4aE9icEpkSXcxYWNpVDFuaWlMaWJ5ZzFJU0Z4?=
 =?utf-8?B?Qy8xM3JmVTA0dDRHeEFxSXU3NFpxMnJhL0VUdDdDRUNuMEphTXNsbk80azdw?=
 =?utf-8?B?VEJsZ01LV2k3OE0yMklLazRNWk5uZnZTRkErckFCYTJFTHlKc1I3VENjczZQ?=
 =?utf-8?B?WnZDZEJza0Q0dy82WFR2cW83L05VUnMzb0dINGVHdWxEeWlBcjZtYlYvMUxs?=
 =?utf-8?B?ZWFKQmZZcHA4QWl0bWlJZGZuZWN1SEY4TWxjSWlOczY1TDFWbWo1Zy82MXh0?=
 =?utf-8?B?YU1PRDBFS3p1R1R5Wnh0RGNxMWJHaHNpZXAxRmJaVHk0K0k1N1Z0MU1HWnFG?=
 =?utf-8?B?ZGFXUmtkdHJFS0l4aWVHUEZkU21JZnFzeUMvSjB4NGMvUXY4Ukh3MjkvMGZp?=
 =?utf-8?B?SlcveUlrb3RyS3piR25RQW10ZG1xTFUxZ2dBTlJ5Y3BBNzQrVW1idWxLRTFq?=
 =?utf-8?B?M3hpRjlDSysxU3hQMStEWStFV3pMVjNZa3ZTcnFMamRKbUtIdk5ZVnJ1VGhn?=
 =?utf-8?B?T0F5Z0ZKZDZENThRbzQxRFpEUWN3MXBZMit6cC83czgyWHJzdGVORE80ZTYx?=
 =?utf-8?B?OEZ6enl6Z09zMzVyaTBkOVFiUXhWUjNaU3RveC9oS0tCOTFKN2EzR0tURjdw?=
 =?utf-8?B?NDhHcjkzclhoY2VkV0xQalRLSUQwWmphQ3ZDcFdGQ09MK1pzM0h1cmtCdk5P?=
 =?utf-8?B?L1NIQSt0aFFtK0hNekdGeWRFbFh1YWVteU5wNEQ0eW82UWRtQ1lIeWNWYkh6?=
 =?utf-8?B?bEtleHpwdklzK05yaGFBRERSdUs2Q3JTZWJTK2huVnBFajVpN2RPeVJrWi8y?=
 =?utf-8?B?YXp1bm91RldnM1ZVZHR1alVFSXZwRTQxelRncWpDSWkyQ0VzOTMzMHhOZTJO?=
 =?utf-8?B?WDZVZnN6SFBGTmxMcTQwR2o3RmVvRTNteEZUV0JQSzYvZzZ0cFlDcHlsei9t?=
 =?utf-8?B?cjNzaVZMSm9HejdmbVVEM204c2M5Um0xa1hvcjNCNVJVVVI3ckc4UmZXWFdK?=
 =?utf-8?B?dHZLdmVQRVpFRnl1OFBLYzEvMkluZENVSE4yTXJGSk04aGwwWGxhQnB6TEpz?=
 =?utf-8?B?eXMxN0hUNUw3Y3ArTzNxNWVCazJnaEk5VmwwK2NNUFl0T3VhbEtPS1lLRENi?=
 =?utf-8?B?U0tEQ3UwSWtJMnlrVWtrZXFEdVpoYlY1OUtJTGI0aHhCN2FxaW9BR2NLU3FP?=
 =?utf-8?B?T0FGbUFkZmlQc2FiUkdsODNGWVpEcFM5b1B2RkQzTGdYc0M4Mlozem9RekNQ?=
 =?utf-8?B?ajlkZkRvOXJ6aE1jZ1dBdnNmZFBjaWNpWmliam9pRHdXSFJjUFpOa3JEb3d3?=
 =?utf-8?B?bGd3U1pTTGhsNXY1WC9CS2c0dTNQVE1TOXFHWGxpamR1RDN1OTdRSXFIZzJ0?=
 =?utf-8?B?dWlLMjkyZVRhdzRWaEJLcmZLTmlYYlIvUktreFZzNER2R1luUTBHQndrbDdq?=
 =?utf-8?B?czFaZk1ZcWZ2Q2EvRnFZbFQvZkQ3TUR1TnNOSEthSVlRaFpmd1ZKWjBXL1kx?=
 =?utf-8?B?ZjIrdzBIQWVlWlkyZHB5OVVYeXVrSHhHQnFwWVE5WFFBZ0ttejFNMXcvT1ND?=
 =?utf-8?B?ditVa0UzZ3pxSnp2RFNRajJkMmR1MFpTd3NtOW84d3A4TndRa3BVUXoxVVhX?=
 =?utf-8?B?TThkUG9mT3NvZ3pNTCswRW1VN1BxVGoxeFhSTVRFeXZCSlU5NlFjTnRYVXcr?=
 =?utf-8?B?VlRON1dOSElmS2t4ak9wV1JBYTd4TkRSZnJmdDdvcUtHWUxZTE5IcTFtRzVC?=
 =?utf-8?B?bXVaSTV4Rms1WFpVaW44UzdPam9NOWlMbTJDeVArUjl6eDVOR1QwSmpvUTE3?=
 =?utf-8?B?WllQQks2Tzc1bzhsVUdRYXhpSVFsNGhzeHB4UHdtMnhNQ0pRcDN2MHpwTTMw?=
 =?utf-8?B?QUdiem1kMHlFdGR6c0xvTTNKS1lvakRyNUNXendEQUQzZnhJcThaZGUvZ3Bt?=
 =?utf-8?B?eWtsTXhuRXpqWVc1YkF5anJPd0hUSzAwK1RBVlVWRE5yOHpubENwckxSRU1n?=
 =?utf-8?B?R2NjOVphbzZPcFF1L21rVlN4aFlYakR3UEIzbldibXVlRzEvdnFKakdhdUJP?=
 =?utf-8?B?M0hCTVd2UHZwaWExdjB0MkE5UmxkeWxxMGVzakJaQmdnanQvRzhOUklDWlRV?=
 =?utf-8?Q?pTGwGW8dPcCebGh+42f0NEluld5PDkXy0Ru5aiw46w=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a32cead6-1212-4105-04c7-08d9c0c145c7
X-MS-Exchange-CrossTenant-AuthSource: MW5PR11MB5810.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Dec 2021 18:24:18.3967
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: R7NENlIP+/XeQ5V8zQzdfTAX/+7MQz8IzY/SVc8SJysdHLAAu3ysJaZ7S5IwNRl+JUxfu+7CuoCDcSHToSR1qL4WIHDVkXick4vQRTE1tqQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR11MB1245
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 12/16/2021 2:27 PM, Thomas Hellström wrote:
> Hi, Rafael,
>
> On 11/4/21 18:26, Rafael J. Wysocki wrote:
>> It is generally unsafe to call put_device() with dpm_list_mtx held,
>> because the given device's release routine may carry out an action
>> depending on that lock which then may deadlock, so modify the
>> system-wide suspend and resume of devices to always drop dpm_list_mtx
>> before calling put_device() (and adjust white space somewhat while
>> at it).
>>
>> For instance, this prevents the following splat from showing up in
>> the kernel log after a system resume in certain configurations:
>
>
> <snip>
>
>
>> @@ -1748,21 +1769,27 @@ int dpm_suspend(pm_message_t state)
>>           struct device *dev = to_device(dpm_prepared_list.prev);
>>             get_device(dev);
>> +
>>           mutex_unlock(&dpm_list_mtx);
>>             error = device_suspend(dev);
>>             mutex_lock(&dpm_list_mtx);
>> +
>>           if (error) {
>>               pm_dev_err(dev, state, "", error);
>>               dpm_save_failed_dev(dev_name(dev));
>> -            put_device(dev);
>> -            break;
>> -        }
>> -        if (!list_empty(&dev->power.entry))
>> +        } else if (!list_empty(&dev->power.entry)) {
>>               list_move(&dev->power.entry, &dpm_suspended_list);
>> +        }
>> +
>> +        mutex_unlock(&dpm_list_mtx);
>> +
>>           put_device(dev);
>> -        if (async_error)
>> +
>> +        mutex_lock(&dpm_list_mtx);
>> +
>> +        if (error || async_error)
>>               break;
>>       }
>>       mutex_unlock(&dpm_list_mtx);
>> @@ -1879,6 +1906,7 @@ int dpm_prepare(pm_message_t state)
>>           struct device *dev = to_device(dpm_list.next);
>>             get_device(dev);
>> +
>>           mutex_unlock(&dpm_list_mtx);
>>             trace_device_pm_callback_start(dev, "", state.event);
>> @@ -1886,21 +1914,23 @@ int dpm_prepare(pm_message_t state)
>>           trace_device_pm_callback_end(dev, error);
>>             mutex_lock(&dpm_list_mtx);
>> -        if (error) {
>> -            if (error == -EAGAIN) {
>> -                put_device(dev);
>> -                error = 0;
>> -                continue;
>> -            }
>> +
>> +        if (!error) {
>> +            dev->power.is_prepared = true;
>> +            if (!list_empty(&dev->power.entry))
>> +                list_move_tail(&dev->power.entry, &dpm_prepared_list);
>> +        } else if (error == -EAGAIN) {
>> +            error = 0;
>> +        } else {
>>               dev_info(dev, "not prepared for power transition: code 
>> %d\n",
>>                    error);
>> -            put_device(dev);
>> -            break;
>
> It appears the above break disappeared.
>
>
>>           }
>> -        dev->power.is_prepared = true;
>> -        if (!list_empty(&dev->power.entry))
>> -            list_move_tail(&dev->power.entry, &dpm_prepared_list);
>> +
>> +        mutex_unlock(&dpm_list_mtx);
>> +
>>           put_device(dev);
>
> Should be
>
>                  if (error)
>
>                         break;
>
> Here?
>
Looks like that.


> Symptoms is if we return an error from the device prepare callback, we 
> end up spinning forever with little clue what's going on.
>
>
>> +
>> +        mutex_lock(&dpm_list_mtx);
>>       }
>>       mutex_unlock(&dpm_list_mtx);
>>       trace_suspend_resume(TPS("dpm_prepare"), state.event, false);
>
I'll have a look, thanks for the report!