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 32F57C43462 for ; Thu, 29 Apr 2021 15:17:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id F1EE961411 for ; Thu, 29 Apr 2021 15:17:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240543AbhD2PRv (ORCPT ); Thu, 29 Apr 2021 11:17:51 -0400 Received: from mail.kernel.org ([198.145.29.99]:36092 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233420AbhD2PRs (ORCPT ); Thu, 29 Apr 2021 11:17:48 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id DADC8613F8; Thu, 29 Apr 2021 15:17:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1619709421; bh=0TF/rgI+kIrUM4ucLgyoRQVNDZwjwMCttPCbdF1J2ts=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=TNblwH+As0w11JRNen3DEa/sKtquJO7N/ed+Wi2cPt5DPo/pzApjHeB4CYlT9czKN RFJnIUjSlU/iXNkujkJ9GfUIBpZYvUPaxBLDLGXBQdVlv5qb6mSpE1DcETxlBJLjdH 3g63qgtE3JXAaoo4Aq3ZJQdiRo/RBYWmOp0+Glxzsc1YjsicyiOaSi7GIhQYDFx0Ss xxX7tIodOuoDf+uK3PdOkr9EHdavuGSGLxTjoqehEjaUHaupkr1YkvsH7C4pSmGQvt kJLY88R3n7qjm+eBETbZXRLROLJk7BZhgXtoeO/pKC8+dHkhcGgjRgK0xEL1TOs1c0 knRtXQpP0PexA== Received: from johan by xi.lan with local (Exim 4.93.0.4) (envelope-from ) id 1lc8Ph-0006kM-4M; Thu, 29 Apr 2021 17:17:13 +0200 Date: Thu, 29 Apr 2021 17:17:13 +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: <20210429121215.64a7cbdb@coco.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210429121215.64a7cbdb@coco.lan> Precedence: bulk List-ID: X-Mailing-List: linux-tegra@vger.kernel.org On Thu, Apr 29, 2021 at 12:18:16PM +0200, Mauro Carvalho Chehab wrote: > Em Wed, 28 Apr 2021 17:50:08 +0200 > Johan Hovold escreveu: > > > On Wed, Apr 28, 2021 at 04:51:21PM +0200, Mauro Carvalho Chehab wrote: > > > > 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. > > Ok, but this should equally work: > > pm_runtime_disable(&client->dev); > pm_runtime_set_suspended(&client->dev); > > ov7740_set_power(ov7740, 0); > > as there's no additional cleanup made on this particular driver > between pm_runtime_get_sync() and pm_runtime_put_noidle(). No, that would break the driver as I pointed out to you yesterday: https://lore.kernel.org/r/YImG1klSPkFSaS3a@hovoldconsulting.com If the device is already suspended when remove is called then you'll end up with an unbalanced call to ov7740_set_power() that will try to disable an already disabled clock. > > So this has nothing to do with pm_runtime_get_sync() per se. > > Yes, but some patches on this series are cleaning up the driver release > logic. You mentioned this example as an argument against using pm_runtime_get_sync(), which I don't think makes sense. > > > 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. > > Granted, it makes sense along the pm_runtime kAPI. > > It is inconsistent with the behavior of kobject_get*() and other > *_get*() methods that are based or inspired on it, as, on those, the > operations are atomic: either everything succeeds and it doesn't return > an error, or the usage counter is not incremented and the object > state doesn't change after the call. Right, and I'm aware that some people have overlooked this. But its not the end of the world since hardly any driver can handle resume failures properly anyway. This is mostly just an exercise to shut up static checkers. > > > 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. > > Agreed. A name that would be more consistent with PM runtime would > probably be: > > pm_runtime_resume_if_get() Naw, since the get part always succeeds. It should start with pm_runtime_get, but pm_runtime_get_sync() is unfortunately taken. > as there are already: > > pm_runtime_get_if_in_use() > pm_runtime_get_if_active() > > But any such discussions are out of the scope of this patchset ;-) Right. > > > 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. > > Very true. Yet, it is a lot simpler to use functions that won't change > the state of the objects when returning an error, as this is by far > the most common pattern within the Kernel. A resume failure does change the state (and needs to be recovered from), but I get what you're saying. > Human brains are trained to identify certain patterns. When there's > something using a similar pattern, but with a different behavior, > our brains are more subject to fail identifying problems. Sure. But I'm not sure that having two interfaces with different semantics to do the job is doing us any favours here. But again, that discussion has already been had. And I realise that this is partly also your motive here (even if the old interface isn't going to go away). > > > 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. 80 patches in one series (posted to lkml) is a bit excessive. Perhaps you can break it up in a fixes part and one or more cleanups parts? 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,SPF_PASS,URIBL_BLOCKED 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 5AEA2C43470 for ; Thu, 29 Apr 2021 15:17:07 +0000 (UTC) Received: from smtp4.osuosl.org (smtp4.osuosl.org [140.211.166.137]) (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 F216261441 for ; Thu, 29 Apr 2021 15:17:06 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org F216261441 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 smtp4.osuosl.org (Postfix) with ESMTP id 96125419C9; Thu, 29 Apr 2021 15:17:06 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id xi_YILGH9EJ4; Thu, 29 Apr 2021 15:17:04 +0000 (UTC) Received: from ash.osuosl.org (ash.osuosl.org [140.211.166.34]) by smtp4.osuosl.org (Postfix) with ESMTP id 7C47C41883; Thu, 29 Apr 2021 15:17:04 +0000 (UTC) Received: from smtp2.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by ash.osuosl.org (Postfix) with ESMTP id DB21C1BF39C for ; Thu, 29 Apr 2021 15:17:02 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id C9507400F3 for ; Thu, 29 Apr 2021 15:17:02 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Authentication-Results: smtp2.osuosl.org (amavisd-new); dkim=pass (2048-bit key) header.d=kernel.org Received: from smtp2.osuosl.org ([127.0.0.1]) by localhost (smtp2.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id VTZJxXhE31J9 for ; Thu, 29 Apr 2021 15:17:01 +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 smtp2.osuosl.org (Postfix) with ESMTPS id DC219400CC for ; Thu, 29 Apr 2021 15:17:01 +0000 (UTC) Received: by mail.kernel.org (Postfix) with ESMTPSA id DADC8613F8; Thu, 29 Apr 2021 15:17:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1619709421; bh=0TF/rgI+kIrUM4ucLgyoRQVNDZwjwMCttPCbdF1J2ts=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=TNblwH+As0w11JRNen3DEa/sKtquJO7N/ed+Wi2cPt5DPo/pzApjHeB4CYlT9czKN RFJnIUjSlU/iXNkujkJ9GfUIBpZYvUPaxBLDLGXBQdVlv5qb6mSpE1DcETxlBJLjdH 3g63qgtE3JXAaoo4Aq3ZJQdiRo/RBYWmOp0+Glxzsc1YjsicyiOaSi7GIhQYDFx0Ss xxX7tIodOuoDf+uK3PdOkr9EHdavuGSGLxTjoqehEjaUHaupkr1YkvsH7C4pSmGQvt kJLY88R3n7qjm+eBETbZXRLROLJk7BZhgXtoeO/pKC8+dHkhcGgjRgK0xEL1TOs1c0 knRtXQpP0PexA== Received: from johan by xi.lan with local (Exim 4.93.0.4) (envelope-from ) id 1lc8Ph-0006kM-4M; Thu, 29 Apr 2021 17:17:13 +0200 Date: Thu, 29 Apr 2021 17:17:13 +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: <20210429121215.64a7cbdb@coco.lan> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20210429121215.64a7cbdb@coco.lan> 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 Thu, Apr 29, 2021 at 12:18:16PM +0200, Mauro Carvalho Chehab wrote: > Em Wed, 28 Apr 2021 17:50:08 +0200 > Johan Hovold escreveu: > > > On Wed, Apr 28, 2021 at 04:51:21PM +0200, Mauro Carvalho Chehab wrote: > > > > 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. > > Ok, but this should equally work: > > pm_runtime_disable(&client->dev); > pm_runtime_set_suspended(&client->dev); > > ov7740_set_power(ov7740, 0); > > as there's no additional cleanup made on this particular driver > between pm_runtime_get_sync() and pm_runtime_put_noidle(). No, that would break the driver as I pointed out to you yesterday: https://lore.kernel.org/r/YImG1klSPkFSaS3a@hovoldconsulting.com If the device is already suspended when remove is called then you'll end up with an unbalanced call to ov7740_set_power() that will try to disable an already disabled clock. > > So this has nothing to do with pm_runtime_get_sync() per se. > > Yes, but some patches on this series are cleaning up the driver release > logic. You mentioned this example as an argument against using pm_runtime_get_sync(), which I don't think makes sense. > > > 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. > > Granted, it makes sense along the pm_runtime kAPI. > > It is inconsistent with the behavior of kobject_get*() and other > *_get*() methods that are based or inspired on it, as, on those, the > operations are atomic: either everything succeeds and it doesn't return > an error, or the usage counter is not incremented and the object > state doesn't change after the call. Right, and I'm aware that some people have overlooked this. But its not the end of the world since hardly any driver can handle resume failures properly anyway. This is mostly just an exercise to shut up static checkers. > > > 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. > > Agreed. A name that would be more consistent with PM runtime would > probably be: > > pm_runtime_resume_if_get() Naw, since the get part always succeeds. It should start with pm_runtime_get, but pm_runtime_get_sync() is unfortunately taken. > as there are already: > > pm_runtime_get_if_in_use() > pm_runtime_get_if_active() > > But any such discussions are out of the scope of this patchset ;-) Right. > > > 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. > > Very true. Yet, it is a lot simpler to use functions that won't change > the state of the objects when returning an error, as this is by far > the most common pattern within the Kernel. A resume failure does change the state (and needs to be recovered from), but I get what you're saying. > Human brains are trained to identify certain patterns. When there's > something using a similar pattern, but with a different behavior, > our brains are more subject to fail identifying problems. Sure. But I'm not sure that having two interfaces with different semantics to do the job is doing us any favours here. But again, that discussion has already been had. And I realise that this is partly also your motive here (even if the old interface isn't going to go away). > > > 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. 80 patches in one series (posted to lkml) is a bit excessive. Perhaps you can break it up in a fixes part and one or more cleanups parts? 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, URIBL_BLOCKED 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 95B39C433ED for ; Thu, 29 Apr 2021 15:17:23 +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 EAFDC61411 for ; Thu, 29 Apr 2021 15:17:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org EAFDC61411 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=WccRxqWSWor9xAIXFmwxvT8TYwNzu8RHYsneTZu/wg4=; b=nNTGv5WZJUQnvpevuJ4CBkZbX yuOVBM/w5tcneGn6fnzv70qdMkzdfkZOf5kx1qmZIltMdKV29yuMlQXfmtP9n6OIMp/BU4R0Uutlv jPwGpa2SO3lXoiIzqWXxlsHWn0+mFAm/8t2cJ9L66O9rmnVcxnWWtnViI7T5DqNYPFSkEu3AgrCBT uY2QvAsE2qaizmQsonmdGtXh/xvVhD22Ot6f78J47lpdC2oobi6TNjWmI0j7CMsD3kmLDZ3EWxejN WKK82p4Y5yomaCpxTgKJKZSB5xEl4Ua37UyIn0pGIpysCycxvr8oaalrYdTqnONDtDgOIc7ZvuCF2 tk/uIRiYg==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lc8Pf-005vwL-Rn; Thu, 29 Apr 2021 15:17:11 +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 1lc8Pb-005vvm-F5; Thu, 29 Apr 2021 15:17:07 +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=jJ48jMIp0hSfiCLziKFhP9eV9v1tPJ0WIyCQG8H3xMI=; b=veu8asuw6K56NcBelT6Hw1nAQG sJi0G+4AdJyvQRNvHgQCdFJU06FBZ8buNx5k0EhH2YjOo9eMiw3od7tkV9oe5we01PwfT9JG4HFoo HnFPE+H9/Z4njr2x0f4CtGlYK+2f/LSTDf/YH9nf1EP2CrT+7QgF1mHS3eCGglEvD/UEgIrF44+j2 edBiyhdqPBAPqFQOx3KOav3xl11A3SYZ+Zd60ELqt9fIxl5IrXYquIWpzU1mPmHcQnSaYwC5dE1Lf 0kPBKGwsS2s8u7lIh/0j5ryKw64ZuK1FTS5WdW/iXdDlT9WCOECjNHDwMvzi3ectRHNQJRNiMkm7L 3O7yE5Rg==; Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lc8PW-000iew-PX; Thu, 29 Apr 2021 15:17:06 +0000 Received: by mail.kernel.org (Postfix) with ESMTPSA id DADC8613F8; Thu, 29 Apr 2021 15:17:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1619709421; bh=0TF/rgI+kIrUM4ucLgyoRQVNDZwjwMCttPCbdF1J2ts=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=TNblwH+As0w11JRNen3DEa/sKtquJO7N/ed+Wi2cPt5DPo/pzApjHeB4CYlT9czKN RFJnIUjSlU/iXNkujkJ9GfUIBpZYvUPaxBLDLGXBQdVlv5qb6mSpE1DcETxlBJLjdH 3g63qgtE3JXAaoo4Aq3ZJQdiRo/RBYWmOp0+Glxzsc1YjsicyiOaSi7GIhQYDFx0Ss xxX7tIodOuoDf+uK3PdOkr9EHdavuGSGLxTjoqehEjaUHaupkr1YkvsH7C4pSmGQvt kJLY88R3n7qjm+eBETbZXRLROLJk7BZhgXtoeO/pKC8+dHkhcGgjRgK0xEL1TOs1c0 knRtXQpP0PexA== Received: from johan by xi.lan with local (Exim 4.93.0.4) (envelope-from ) id 1lc8Ph-0006kM-4M; Thu, 29 Apr 2021 17:17:13 +0200 Date: Thu, 29 Apr 2021 17:17:13 +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: <20210429121215.64a7cbdb@coco.lan> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20210429121215.64a7cbdb@coco.lan> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210429_081702_926296_5B9566AA X-CRM114-Status: GOOD ( 51.17 ) 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 Thu, Apr 29, 2021 at 12:18:16PM +0200, Mauro Carvalho Chehab wrote: > Em Wed, 28 Apr 2021 17:50:08 +0200 > Johan Hovold escreveu: > > > On Wed, Apr 28, 2021 at 04:51:21PM +0200, Mauro Carvalho Chehab wrote: > > > > 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. > > Ok, but this should equally work: > > pm_runtime_disable(&client->dev); > pm_runtime_set_suspended(&client->dev); > > ov7740_set_power(ov7740, 0); > > as there's no additional cleanup made on this particular driver > between pm_runtime_get_sync() and pm_runtime_put_noidle(). No, that would break the driver as I pointed out to you yesterday: https://lore.kernel.org/r/YImG1klSPkFSaS3a@hovoldconsulting.com If the device is already suspended when remove is called then you'll end up with an unbalanced call to ov7740_set_power() that will try to disable an already disabled clock. > > So this has nothing to do with pm_runtime_get_sync() per se. > > Yes, but some patches on this series are cleaning up the driver release > logic. You mentioned this example as an argument against using pm_runtime_get_sync(), which I don't think makes sense. > > > 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. > > Granted, it makes sense along the pm_runtime kAPI. > > It is inconsistent with the behavior of kobject_get*() and other > *_get*() methods that are based or inspired on it, as, on those, the > operations are atomic: either everything succeeds and it doesn't return > an error, or the usage counter is not incremented and the object > state doesn't change after the call. Right, and I'm aware that some people have overlooked this. But its not the end of the world since hardly any driver can handle resume failures properly anyway. This is mostly just an exercise to shut up static checkers. > > > 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. > > Agreed. A name that would be more consistent with PM runtime would > probably be: > > pm_runtime_resume_if_get() Naw, since the get part always succeeds. It should start with pm_runtime_get, but pm_runtime_get_sync() is unfortunately taken. > as there are already: > > pm_runtime_get_if_in_use() > pm_runtime_get_if_active() > > But any such discussions are out of the scope of this patchset ;-) Right. > > > 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. > > Very true. Yet, it is a lot simpler to use functions that won't change > the state of the objects when returning an error, as this is by far > the most common pattern within the Kernel. A resume failure does change the state (and needs to be recovered from), but I get what you're saying. > Human brains are trained to identify certain patterns. When there's > something using a similar pattern, but with a different behavior, > our brains are more subject to fail identifying problems. Sure. But I'm not sure that having two interfaces with different semantics to do the job is doing us any favours here. But again, that discussion has already been had. And I realise that this is partly also your motive here (even if the old interface isn't going to go away). > > > 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. 80 patches in one series (posted to lkml) is a bit excessive. Perhaps you can break it up in a fixes part and one or more cleanups parts? Johan _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek