Received: by 2002:a05:6a10:a841:0:0:0:0 with SMTP id d1csp259274pxy; Wed, 28 Apr 2021 03:51:49 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxlpChupad5ufOnKbWNC0tcqwOXOyAIIg6ttZqwNtUpxGyJ4geRaNTCCs9WBJdDBy/nZB+O X-Received: by 2002:a05:6a00:2c4:b029:279:c266:abe6 with SMTP id b4-20020a056a0002c4b0290279c266abe6mr8935106pft.48.1619607108955; Wed, 28 Apr 2021 03:51:48 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1619607108; cv=none; d=google.com; s=arc-20160816; b=Y6ZCVsrZ0nZEasQB2VUYMPzqzfTI0u+abdtGsH+pMpWeRKeHsQOx9Jvy23FCalI9FN xBH37BbuI63jgBB2+H4Utr+55TwlZJh54wu67rr1VHNQHseGBGZATbd8hpgJb/uRZgCn Ij3sZBa4u+moUIMOu2LWDjuZq2jnFE12v1IAYgsMDeL6PKTrBtarJwPTXV+QdWxij6eb PNrrpqE4OX3JFODsZL2FP5mlYvGEmEj3ipz60xEWTE3rHxTVxrPnRyst6ILUd3ob2loM WHrthG6V4Ow5vVZ6wl1c/0gH3YwKdn9WkJAzSeyLGpEwUeyeul2YNLoolRQ1g5AWltB9 yNEw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:dkim-signature; bh=/GxKGTH8CXtn6WBbDrlJ9GaCEVxfLLETycskyt2Zm+0=; b=QyKIpw6uof9L+zevDYXpMQfG22TMJ5R9DV+Zl3yYveOW36XgBeZ/jN5aAW2EXfTkiT wDYQxgrKAZbPxUgzwKO06typ2C1wbRB9PrX0NEjIdKwQM7ntv3Xq9F17OkUxxDV7zxWJ OoS9vyp9QOBisYWRW4dqoLqppFyLcjxo6mpje8B8GIjapFCuKI/b3+NZynqZQ894kbXY D9ISl9DIUZbZakNdOY1DfwI08AqvJ5sfSPWjmqK6qglZ9BAhki1drPzWZzHXqeyDeO4W AeBN/fmHSp2e6lp/QTl/JhDVCL+AylsgttJrKJfz3z3R9l/sNJaGL+tK1X9wlXpRmiDG x76g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=jsg5uBYN; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id k10si6774469pjm.70.2021.04.28.03.51.35; Wed, 28 Apr 2021 03:51:48 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=jsg5uBYN; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239097AbhD1KF6 (ORCPT + 99 others); Wed, 28 Apr 2021 06:05:58 -0400 Received: from mail.kernel.org ([198.145.29.99]:32908 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238345AbhD1KF6 (ORCPT ); Wed, 28 Apr 2021 06:05:58 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id 942BE61026; Wed, 28 Apr 2021 10:05:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1619604313; bh=MyeSwjuWDkzwkgFnTJD3WNHelmDMb1YG6N+hdAddu3g=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=jsg5uBYN0tqjOsXJoDGuhGzUgumrzePDIHuFSW+c8aeNPs5+ayoQ4bXXomBh6mwVZ NeWmNhQB4KslN0JoubOPJRNj6e6XuY3Z+2D3XrgrGl8tr62HPWL0zFpUHOPlWJNUPS j6+1PjcmN8VTI2NDtO/pKOKymfmPHQLKHNmRZ7EY3+rjudM3vi/9PZJKQb+wjXaBR5 Rer55BJBxx9YMHsw8YkSmg0q9PF0bjx+cOOkXyK0UibOkSMuGRNBKfaJqxF1uIlEtk hnqgCLTCdGVaeK8J+n7RObpHWJrM4lrE+SA3Dt+VJq4AzJ3RfyPqaba0gM4+q4vtrx XLmMhMYPWQ7Tw== Received: from johan by xi.lan with local (Exim 4.93.0.4) (envelope-from ) id 1lbh4Q-0003ws-5z; Wed, 28 Apr 2021 12:05:26 +0200 Date: Wed, 28 Apr 2021 12:05:26 +0200 From: Johan Hovold To: Mauro Carvalho Chehab Cc: Jacopo Mondi , linuxarm@huawei.com, mauro.chehab@huawei.com, Hans Verkuil , Jacopo Mondi , Mauro Carvalho Chehab , linux-kernel@vger.kernel.org, linux-media@vger.kernel.org Subject: Re: [PATCH 38/78] media: i2c: mt9m001: use pm_runtime_resume_and_get() Message-ID: References: <20210424082454.2ciold3j3h2jw47m@uno.localdomain> <20210426163840.67ea8af9@coco.lan> <20210428103148.590191ac@coco.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210428103148.590191ac@coco.lan> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Apr 28, 2021 at 10:31:48AM +0200, Mauro Carvalho Chehab wrote: > Em Tue, 27 Apr 2021 14:23:20 +0200 > Johan Hovold escreveu: > > pm_runtime_get_sync() has worked this way since it was merged 12 years > > ago, and for someone who's used to this interface this is not such a big > > deal as you seem to think. Sure, you need to remember to put the usage > > counter on errors, but that's it (and the other side of that is that you > > don't need to worry about error handling where it doesn't matter). > > Before we have those at PM subsystem, the media had its own way to > set/disable power for their sub-devices. The PCI and USB drivers > still use it, instead of pm_runtime, mostly due to historic reasons. > > So, basically, its usage at the media subsystem is restricted to > drivers for embedded systems. The vast majority of drivers supporting > PM runtime are the I2C camera drivers. The camera drivers can be used > interchangeable. So, in practice, the same bridge driver can work > with a lot of different camera models, depending on the hardware > vendors' personal preferences and the desired max resolution. > > So, in thesis, all such drivers should behave exactly the same > with regards to PM. > > However, on most existing drivers, the pm_runtime was added a > couple of years ago, and by people that are not too familiar > with the PM subsystem. > > That probably explains why there were/are several places that > do things like this[1]: > > ret = pm_runtime_get_sync(dev); > if (ret < 0) > return ret; > > without taking care of calling a pm_runtime_put*() function. > > [1] besides the 13 patches made by UCN addressing it on > existing code, I discovered the same pattern on a > couple of other drivers with current upstream code. > > That shows a pattern: several media developers are not familiar > with the usage_count behavior for pm_runtime_get functions. > > So, doing this work is not only helping to make the PM support > more uniform, but it is also helping to solve existing issues. Sure, I don't doubt that there are issues with the current code too. > > You're call, but converting functioning drivers where the authors knew > > what they were doing just because you're not used to the API and risk > > introducing new bugs in the process isn't necessarily a good idea. > > The problem is that the above assumption is not necessarily true: > based on the number of drivers that pm_runtime_get_sync() weren't > decrementing usage_count on errors, several driver authors were not > familiar enough with the PM runtime behavior by the time the drivers > were written or converted to use the PM runtime, instead of the > media .s_power()/.s_stream() callbacks. That may very well be the case. My point is just that this work needs to be done carefully and by people familiar with the code (and runtime pm) or you risk introducing new issues. I really don't want the bot-warning-suppression crew to start with this for example. > > Especially since the pm_runtime_get_sync() will continue to be used > > elsewhere, and possibly even in media in cases where you don't need to > > check for errors (e.g. remove()). > > Talking about the remove(), I'm not sure if just ignoring errors > there would do the right thing. I mean, if pm_runtime_get_sync() > fails, probably any attempts to disable clocks and other things > that depend on PM runtime will also (silently) fail. > > This may put the device on an unknown PM and keep clock lines enabled > after its removal. Right, a resume failure is a pretty big issue and it's not really clear how to to even handle that generally. But at remove() time you don't have much choice but to go on and release resource anyway. So unless actually implementing some error handling too, using pm_runtime_sync_get() without checking for errors is still preferred over pm_runtime_resume_and_get(). That is pm_runtime_get_sync(); /* cleanup */ pm_runtime_disable() pm_runtime_put_noidle(); is better than: ret = pm_runtime_resume_and_get(); /* cleanup */ pm_runtime_disable(); if (ret == 0) pm_runtime_put_noidle(); unless you also start doing something ret. Johan