Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5caa944f-c841-6f74-8e43-a278b2b93b06@suse.com>
Date:   Tue, 21 Jun 2022 11:38:33 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: PM runtime_error handling missing in many drivers?
Content-Language: en-US
To:     Vincent Whitchurch <vincent.whitchurch@axis.com>,
        rafael.j.wysocki@intel.com, jic23@kernel.org
Cc:     linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-iio@vger.kernel.org
References: <20220620144231.GA23345@axis.com>
From:   Oliver Neukum <oneukum@suse.com>
In-Reply-To: <20220620144231.GA23345@axis.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 2
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MnY5dWFFSHNQT1kvRzJTZ1FMcHFVQ1lSUlFEcU1uQ0xRcTF0ZVFkc095U3JZ?=
 =?utf-8?B?QkZMVTV4a1lUZmQ1ZjlYWTVqbnozZjFjbU1jN2V4ejJVWDBrN0ltOFNsMk40?=
 =?utf-8?B?UlpPUlRHYjl5d0E3Q0xTS3I1a096NU5lLzBzRTQ0S2cyR2t0azdHU0RrWkpX?=
 =?utf-8?B?Uml5VTFEbmdBTThvYnJzcnpSV3JmcXhlY0hGSHNVZ0ZWZVZYeEtETjFVSjZn?=
 =?utf-8?B?UkFLTzNIREFINjMyc2VGY293cWFGWXlFdXg1TDdNOGprbEpYTmJ4UXRwL3Q5?=
 =?utf-8?B?ZXZPSUlpUDVScDZqaUtLR0pJQk5pK0kzRTNCNHJ4M2d0RHB6RURYRmdiT0xy?=
 =?utf-8?B?TEtTd0dRVjRqR2NUcG5yaVRzMS9URHUzSFZEZ0VxYThQR3RUa3VxK050R0pj?=
 =?utf-8?B?VUswdW1uVjJzTmRyWWMvK3NGOWlKZEpvdE5KazVaR0RTcmZhaTJOckRyYit5?=
 =?utf-8?B?aWpESkpuZ1NIN2ExVVMwRklnOElFVi92aTI1TlZNNFh4VXlYVFpNbHZkb2xP?=
 =?utf-8?B?VkVCMVhMaFplWFlENVpldkd1VGlCU28yMHkvSENvRXRRcGt2UnlCV09TblBC?=
 =?utf-8?B?YjQrN1phMHp0cWRNZU9TTjNvSTJBd1N6YkIrZVE1VVp0bnJIeVFoVndTZFRT?=
 =?utf-8?B?WWRSRWVvU0c2T3QyVzNCM0RuNE5paG1oQjRMWFJrcWU1MmdrVEJJd1dWR3Ju?=
 =?utf-8?B?Ykw2RmRBTjRYZUN6bkFMc20zNVQzQWZ4VjFCaFZ4bk1zWWttb1FFS1F4OEhO?=
 =?utf-8?B?ckpLOU5zNUJjbmFRMjA1N3JpT1lFaittWWpWK2RxTTY0akZNTjY5Y05WVDRM?=
 =?utf-8?B?V0wvdEs3eHVSbmZLZVo3b0RQSHhJbW1jTytSblBJclVVQXR0am1oRkxZbCto?=
 =?utf-8?B?Y3B0SDYySVJ3b0NYZktIV292RFhiaWhoSHFla1hRMVFNMVF4cldKS1BlSlVl?=
 =?utf-8?B?QU5QMngwNVV2eVNRRjRPaHF5K1hva3E2SDhFY0J0Q3pFMHhPaitXbnFndFdy?=
 =?utf-8?B?amRKaUtrblhkV2NPVGJ6bnRwZnl4TmIrbSs5b3dMSTFvZEUxcEhOZ2FlUGpG?=
 =?utf-8?B?Vlh6SE96djYrbFNrMklzYi9heERJTEFkdFFIUVZTa1l5WUc1MkxPVzd2RjNn?=
 =?utf-8?B?c2U4ZkJFMWxCaW9NZm5UeE11TVd2U0lGQU9qVHVveGxBK0pNbG5ML0pIb2Rr?=
 =?utf-8?B?ZW9vWUlRcXN3YWF6YUlrWGUwcmk3SXRrNEVvTk02ckFjNEtuMjBNWnF3cStB?=
 =?utf-8?B?YUxEVjRlVmNBWkZNbGxNZFVBamFhL0xHZWROK2ZiVnc0VWNTbGRmQnZ3YWF2?=
 =?utf-8?B?ZHJHcjAwNHp2dmE4amcvNHhCdWhLVTNLbTVvUUNrdFptbW15UGtXdk5ESVM3?=
 =?utf-8?B?NjZRcmNlT1RBVFFiS3VNYmpZUFBBTmxZVnhUbldUaXBYVUt6SXlTSUN1YzJr?=
 =?utf-8?B?d0dkK3NZQkhoVTFJcWN6aURPY0UxVGhCemVYN28rRmdDamhpZlhGMkwxUThh?=
 =?utf-8?B?Nmxpbm9uLzFGWW9DajEzSVhjLzMybi9OZWRBeHVIMldJeUF3K205OWZDV1U0?=
 =?utf-8?B?MlpnZ3VOMFhHUlFtT3A5UnpZMlJaek5zejhmczNZM3hCTkU2UWU0QmUwQ1Bx?=
 =?utf-8?B?UDJ1NWtwTjlud0doVDNON0JWNnpER2tuTEk5dXQxQTZjQ21pZ3UxSnpBSlBy?=
 =?utf-8?B?SEljbHkvbVpCN0Znei85aTRWb21MdFp2OW1TVlFzMWVQc05XZE94dTZGZXJz?=
 =?utf-8?B?NnpkVmhJdjBWdXNEQi96cjBSTDR5Q1NoTnVjRDE0aW9JZ2hmN3RwT25KOUxq?=
 =?utf-8?B?c2x4NGxjQ3JxSWpDTTVrdk5COGVrUXp0MitkMVdBcmp3Qzc0S3U3enRLZXhC?=
 =?utf-8?B?QktxQVppK1FlQ3FxQU41MVFVdnlXMHNSNmdNVlpGck52eEJXSmVCbzMwVGt2?=
 =?utf-8?B?U3A0U1hvT0w1bXE4UDZhUGZMRitYVmU2bEpNdWNZZ1Z6WHh3amQwa1Q3dkFx?=
 =?utf-8?B?NlpXWVdWczdNdFY5cFFYMkg2RFJMa0pvbXR6UmVpTVlTc1BwdVd2R3RGOWlF?=
 =?utf-8?B?bC9nSlJzZjkzWGhwanRlMTVzN1BjeFV6cmxiMEk3UVFKcXQrNEo2OWtXbVlV?=
 =?utf-8?B?QXRBczFrUWc0c1doUSt6K2ZnN00xMTVNc3hSandLaytiOVE4YndidWtsc2lx?=
 =?utf-8?B?WXFoVitnekRTSklnMUxCMVgyUy9rNTd1UndtYkxQUUJOS1l1cGtBSzkySlBG?=
 =?utf-8?B?QnhJSks5ZFlaRFhTTlVxRStER29QYUZ3RnRCbzllTkNVa1JDUy9ybER5bGpG?=
 =?utf-8?B?SjdERTZpRnRrNGhmOGJtVGRFS2dSYTN4R2ZPaUlyRUxoTDlqUWFaVlRJSm9p?=
 =?utf-8?Q?zYLfN7mw6Y7W6MR3MJg9vyfRI1tU4yPTzQWNofWxxQYAJ?=
X-MS-Exchange-AntiSpam-MessageData-1: 15pw6wToXESthw==
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9d234e70-9f93-460b-225d-08da5369cff0
X-MS-Exchange-CrossTenant-AuthSource: VI1PR0401MB2526.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Jun 2022 09:38:35.3357
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 9e2QM2Pj16VhJ4iEbWr1UT8xyUo1xfY7RXvt7mdcuCSQjj2afZt+iG+hbzKfPFVYGY3QoHfMQkm7B/l3TGbHSQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VE1PR04MB7454
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 20.06.22 16:42, Vincent Whitchurch wrote:

Hi,

> Many drivers do something like the following to resume their hardware
> before performing some hardware access when user space ask for it:
> 
> 	ret = pm_runtime_resume_and_get(dev);
> 	if (ret)
> 		return ret;
> 
> But if the ->runtime_resume() callback fails, then the
> power.runtime_error is set and any further attempts to use
> pm_runtime_resume_and_get() will fail, as documented in
> Documentation/power/runtime_pm.rst.

Whether this is properly documented is debatable.
4. Runtime PM Device Helper Functions (as a chapter)
does _not_ document it.


> [110778.050000][   T27] rpm_resume: 0-0009 flags-4 cnt-1  dep-0  auto-1 p-0 irq-0 child-0
> [110778.050000][   T27] rpm_return_int: rpm_resume+0x24d/0x11d0:0-0009 ret=-22
> 
> The following patch fixes the issue on vcnl4000, but is this the right in the


> fix?  And, unless I'm missing something, there are dozens of drivers
> with the same problem.

Yes. The point of pm_runtime_resume_and_get() is to remove the need
for handling errors when the resume fails. So I fail to see why a
permanent record of a failure makes sense for this API.

> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index e02e92bc2928..082b8969fe2f 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -414,6 +414,8 @@ static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data *data, bool on)
>  
>  	if (on) {
>  		ret = pm_runtime_resume_and_get(dev);
> +		if (ret)
> +			pm_runtime_set_suspended(dev);
>  	} else {
>  		pm_runtime_mark_last_busy(dev);
>  		ret = pm_runtime_put_autosuspend(dev);

If you need to add this to every driver, you can just as well add it to
pm_runtime_resume_and_get() to avoid the duplication.

But I am afraid we need to ask a deeper question. Is there a point
in recording failures to resume? The error code is reported back.
If a driver wishes to act upon it, it can. The core really only
uses the result to block new PM operations.
But nobody requests a resume unless it is necessary. Thus I fail
to see the point of checking this flag in resume as opposed to
suspend. If we fail, we fail, why not retry? It seems to me that the
record should be used only during runtime suspend.

And as an immediate band aid, some errors like ENOMEM should
never be recorded.

	Regards
		Oliver