Hi Jerome,
On Wed, Apr 29, 2020 at 2:37 PM Jerome Brunet <jbru...@baylibre.com> wrote:
>
>
> On Wed 29 Apr 2020 at 05:14, Bernard Zhao <bern...@vivo.com> 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);
