Re: [PATCH] clk: scpi: error when clock fails to register

[Jerome Brunet]

On Wed, 2017-06-28 at 16:52 +0100, Sudeep Holla wrote:
> 
> On 28/06/17 16:38, Jerome Brunet wrote:
> > 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
> > 
> 
> Thanks for this stack. I just worked out the same path now. I did come
> up with the patch as below. That should work if my understanding is correct.

I tried.
It does not work unfortunately. Still crashes but somewhere else:
[ÂÂÂÂ2.301482] [<ffff00000849e67c>] scpi_of_clk_src_get+0x14/0x58
[ÂÂÂÂ2.307261] [<ffff000008495f40>] __of_clk_get_by_name+0x100/0x118
[ÂÂÂÂ2.313297] [<ffff000008495fac>] clk_get+0x2c/0x78
[ÂÂÂÂ2.318044] [<ffff00000856f4d0>] dev_pm_opp_get_opp_table+0xb0/0x118
[ÂÂÂÂ2.324338] [<ffff00000856fd00>] dev_pm_opp_add+0x20/0x68
[ÂÂÂÂ2.329687] [<ffff0000087a04f8>] scpi_init_opp_table+0xa8/0x188
[ÂÂÂÂ2.335550] [<ffff00000879fb20>] _get_cluster_clk_and_freq_table+0x80/0x180
[ÂÂÂÂ2.342450] [<ffff0000087a0010>] bL_cpufreq_init+0x3f0/0x480
[ÂÂÂÂ2.348056] [<ffff00000879e4a0>] cpufreq_online+0xc0/0x658
[ÂÂÂÂ2.353490] [<ffff00000879eac8>] cpufreq_add_dev+0x78/0x88
[ÂÂÂÂ2.358924] [<ffff00000855b684>] subsys_interface_register+0x84/0xc8
[ÂÂÂÂ2.365220] [<ffff00000879d8f8>] cpufreq_register_driver+0x138/0x1b8
[ÂÂÂÂ2.371516] [<ffff0000087a0114>] bL_cpufreq_register+0x74/0x120
[ÂÂÂÂ2.377381] [<ffff0000087a0600>] scpi_cpufreq_probe+0x28/0x38
[ÂÂÂÂ2.383076] [<ffff00000855efb0>] platform_drv_probe+0x50/0xb8
[ÂÂÂÂ2.388766] [<ffff00000855d144>] driver_probe_device+0x21c/0x2d8

I have not looked at ALL the clock providers, but I have seen a few and I don't
remember seeing any which fails, at some point, to register a clocks and still
register successfully. 

It seems strange to continue with a broken controller.

> 
> > 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.
> > 
> 
> Agreed, I want to look at ways to fix that, hence requested you more data.
> 
> > If you have a better solution later on, I don't think there would be any
> > problem
> > to revert this patch.
> > 
> 
> Sure I am not against the patch as a fix. I was just trying to better
> understand the problem. I had seen the usefulness of skipping on Juno
> platforms
> in earlier days. Also I am now working on the new SCMI[1] specification
> and just want to cover this.
> 
> ---
> 
> diff --git i/drivers/clk/clk-scpi.c w/drivers/clk/clk-scpi.c
> index 96d37175d0ad..d83c0b84798d 100644
> --- i/drivers/clk/clk-scpi.c
> +++ w/drivers/clk/clk-scpi.c
> @@ -245,11 +245,14 @@ 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
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂclk_data->clk[idx] = NULL;
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdevm_kfree(dev, sclk);
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ} else {
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdev_dbg(dev, "Registered clock '%s'\n", name);
> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂclk_data->clk[idx] = sclk;
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂclk_data->clk[idx] = sclk;
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> ÂÂÂÂÂÂÂÂ}
> 
> ÂÂÂÂÂÂÂÂreturn of_clk_add_hw_provider(np, scpi_of_clk_src_get, clk_data);
>