Re: [PATCH v3 01/11] stm class: Introduce an abstraction for System Trace Module devices

2015-08-05 Thread Mathieu Poirier
On 6 July 2015 at 04:08, Alexander Shishkin
 wrote:
> A System Trace Module (STM) is a device exporting data in System Trace
> Protocol (STP) format as defined by MIPI STP standards. Examples of such
> devices are Intel Trace Hub and Coresight STM.
>
> This abstraction provides a unified interface for software trace sources
> to send their data over an STM device to a debug host. In order to do
> that, such a trace source needs to be assigned a pair of master/channel
> identifiers that all the data from this source will be tagged with. The
> STP decoder on the debug host side will use these master/channel tags to
> distinguish different trace streams from one another inside one STP
> stream.
>
> This abstraction provides a configfs-based policy management mechanism
> for dynamic allocation of these master/channel pairs based on trace
> source-supplied string identifier. It has the flexibility of being
> defined at runtime and at the same time (provided that the policy
> definition is aligned with the decoding end) consistency.
>
> For userspace trace sources, this abstraction provides write()-based and
> mmap()-based (if the underlying stm device allows this) output mechanism.
>
> For kernel-side trace sources, we provide "stm_source" device class that
> can be connected to an stm device at run time.
>
> Cc: linux-...@vger.kernel.org
> Cc: Mathieu Poirier 
> Signed-off-by: Alexander Shishkin 
> ---
>  Documentation/ABI/testing/configfs-stp-policy|   48 +
>  Documentation/ABI/testing/sysfs-class-stm|   14 +
>  Documentation/ABI/testing/sysfs-class-stm_source |   11 +
>  Documentation/ioctl/ioctl-number.txt |3 +
>  Documentation/trace/stm.txt  |   80 ++
>  drivers/Kconfig  |2 +
>  drivers/Makefile |1 +
>  drivers/hwtracing/stm/Kconfig|8 +
>  drivers/hwtracing/stm/Makefile   |3 +
>  drivers/hwtracing/stm/core.c | 1029 
> ++
>  drivers/hwtracing/stm/policy.c   |  529 +++
>  drivers/hwtracing/stm/stm.h  |   87 ++
>  include/linux/stm.h  |  126 +++
>  include/uapi/linux/stm.h |   50 ++
>  14 files changed, 1991 insertions(+)
>  create mode 100644 Documentation/ABI/testing/configfs-stp-policy
>  create mode 100644 Documentation/ABI/testing/sysfs-class-stm
>  create mode 100644 Documentation/ABI/testing/sysfs-class-stm_source
>  create mode 100644 Documentation/trace/stm.txt
>  create mode 100644 drivers/hwtracing/stm/Kconfig
>  create mode 100644 drivers/hwtracing/stm/Makefile
>  create mode 100644 drivers/hwtracing/stm/core.c
>  create mode 100644 drivers/hwtracing/stm/policy.c
>  create mode 100644 drivers/hwtracing/stm/stm.h
>  create mode 100644 include/linux/stm.h
>  create mode 100644 include/uapi/linux/stm.h
>
> diff --git a/Documentation/ABI/testing/configfs-stp-policy 
> b/Documentation/ABI/testing/configfs-stp-policy
> new file mode 100644
> index 00..421ce6825c
> --- /dev/null
> +++ b/Documentation/ABI/testing/configfs-stp-policy
> @@ -0,0 +1,48 @@
> +What:  /config/stp-policy
> +Date:  June 2015
> +KernelVersion: 4.3
> +Description:
> +   This group contains policies mandating Master/Channel 
> allocation
> +   for software sources wishing to send trace data over an STM
> +   device.
> +
> +What:  /config/stp-policy/.
> +Date:  June 2015
> +KernelVersion: 4.3
> +Description:
> +   This group is the root of a policy; its name is a 
> concatenation
> +   of an stm device name to which this policy applies and an
> +   arbitrary string. If  part doesn't match an existing
> +   stm device, mkdir will fail with ENODEV; if that device 
> already
> +   has a policy assigned to it, mkdir will fail with EBUSY.
> +
> +What:  /config/stp-policy/./device
> +Date:  June 2015
> +KernelVersion: 4.3
> +Description:
> +   STM device to which this policy applies, read only. Same as 
> the
> +component of its parent directory.
> +
> +What:  /config/stp-policy/./
> +Date:  June 2015
> +KernelVersion: 4.3
> +Description:
> +   Policy node is a string identifier that software clients will
> +   use to request a master/channel to be allocated and assigned 
> to
> +   them.
> +
> +What:  /config/stp-policy/.//masters
> +Date:  June 2015
> +KernelVersion: 4.3
> +Description:
> +   Range of masters from which to allocate for users of this 
> node.
> +   Write two numbers: the first master and the last master 
> number.
> +
> +What:  /config/stp-policy/.//channels
> +Date:  June 2015
> +KernelVersion: 4.3
> +Description:
> +   Range of chan

Re: [PATCH v3 01/11] stm class: Introduce an abstraction for System Trace Module devices

2015-07-30 Thread Alexander Shishkin
Chunyan Zhang  writes:

> The code has already been submitted like I said in the earlier emails,
> you may refer [1].
>
> Thanks,
> Chunyan
>
> [1] https://lkml.org/lkml/2015/2/4/729

This code does the following (pasting from that patch):

in stm_probe():

drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);

... something irrelevant ...

ret = clk_prepare_enable(drvdata->clk);

... something irrelevant ...

clk_disable_unprepare(drvdata->clk);

... something irrelevant ...

drvdata->clk = adev->pclk;

How is this supposed to even work?

Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v3 01/11] stm class: Introduce an abstraction for System Trace Module devices

2015-07-30 Thread Chunyan Zhang
On Thu, Jul 30, 2015 at 2:59 PM, Chunyan Zhang  wrote:
> On Thu, Jul 30, 2015 at 2:37 PM, Alexander Shishkin
>  wrote:
>> Chunyan Zhang  writes:
>>
>>> Sure, I mean, the root reason of this problem is here ( i.e.
>>> "stm_core_up" was zero then):
>>>  if (!stm_core_up)
>>>  return -EPROBE_DEFER;
>>>
>>> Why it was zero?
>>> Because the function (i.e. stm_core_init() ) in which "stm_core_up"
>>> would be added one hasn't been executed at this moment. It would be
>>> executed on module_init stage for you this version of patch.
>>
>> Again, this is the indented behavior.
>>
>>> The reason of this warning is:
>>> After stm_probe() failed, clk_core_disable() would be called from
>>> amba_put_disable_pclk(), then WARN_ON() happened:
>>>  if (WARN_ON(core->enable_count == 0))
>>>  return;
>>>
>>> I'm guessing the reason why "core->enable_count" was 0 at this moment is:
>>> I don't know who created a thread to process the
>>> amba_pm_runtime_suspend(), in which clk_core_disable() was already
>>> called, "core->enable_count" was, of course, cleared to zero then.
>>> And this thread run before amba_put_disable_pclk(pcdev) which is just
>>> the one called from amba_probe() after
>>> "->probe"(i.e. stm_probe in this case) returning a non-zero value.
>>
>> No, this is guesswork. In amba_probe(), clocks are enabled for the
>> drv->probe() and then disabled afterwards and that's where the refcount
>> ends up unbalanced, the probe is the culprit.
>>
>> I can debug your driver for you but you'll at least need to put the code
>> up somewhere so I can see it.
>
> The code has already been submitted like I said in the earlier emails,
> you may refer [1].
>
> Thanks,
> Chunyan
>
> [1] https://lkml.org/lkml/2015/2/4/729

Er, sorry, it's too old, I'll send out an updated version soon.

Thanks for your patience,
Chunyan

>
>>
>> Regards,
>> --
>> Alex
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v3 01/11] stm class: Introduce an abstraction for System Trace Module devices

2015-07-29 Thread Chunyan Zhang
On Thu, Jul 30, 2015 at 2:37 PM, Alexander Shishkin
 wrote:
> Chunyan Zhang  writes:
>
>> Sure, I mean, the root reason of this problem is here ( i.e.
>> "stm_core_up" was zero then):
>>  if (!stm_core_up)
>>  return -EPROBE_DEFER;
>>
>> Why it was zero?
>> Because the function (i.e. stm_core_init() ) in which "stm_core_up"
>> would be added one hasn't been executed at this moment. It would be
>> executed on module_init stage for you this version of patch.
>
> Again, this is the indented behavior.
>
>> The reason of this warning is:
>> After stm_probe() failed, clk_core_disable() would be called from
>> amba_put_disable_pclk(), then WARN_ON() happened:
>>  if (WARN_ON(core->enable_count == 0))
>>  return;
>>
>> I'm guessing the reason why "core->enable_count" was 0 at this moment is:
>> I don't know who created a thread to process the
>> amba_pm_runtime_suspend(), in which clk_core_disable() was already
>> called, "core->enable_count" was, of course, cleared to zero then.
>> And this thread run before amba_put_disable_pclk(pcdev) which is just
>> the one called from amba_probe() after
>> "->probe"(i.e. stm_probe in this case) returning a non-zero value.
>
> No, this is guesswork. In amba_probe(), clocks are enabled for the
> drv->probe() and then disabled afterwards and that's where the refcount
> ends up unbalanced, the probe is the culprit.
>
> I can debug your driver for you but you'll at least need to put the code
> up somewhere so I can see it.

The code has already been submitted like I said in the earlier emails,
you may refer [1].

Thanks,
Chunyan

[1] https://lkml.org/lkml/2015/2/4/729

>
> Regards,
> --
> Alex
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v3 01/11] stm class: Introduce an abstraction for System Trace Module devices

2015-07-29 Thread Alexander Shishkin
Chunyan Zhang  writes:

> Sure, I mean, the root reason of this problem is here ( i.e.
> "stm_core_up" was zero then):
>  if (!stm_core_up)
>  return -EPROBE_DEFER;
>
> Why it was zero?
> Because the function (i.e. stm_core_init() ) in which "stm_core_up"
> would be added one hasn't been executed at this moment. It would be
> executed on module_init stage for you this version of patch.

Again, this is the indented behavior.

> The reason of this warning is:
> After stm_probe() failed, clk_core_disable() would be called from
> amba_put_disable_pclk(), then WARN_ON() happened:
>  if (WARN_ON(core->enable_count == 0))
>  return;
>
> I'm guessing the reason why "core->enable_count" was 0 at this moment is:
> I don't know who created a thread to process the
> amba_pm_runtime_suspend(), in which clk_core_disable() was already
> called, "core->enable_count" was, of course, cleared to zero then.
> And this thread run before amba_put_disable_pclk(pcdev) which is just
> the one called from amba_probe() after
> "->probe"(i.e. stm_probe in this case) returning a non-zero value.

No, this is guesswork. In amba_probe(), clocks are enabled for the
drv->probe() and then disabled afterwards and that's where the refcount
ends up unbalanced, the probe is the culprit.

I can debug your driver for you but you'll at least need to put the code
up somewhere so I can see it.

Regards,
--
Alex

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v3 01/11] stm class: Introduce an abstraction for System Trace Module devices

2015-07-29 Thread Chunyan Zhang
On Thu, Jul 30, 2015 at 1:45 PM, Alexander Shishkin
 wrote:
> Chunyan Zhang  writes:
>
>> If let stm_probe() implement probe deferral, it has to have a global
>> variable for the later calling of "stm_register_device", because the
>
> No, it doesn't. Please read about probe deferral.

Could you please read my another email sent out today, I explained
more details in that email.

Thanks,
Chunyan

>
> Regards,
> --
> Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v3 01/11] stm class: Introduce an abstraction for System Trace Module devices

2015-07-29 Thread Alexander Shishkin
Chunyan Zhang  writes:

> If let stm_probe() implement probe deferral, it has to have a global
> variable for the later calling of "stm_register_device", because the

No, it doesn't. Please read about probe deferral.

Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v3 01/11] stm class: Introduce an abstraction for System Trace Module devices

2015-07-29 Thread Chunyan Zhang
On Wed, Jul 29, 2015 at 9:46 PM, Alexander Shishkin
 wrote:
> Mark Brown  writes:
>
>> On Wed, Jul 29, 2015 at 04:25:10PM +0300, Alexander Shishkin wrote:
>>
>>> There has to be a way to defer stm_probe(), although a quick look at
>>> amba code suggests it's not implemented.
>>
>> What makes you say this?  Probe deferral is implemented in the driver
>> core rather than individual buses, the buses don't need to know anything
>> about it.
>
> I stand corrected, it indeed is.
>
> So returning EPROBE_DEFER from stm_probe() should Just Work (provided
> stm_probe() handles its error paths correctly).

If let stm_probe() implement probe deferral, it has to have a global
variable for the later calling of "stm_register_device", because the
first parameter of "stm_register_device" is " struct device * " which
comes from amba_probe(), after finished amba_probe(), we may not get
this structure by other means.

This was a similar policy that we both thought was not good :)

>
> Regards,
> --
> Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v3 01/11] stm class: Introduce an abstraction for System Trace Module devices

2015-07-29 Thread Chunyan Zhang
On Wed, Jul 29, 2015 at 9:25 PM, Alexander Shishkin
 wrote:
> Chunyan Zhang  writes:
>
>>> +/**
>>> + * 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?

Sure, I mean, the root reason of this problem is here ( i.e.
"stm_core_up" was zero then):
 if (!stm_core_up)
 return -EPROBE_DEFER;

Why it was zero?
Because the function (i.e. stm_core_init() ) in which "stm_core_up"
would be added one hasn't been executed at this moment. It would be
executed on module_init stage for you this version of patch.

>
>> 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] [] dump_backtrace+0x0/0x138
>> [1.120761] [] show_stack+0x1c/0x28
>> [1.125972] [] dump_stack+0x84/0xc8
>> [1.131179] [] warn_slowpath_common+0xa4/0xdc
>> [1.137285] [] warn_slowpath_null+0x34/0x44
>> [1.143213] [] 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.

The reason of this warning is:
After stm_probe() failed, clk_core_disable() would be called from
amba_put_disable_pclk(), then WARN_ON() happened:
 if (WARN_ON(core->enable_count == 0))
 return;

I'm guessing the reason why "core->enable_count" was 0 at this moment is:
I don't know who created a thread to process the
amba_pm_runtime_suspend(), in which clk_core_disable() was already
called, "core->enable_count" was, of course, cleared to zero then.
And this thread run before amba_put_disable_pclk(pcdev) which is just
the one called from amba_probe() after
"->probe"(i.e. stm_probe in this case) returning a non-zero value.

In a word, if clk_core_disable() is called again after
"core->enable_count" has already been cleared to zero in the first
round of clk_core_disable()'s.

As such, the WARN_ON occurred.

>
>>> +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_

Re: [PATCH v3 01/11] stm class: Introduce an abstraction for System Trace Module devices

2015-07-29 Thread Alexander Shishkin
Mark Brown  writes:

> On Wed, Jul 29, 2015 at 04:25:10PM +0300, Alexander Shishkin wrote:
>
>> There has to be a way to defer stm_probe(), although a quick look at
>> amba code suggests it's not implemented.
>
> What makes you say this?  Probe deferral is implemented in the driver
> core rather than individual buses, the buses don't need to know anything
> about it.

I stand corrected, it indeed is.

So returning EPROBE_DEFER from stm_probe() should Just Work (provided
stm_probe() handles its error paths correctly).

Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v3 01/11] stm class: Introduce an abstraction for System Trace Module devices

2015-07-29 Thread Mark Brown
On Wed, Jul 29, 2015 at 04:25:10PM +0300, Alexander Shishkin wrote:

> There has to be a way to defer stm_probe(), although a quick look at
> amba code suggests it's not implemented.

What makes you say this?  Probe deferral is implemented in the driver
core rather than individual buses, the buses don't need to know anything
about it.


signature.asc
Description: Digital signature


Re: [PATCH v3 01/11] stm class: Introduce an abstraction for System Trace Module devices

2015-07-29 Thread Alexander Shishkin
Chunyan Zhang  writes:

>> +/**
>> + * 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] [] dump_backtrace+0x0/0x138
> [1.120761] [] show_stack+0x1c/0x28
> [1.125972] [] dump_stack+0x84/0xc8
> [1.131179] [] warn_slowpath_common+0xa4/0xdc
> [1.137285] [] warn_slowpath_null+0x34/0x44
> [1.143213] [] 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.

>> +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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v3 01/11] stm class: Introduce an abstraction for System Trace Module devices

2015-07-28 Thread Chunyan Zhang
On Mon, Jul 6, 2015 at 6:08 PM, Alexander Shishkin
 wrote:
> A System Trace Module (STM) is a device exporting data in System Trace
> Protocol (STP) format as defined by MIPI STP standards. Examples of such
> devices are Intel Trace Hub and Coresight STM.
>
> This abstraction provides a unified interface for software trace sources
> to send their data over an STM device to a debug host. In order to do
> that, such a trace source needs to be assigned a pair of master/channel
> identifiers that all the data from this source will be tagged with. The
> STP decoder on the debug host side will use these master/channel tags to
> distinguish different trace streams from one another inside one STP
> stream.
>
> This abstraction provides a configfs-based policy management mechanism
> for dynamic allocation of these master/channel pairs based on trace
> source-supplied string identifier. It has the flexibility of being
> defined at runtime and at the same time (provided that the policy
> definition is aligned with the decoding end) consistency.
>
> For userspace trace sources, this abstraction provides write()-based and
> mmap()-based (if the underlying stm device allows this) output mechanism.
>
> For kernel-side trace sources, we provide "stm_source" device class that
> can be connected to an stm device at run time.
>
> Cc: linux-...@vger.kernel.org
> Cc: Mathieu Poirier 
> Signed-off-by: Alexander Shishkin 
> ---
>  Documentation/ABI/testing/configfs-stp-policy|   48 +
>  Documentation/ABI/testing/sysfs-class-stm|   14 +
>  Documentation/ABI/testing/sysfs-class-stm_source |   11 +
>  Documentation/ioctl/ioctl-number.txt |3 +
>  Documentation/trace/stm.txt  |   80 ++
>  drivers/Kconfig  |2 +
>  drivers/Makefile |1 +
>  drivers/hwtracing/stm/Kconfig|8 +
>  drivers/hwtracing/stm/Makefile   |3 +
>  drivers/hwtracing/stm/core.c | 1029 
> ++
>  drivers/hwtracing/stm/policy.c   |  529 +++
>  drivers/hwtracing/stm/stm.h  |   87 ++
>  include/linux/stm.h  |  126 +++
>  include/uapi/linux/stm.h |   50 ++
>  14 files changed, 1991 insertions(+)
>  create mode 100644 Documentation/ABI/testing/configfs-stp-policy
>  create mode 100644 Documentation/ABI/testing/sysfs-class-stm
>  create mode 100644 Documentation/ABI/testing/sysfs-class-stm_source
>  create mode 100644 Documentation/trace/stm.txt
>  create mode 100644 drivers/hwtracing/stm/Kconfig
>  create mode 100644 drivers/hwtracing/stm/Makefile
>  create mode 100644 drivers/hwtracing/stm/core.c
>  create mode 100644 drivers/hwtracing/stm/policy.c
>  create mode 100644 drivers/hwtracing/stm/stm.h
>  create mode 100644 include/linux/stm.h
>  create mode 100644 include/uapi/linux/stm.h
>
> diff --git a/Documentation/ABI/testing/configfs-stp-policy 
> b/Documentation/ABI/testing/configfs-stp-policy
> new file mode 100644
> index 00..421ce6825c
> --- /dev/null
> +++ b/Documentation/ABI/testing/configfs-stp-policy
> @@ -0,0 +1,48 @@
> +What:  /config/stp-policy
> +Date:  June 2015
> +KernelVersion: 4.3
> +Description:
> +   This group contains policies mandating Master/Channel 
> allocation
> +   for software sources wishing to send trace data over an STM
> +   device.
> +
> +What:  /config/stp-policy/.
> +Date:  June 2015
> +KernelVersion: 4.3
> +Description:
> +   This group is the root of a policy; its name is a 
> concatenation
> +   of an stm device name to which this policy applies and an
> +   arbitrary string. If  part doesn't match an existing
> +   stm device, mkdir will fail with ENODEV; if that device 
> already
> +   has a policy assigned to it, mkdir will fail with EBUSY.
> +
> +What:  /config/stp-policy/./device
> +Date:  June 2015
> +KernelVersion: 4.3
> +Description:
> +   STM device to which this policy applies, read only. Same as 
> the
> +component of its parent directory.
> +
> +What:  /config/stp-policy/./
> +Date:  June 2015
> +KernelVersion: 4.3
> +Description:
> +   Policy node is a string identifier that software clients will
> +   use to request a master/channel to be allocated and assigned 
> to
> +   them.
> +
> +What:  /config/stp-policy/.//masters
> +Date:  June 2015
> +KernelVersion: 4.3
> +Description:
> +   Range of masters from which to allocate for users of this 
> node.
> +   Write two numbers: the first master and the last master 
> number.
> +
> +What:  /config/stp-policy/.//channels
> +Date:  June 2015
> +KernelVersion: 4.3
> +Description:
> +   Range 

Re: [PATCH v3 01/11] stm class: Introduce an abstraction for System Trace Module devices

2015-07-08 Thread Alexander Shishkin
Chunyan Zhang  writes:

>> +int __init stp_configfs_init(void)
>> +{
>> +   int err;
>> +
>> +   config_group_init(&stp_policy_subsys.su_group);
>> +   mutex_init(&stp_policy_subsys.su_mutex);
>> +   err = configfs_register_subsystem(&stp_policy_subsys);
>> +
>> +   return err;
>
> One small suggestion, is it better like this:
> return configfs_register_subsystem(&stp_policy_subsys);
> As such, we don't need the local variable "err" any more.

You are right, err doesn't serve any purpose here.

Thanks,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v3 01/11] stm class: Introduce an abstraction for System Trace Module devices

2015-07-08 Thread Chunyan Zhang
On Mon, Jul 6, 2015 at 6:08 PM, Alexander Shishkin
 wrote:
> A System Trace Module (STM) is a device exporting data in System Trace
> Protocol (STP) format as defined by MIPI STP standards. Examples of such
> devices are Intel Trace Hub and Coresight STM.
>
> This abstraction provides a unified interface for software trace sources
> to send their data over an STM device to a debug host. In order to do
> that, such a trace source needs to be assigned a pair of master/channel
> identifiers that all the data from this source will be tagged with. The
> STP decoder on the debug host side will use these master/channel tags to
> distinguish different trace streams from one another inside one STP
> stream.
>
> This abstraction provides a configfs-based policy management mechanism
> for dynamic allocation of these master/channel pairs based on trace
> source-supplied string identifier. It has the flexibility of being
> defined at runtime and at the same time (provided that the policy
> definition is aligned with the decoding end) consistency.
>
> For userspace trace sources, this abstraction provides write()-based and
> mmap()-based (if the underlying stm device allows this) output mechanism.
>
> For kernel-side trace sources, we provide "stm_source" device class that
> can be connected to an stm device at run time.
>
> Cc: linux-...@vger.kernel.org
> Cc: Mathieu Poirier 
> Signed-off-by: Alexander Shishkin 
> ---
>  Documentation/ABI/testing/configfs-stp-policy|   48 +
>  Documentation/ABI/testing/sysfs-class-stm|   14 +
>  Documentation/ABI/testing/sysfs-class-stm_source |   11 +
>  Documentation/ioctl/ioctl-number.txt |3 +
>  Documentation/trace/stm.txt  |   80 ++
>  drivers/Kconfig  |2 +
>  drivers/Makefile |1 +
>  drivers/hwtracing/stm/Kconfig|8 +
>  drivers/hwtracing/stm/Makefile   |3 +
>  drivers/hwtracing/stm/core.c | 1029 
> ++
>  drivers/hwtracing/stm/policy.c   |  529 +++
>  drivers/hwtracing/stm/stm.h  |   87 ++
>  include/linux/stm.h  |  126 +++
>  include/uapi/linux/stm.h |   50 ++
>  14 files changed, 1991 insertions(+)
>  create mode 100644 Documentation/ABI/testing/configfs-stp-policy
>  create mode 100644 Documentation/ABI/testing/sysfs-class-stm
>  create mode 100644 Documentation/ABI/testing/sysfs-class-stm_source
>  create mode 100644 Documentation/trace/stm.txt
>  create mode 100644 drivers/hwtracing/stm/Kconfig
>  create mode 100644 drivers/hwtracing/stm/Makefile
>  create mode 100644 drivers/hwtracing/stm/core.c
>  create mode 100644 drivers/hwtracing/stm/policy.c
>  create mode 100644 drivers/hwtracing/stm/stm.h
>  create mode 100644 include/linux/stm.h
>  create mode 100644 include/uapi/linux/stm.h
>
> diff --git a/Documentation/ABI/testing/configfs-stp-policy 
> b/Documentation/ABI/testing/configfs-stp-policy
> new file mode 100644
> index 00..421ce6825c
> --- /dev/null
> +++ b/Documentation/ABI/testing/configfs-stp-policy
> @@ -0,0 +1,48 @@
> +What:  /config/stp-policy
> +Date:  June 2015
> +KernelVersion: 4.3
> +Description:
> +   This group contains policies mandating Master/Channel 
> allocation
> +   for software sources wishing to send trace data over an STM
> +   device.
> +
> +What:  /config/stp-policy/.
> +Date:  June 2015
> +KernelVersion: 4.3
> +Description:
> +   This group is the root of a policy; its name is a 
> concatenation
> +   of an stm device name to which this policy applies and an
> +   arbitrary string. If  part doesn't match an existing
> +   stm device, mkdir will fail with ENODEV; if that device 
> already
> +   has a policy assigned to it, mkdir will fail with EBUSY.
> +
> +What:  /config/stp-policy/./device
> +Date:  June 2015
> +KernelVersion: 4.3
> +Description:
> +   STM device to which this policy applies, read only. Same as 
> the
> +component of its parent directory.
> +
> +What:  /config/stp-policy/./
> +Date:  June 2015
> +KernelVersion: 4.3
> +Description:
> +   Policy node is a string identifier that software clients will
> +   use to request a master/channel to be allocated and assigned 
> to
> +   them.
> +
> +What:  /config/stp-policy/.//masters
> +Date:  June 2015
> +KernelVersion: 4.3
> +Description:
> +   Range of masters from which to allocate for users of this 
> node.
> +   Write two numbers: the first master and the last master 
> number.
> +
> +What:  /config/stp-policy/.//channels
> +Date:  June 2015
> +KernelVersion: 4.3
> +Description:
> +   Range 

[PATCH v3 01/11] stm class: Introduce an abstraction for System Trace Module devices

2015-07-06 Thread Alexander Shishkin
A System Trace Module (STM) is a device exporting data in System Trace
Protocol (STP) format as defined by MIPI STP standards. Examples of such
devices are Intel Trace Hub and Coresight STM.

This abstraction provides a unified interface for software trace sources
to send their data over an STM device to a debug host. In order to do
that, such a trace source needs to be assigned a pair of master/channel
identifiers that all the data from this source will be tagged with. The
STP decoder on the debug host side will use these master/channel tags to
distinguish different trace streams from one another inside one STP
stream.

This abstraction provides a configfs-based policy management mechanism
for dynamic allocation of these master/channel pairs based on trace
source-supplied string identifier. It has the flexibility of being
defined at runtime and at the same time (provided that the policy
definition is aligned with the decoding end) consistency.

For userspace trace sources, this abstraction provides write()-based and
mmap()-based (if the underlying stm device allows this) output mechanism.

For kernel-side trace sources, we provide "stm_source" device class that
can be connected to an stm device at run time.

Cc: linux-...@vger.kernel.org
Cc: Mathieu Poirier 
Signed-off-by: Alexander Shishkin 
---
 Documentation/ABI/testing/configfs-stp-policy|   48 +
 Documentation/ABI/testing/sysfs-class-stm|   14 +
 Documentation/ABI/testing/sysfs-class-stm_source |   11 +
 Documentation/ioctl/ioctl-number.txt |3 +
 Documentation/trace/stm.txt  |   80 ++
 drivers/Kconfig  |2 +
 drivers/Makefile |1 +
 drivers/hwtracing/stm/Kconfig|8 +
 drivers/hwtracing/stm/Makefile   |3 +
 drivers/hwtracing/stm/core.c | 1029 ++
 drivers/hwtracing/stm/policy.c   |  529 +++
 drivers/hwtracing/stm/stm.h  |   87 ++
 include/linux/stm.h  |  126 +++
 include/uapi/linux/stm.h |   50 ++
 14 files changed, 1991 insertions(+)
 create mode 100644 Documentation/ABI/testing/configfs-stp-policy
 create mode 100644 Documentation/ABI/testing/sysfs-class-stm
 create mode 100644 Documentation/ABI/testing/sysfs-class-stm_source
 create mode 100644 Documentation/trace/stm.txt
 create mode 100644 drivers/hwtracing/stm/Kconfig
 create mode 100644 drivers/hwtracing/stm/Makefile
 create mode 100644 drivers/hwtracing/stm/core.c
 create mode 100644 drivers/hwtracing/stm/policy.c
 create mode 100644 drivers/hwtracing/stm/stm.h
 create mode 100644 include/linux/stm.h
 create mode 100644 include/uapi/linux/stm.h

diff --git a/Documentation/ABI/testing/configfs-stp-policy 
b/Documentation/ABI/testing/configfs-stp-policy
new file mode 100644
index 00..421ce6825c
--- /dev/null
+++ b/Documentation/ABI/testing/configfs-stp-policy
@@ -0,0 +1,48 @@
+What:  /config/stp-policy
+Date:  June 2015
+KernelVersion: 4.3
+Description:
+   This group contains policies mandating Master/Channel allocation
+   for software sources wishing to send trace data over an STM
+   device.
+
+What:  /config/stp-policy/.
+Date:  June 2015
+KernelVersion: 4.3
+Description:
+   This group is the root of a policy; its name is a concatenation
+   of an stm device name to which this policy applies and an
+   arbitrary string. If  part doesn't match an existing
+   stm device, mkdir will fail with ENODEV; if that device already
+   has a policy assigned to it, mkdir will fail with EBUSY.
+
+What:  /config/stp-policy/./device
+Date:  June 2015
+KernelVersion: 4.3
+Description:
+   STM device to which this policy applies, read only. Same as the
+component of its parent directory.
+
+What:  /config/stp-policy/./
+Date:  June 2015
+KernelVersion: 4.3
+Description:
+   Policy node is a string identifier that software clients will
+   use to request a master/channel to be allocated and assigned to
+   them.
+
+What:  /config/stp-policy/.//masters
+Date:  June 2015
+KernelVersion: 4.3
+Description:
+   Range of masters from which to allocate for users of this node.
+   Write two numbers: the first master and the last master number.
+
+What:  /config/stp-policy/.//channels
+Date:  June 2015
+KernelVersion: 4.3
+Description:
+   Range of channels from which to allocate for users of this node.
+   Write two numbers: the first channel and the last channel
+   number.
+
diff --git a/Documentation/ABI/testing/sysfs-class-stm 
b/Documentation/ABI/testing/sysfs-class-stm
new file mode 100644
index 0