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

[Martin Blumenstingl]

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


Martin
[    0.000000] Booting Linux on physical CPU 0x200
[    0.000000] Linux version 5.7.0-rc3-00142-g0be442fc16c8-dirty (xdarklight@blackbox) (gcc version 9.3.0 (Arch Repository), GNU ld (GNU Binutils) 2.34) #6397 SMP PREEMPT Wed Apr 29 19:32:07 CEST 2020
[    0.000000] CPU: ARMv7 Processor [410fc051] revision 1 (ARMv7), cr=10c5387d
[    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache
[    0.000000] OF: fdt: Machine model: Endless Computers Endless Mini
[    0.000000] Memory policy: Data cache writealloc
[    0.000000] cma: Reserved 64 MiB at 0x7c000000
[    0.000000] percpu: Embedded 16 pages/cpu s34824 r8192 d22520 u65536
[    0.000000] Built 1 zonelists, mobility grouping on.  Total pages: 522052
[    0.000000] Kernel command line: console=ttyAML0,115200 root=/dev/sda1 rw
[    0.000000] Dentry cache hash table entries: 131072 (order: 7, 524288 bytes, linear)
[    0.000000] Inode-cache hash table entries: 65536 (order: 6, 262144 bytes, linear)
[    0.000000] mem auto-init: stack:off, heap alloc:off, heap free:off
[    0.000000] Memory: 1965696K/2095104K available (18432K kernel code, 727K rwdata, 6820K rodata, 1024K init, 958K bss, 63872K reserved, 65536K cma-reserved, 1245184K highmem)
[    0.000000] random: get_random_u32 called from __kmem_cache_create+0x28/0x38c with crng_init=0
[    0.000000] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=4, Nodes=1
[    0.000000] rcu: Preemptible hierarchical RCU implementation.
[    0.000000] rcu:     RCU restricting CPUs from NR_CPUS=8 to nr_cpu_ids=4.
[    0.000000]  Tasks RCU enabled.
[    0.000000] rcu: RCU calculated value of scheduler-enlistment delay is 25 jiffies.
[    0.000000] rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=4
[    0.000000] NR_IRQS: 16, nr_irqs: 16, preallocated irqs: 16
[    0.000000] irq_meson_gpio: 119 to 8 gpio interrupt mux initialized
[    0.000000] L2C: DT/platform modifies aux control register: 0x02060000 -> 0x02460000
[    0.000000] L2C-310 erratum 769419 enabled
[    0.000000] L2C-310 ID prefetch enabled, offset 1 lines
[    0.000000] L2C-310 dynamic clock gating enabled, standby mode enabled
[    0.000000] L2C-310 cache controller enabled, 8 ways, 512 kB
[    0.000000] L2C-310: CACHE_ID 0x4100a0c9, AUX_CTRL 0x36460000
[    0.000020] sched_clock: 32 bits at 1000kHz, resolution 1000ns, wraps every 2147483647500ns
[    0.000048] clocksource: timer: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 1911260446275 ns
[    0.000097] Switching to timer-based delay loop, resolution 1000ns
[    0.000913] Console: colour dummy device 80x30
[    0.000970] Calibrating delay loop (skipped), value calculated using timer frequency.. 2.00 BogoMIPS (lpj=4000)
[    0.000989] pid_max: default: 32768 minimum: 301
[    0.001362] LSM: Security Framework initializing
[    0.001696] Smack:  Initializing.
[    0.001706] Smack:  IPv6 port labeling enabled.
[    0.001934] Mount-cache hash table entries: 2048 (order: 1, 8192 bytes, linear)
[    0.001958] Mountpoint-cache hash table entries: 2048 (order: 1, 8192 bytes, linear)
[    0.003392] CPU: Testing write buffer coherency: ok
[    0.003787] CPU0: thread -1, cpu 0, socket 2, mpidr 80000200
[    0.005247] Setting up static identity map for 0x300000 - 0x300060
[    0.005509] rcu: Hierarchical SRCU implementation.
[    0.006663] smp: Bringing up secondary CPUs ...
[    0.007782] CPU1: thread -1, cpu 1, socket 2, mpidr 80000201
[    0.009161] CPU2: thread -1, cpu 2, socket 2, mpidr 80000202
[    0.010384] CPU3: thread -1, cpu 3, socket 2, mpidr 80000203
[    0.010573] smp: Brought up 1 node, 4 CPUs
[    0.010599] SMP: Total of 4 processors activated (8.00 BogoMIPS).
[    0.010608] CPU: All CPU(s) started in SVC mode.
[    0.011595] devtmpfs: initialized
[    0.020125] VFP support v0.3: implementor 41 architecture 2 part 30 variant 5 rev 1
[    0.021092] clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 7645041785100000 ns
[    0.021123] futex hash table entries: 1024 (order: 4, 65536 bytes, linear)
[    0.025439] xor: measuring software checksum speed
[    0.064336]    arm4regs  :   666.000 MB/sec
[    0.104337]    8regs     :   633.000 MB/sec
[    0.144344]    32regs    :   586.000 MB/sec
[    0.184343]    neon      :   657.000 MB/sec
[    0.184356] xor: using function: arm4regs (666.000 MB/sec)
[    0.184435] pinctrl core: initialized pinctrl subsystem
[    0.185471] thermal_sys: Registered thermal governor 'fair_share'
[    0.185477] thermal_sys: Registered thermal governor 'bang_bang'
[    0.185491] thermal_sys: Registered thermal governor 'step_wise'
[    0.185501] thermal_sys: Registered thermal governor 'user_space'
[    0.186789] NET: Registered protocol family 16
[    0.189976] DMA: preallocated 256 KiB pool for atomic coherent allocations
[    0.190729] audit: initializing netlink subsys (disabled)
[    0.191046] audit: type=2000 audit(0.188:1): state=initialized audit_enabled=0 res=1
[    0.192079] cpuidle: using governor menu
[    0.192367] No ATAGs?
[    0.192566] hw-breakpoint: found 2 (+1 reserved) breakpoint and 1 watchpoint registers.
[    0.192583] hw-breakpoint: maximum watchpoint size is 4 bytes.
[    0.194443] clk-m8b-test c1104000.system-controller:clock-controller: m8b_clkc_test INIT
....
diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
index 95678a2b017c..92ad13418605 100644
--- a/drivers/clk/meson/meson8b.c
+++ b/drivers/clk/meson/meson8b.c
@@ -3863,3 +3863,32 @@ CLK_OF_DECLARE_DRIVER(meson8b_clkc, "amlogic,meson8b-clkc",
 		      meson8b_clkc_init);
 CLK_OF_DECLARE_DRIVER(meson8m2_clkc, "amlogic,meson8m2-clkc",
 		      meson8m2_clkc_init);
+
+#include <linux/platform_device.h>
+
+static const struct of_device_id of_match_m8b_clkc_test[] = {
+	{ .compatible = "amlogic,meson8b-clkc", },
+	{ .compatible = "amlogic,meson8m2-clkc", },
+	{ /* sentinel */ }
+};
+
+static int m8b_clkc_test_probe(struct platform_device *pdev)
+{
+	dev_err(&pdev->dev, "m8b_clkc_test INIT\n");
+	return 0;
+}
+
+static struct platform_driver m8b_clkc_test_drv = {
+	.probe = m8b_clkc_test_probe,
+	.driver = {
+		.name = "clk-m8b-test",
+		.of_match_table = of_match_m8b_clkc_test,
+	},
+};
+
+static int __init m8b_clkc_test_init(void)
+{
+	return platform_driver_register(&m8b_clkc_test_drv);
+}
+
+arch_initcall(m8b_clkc_test_init);