Re: clk: rockchip: Checking a kmemdup() call in rockchip_clk_register_pll()

[Heiko Stuebner]

Am Sonntag, 13. Oktober 2019, 10:45:09 CEST schrieb Markus Elfring:
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/rockchip/clk-pll.c?id=1c0cc5f1ae5ee5a6913704c0d75a6e99604ee30a#n913
> >> https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/clk/rockchip/clk-pll.c#L913
> >>
> >> * Do you find the usage of the format string â%s: could not allocate
> >>   rate table for %s\nâ still appropriate at this place?
> >
> > If there is an internal "no-memory" output from inside kmemdup now,
> > I guess the one in the clock driver would be a duplicate and could go away.
> 
> How do you think about to recheck information sources around
> the Linux allocation failure report?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=da94001239cceb93c132a31928d6ddc4214862d5#n878
> 
> 
> >> * Is there a need to adjust the error handling here?
> >
> > There is no need for additional error handling.
> 
> If you would like to omit the macro call âWARNâ, I would expect also
> to express a corresponding null pointer check.

So I guess we want something like the change at the bottom.


> > Like if the rate-table could not be duplicated,
> > the clock will still report the correct clockrate
> > you can just not set a new rate.
> 
> How much will a different system configuration matter finally?
> (Do you really want to treat this setting as âoptionalâ?)
> 
> > And for a system it's always better to have the clock driver present
> > than for all device-drivers to fail probing. Especially as this start as
> > core clock driver, so there is no deferring possible.
> 
> I imagine that such a view can be clarified further.

The core soc clock driver gets initialized through CLK_OF_DECLARE(),
aka real early during boot. So if the kmemdup fails there can not be
any -EPROBE_DEFER, as there is no kernel-driver-model running yet.

All other components of the system of course depend on the clock-
controller being available, so that way the system can at least come
up further so that people might be able to debug their issue further.

The other option would be to panic, but the kernel should not
panic if other options are available - and continuing with a static
pll frequency is less invasive in the error case.

Heiko

------ 8< -------
diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
index 198417d56300..17bfac611e79 100644
--- a/drivers/clk/rockchip/clk-pll.c
+++ b/drivers/clk/rockchip/clk-pll.c
@@ -909,14 +909,16 @@ struct clk *rockchip_clk_register_pll(struct rockchip_clk_provider *ctx,
 		for (len = 0; rate_table[len].rate != 0; )
 			len++;
 
-		pll->rate_count = len;
 		pll->rate_table = kmemdup(rate_table,
 					pll->rate_count *
 					sizeof(struct rockchip_pll_rate_table),
 					GFP_KERNEL);
-		WARN(!pll->rate_table,
-			"%s: could not allocate rate table for %s\n",
-			__func__, name);
+
+		/*
+		 * Set num rates to 0 if kmemdup fails. That way the clock
+		 * at least can report its rate and stays usable.
+		 */
+		pll->rate_count = pll->rate_table ? len : 0;
 	}
 
 	switch (pll_type) {