Message ID | 20230405092904.1129395-2-martin.kepplinger@puri.sm (mailing list archive) |
---|---|
State | Superseded |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1pjzbt-00CozQ-Mk; Wed, 05 Apr 2023 09:39:21 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237259AbjDEJjT (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Wed, 5 Apr 2023 05:39:19 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46112 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237250AbjDEJjR (ORCPT <rfc822;linux-media@vger.kernel.org>); Wed, 5 Apr 2023 05:39:17 -0400 Received: from comms.puri.sm (comms.puri.sm [159.203.221.185]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D10A6E45; Wed, 5 Apr 2023 02:39:13 -0700 (PDT) Received: from localhost (localhost [127.0.0.1]) by comms.puri.sm (Postfix) with ESMTP id 92502EBEA7; Wed, 5 Apr 2023 02:29:50 -0700 (PDT) Received: from comms.puri.sm ([127.0.0.1]) by localhost (comms.puri.sm [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id XkIvBR3ZAw1f; Wed, 5 Apr 2023 02:29:49 -0700 (PDT) From: Martin Kepplinger <martin.kepplinger@puri.sm> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=puri.sm; s=comms; t=1680686989; bh=J+SamVitwrn6CSgDX3gHlDODJDXG4R3njYfPnja5Jks=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=RDNMOi4apUwBdA6TXv0o8U8+by8UsFv1XJ5M6skx4DGbgp2CDK461L5iRaVc4ww0J M+/zQYDA6IR4BhqXlxvu+oFW3V5stI27gUa3cPeJFr5bY/P2xL8BAa+g6t32wQXjWT kNjdPdJ0R0ALVZoUk35Fben1X5i3tBTAXGp9XkwXIf4nczOxVqnE6UuoCGxTya1pKx g2erFkmlWiboKNeSe/lc/ZX9SIKPJp/V1Fs9Q6kz0OJObYtWT377N/tffdac+tpVYb Ya1tiupdpMorc3npNqmBwARch79BffWirqR/H248dZnDumjArgkS2qmzqGruX+ZJRj fJ+TU8dZw/kmg== To: mchehab@kernel.org, laurent.pinchart@ideasonboard.com Cc: kernel@puri.sm, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, Martin Kepplinger <martin.kepplinger@puri.sm> Subject: [PATCH v1 1/2] media: hi846: fix usage of pm_runtime_get_if_in_use() Date: Wed, 5 Apr 2023 11:29:03 +0200 Message-Id: <20230405092904.1129395-2-martin.kepplinger@puri.sm> In-Reply-To: <20230405092904.1129395-1-martin.kepplinger@puri.sm> References: <20230405092904.1129395-1-martin.kepplinger@puri.sm> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.2 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS autolearn=unavailable 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-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.5 (--) X-LSpam-Report: No, score=-2.5 required=5.0 tests=BAYES_00=-1.9,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1 autolearn=ham autolearn_force=no |
Series |
media: hi846: support system suspend while streaming
|
|
Commit Message
Martin Kepplinger
April 5, 2023, 9:29 a.m. UTC
pm_runtime_get_if_in_use() does not only return nonzero values when
the device is in use, it can return a negative errno too.
And especially during resuming from system suspend, when runtime pm
is not yet up again, this can very well happen. And in such a case
the subsequent pm_runtime_put() call would result in a refcount underflow!
Fix it by correctly using pm_runtime_get_if_in_use().
Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
---
drivers/media/i2c/hi846.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
Hi Martin, On Wed, Apr 05, 2023 at 11:29:03AM +0200, Martin Kepplinger wrote: > pm_runtime_get_if_in_use() does not only return nonzero values when > the device is in use, it can return a negative errno too. > > And especially during resuming from system suspend, when runtime pm > is not yet up again, this can very well happen. And in such a case > the subsequent pm_runtime_put() call would result in a refcount underflow! I think this issue should have a more generic solution, it's very difficult to address this in drivers only with the current APIs. pm_runtime_get_if_in_use() will also return an error if runtime PM is disabled, so this patch will break the driver for that configuration. > > Fix it by correctly using pm_runtime_get_if_in_use(). > > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm> > --- > drivers/media/i2c/hi846.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c > index 5b5ea5425e984..0b0eda2e223cd 100644 > --- a/drivers/media/i2c/hi846.c > +++ b/drivers/media/i2c/hi846.c > @@ -1544,7 +1544,7 @@ static int hi846_set_ctrl(struct v4l2_ctrl *ctrl) > exposure_max); > } > > - if (!pm_runtime_get_if_in_use(&client->dev)) > + if (pm_runtime_get_if_in_use(&client->dev) <= 0) > return 0; > > switch (ctrl->id) {
Hi Martin, Thank you for the patch. On Wed, Apr 05, 2023 at 11:29:03AM +0200, Martin Kepplinger wrote: > pm_runtime_get_if_in_use() does not only return nonzero values when > the device is in use, it can return a negative errno too. > > And especially during resuming from system suspend, when runtime pm > is not yet up again, this can very well happen. And in such a case > the subsequent pm_runtime_put() call would result in a refcount underflow! > > Fix it by correctly using pm_runtime_get_if_in_use(). > > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/i2c/hi846.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c > index 5b5ea5425e984..0b0eda2e223cd 100644 > --- a/drivers/media/i2c/hi846.c > +++ b/drivers/media/i2c/hi846.c > @@ -1544,7 +1544,7 @@ static int hi846_set_ctrl(struct v4l2_ctrl *ctrl) > exposure_max); > } > > - if (!pm_runtime_get_if_in_use(&client->dev)) > + if (pm_runtime_get_if_in_use(&client->dev) <= 0) > return 0; > > switch (ctrl->id) {
Hi Sakari, On Wed, Apr 05, 2023 at 03:52:52PM +0300, Sakari Ailus wrote: > On Wed, Apr 05, 2023 at 11:29:03AM +0200, Martin Kepplinger wrote: > > pm_runtime_get_if_in_use() does not only return nonzero values when > > the device is in use, it can return a negative errno too. > > > > And especially during resuming from system suspend, when runtime pm > > is not yet up again, this can very well happen. And in such a case > > the subsequent pm_runtime_put() call would result in a refcount underflow! > > I think this issue should have a more generic solution, it's very difficult > to address this in drivers only with the current APIs. > > pm_runtime_get_if_in_use() will also return an error if runtime PM is > disabled, so this patch will break the driver for that configuration. I'm increasingly inclined to depend on CONFIG_PM for all camera sensor drivers. > > Fix it by correctly using pm_runtime_get_if_in_use(). > > > > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm> > > --- > > drivers/media/i2c/hi846.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c > > index 5b5ea5425e984..0b0eda2e223cd 100644 > > --- a/drivers/media/i2c/hi846.c > > +++ b/drivers/media/i2c/hi846.c > > @@ -1544,7 +1544,7 @@ static int hi846_set_ctrl(struct v4l2_ctrl *ctrl) > > exposure_max); > > } > > > > - if (!pm_runtime_get_if_in_use(&client->dev)) > > + if (pm_runtime_get_if_in_use(&client->dev) <= 0) > > return 0; > > > > switch (ctrl->id) {
Hi Laurent, On Thu, Apr 06, 2023 at 04:33:38AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Wed, Apr 05, 2023 at 03:52:52PM +0300, Sakari Ailus wrote: > > On Wed, Apr 05, 2023 at 11:29:03AM +0200, Martin Kepplinger wrote: > > > pm_runtime_get_if_in_use() does not only return nonzero values when > > > the device is in use, it can return a negative errno too. > > > > > > And especially during resuming from system suspend, when runtime pm > > > is not yet up again, this can very well happen. And in such a case > > > the subsequent pm_runtime_put() call would result in a refcount underflow! > > > > I think this issue should have a more generic solution, it's very difficult > > to address this in drivers only with the current APIs. > > > > pm_runtime_get_if_in_use() will also return an error if runtime PM is > > disabled, so this patch will break the driver for that configuration. > > I'm increasingly inclined to depend on CONFIG_PM for all camera sensor > drivers. For what reason? This is the smallest of all problems related to power management. Also runtime PM has no-op functions for just this purpose. (Frankly it'd be great if we could make CONFIG_PM go away. So perhaps requiring it everywhere is one feasible approach to do that.)
Hi Sakari, On Thu, Apr 06, 2023 at 04:36:25PM +0300, Sakari Ailus wrote: > On Thu, Apr 06, 2023 at 04:33:38AM +0300, Laurent Pinchart wrote: > > On Wed, Apr 05, 2023 at 03:52:52PM +0300, Sakari Ailus wrote: > > > On Wed, Apr 05, 2023 at 11:29:03AM +0200, Martin Kepplinger wrote: > > > > pm_runtime_get_if_in_use() does not only return nonzero values when > > > > the device is in use, it can return a negative errno too. > > > > > > > > And especially during resuming from system suspend, when runtime pm > > > > is not yet up again, this can very well happen. And in such a case > > > > the subsequent pm_runtime_put() call would result in a refcount underflow! > > > > > > I think this issue should have a more generic solution, it's very difficult > > > to address this in drivers only with the current APIs. > > > > > > pm_runtime_get_if_in_use() will also return an error if runtime PM is > > > disabled, so this patch will break the driver for that configuration. > > > > I'm increasingly inclined to depend on CONFIG_PM for all camera sensor > > drivers. > > For what reason? This is the smallest of all problems related to power > management. Also runtime PM has no-op functions for just this purpose. Because it creates a myriad of sleep (or bigger) issues like this one, and more seem to be popping up (or coming to my attention at least) over time. > (Frankly it'd be great if we could make CONFIG_PM go away. So perhaps > requiring it everywhere is one feasible approach to do that.) I'm all for it :-) For camera sensor drivers, are you aware of use cases where !CONFIG_PM would be desired ?
Am Mittwoch, dem 05.04.2023 um 15:52 +0300 schrieb Sakari Ailus: > Hi Martin, > > On Wed, Apr 05, 2023 at 11:29:03AM +0200, Martin Kepplinger wrote: > > pm_runtime_get_if_in_use() does not only return nonzero values when > > the device is in use, it can return a negative errno too. > > > > And especially during resuming from system suspend, when runtime pm > > is not yet up again, this can very well happen. And in such a case > > the subsequent pm_runtime_put() call would result in a refcount > > underflow! > > I think this issue should have a more generic solution, it's very > difficult > to address this in drivers only with the current APIs. > > pm_runtime_get_if_in_use() will also return an error if runtime PM is > disabled, so this patch will break the driver for that configuration. ok but the driver is currently broken for any *other* error returned by pm_runtime_get_if_in_use() (than the runtime-PM disabled error). The execution-path during system-resume I'm interested in gets -EAGAIN here. Would it be ok for you if I'd return early only for that one error only here? > > > > > Fix it by correctly using pm_runtime_get_if_in_use(). > > > > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm> > > --- > > drivers/media/i2c/hi846.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c > > index 5b5ea5425e984..0b0eda2e223cd 100644 > > --- a/drivers/media/i2c/hi846.c > > +++ b/drivers/media/i2c/hi846.c > > @@ -1544,7 +1544,7 @@ static int hi846_set_ctrl(struct v4l2_ctrl > > *ctrl) > > exposure_max); > > } > > > > - if (!pm_runtime_get_if_in_use(&client->dev)) > > + if (pm_runtime_get_if_in_use(&client->dev) <= 0) > > return 0; > > > > switch (ctrl->id) { >
Hi Martin, On Fri, Apr 07, 2023 at 03:31:02PM +0200, Martin Kepplinger wrote: > Am Mittwoch, dem 05.04.2023 um 15:52 +0300 schrieb Sakari Ailus: > > Hi Martin, > > > > On Wed, Apr 05, 2023 at 11:29:03AM +0200, Martin Kepplinger wrote: > > > pm_runtime_get_if_in_use() does not only return nonzero values when > > > the device is in use, it can return a negative errno too. > > > > > > And especially during resuming from system suspend, when runtime pm > > > is not yet up again, this can very well happen. And in such a case > > > the subsequent pm_runtime_put() call would result in a refcount > > > underflow! > > > > I think this issue should have a more generic solution, it's very > > difficult > > to address this in drivers only with the current APIs. > > > > pm_runtime_get_if_in_use() will also return an error if runtime PM is > > disabled, so this patch will break the driver for that configuration. > > ok but the driver is currently broken for any *other* error returned by > pm_runtime_get_if_in_use() (than the runtime-PM disabled error). > > The execution-path during system-resume I'm interested in gets -EAGAIN > here. Would it be ok for you if I'd return early only for that one > error only here? I guess... but I think to address this in a way that's reasonable to drivers, we'll need improvements to runtime PM API. A largish number of drivers need changes and before doing that we should figure out exactly what should be done. I thought you could effectively trigger this issue by calling runtime PM resume/suspend functions before enabling runtime PM, but this seems to be a different case.
diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c index 5b5ea5425e984..0b0eda2e223cd 100644 --- a/drivers/media/i2c/hi846.c +++ b/drivers/media/i2c/hi846.c @@ -1544,7 +1544,7 @@ static int hi846_set_ctrl(struct v4l2_ctrl *ctrl) exposure_max); } - if (!pm_runtime_get_if_in_use(&client->dev)) + if (pm_runtime_get_if_in_use(&client->dev) <= 0) return 0; switch (ctrl->id) {