diff mbox series

[v3,14/17] media: i2c: imx290: Initialize runtime PM before subdev

Message ID 20230116144454.1012-15-laurent.pinchart@ideasonboard.com
State Accepted
Commit 02852c01f65402e2fe4a8a5fe5a0b641f245b529
Headers show
Series media: i2c: imx290: Miscellaneous improvements | expand

Commit Message

Laurent Pinchart Jan. 16, 2023, 2:44 p.m. UTC 
Initializing the subdev before runtime PM means that no subdev
initialization can interact with the runtime PM framework. This can be
problematic when modifying controls, as the .s_ctrl() handler commonly
calls pm_runtime_get_if_in_use(). These code paths are not trivial,
making the driver fragile and possibly causing subtle bugs.

To make the subdev initialization more robust, initialize runtime PM
first.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes since v1:

    - Use the correct error label when subdev registration fails
---
 drivers/media/i2c/imx290.c | 59 ++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 25 deletions(-)

Comments

Guenter Roeck Feb. 27, 2023, 5:52 p.m. UTC | #1 
Hi,

On Mon, Jan 16, 2023 at 04:44:51PM +0200, Laurent Pinchart wrote:
> Initializing the subdev before runtime PM means that no subdev
> initialization can interact with the runtime PM framework. This can be
> problematic when modifying controls, as the .s_ctrl() handler commonly
> calls pm_runtime_get_if_in_use(). These code paths are not trivial,
> making the driver fragile and possibly causing subtle bugs.
> 
> To make the subdev initialization more robust, initialize runtime PM
> first.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---

This patch results in

Error log:
<stdin>:1517:2: warning: #warning syscall clone3 not implemented [-Wcpp]
drivers/media/i2c/imx290.c:1090:12: error: 'imx290_runtime_suspend' defined but not used [-Werror=unused-function]
 1090 | static int imx290_runtime_suspend(struct device *dev)
      |            ^~~~~~~~~~~~~~~~~~~~~~
drivers/media/i2c/imx290.c:1082:12: error: 'imx290_runtime_resume' defined but not used [-Werror=unused-function]
 1082 | static int imx290_runtime_resume(struct device *dev)

if PM runtime support is disabled( alpha:allmodconfig, csky:allmodconfig,
and others).

Guenter

---
#regzbot ^introduced 02852c01f6540
#regzbot title Build error in drivers/media/i2c/imx290.c if PM support is disabled

---
# bad: [f3a2439f20d918930cc4ae8f76fe1c1afd26958f] Merge tag 'rproc-v6.3' of git://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux
# good: [116b41162f8b267987ea9a73eb7e73eaa7c2cce5] Merge tag 'probes-v6.3-2' of git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace
git bisect start 'HEAD' '116b41162f8b'
# good: [d4563201f33a022fc0353033d9dfeb1606a88330] Documentation: simplify and clarify DCO contribution example language
git bisect good d4563201f33a022fc0353033d9dfeb1606a88330
# bad: [bca7822cbc76b22572faf2e17ca9517b68ebeb3e] media: atomisp: ov2680: Drop MAX_FMTS define
git bisect bad bca7822cbc76b22572faf2e17ca9517b68ebeb3e
# bad: [27e45f2e59c9db2c83ed67775e911c8a3c776db2] media: ti: omap4iss: Use media_pipeline_for_each_entity()
git bisect bad 27e45f2e59c9db2c83ed67775e911c8a3c776db2
# good: [8a54644571fed484d55b3807f25f64cba8a9ca77] media: subdev: Require code change to enable [GS]_ROUTING
git bisect good 8a54644571fed484d55b3807f25f64cba8a9ca77
# good: [8508455961d5a9e8907bcfd8dcd58f19d9b6ce47] media: i2c: imx219: Split common registers from mode tables
git bisect good 8508455961d5a9e8907bcfd8dcd58f19d9b6ce47
# good: [10591fe63691bd8199d5e7244029cc065959ffc9] media: i2c: imx290: Rename, extend and expand usage of imx290_pixfmt
git bisect good 10591fe63691bd8199d5e7244029cc065959ffc9
# bad: [e14d3ac81bd2264edc76bf5796305b2dfea44487] media: i2c: Add driver for OmniVision OV8858
git bisect bad e14d3ac81bd2264edc76bf5796305b2dfea44487
# bad: [7d399658f7c666ead4bc3dbe88944bb8ea7746ca] media: i2c: imx290: Configure data lanes at start time
git bisect bad 7d399658f7c666ead4bc3dbe88944bb8ea7746ca
# bad: [02852c01f65402e2fe4a8a5fe5a0b641f245b529] media: i2c: imx290: Initialize runtime PM before subdev
git bisect bad 02852c01f65402e2fe4a8a5fe5a0b641f245b529
# good: [a8c3e0c1bf1e97b5ee094951ed0f1e57e3b378c7] media: i2c: imx290: Use runtime PM autosuspend
git bisect good a8c3e0c1bf1e97b5ee094951ed0f1e57e3b378c7
# first bad commit: [02852c01f65402e2fe4a8a5fe5a0b641f245b529] media: i2c: imx290: Initialize runtime PM before subdev
Linux regression tracking (Thorsten Leemhuis) March 12, 2023, 1:10 p.m. UTC | #2 
On 27.02.23 18:52, Guenter Roeck wrote:
> On Mon, Jan 16, 2023 at 04:44:51PM +0200, Laurent Pinchart wrote:
>> Initializing the subdev before runtime PM means that no subdev
>> initialization can interact with the runtime PM framework. This can be
>> problematic when modifying controls, as the .s_ctrl() handler commonly
>> calls pm_runtime_get_if_in_use(). These code paths are not trivial,
>> making the driver fragile and possibly causing subtle bugs.
>>
>> To make the subdev initialization more robust, initialize runtime PM
>> first.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>> ---
> 
> This patch results in
> 
> Error log:
> <stdin>:1517:2: warning: #warning syscall clone3 not implemented [-Wcpp]
> drivers/media/i2c/imx290.c:1090:12: error: 'imx290_runtime_suspend' defined but not used [-Werror=unused-function]
>  1090 | static int imx290_runtime_suspend(struct device *dev)
>       |            ^~~~~~~~~~~~~~~~~~~~~~
> drivers/media/i2c/imx290.c:1082:12: error: 'imx290_runtime_resume' defined but not used [-Werror=unused-function]
>  1082 | static int imx290_runtime_resume(struct device *dev)
> 
> if PM runtime support is disabled( alpha:allmodconfig, csky:allmodconfig,
> and others).

Looks like Guenter never got a reply, but from a recent kernelci report
it looks like above warning still happens:
https://lore.kernel.org/all/640bceb7.a70a0220.af8cd.146b@mx.google.com/

Laurent, do you still have it on your radar?

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

#regzbot poke
Laurent Pinchart March 12, 2023, 1:34 p.m. UTC | #3 
Hello Thorsten,

On Sun, Mar 12, 2023 at 02:10:16PM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 27.02.23 18:52, Guenter Roeck wrote:
> > On Mon, Jan 16, 2023 at 04:44:51PM +0200, Laurent Pinchart wrote:
> >> Initializing the subdev before runtime PM means that no subdev
> >> initialization can interact with the runtime PM framework. This can be
> >> problematic when modifying controls, as the .s_ctrl() handler commonly
> >> calls pm_runtime_get_if_in_use(). These code paths are not trivial,
> >> making the driver fragile and possibly causing subtle bugs.
> >>
> >> To make the subdev initialization more robust, initialize runtime PM
> >> first.
> >>
> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> >> ---
> > 
> > This patch results in
> > 
> > Error log:
> > <stdin>:1517:2: warning: #warning syscall clone3 not implemented [-Wcpp]
> > drivers/media/i2c/imx290.c:1090:12: error: 'imx290_runtime_suspend' defined but not used [-Werror=unused-function]
> >  1090 | static int imx290_runtime_suspend(struct device *dev)
> >       |            ^~~~~~~~~~~~~~~~~~~~~~
> > drivers/media/i2c/imx290.c:1082:12: error: 'imx290_runtime_resume' defined but not used [-Werror=unused-function]
> >  1082 | static int imx290_runtime_resume(struct device *dev)
> > 
> > if PM runtime support is disabled( alpha:allmodconfig, csky:allmodconfig,
> > and others).
> 
> Looks like Guenter never got a reply, but from a recent kernelci report
> it looks like above warning still happens:
> https://lore.kernel.org/all/640bceb7.a70a0220.af8cd.146b@mx.google.com/
> 
> Laurent, do you still have it on your radar?

I don't. Arnd has sent a fix
(https://lore.kernel.org/linux-media/20230207161316.293923-1-arnd@kernel.org),
I've reviewed it, now I expect Sakari to pick it up and get it upstream.

> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
> 
> #regzbot poke
Linux regression tracking (Thorsten Leemhuis) March 12, 2023, 1:59 p.m. UTC | #4 
On 12.03.23 14:34, Laurent Pinchart wrote:
> On Sun, Mar 12, 2023 at 02:10:16PM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
>> On 27.02.23 18:52, Guenter Roeck wrote:
>>> On Mon, Jan 16, 2023 at 04:44:51PM +0200, Laurent Pinchart wrote:
>>>> Initializing the subdev before runtime PM means that no subdev
>>>> initialization can interact with the runtime PM framework. This can be
>>>> problematic when modifying controls, as the .s_ctrl() handler commonly
>>>> calls pm_runtime_get_if_in_use(). These code paths are not trivial,
>>>> making the driver fragile and possibly causing subtle bugs.
>>>>
>>>> To make the subdev initialization more robust, initialize runtime PM
>>>> first.
>>>>
>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>> Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>>>> ---
>>>
>>> This patch results in
>>>
>>> Error log:
>>> <stdin>:1517:2: warning: #warning syscall clone3 not implemented [-Wcpp]
>>> drivers/media/i2c/imx290.c:1090:12: error: 'imx290_runtime_suspend' defined but not used [-Werror=unused-function]
>>>  1090 | static int imx290_runtime_suspend(struct device *dev)
>>>       |            ^~~~~~~~~~~~~~~~~~~~~~
>>> drivers/media/i2c/imx290.c:1082:12: error: 'imx290_runtime_resume' defined but not used [-Werror=unused-function]
>>>  1082 | static int imx290_runtime_resume(struct device *dev)
>>>
>>> if PM runtime support is disabled( alpha:allmodconfig, csky:allmodconfig,
>>> and others).
>>
>> Looks like Guenter never got a reply, but from a recent kernelci report
>> it looks like above warning still happens:
>> https://lore.kernel.org/all/640bceb7.a70a0220.af8cd.146b@mx.google.com/
>>
>> Laurent, do you still have it on your radar?
> 
> I don't. Arnd has sent a fix
> (https://lore.kernel.org/linux-media/20230207161316.293923-1-arnd@kernel.org),
> I've reviewed it, now I expect Sakari to pick it up and get it upstream.

Ahh, great, thx for taking the time and letting me know, much appreciated.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

P.S.: update the status

#regzbot monitor:
https://lore.kernel.org/linux-media/20230207161316.293923-1-arnd@kernel.org/
#regzbot fix: media: i2c: imx290: fix conditional function defintions
#regzbot ignore-activity
diff mbox series

Patch

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 324d30ed5617..4185835f065d 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -581,9 +581,7 @@  static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 
 	/*
 	 * Return immediately for controls that don't need to be applied to the
-	 * device. Those controls are modified in imx290_ctrl_update(), which
-	 * is called at probe time before runtime PM is initialized, so we
-	 * can't proceed to the pm_runtime_get_if_in_use() call below.
+	 * device.
 	 */
 	if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
 		return 0;
@@ -1049,22 +1047,20 @@  static void imx290_subdev_cleanup(struct imx290 *imx290)
  * Power management
  */
 
-static int imx290_power_on(struct device *dev)
+static int imx290_power_on(struct imx290 *imx290)
 {
-	struct v4l2_subdev *sd = dev_get_drvdata(dev);
-	struct imx290 *imx290 = to_imx290(sd);
 	int ret;
 
 	ret = clk_prepare_enable(imx290->xclk);
 	if (ret) {
-		dev_err(dev, "Failed to enable clock\n");
+		dev_err(imx290->dev, "Failed to enable clock\n");
 		return ret;
 	}
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(imx290->supplies),
 				    imx290->supplies);
 	if (ret) {
-		dev_err(dev, "Failed to enable regulators\n");
+		dev_err(imx290->dev, "Failed to enable regulators\n");
 		clk_disable_unprepare(imx290->xclk);
 		return ret;
 	}
@@ -1079,20 +1075,33 @@  static int imx290_power_on(struct device *dev)
 	return 0;
 }
 
-static int imx290_power_off(struct device *dev)
+static void imx290_power_off(struct imx290 *imx290)
 {
-	struct v4l2_subdev *sd = dev_get_drvdata(dev);
-	struct imx290 *imx290 = to_imx290(sd);
-
 	clk_disable_unprepare(imx290->xclk);
 	gpiod_set_value_cansleep(imx290->rst_gpio, 1);
 	regulator_bulk_disable(ARRAY_SIZE(imx290->supplies), imx290->supplies);
+}
+
+static int imx290_runtime_resume(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct imx290 *imx290 = to_imx290(sd);
+
+	return imx290_power_on(imx290);
+}
+
+static int imx290_runtime_suspend(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct imx290 *imx290 = to_imx290(sd);
+
+	imx290_power_off(imx290);
 
 	return 0;
 }
 
 static const struct dev_pm_ops imx290_pm_ops = {
-	SET_RUNTIME_PM_OPS(imx290_power_off, imx290_power_on, NULL)
+	SET_RUNTIME_PM_OPS(imx290_runtime_suspend, imx290_runtime_resume, NULL)
 };
 
 /* ----------------------------------------------------------------------------
@@ -1271,20 +1280,15 @@  static int imx290_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
-	/* Initialize the V4L2 subdev. */
-	ret = imx290_subdev_init(imx290);
-	if (ret)
-		return ret;
-
 	/*
 	 * Enable power management. The driver supports runtime PM, but needs to
 	 * work when runtime PM is disabled in the kernel. To that end, power
 	 * the sensor on manually here.
 	 */
-	ret = imx290_power_on(dev);
+	ret = imx290_power_on(imx290);
 	if (ret < 0) {
 		dev_err(dev, "Could not power on the device\n");
-		goto err_subdev;
+		return ret;
 	}
 
 	/*
@@ -1298,6 +1302,11 @@  static int imx290_probe(struct i2c_client *client)
 	pm_runtime_set_autosuspend_delay(dev, 1000);
 	pm_runtime_use_autosuspend(dev);
 
+	/* Initialize the V4L2 subdev. */
+	ret = imx290_subdev_init(imx290);
+	if (ret)
+		goto err_pm;
+
 	/*
 	 * Finally, register the V4L2 subdev. This must be done after
 	 * initializing everything as the subdev can be used immediately after
@@ -1306,7 +1315,7 @@  static int imx290_probe(struct i2c_client *client)
 	ret = v4l2_async_register_subdev(&imx290->sd);
 	if (ret < 0) {
 		dev_err(dev, "Could not register v4l2 device\n");
-		goto err_pm;
+		goto err_subdev;
 	}
 
 	/*
@@ -1318,12 +1327,12 @@  static int imx290_probe(struct i2c_client *client)
 
 	return 0;
 
+err_subdev:
+	imx290_subdev_cleanup(imx290);
 err_pm:
 	pm_runtime_disable(dev);
 	pm_runtime_put_noidle(dev);
-	imx290_power_off(dev);
-err_subdev:
-	imx290_subdev_cleanup(imx290);
+	imx290_power_off(imx290);
 	return ret;
 }
 
@@ -1341,7 +1350,7 @@  static void imx290_remove(struct i2c_client *client)
 	 */
 	pm_runtime_disable(imx290->dev);
 	if (!pm_runtime_status_suspended(imx290->dev))
-		imx290_power_off(imx290->dev);
+		imx290_power_off(imx290);
 	pm_runtime_set_suspended(imx290->dev);
 }