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

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

[Guenter Roeck]

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

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

[Linux regression tracking #update (Thorsten Leemhuis)]

[TLDR: This mail in primarily relevant for Linux kernel regression
tracking. See link in footer if these mails annoy you.]

On 27.02.23 18:52, Guenter Roeck wrote:
> ---
> #regzbot ^introduced 02852c01f6540
> 

[this mail is just fixing up an accidental use of "^"; this is kinda my
fault, the regzbot syntax is to confusing; I need to introduce something
like "regzbot forward" to make things more obvious, I just didn't get
around to it yet :-/ ]

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

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
That page also explains what to do if mails like this annoy you.

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

[Linux regression tracking #adding (Thorsten Leemhuis)]

On 28.02.23 10:56, Linux regression tracking #update (Thorsten Leemhuis)
wrote:
> [TLDR: This mail in primarily relevant for Linux kernel regression
> tracking. See link in footer if these mails annoy you.]
> 
> On 27.02.23 18:52, Guenter Roeck wrote:
>> ---
>> #regzbot ^introduced 02852c01f6540
>>
> 
> [this mail is just fixing up an accidental use of "^"; this is kinda my
> fault, the regzbot syntax is to confusing; I need to introduce something
> like "regzbot forward" to make things more obvious, I just didn't get
> around to it yet :-/ ]
> 
> #regzbot ^introduced 02852c01f6540
> #regzbot title Build error in drivers/media/i2c/imx290.c if PM support
> is disabled
> #regzbot ignore-activity

#regzbot fix: media: i2c: imx290: fix conditional function definitions
#regzbot ignore-activity

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
That page also explains what to do if mails like this annoy you.

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

[Thorsten Leemhuis]

[TLDR: This mail in primarily relevant for Linux kernel regression
tracking. See link in footer if these mails annoy you.]

On 20.03.23 11:50, Linux regression tracking #adding (Thorsten Leemhuis)
wrote:
> On 28.02.23 10:56, Linux regression tracking #update (Thorsten Leemhuis)
> wrote:
>>
>> On 27.02.23 18:52, Guenter Roeck wrote:
>>> ---
>>> #regzbot ^introduced 02852c01f6540
>>>
>>
>> [this mail is just fixing up an accidental use of "^"; this is kinda my
>> fault, the regzbot syntax is to confusing; I need to introduce something
>> like "regzbot forward" to make things more obvious, I just didn't get
>> around to it yet :-/ ]
>>
>> #regzbot ^introduced 02852c01f6540
>> #regzbot title Build error in drivers/media/i2c/imx290.c if PM support
>> is disabled
>> #regzbot ignore-activity
> 
> #regzbot fix: media: i2c: imx290: fix conditional function definitions

Sadly the subject changed slightly, which makes this necessary:

#regzbot fix: 7b50567bdcad8
#regzbot ignore-activity

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
That page also explains what to do if mails like this annoy you.

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

[Linux regression tracking (Thorsten Leemhuis)]

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

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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart

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

[Linux regression tracking (Thorsten Leemhuis)]

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