Re:Re: [PATCH] clk/meson: fixes memleak issue in init err branch

[èåå]

From: Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
Date: 2020-04-30 01:43:55
To:  Jerome Brunet <jbrunet@xxxxxxxxxxxx>
Cc:  Bernard Zhao <bernard@xxxxxxxx>,Neil Armstrong <narmstrong@xxxxxxxxxxxx>,Stephen Boyd <sboyd@xxxxxxxxxx>,Kevin Hilman <khilman@xxxxxxxxxxxx>,linux-amlogic@xxxxxxxxxxxxxxxxxxx,linux-clk@xxxxxxxxxxxxxxx,linux-kernel@xxxxxxxxxxxxxxx,opensource.kernel@xxxxxxxx
Subject: Re: [PATCH] clk/meson: fixes memleak issue in init err branch>Hi Jerome,
>
>On Wed, Apr 29, 2020 at 2:37 PM Jerome Brunet <jbrunet@xxxxxxxxxxxx> wrote:
>>
>>
>> On Wed 29 Apr 2020 at 05:14, Bernard Zhao <bernard@xxxxxxxx> wrote:
>>
>> > In common init function, when run into err branch, we didn`t
>> > use kfree to release kzmalloc area, this may bring in memleak
>>
>> Thx for reporting this Bernard.
>> I'm not a fan of adding kfree everywhere. I'd much prefer a label and
>> clear error exit path.
>>
>> That being said, the allocation is probably not the only thing that
>> needs to be undone in case of error. I guess this is due to conversion
>> to CLK_OF_DECLARE_DRIVER() which forced to drop all the devm_
>> This was done because the clock controller was required early in the
>> boot sequence.
>>
>> There is 2 paths to properly solve this:
>> 1) Old school: manually undo everything with every error exit condition
>>    Doable but probably a bit messy
>> 2) Convert back the driver to a real platform driver and use devm_.
>>    We would still need the controller to register early but I wonder if
>>    we could use the same method as drivers/clk/mediatek/clk-mt2701.c and
>>    use arch_initcall() ?
>>
>> Martin, you did the initial conversion, what do you think of option 2 ?
>I tried it with the attached patch
>unfortunately my "m8b_clkc_test_probe" is still run too late
>
>> Would it still answer the problem you were trying to solve back then ?
>I'm afraid it does not:
>- the resets are needed early for SMP initialization
>- the clocks are needed even earlier for timer registration (we have
>both, the ARM TWD timer and some Amlogic custom timer. both have clock
>inputs)
>
>> One added benefit of option 2 is we could drop CLK_OF_DECLARE_DRIVER().
>> We could even do the same in for the other SoCs, which I suppose would
>> avoid a fair amount of probe deferral.
>it would be great, indeed
>but this will only work once timer initialization and SMP boot can
>happen at a later stage
>
>If the clock controller registration fails the board won't boot. Yes,
>cleaning up memory is good, but in this specific case it will add a
>couple of extra CPU cycles before the kernel is dead
>So, if we want to ignore that fact then I agree with your first option
>(undoing things the "old school" way).
>

Hi
I am not sure whether my understanding is correct. 
I feels that the failure of our module can not block the entire kernel from starting. 
Maybe we should throw an exception, clear the status, as "old way", 
and continue to execute the kernel.

And if our module requires that the kernel cannot continue to run when the exception occurs,
then we do not need to return in the error branch, also we do not need to kfree, just BUG_ON().

Regards,
Bernard

>Martin