From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 291DFC43462 for ; Wed, 28 Apr 2021 15:50:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id EAF1B613BF for ; Wed, 28 Apr 2021 15:50:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240906AbhD1Put (ORCPT ); Wed, 28 Apr 2021 11:50:49 -0400 Received: from mail.kernel.org ([198.145.29.99]:52502 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S240429AbhD1Pum (ORCPT ); Wed, 28 Apr 2021 11:50:42 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id B3364610A2; Wed, 28 Apr 2021 15:49:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1619624996; bh=6/CoqHvXA5fOpA3A5RVJksN1jb3qc28vss2bW2Ps1h4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=u/FPy0GSqykv7UbP1eGyuKva4OKLzag9Bw/Jd4D0kbJTEf9nFrUMmEvhv5EK6vzMK BjB3VRHy0SWTGSvkxD+onAV4Fieqtf2ZXtTiexGI+RyxJutQZq8VxflGKixqYFOpUz On6PoASK7Vh8yjg3wm62eMFNCYbQtk5gn6WgkeIV2oMUglcpz/AqBzcYM/M4tD51cu QunLw6fSqD+ZQ8VF44WRxVz+8sqWccc8jGe1cZAgUj2axVmUS0f3tcXnA28yKRMb0f 56N8MN7/SJV9CcbdLHAoqFUGEthzX/mxeThJcNnMD8j9mKUhq91Kz9hVw+Gg5LOHLo GhnW8kKeIOrcw== Received: from johan by xi.lan with local (Exim 4.93.0.4) (envelope-from ) id 1lbmS0-000592-V7; Wed, 28 Apr 2021 17:50:09 +0200 Date: Wed, 28 Apr 2021 17:50:08 +0200 From: Johan Hovold To: Mauro Carvalho Chehab Cc: Shawn Tu , Ricardo Ribalda , Dafna Hirschfeld , Heiko Stuebner , linuxarm@huawei.com, Todor Tomov , Bjorn Andersson , Andrzej Hajda , "Lad, Prabhakar" , Thierry Reding , Pengutronix Kernel Team , Dmitry Osipenko , linux-stm32@st-md-mailman.stormreply.com, Andrzej Pietrasiewicz , Leon Luo , Paul Kocialkowski , Mauro Carvalho Chehab , Dave Stevenson , Matt Ranostay , Krzysztof Kozlowski , Jonathan Hunter , linux-rockchip@lists.infradead.org, Chen-Yu Tsai , Andy Gross , Matthias Brugger , Dongchun Zhu , Sakari Ailus , Bingbu Cao , Marek Szyprowski , Shunqian Zheng , Tianshu Qiu , NXP Linux Team , Philipp Zabel , devel@driverdev.osuosl.org, Jacopo Mondi , Sylwester Nawrocki , linux-tegra@vger.kernel.org, Alexandre Torgue , Wenyou Yang , Manivannan Sadhasivam , linux-arm-msm@vger.kernel.org, Sascha Hauer , Steve Longerbeam , linux-media@vger.kernel.org, Maxime Ripard , Stanimir Varbanov , Benoit Parrot , Helen Koike , linux-samsung-soc@vger.kernel.org, linux-mediatek@lists.infradead.org, Jacek Anaszewski , mauro.chehab@huawei.com, Sylwester Nawrocki , "Paul J. Murphy" , Ezequiel Garcia , Daniele Alessandrelli , Chiranjeevi Rapolu , linux-arm-kernel@lists.infradead.org, Jacob Chen , Jernej Skrabec , Hyungwoo Yang , linux-kernel@vger.kernel.org, Robert Foss , Dan Scally , Sowjanya Komatineni , Maxime Coquelin , linux-renesas-soc@vger.kernel.org, Yong Zhi , Shawn Guo Subject: Re: [PATCH v4 00/79] Address some issues with PM runtime at media subsystem Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Apr 28, 2021 at 04:51:21PM +0200, Mauro Carvalho Chehab wrote: > During the review of the patches from unm.edu, one of the patterns > I noticed is the amount of patches trying to fix pm_runtime_get_sync() > calls. > > After analyzing the feedback from version 1 of this series, I noticed > a few other weird behaviors at the PM runtime resume code. So, this > series start addressing some bugs and issues at the current code. > Then, it gets rid of pm_runtime_get_sync() at the media subsystem > (with 2 exceptions). > > It should be noticed that > Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") > added a new method to does a pm_runtime get, which increments > the usage count only on success. > > The rationale of getting rid of pm_runtime_get_sync() is: > > 1. despite its name, this is actually a PM runtime resume call, > but some developers didn't seem to realize that, as I got this > pattern on some drivers: > > pm_runtime_get_sync(&client->dev); > pm_runtime_disable(&client->dev); > pm_runtime_set_suspended(&client->dev); > pm_runtime_put_noidle(&client->dev); > > It makes no sense to resume PM just to suspend it again ;-) This is perfectly alright. Take a look at ov7740_remove() for example: pm_runtime_get_sync(&client->dev); pm_runtime_disable(&client->dev); pm_runtime_set_suspended(&client->dev); pm_runtime_put_noidle(&client->dev); ov7740_set_power(ov7740, 0); There's an explicit power-off after balancing the PM count and this will work regardless of the power state when entering this function. So this has nothing to do with pm_runtime_get_sync() per se. > 2. Usual *_get() methods only increment their use count on success, > but pm_runtime_get_sync() increments it unconditionally. Due to > that, several drivers were mistakenly not calling > pm_runtime_put_noidle() when it fails; Sure, but pm_runtime_get_async() also works this way. You just won't be notified if the async resume fails. > 3. The name of the new variant is a lot clearer: > pm_runtime_resume_and_get() > As its same clearly says that this is a PM runtime resume function, > that also increments the usage counter on success; It also introduced an inconsistency in the API and does not pair as well with the pm_runtime_put variants. > 4. Consistency: we did similar changes subsystem wide with > for instance strlcpy() and strcpy() that got replaced by > strscpy(). Having all drivers using the same known-to-be-safe > methods is a good thing; It's not known to be safe; there are ways to get also this interface wrong as for example this series has shown. > 5. Prevent newer drivers to copy-and-paste a code that it would > be easier to break if they don't truly understand what's behind > the scenes. Cargo-cult programming always runs that risk. > This series replace places pm_runtime_get_sync(), by calling > pm_runtime_resume_and_get() instead. > > This should help to avoid future mistakes like that, as people > tend to use the existing drivers as examples for newer ones. The only valid point about and use for pm_runtime_resume_and_get() is to avoid leaking a PM usage count reference in the unlikely case that resume fails (something which hardly any driver implements recovery from anyway). It's a convenience wrapper that saves you from writing one extra line in some cases (depending on how you implement runtime-pm support) and not a silver bullet against bugs. > compile-tested only. > > Patches 1 to 7 fix some issues that already exists at the current > PM runtime code; > > patches 8 to 20 fix some usage_count problems that still exists > at the media subsystem; > > patches 21 to 78 repaces pm_runtime_get_sync() by > pm_runtime_resume_and_get(); > > Patch 79 (and a hunk on patch 78) documents the two exceptions > where pm_runtime_get_sync() will still be used for now. > > --- > > v4: > - Added a couple of additional fixes at existing PM runtime code; > - Some patches are now more conservative in order to avoid causing > regressions. > v3: > - fix a compilation error; > v2: > - addressed pointed issues and fixed a few other PM issues. This really doesn't say much more than "changed stuff" so kinda hard to track if review feedback has been taken into account for example. Johan