Re: [PATCH v3 01/11] stm class: Introduce an abstraction for System Trace Module devices
From: Alexander Shishkin <hidden>
Date: 2015-07-29 13:25:18
Also in:
lkml
Chunyan Zhang [off-list ref] writes:
quoted
+/** + * stm_source_register_device() - register an stm_source device + * @parent: parent device + * @data: device description structure + * + * This will create a device of stm_source class that can write + * data to an stm device once linked. + * + * Return: 0 on success, -errno otherwise. + */ +int stm_source_register_device(struct device *parent, + struct stm_source_data *data) +{ + struct stm_source_device *src; + int err; + + if (!stm_core_up) + return -EPROBE_DEFER; +I tried to update Coresight-stm driver[1] based on your this version patch, but the Coresight-stm driver probe() failed. the reason was: In the end of Coresight stm_probe(), we called this function, but "stm_core_up" was zero then, so the error returned value "-EPROBE_DEFER" was received.
Yes, that is the intended behavior if stm core is not initialized yet.
In fact, "stm_core_up" would increase itself until "stm_core_init" be called - it's the root of this problem, I'll explain this where the function "stm_core_init" defined.
I'm sorry, I didn't understand this, can you rephrase?
And redoing Coresight stm_probe() will incur a WARN_ON() like below: [ 1.075746] coresight-stm 10006000.stm: stm_register_device failed [ 1.082118] ------------[ cut here ]------------ [ 1.086819] WARNING: CPU: 1 PID: 1 at drivers/clk/clk.c:657 clk_core_disable+0x138/0x13c() [ 1.095353] Modules linked in: [ 1.098487] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G S 4.2.0-rc1+ #107 [ 1.106398] Hardware name: Spreadtrum SC9836 Openphone Board (DT) [ 1.112678] Call trace: [ 1.115194] [<ffffffc00008a5b4>] dump_backtrace+0x0/0x138 [ 1.120761] [<ffffffc00008a708>] show_stack+0x1c/0x28 [ 1.125972] [<ffffffc0003320e0>] dump_stack+0x84/0xc8 [ 1.131179] [<ffffffc00009b580>] warn_slowpath_common+0xa4/0xdc [ 1.137285] [<ffffffc00009b700>] warn_slowpath_null+0x34/0x44 [ 1.143213] [<ffffffc000321eb4>] clk_core_disable+0x134/0x13c
Well, like I said in the offline thread, this has to do with cleaning up in the error path of stm_probe(). What happens if stm_probe() fails for any other reason? I'm guessing the same warning.
quoted
+static int __init stm_core_init(void) +{ + int err; + + err = class_register(&stm_class); + if (err) + return err; + + err = class_register(&stm_source_class); + if (err) + goto err_stm; + + err = stp_configfs_init(); + if (err) + goto err_src; + + init_srcu_struct(&stm_source_srcu); + + stm_core_up++; + + return 0; + +err_src: + class_unregister(&stm_source_class); +err_stm: + class_unregister(&stm_class); + + return err; +} + +module_init(stm_core_init);Since you are using module_init() instead of postcore_initcall() which was in the last version patch, as such, this function would be executed after Coresight "stm_probe" finished.
Yes, iirc on arm the initcall order somehow forced postcore stm_core_init() before configfs, which it relies on, causing a crash. Now I see that somebody hacked configfs to start at core_initcall (f5b697700c8) instead. There has to be a way to defer stm_probe(), although a quick look at amba code suggests it's not implemented.
So, we think there a few optional solutions:
1) Remove the "stm_register_device" out from Coresight "stm_probe",
but we have to save another global variable:
struct device *stm_dev;
in the process of Coresight "stm_probe".Sorry, didn't understand this one. Except for I can say that having a global variable like that is a bad idea, but that's not relevant to the problem at hand.
2) Change module_init() to other XYX_init() which would run prior to "amba_probe()" (i.e. the caller of Coresight stm_probe), this may be a better one.
I'm really not a big fan of the initcall games, to be honest, it will always be a problem on some architecture or other. Having said that, if stm_core_init() runs at postcore_initcall level, does that solve your problem?
3) stm_core_init() could be turned into a library call where initialisation of the internals is done when first called.
Well, it's not that simple: stm is used by both stm and stm_source
devices, in this case we'll need to make sure that the first call to
either of the {stm,stm_source}_register_device() results in the actual
initialization of the stm core. I think it's a cleaner solution than the
initcall games, though.
Regards,
--
Alex