| Message ID | 20170628135345.9337-1-jbrunet@baylibre.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Stephen Boyd |
| Headers | show |
On 28/06/17 14:53, Jerome Brunet wrote: > Current implementation of scpi_clk_add just print a warning when clock > fails to register but then keep going as if nothing happened. The > provider is then registered with bogus data. > > This may latter lead to an Oops in __clk_create_clk when > hlist_add_head(&clk->clks_node, &hw->core->clks) is called. > What's the path of this call ? I see one in devm_clk_hw_register but that's one which failed. Also one of the reason for keeping it continuing is, if firmware fails on some non-critical clock, that's fine rather than punishing the entire set of clocks and may even fail the boot.
On Wed, 2017-06-28 at 16:04 +0100, Sudeep Holla wrote: > > On 28/06/17 14:53, Jerome Brunet wrote: > > Current implementation of scpi_clk_add just print a warning when clock > > fails to register but then keep going as if nothing happened. The > > provider is then registered with bogus data. > > > > This may latter lead to an Oops in __clk_create_clk when > > hlist_add_head(&clk->clks_node, &hw->core->clks) is called. > > > What's the path of this call ? I see one in devm_clk_hw_register > but that's one which failed. > bL cpu freq driver requesting the cpu clock, which failed to register. Here the Oops call trace: [ 2.202284] [<ffff00000849a058>] __clk_create_clk.part.18+0x68/0xb0 [ 2.208494] [<ffff00000849ac2c>] __of_clk_get_from_provider+0xfc/0x140 [ 2.214962] [<ffff000008496c28>] __of_clk_get_by_name+0x100/0x118 [ 2.220999] [<ffff000008496c94>] clk_get+0x2c/0x78 [ 2.225744] [<ffff000008570110>] dev_pm_opp_get_opp_table+0xb0/0x118 [ 2.232039] [<ffff000008570940>] dev_pm_opp_add+0x20/0x68 [ 2.237388] [<ffff0000087a0f30>] scpi_init_opp_table+0xa8/0x188 [ 2.243252] [<ffff0000087a0558>] _get_cluster_clk_and_freq_table+0x80/0x180 [ 2.250151] [<ffff0000087a0a48>] bL_cpufreq_init+0x3f0/0x480 [ 2.255758] [<ffff00000879eed8>] cpufreq_online+0xc0/0x658 [ 2.261191] [<ffff00000879f500>] cpufreq_add_dev+0x78/0x88 [ 2.266625] [<ffff00000855c2c4>] subsys_interface_register+0x84/0xc8 [ 2.272922] [<ffff00000879e330>] cpufreq_register_driver+0x138/0x1b8 [ 2.279218] [<ffff0000087a0b4c>] bL_cpufreq_register+0x74/0x120 [ 2.285083] [<ffff0000087a1038>] scpi_cpufreq_probe+0x28/0x38 [ 2.290776] [<ffff00000855fbf0>] platform_drv_probe+0x50/0xb8 [ 2.296468] [<ffff00000855dd84>] driver_probe_device+0x21c/0x2d8 But that's not the point. The point is there is path in clk-scpi driver which registers uninitialized data in the clock provider. That's not good. > Also one of the reason for keeping it continuing is, if firmware fails > on some non-critical clock, that's fine rather than punishing the entire > set of clocks and may even fail the boot. I understand, but you have no way to know whether a clock is critical or not so this explanation looks a bit weak, plus it does not keep the boot from failing ... not for me at least. As explained this approach is registering corrupt data in the provider when failing. It makes the kernel Oops, it shall be fixed. If you have a better solution later on, I don't think there would be any problem to revert this patch. -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jerome, Thanks for the fix. On 28/06/17 14:53, Jerome Brunet wrote: > Current implementation of scpi_clk_add just print a warning when clock > fails to register but then keep going as if nothing happened. The > provider is then registered with bogus data. > > This may latter lead to an Oops in __clk_create_clk when > hlist_add_head(&clk->clks_node, &hw->core->clks) is called. > > This patch fixes the issue and errors if a clock fails to register. > > Fixes: cd52c2a4b5c4 ("clk: add support for clocks provided by SCP(System Control Processor)") Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
On 06/28, Jerome Brunet wrote: > Current implementation of scpi_clk_add just print a warning when clock > fails to register but then keep going as if nothing happened. The > provider is then registered with bogus data. > > This may latter lead to an Oops in __clk_create_clk when > hlist_add_head(&clk->clks_node, &hw->core->clks) is called. > > This patch fixes the issue and errors if a clock fails to register. > > Fixes: cd52c2a4b5c4 ("clk: add support for clocks provided by SCP(System Control Processor)") > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- Applied to clk-next
diff --git a/drivers/clk/clk-scpi.c b/drivers/clk/clk-scpi.c index 96d37175d0ad..e44b5ca91fed 100644 --- a/drivers/clk/clk-scpi.c +++ b/drivers/clk/clk-scpi.c @@ -245,10 +245,12 @@ static int scpi_clk_add(struct device *dev, struct device_node *np, sclk->id = val; err = scpi_clk_ops_init(dev, match, sclk, name); - if (err) + if (err) { dev_err(dev, "failed to register clock '%s'\n", name); - else - dev_dbg(dev, "Registered clock '%s'\n", name); + return err; + } + + dev_dbg(dev, "Registered clock '%s'\n", name); clk_data->clk[idx] = sclk; }
Current implementation of scpi_clk_add just print a warning when clock fails to register but then keep going as if nothing happened. The provider is then registered with bogus data. This may latter lead to an Oops in __clk_create_clk when hlist_add_head(&clk->clks_node, &hw->core->clks) is called. This patch fixes the issue and errors if a clock fails to register. Fixes: cd52c2a4b5c4 ("clk: add support for clocks provided by SCP(System Control Processor)") Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/clk/clk-scpi.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)