clk: samsung: Checking a kmemdup() call in _samsung_clk_register_pll()

clk: samsung: Checking a kmemdup() call in _samsung_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 “_samsung_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/samsung/clk-pll.c?id=1c0cc5f1ae5ee5a6913704c0d75a6e99604ee30a#n1275
https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/clk/samsung/clk-pll.c#L1275

* 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: samsung: Checking a kmemdup() call in _samsung_clk_register_pll()

[Tomasz Figa]

Hi Markus,

2019年10月12日(土) 23:17 Markus Elfring <Markus.Elfring@web.de>:
>
> Hello,
>
> I tried another script for the semantic patch language out.
> This source code analysis approach points out that the implementation
> of the function “_samsung_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/samsung/clk-pll.c?id=1c0cc5f1ae5ee5a6913704c0d75a6e99604ee30a#n1275
> https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/clk/samsung/clk-pll.c#L1275

Thanks for the report.

>
> * Do you find the usage of the format string “%s: could not allocate
>   rate table for %s\n” still appropriate at this place?

Yes, AFAICT there is nothing wrong with that format string.

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

No, there isn't much that can be done if we fail the allocation at
such an early stage.

That said, there is no need to print any warnings or error messages on
allocation failure, so technically they could be removed. It doesn't
really give us anything in case of existing code, though, and only
makes a potential for merge conflicts, so I'd just leave it alone.

Best regards,
Tomasz

Re: clk: samsung: Checking a kmemdup() call in _samsung_clk_register_pll()

[Markus Elfring]

> That said, there is no need to print any warnings or error messages on
> allocation failure, so technically they could be removed.

Do you find information sufficient from the Linux allocation failure report?

Regards,
Markus

Re: clk: samsung: Checking a kmemdup() call in _samsung_clk_register_pll()

[Tomasz Figa]

2019年10月16日(水) 2:55 Markus Elfring <Markus.Elfring@web.de>:
>
> > That said, there is no need to print any warnings or error messages on
> > allocation failure, so technically they could be removed.
>
> Do you find information sufficient from the Linux allocation failure report?

A backtrace should be enough for this kind of a failure that shouldn't
normally happen and if happens, then the rest of the system must be in
a state already about to fail anyway.

Best regards,
Tomasz

Re: clk: samsung: Checking a kmemdup() call in _samsung_clk_register_pll()

[Markus Elfring]

> A backtrace should be enough for this kind of a failure that shouldn't
> normally happen and if happens, then the rest of the system must be in
> a state already about to fail anyway.

Does this feedback mean that you would like to omit two extra
error messages from this function implementation?

Regards,
Markus

Re: clk: samsung: Checking a kmemdup() call in _samsung_clk_register_pll()

[Markus Elfring]

> That said, there is no need to print any warnings or error messages on
> allocation failure, so technically they could be removed.

Do you find information sufficient from the Linux allocation failure report?

Regards,
Markus

Re: clk: samsung: Checking a kmemdup() call in _samsung_clk_register_pll()

[Markus Elfring]

>> * Is there a need to adjust the error handling here?
>
> No, there isn't much that can be done if we fail the allocation at
> such an early stage.

Can it matter to perform the setting “pll->rate_count” only according
to a null pointer check for the variable “pll->rate_table”
because of the function call “kmemdup”?

Regards,
Markus

Re: clk: samsung: Checking a kmemdup() call in _samsung_clk_register_pll()

[Tomasz Figa]

2019年10月16日(水) 19:55 Markus Elfring <Markus.Elfring@web.de>:
>
> >> * Is there a need to adjust the error handling here?
> >
> > No, there isn't much that can be done if we fail the allocation at
> > such an early stage.
>
> Can it matter to perform the setting “pll->rate_count” only according
> to a null pointer check for the variable “pll->rate_table”
> because of the function call “kmemdup”?

It would be a good practice indeed, but looking from the code,
pll->rate_table is checked elsewhere, not pll->rate_count.

Best regards,
Tomasz

clk: samsung: Checking a kmemdup() call in _samsung_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 “_samsung_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/samsung/clk-pll.c?id=1c0cc5f1ae5ee5a6913704c0d75a6e99604ee30a#n1275
https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/clk/samsung/clk-pll.c#L1275

* 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: samsung: Checking a kmemdup() call in _samsung_clk_register_pll()

[Chanwoo Choi]

Hello,

On 19. 10. 12. 오후 11:17, Markus Elfring wrote:
> Hello,
> 
> I tried another script for the semantic patch language out.
> This source code analysis approach points out that the implementation
> of the function “_samsung_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/samsung/clk-pll.c?id=1c0cc5f1ae5ee5a6913704c0d75a6e99604ee30a#n1275
> https://protect2.fireeye.com/url?k=7e77b0ebee9a0c3e.7e763ba4-43f341fdfe1d32b1&u=https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/clk/samsung/clk-pll.c#L1275
> 
> * 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?

drivers/clk/samsung/clk-pll.c considers the case of 'pll->rate_table is NULL'
So, maybe just show the warning message if failed to allocate memory
of 'pll->rate_table'. Bu, IMHO, the error handling is necessary
in order to support what 'pll_clk->rate_table' isn't NULL.

> 
> Regards,
> Markus
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: clk: samsung: Checking a kmemdup() call in _samsung_clk_register_pll()

[Markus Elfring]

>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/samsung/clk-pll.c?id=1c0cc5f1ae5ee5a6913704c0d75a6e99604ee30a#n1275
>> https://protect2.fireeye.com/url?k=7e77b0ebee9a0c3e.7e763ba4-43f341fdfe1d32b1&u=https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/clk/samsung/clk-pll.c#L1275
…
> drivers/clk/samsung/clk-pll.c considers the case of 'pll->rate_table is NULL'
> So, maybe just show the warning message if failed to allocate memory
> of 'pll->rate_table'.

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


> Bu, IMHO, the error handling is necessary in order to support
> what 'pll_clk->rate_table' isn't NULL.

Can an other error handling strategy make sense at this place?

Regards,
Markus