| Message ID | 20191225163429.29694-1-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | clk: Don't try to enable critical clocks if prepare failed | expand |
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();
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
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.
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
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
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();
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(-)