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 F1FBFC433B4 for ; Wed, 28 Apr 2021 15:50:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id BFD64613B4 for ; Wed, 28 Apr 2021 15:50:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240836AbhD1Puq (ORCPT ); Wed, 28 Apr 2021 11:50:46 -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-tegra@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 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=-3.8 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,MAILING_LIST_MULTI,SPF_HELO_NONE 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 DF105C43460 for ; Wed, 28 Apr 2021 15:50:01 +0000 (UTC) Received: from smtp1.osuosl.org (smtp1.osuosl.org [140.211.166.138]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 782FB610FA for ; Wed, 28 Apr 2021 15:50:01 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 782FB610FA Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=driverdev-devel-bounces@linuxdriverproject.org Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id 1BD0A8404C; Wed, 28 Apr 2021 15:50:01 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp1.osuosl.org ([127.0.0.1]) by localhost (smtp1.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id JeNS--vXBOXt; Wed, 28 Apr 2021 15:50:00 +0000 (UTC) Received: from ash.osuosl.org (ash.osuosl.org [140.211.166.34]) by smtp1.osuosl.org (Postfix) with ESMTP id D8EE48408F; Wed, 28 Apr 2021 15:49:59 +0000 (UTC) Received: from smtp1.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by ash.osuosl.org (Postfix) with ESMTP id 55CFD1BF5DE for ; Wed, 28 Apr 2021 15:49:58 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id 440F68408F for ; Wed, 28 Apr 2021 15:49:58 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp1.osuosl.org ([127.0.0.1]) by localhost (smtp1.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id nl0l-sf7ZTgu for ; Wed, 28 Apr 2021 15:49:57 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp1.osuosl.org (Postfix) with ESMTPS id 8241F8404C for ; Wed, 28 Apr 2021 15:49:57 +0000 (UTC) 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 Subject: Re: [PATCH v4 00/79] Address some issues with PM runtime at media subsystem Message-ID: References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-BeenThere: driverdev-devel@linuxdriverproject.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux Driver Project Developer List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Ricardo Ribalda , Dafna Hirschfeld , Heiko Stuebner , mauro.chehab@huawei.com, linuxarm@huawei.com, Todor Tomov , Bjorn Andersson , Andrzej Hajda , "Lad, Prabhakar" , Thierry Reding , Robert Foss , Sylwester Nawrocki , Dmitry Osipenko , linux-stm32@st-md-mailman.stormreply.com, Andrzej Pietrasiewicz , Leon Luo , Dan Scally , linux-samsung-soc@vger.kernel.org, Dave Stevenson , Manivannan Sadhasivam , Chen-Yu Tsai , Krzysztof Kozlowski , Jonathan Hunter , linux-rockchip@lists.infradead.org, Matt Ranostay , Andy Gross , Dongchun Zhu , Maxime Coquelin , Steve Longerbeam , Bingbu Cao , Marek Szyprowski , Shunqian Zheng , Tianshu Qiu , NXP Linux Team , Shawn Tu , devel@driverdev.osuosl.org, Jacopo Mondi , linux-tegra@vger.kernel.org, Sakari Ailus , Wenyou Yang , Alexandre Torgue , linux-arm-msm@vger.kernel.org, Sascha Hauer , linux-renesas-soc@vger.kernel.org, Maxime Ripard , Stanimir Varbanov , Benoit Parrot , Helen Koike , Yong Zhi , linux-mediatek@lists.infradead.org, Jacek Anaszewski , Matthias Brugger , Sylwester Nawrocki , Mauro Carvalho Chehab , Ezequiel Garcia , Daniele Alessandrelli , Chiranjeevi Rapolu , linux-arm-kernel@lists.infradead.org, Jacob Chen , Jernej Skrabec , Philipp Zabel , Hyungwoo Yang , linux-kernel@vger.kernel.org, "Paul J. Murphy" , Paul Kocialkowski , Sowjanya Komatineni , Pengutronix Kernel Team , linux-media@vger.kernel.org, Shawn Guo Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: "devel" 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 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel 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=-4.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,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 76213C433B4 for ; Wed, 28 Apr 2021 15:50:18 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id BE32E610FA for ; Wed, 28 Apr 2021 15:50:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org BE32E610FA Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=desiato.20200630; h=Sender:Content-Transfer-Encoding :Content-Type:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=6Ib4NkNAtq/pUKeXNmezGngaEkHCjO+9ghzriC6IH+Q=; b=Dx/FpxoDlH2s1/hUX1pSFmpSB h480wcgitThDyjDRUpSpMwOsEQUCh10KWbxJ4MEvb5miZhrXcpBwcveo5dEzuaKwdMIibOLeUgk1q uIFt2vRk5QDcVLgPyFAzFbpEs07YyUsHYlRvLS2DDBO83OHjeCAvBJp4p94kuOdkX3BAMFkruZgbc wpJzOg7s2NHaz3WdLQUQBjZi6Ub4U64iAz4dxCosee6hcEDC1r38wDLCU9bXwY2VC+B25SJiz/D1o UvKxbVCU4UIse7X6f+RSN87Y0lAY+5FUD23K6jDipglF021ZO8fBlQvYZ2h9ogaeot+ylO1Z7p0fk iJObU2rcg==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lbmRx-003meM-7n; Wed, 28 Apr 2021 15:50:05 +0000 Received: from bombadil.infradead.org ([2607:7c80:54:e::133]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lbmRt-003mdr-I3; Wed, 28 Apr 2021 15:50:01 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20210309; h=In-Reply-To:Content-Type:MIME-Version :References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=bZMjXdeyUCsjpWOTokx/+KHmT0z04nF+Qli+4VLwCUY=; b=ZYvNfV9Pcuo+pfzuttwdssYgIu GG2oywkIUX7e1G9Rr7Xw11nfpDKgboYWzr+tp/6OazByddegayhSkVS7enPdBLL/dBkyp3xw/jFG5 QMoMi/M+xqaLWQGTjdE/LtQqlLKsfkA/IGbJOt3q89WYHVgH8oyLVlFi3MSpMqxsRJ/A1fw+VNlyJ 08ax+hUy8B5aDJIxyJEZCE2vQ3zqprx7Wq1dPt/ht+eSOcWkxZtW0Orp8ruRuDTP1B3Kvx90ZuWtg 9PQGLsPNuIoxQeqkI93VX0/kHrOVVcJnIlLJ0hRNq6t1bXUEz0XfjZFEEMWZ34vkm+MTLM5NF/gRo 8N6oyClg==; Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lbmRp-00HaXT-DA; Wed, 28 Apr 2021 15:50:00 +0000 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-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210428_084957_547416_2298E2CE X-CRM114-Status: GOOD ( 39.34 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.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 _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek