Thread (16 messages) 16 messages, 4 authors, 2015-08-05

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help