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

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

[Markus Elfring]

Hello,

I tried another script for the semantic patch language out.
This source code analysis approach points out that the implementation
of the function “rockchip_clk_register_pll” contains also a call
of the function “kmemdup”.
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?

* Is there a need to adjust the error handling here?

Regards,
Markus

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

[Heiko Stübner]

Hi Markus,

Am Samstag, 12. Oktober 2019, 15:55:44 CEST schrieb Markus Elfring:
> I tried another script for the semantic patch language out.
> This source code analysis approach points out that the implementation
> of the function “rockchip_clk_register_pll” contains also a call
> of the function “kmemdup”.
> 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.

> * Is there a need to adjust the error handling here?

There is no need for additional error handling. 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.

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.

Heiko

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

[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.


> 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.

Regards,
Markus

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) {

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

[Markus Elfring]

> 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.

I would like to point out that this function implementation contains
the following source code already.

…
	/* name the actual pll */
	snprintf(pll_name, sizeof(pll_name), "pll_%s", name);

	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
	if (!pll)
		return ERR_PTR(-ENOMEM);
…



…
> +++ b/drivers/clk/rockchip/clk-pll.c
> @@ -909,14 +909,16 @@ struct clk *rockchip_clk_register_pll(struct rockchip_clk_provider *ctx,
…
> -		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;

Can an other error handling strategy make sense occasionally?

…
		if (!pll->rate_table) {
			clk_unregister(mux_clk);
			mux_clk = ERR_PTR(-ENOMEM);
			goto err_mux;
		}
…


Would you like to adjust such exception handling another bit?

Regards,
Markus

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

[Heiko Stuebner]

Am Montag, 14. Oktober 2019, 09:26:41 CEST schrieb Markus Elfring:
> > 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.
> 
> I would like to point out that this function implementation contains
> the following source code already.
> 
> …
> 	/* name the actual pll */
> 	snprintf(pll_name, sizeof(pll_name), "pll_%s", name);
> 
> 	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
> 	if (!pll)
> 		return ERR_PTR(-ENOMEM);
> …
> 
> 
> 
> …
> > +++ b/drivers/clk/rockchip/clk-pll.c
> > @@ -909,14 +909,16 @@ struct clk *rockchip_clk_register_pll(struct rockchip_clk_provider *ctx,
> …
> > -		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;
> 
> Can an other error handling strategy make sense occasionally?
>
> 
> …
> 		if (!pll->rate_table) {
> 			clk_unregister(mux_clk);
> 			mux_clk = ERR_PTR(-ENOMEM);
> 			goto err_mux;
> 		}
> …
> 
> 
> Would you like to adjust such exception handling another bit?

Nope.

The big difference is that clocks rely heavily on their names to establish
the clock tree parentship. So the PLL cannot work without the name
but can provide some means of functionality without the rate-table
especially as bootloaders do generally initialize a PLL to some form of
sane frequency.

Heiko

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

[Markus Elfring]

>> Would you like to adjust such exception handling another bit?
>
> Nope.
>
> The big difference is that clocks rely heavily on their names to establish
> the clock tree parentship. So the PLL cannot work without the name

This error situation got a specific reaction.


> but can provide some means of functionality without the rate-table
> especially as bootloaders do generally initialize a PLL to some form of
> sane frequency.

I imagine that a choice is available here for the error handling strategy.

* Return “ERR_PTR(-ENOMEM)” as a strict response like in the other error case.

* Fix the setting “pll->rate_count” at least (to be a bit more tolerant).
  Would any system users wonder then about the availability of only
  a single frequency (instead of possibly expected alternatives)?

Regards,
Markus