* [PATCH] clk: Don't try to enable critical clocks if prepare failed
@ 2019-12-25 16:34 Guenter Roeck
2019-12-26 9:51 ` Jerome Brunet
2019-12-26 22:01 ` Stephen Boyd
0 siblings, 2 replies; 6+ messages in thread
From: Guenter Roeck @ 2019-12-25 16:34 UTC (permalink / raw)
To: Michael Turquette
Cc: Stephen Boyd, linux-clk, linux-kernel, Guenter Roeck, Jerome Brunet
The following traceback is seen if a critical clock fails to prepare.
bcm2835-clk 3f101000.cprman: plld: couldn't lock PLL
------------[ cut here ]------------
Enabling unprepared plld_per
WARNING: CPU: 1 PID: 1 at drivers/clk/clk.c:1014 clk_core_enable+0xcc/0x2c0
...
Call trace:
clk_core_enable+0xcc/0x2c0
__clk_register+0x5c4/0x788
devm_clk_hw_register+0x4c/0xb0
bcm2835_register_pll_divider+0xc0/0x150
bcm2835_clk_probe+0x134/0x1e8
platform_drv_probe+0x50/0xa0
really_probe+0xd4/0x308
driver_probe_device+0x54/0xe8
device_driver_attach+0x6c/0x78
__driver_attach+0x54/0xd8
...
Check return values from clk_core_prepare() and clk_core_enable() and
bail out if any of those functions returns an error.
Cc: Jerome Brunet <jbrunet@baylibre.com>
Fixes: 99652a469df1 ("clk: migrate the count of orphaned clocks at init")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
drivers/clk/clk.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 6a11239ccde3..772258de2d1f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3426,11 +3426,17 @@ static int __clk_core_init(struct clk_core *core)
if (core->flags & CLK_IS_CRITICAL) {
unsigned long flags;
- clk_core_prepare(core);
+ ret = clk_core_prepare(core);
+ if (ret)
+ goto out;
flags = clk_enable_lock();
- clk_core_enable(core);
+ ret = clk_core_enable(core);
clk_enable_unlock(flags);
+ if (ret) {
+ clk_core_unprepare(core);
+ goto out;
+ }
}
clk_core_reparent_orphans_nolock();
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] clk: Don't try to enable critical clocks if prepare failed
2019-12-25 16:34 [PATCH] clk: Don't try to enable critical clocks if prepare failed Guenter Roeck
@ 2019-12-26 9:51 ` Jerome Brunet
2019-12-26 17:22 ` Guenter Roeck
2019-12-26 22:01 ` Stephen Boyd
1 sibling, 1 reply; 6+ messages in thread
From: Jerome Brunet @ 2019-12-26 9:51 UTC (permalink / raw)
To: Guenter Roeck, Michael Turquette; +Cc: Stephen Boyd, linux-clk, linux-kernel
On Wed 25 Dec 2019 at 17:34, Guenter Roeck <linux@roeck-us.net> wrote:
> The following traceback is seen if a critical clock fails to prepare.
>
> bcm2835-clk 3f101000.cprman: plld: couldn't lock PLL
> ------------[ cut here ]------------
> Enabling unprepared plld_per
> WARNING: CPU: 1 PID: 1 at drivers/clk/clk.c:1014 clk_core_enable+0xcc/0x2c0
> ...
> Call trace:
> clk_core_enable+0xcc/0x2c0
> __clk_register+0x5c4/0x788
> devm_clk_hw_register+0x4c/0xb0
> bcm2835_register_pll_divider+0xc0/0x150
> bcm2835_clk_probe+0x134/0x1e8
> platform_drv_probe+0x50/0xa0
> really_probe+0xd4/0x308
> driver_probe_device+0x54/0xe8
> device_driver_attach+0x6c/0x78
> __driver_attach+0x54/0xd8
> ...
>
> Check return values from clk_core_prepare() and clk_core_enable() and
> bail out if any of those functions returns an error.
>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Fixes: 99652a469df1 ("clk: migrate the count of orphaned clocks at init")
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> drivers/clk/clk.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 6a11239ccde3..772258de2d1f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3426,11 +3426,17 @@ static int __clk_core_init(struct clk_core *core)
> if (core->flags & CLK_IS_CRITICAL) {
> unsigned long flags;
>
> - clk_core_prepare(core);
> + ret = clk_core_prepare(core);
> + if (ret)
> + goto out;
>
> flags = clk_enable_lock();
> - clk_core_enable(core);
> + ret = clk_core_enable(core);
> clk_enable_unlock(flags);
> + if (ret) {
> + clk_core_unprepare(core);
> + goto out;
> + }
Hi Guenter,
It looks like it was a mistake to discard the possibility of a failure
here. Thanks for correcting this.
However, we would not want a critical clock to silently fail to
enable. This might lead to unexpected behavior which are generally hard
(and annoying) to debug.
Would you mind adding some kind of warning trace in case this fails ?
Thx
> }
>
> clk_core_reparent_orphans_nolock();
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] clk: Don't try to enable critical clocks if prepare failed
2019-12-26 9:51 ` Jerome Brunet
@ 2019-12-26 17:22 ` Guenter Roeck
2019-12-26 21:59 ` Stephen Boyd
0 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2019-12-26 17:22 UTC (permalink / raw)
To: Jerome Brunet, Michael Turquette; +Cc: Stephen Boyd, linux-clk, linux-kernel
On 12/26/19 1:51 AM, Jerome Brunet wrote:
>
> On Wed 25 Dec 2019 at 17:34, Guenter Roeck <linux@roeck-us.net> wrote:
>
>> The following traceback is seen if a critical clock fails to prepare.
>>
>> bcm2835-clk 3f101000.cprman: plld: couldn't lock PLL
>> ------------[ cut here ]------------
>> Enabling unprepared plld_per
>> WARNING: CPU: 1 PID: 1 at drivers/clk/clk.c:1014 clk_core_enable+0xcc/0x2c0
>> ...
>> Call trace:
>> clk_core_enable+0xcc/0x2c0
>> __clk_register+0x5c4/0x788
>> devm_clk_hw_register+0x4c/0xb0
>> bcm2835_register_pll_divider+0xc0/0x150
>> bcm2835_clk_probe+0x134/0x1e8
>> platform_drv_probe+0x50/0xa0
>> really_probe+0xd4/0x308
>> driver_probe_device+0x54/0xe8
>> device_driver_attach+0x6c/0x78
>> __driver_attach+0x54/0xd8
>> ...
>>
>> Check return values from clk_core_prepare() and clk_core_enable() and
>> bail out if any of those functions returns an error.
>>
>> Cc: Jerome Brunet <jbrunet@baylibre.com>
>> Fixes: 99652a469df1 ("clk: migrate the count of orphaned clocks at init")
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> drivers/clk/clk.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 6a11239ccde3..772258de2d1f 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -3426,11 +3426,17 @@ static int __clk_core_init(struct clk_core *core)
>> if (core->flags & CLK_IS_CRITICAL) {
>> unsigned long flags;
>>
>> - clk_core_prepare(core);
>> + ret = clk_core_prepare(core);
>> + if (ret)
>> + goto out;
>>
>> flags = clk_enable_lock();
>> - clk_core_enable(core);
>> + ret = clk_core_enable(core);
>> clk_enable_unlock(flags);
>> + if (ret) {
>> + clk_core_unprepare(core);
>> + goto out;
>> + }
>
> Hi Guenter,
>
> It looks like it was a mistake to discard the possibility of a failure
> here. Thanks for correcting this.
>
> However, we would not want a critical clock to silently fail to
> enable. This might lead to unexpected behavior which are generally hard
> (and annoying) to debug.
>
> Would you mind adding some kind of warning trace in case this fails ?
>
The really relevant information is:
bcm2835-clk 3f101000.cprman: plld: couldn't lock PLL
which is already displayed (and not surprising since cprman isn't implemented
in qemu). While I agree that an error message might be useful, replacing
one traceback with another doesn't really make sense to me, and I am not
really a friend of spreading tracebacks throughout the kernel. Please feel
free to consider this patch to be a bug report, and feel free to ignore it
and suggest something else.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] clk: Don't try to enable critical clocks if prepare failed
2019-12-26 17:22 ` Guenter Roeck
@ 2019-12-26 21:59 ` Stephen Boyd
2019-12-27 4:05 ` Guenter Roeck
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2019-12-26 21:59 UTC (permalink / raw)
To: Guenter Roeck, Jerome Brunet, Michael Turquette; +Cc: linux-clk, linux-kernel
Quoting Guenter Roeck (2019-12-26 09:22:10)
> On 12/26/19 1:51 AM, Jerome Brunet wrote:
> >
> > However, we would not want a critical clock to silently fail to
> > enable. This might lead to unexpected behavior which are generally hard
> > (and annoying) to debug.
> >
> > Would you mind adding some kind of warning trace in case this fails ?
> >
>
> The really relevant information is:
>
> bcm2835-clk 3f101000.cprman: plld: couldn't lock PLL
>
> which is already displayed (and not surprising since cprman isn't implemented
> in qemu). While I agree that an error message might be useful, replacing
> one traceback with another doesn't really make sense to me, and I am not
> really a friend of spreading tracebacks throughout the kernel. Please feel
> free to consider this patch to be a bug report, and feel free to ignore it
> and suggest something else.
Can the cprman device node be disabled or removed in the DT that qemu
uses? If it isn't actually implemented then it shouldn't be in the DT.
Presumably that will make this traceback go away.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] clk: Don't try to enable critical clocks if prepare failed
2019-12-26 21:59 ` Stephen Boyd
@ 2019-12-27 4:05 ` Guenter Roeck
0 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2019-12-27 4:05 UTC (permalink / raw)
To: Stephen Boyd; +Cc: Jerome Brunet, Michael Turquette, linux-clk, linux-kernel
On Thu, Dec 26, 2019 at 01:59:19PM -0800, Stephen Boyd wrote:
> Quoting Guenter Roeck (2019-12-26 09:22:10)
> > On 12/26/19 1:51 AM, Jerome Brunet wrote:
> > >
> > > However, we would not want a critical clock to silently fail to
> > > enable. This might lead to unexpected behavior which are generally hard
> > > (and annoying) to debug.
> > >
> > > Would you mind adding some kind of warning trace in case this fails ?
> > >
> >
> > The really relevant information is:
> >
> > bcm2835-clk 3f101000.cprman: plld: couldn't lock PLL
> >
> > which is already displayed (and not surprising since cprman isn't implemented
> > in qemu). While I agree that an error message might be useful, replacing
> > one traceback with another doesn't really make sense to me, and I am not
> > really a friend of spreading tracebacks throughout the kernel. Please feel
> > free to consider this patch to be a bug report, and feel free to ignore it
> > and suggest something else.
>
> Can the cprman device node be disabled or removed in the DT that qemu
> uses? If it isn't actually implemented then it shouldn't be in the DT.
> Presumably that will make this traceback go away.
>
cprman feeds all clocks. If the node isn't there, the system doesn't boot.
Also, I don't modify devicetree files in my boot tests; that would defeat
the purpose - like, in this case, to find missing error handling.
Again, please feel free to ignore this patch.
Guenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] clk: Don't try to enable critical clocks if prepare failed
2019-12-25 16:34 [PATCH] clk: Don't try to enable critical clocks if prepare failed Guenter Roeck
2019-12-26 9:51 ` Jerome Brunet
@ 2019-12-26 22:01 ` Stephen Boyd
1 sibling, 0 replies; 6+ messages in thread
From: Stephen Boyd @ 2019-12-26 22:01 UTC (permalink / raw)
To: Guenter Roeck, Michael Turquette
Cc: linux-clk, linux-kernel, Guenter Roeck, Jerome Brunet
Quoting Guenter Roeck (2019-12-25 08:34:29)
> The following traceback is seen if a critical clock fails to prepare.
>
> bcm2835-clk 3f101000.cprman: plld: couldn't lock PLL
> ------------[ cut here ]------------
> Enabling unprepared plld_per
> WARNING: CPU: 1 PID: 1 at drivers/clk/clk.c:1014 clk_core_enable+0xcc/0x2c0
> ...
> Call trace:
> clk_core_enable+0xcc/0x2c0
> __clk_register+0x5c4/0x788
> devm_clk_hw_register+0x4c/0xb0
> bcm2835_register_pll_divider+0xc0/0x150
> bcm2835_clk_probe+0x134/0x1e8
> platform_drv_probe+0x50/0xa0
> really_probe+0xd4/0x308
> driver_probe_device+0x54/0xe8
> device_driver_attach+0x6c/0x78
> __driver_attach+0x54/0xd8
> ...
>
> Check return values from clk_core_prepare() and clk_core_enable() and
> bail out if any of those functions returns an error.
>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Fixes: 99652a469df1 ("clk: migrate the count of orphaned clocks at init")
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
Applied to clk-fixes
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-12-27 4:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-25 16:34 [PATCH] clk: Don't try to enable critical clocks if prepare failed Guenter Roeck
2019-12-26 9:51 ` Jerome Brunet
2019-12-26 17:22 ` Guenter Roeck
2019-12-26 21:59 ` Stephen Boyd
2019-12-27 4:05 ` Guenter Roeck
2019-12-26 22:01 ` Stephen Boyd
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).