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

[Alexander Shishkin]

Mathieu Poirier <mathieu.poirier@xxxxxxxxxx> writes:

> On 7 March 2015 at 04:35, Alexander Shishkin
> <alexander.shishkin@xxxxxxxxxxxxxxx> 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-api@xxxxxxxxxxxxxxx
>> Cc: Pratik Patel <pratikp@xxxxxxxxxxxxxx>
>> Cc: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
>> Signed-off-by: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
>> ---
>>  Documentation/ABI/testing/configfs-stp-policy    |  44 ++
>>  Documentation/ABI/testing/sysfs-class-stm        |  14 +
>>  Documentation/ABI/testing/sysfs-class-stm_source |  11 +
>>  Documentation/trace/stm.txt                      |  77 +++
>>  drivers/Kconfig                                  |   2 +
>>  drivers/Makefile                                 |   1 +
>>  drivers/stm/Kconfig                              |   8 +
>>  drivers/stm/Makefile                             |   3 +
>>  drivers/stm/core.c                               | 839 +++++++++++++++++++++++
>>  drivers/stm/policy.c                             | 470 +++++++++++++
>>  drivers/stm/stm.h                                |  77 +++
>>  include/linux/stm.h                              |  87 +++
>>  include/uapi/linux/stm.h                         |  47 ++
>>  13 files changed, 1680 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/stm/Kconfig
>>  create mode 100644 drivers/stm/Makefile
>>  create mode 100644 drivers/stm/core.c
>>  create mode 100644 drivers/stm/policy.c
>>  create mode 100644 drivers/stm/stm.h
>>  create mode 100644 include/linux/stm.h
>>  create mode 100644 include/uapi/linux/stm.h
>>
>
> [Snip...]
>
>> +
>> +static int stm_output_assign(struct stm_device *stm, unsigned int width,
>> +                            struct stp_policy_node *policy_node,
>> +                            struct stm_output *output)
>> +{
>> +       unsigned int midx, cidx, mend, cend;
>> +       int ret = -EBUSY;
>> +
>> +       if (width > stm->data->sw_nchannels)
>> +               return -EINVAL;
>> +
>> +       if (policy_node) {
>
> Where does this get set?  All I found is code that is switching on it.

It comes from stp_policy_node_lookup() in stm_file_assign() or
stm_source_link_add().

>> +               stp_policy_node_get_ranges(policy_node,
>> +                                          &midx, &mend, &cidx, &cend);
>> +       } else {
>> +               midx = stm->data->sw_start;
>> +               cidx = 0;
>> +               mend = stm->data->sw_end;
>> +               cend = stm->data->sw_nchannels - 1;
>> +       }
>> +
>> +       spin_lock(&stm->mc_lock);
>> +       if (output->nr_chans)
>> +               goto unlock;
>> +
>> +       ret = stm_find_master_chan(stm, width, &midx, mend, &cidx, cend);
>> +       if (ret)
>> +               goto unlock;
>> +
>> +       output->master = midx;
>> +       output->channel = cidx;
>> +       output->nr_chans = width;
>> +       stm_output_claim(stm, output);
>> +       dev_dbg(stm->dev, "assigned %u:%u (+%u)\n", midx, cidx, width);
>> +
>> +       ret = 0;
>> +unlock:
>> +       spin_unlock(&stm->mc_lock);
>> +
>> +       return ret;
>> +}
>> +
>
> [Snip...]
>
>> +
>> +/**
>> + * 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;
>> +       struct device *dev;
>> +
>> +       if (!stm_core_up)
>> +               return -EPROBE_DEFER;
>> +
>> +       src = kzalloc(sizeof(*src), GFP_KERNEL);
>> +       if (!src)
>> +               return -ENOMEM;
>> +
>> +       dev = device_create(&stm_source_class, parent, MKDEV(0, 0), NULL, "%s",
>> +                           data->name);
>> +       if (IS_ERR(dev)) {
>> +               kfree(src);
>> +               return PTR_ERR(dev);
>> +       }
>> +
>> +       spin_lock_init(&src->link_lock);
>> +       INIT_LIST_HEAD(&src->link_entry);
>> +       src->dev = dev;
>> +       src->data = data;
>> +       data->src = src;
>> +       dev_set_drvdata(dev, src);
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(stm_source_register_device);
>> +
>
> [Snip...]
>
> It's really not clear (at least to me) how stm_source_device works.
> They make a bridge between the kernel and the STM devices but the
> "link" between them is not obvious.  More documentation perhaps?

Sure, I need to elaborate on this a bit. The idea the stm_source device
gets "linked" to an stm device by way of writing that stm device name to
its stm_source_link attribute and if everything's fine, stm_sounrce's
.link callback is called, after which it can start sending data out with
stm_sounce_write().

>> +/**
>> + * struct stm_data - STM device description and callbacks
>> + * @name:              device name
>> + * @stm:               internal structure, only used by stm class code
>> + * @sw_start:          first STP master
>> + * @sw_end:            last STP master
>> + * @sw_nchannels:      number of STP channels per master
>> + * @sw_mmiosz:         size of one channel's IO space, for mmap, optional
>> + * @write:             write callback
>> + * @mmio_addr:         mmap callback, optional
>> + *
>> + * Fill out this structure before calling stm_register_device() to create
>> + * an STM device and stm_unregister_device() to destroy it. It will also be
>> + * passed back to write() and mmio_addr() callbacks.
>> + */
>> +struct stm_data {
>> +       const char              *name;
>> +       struct stm_device       *stm;
>> +       unsigned int            sw_start;
>> +       unsigned int            sw_end;
>
> The above kernel doc is the only place where "sw_start" and "sw_end"
> are described as first and last STP masters.  Other than that it takes
> a really long time to figure out what they really are.  I think a
> better naming convention can be chosen here.

The idea is that only a certain subset of masters is available for
software (others being statically assigned to different hw blocks or
otherwise unavailable to software). These two mark start and end of this
subset.

>
>> +       unsigned int            sw_nchannels;
>> +       unsigned int            sw_mmiosz;
>> +       ssize_t                 (*write)(struct stm_data *, unsigned int,
>> +                                        unsigned int, const char *, size_t);
>> +       phys_addr_t             (*mmio_addr)(struct stm_data *, unsigned int,
>> +                                            unsigned int, unsigned int);
>> +       void                    (*link)(struct stm_data *, unsigned int,
>> +                                       unsigned int);
>> +       void                    (*unlink)(struct stm_data *, unsigned int,
>> +                                         unsigned int);
>
> It is really not clear to me what the "link" and "unlink" functions do
> - documenting what they're for and explain when to use them and (not
> use them) would be appreciated.

Will do.

> I think we should also add two things to this structure:  1) a private
> field and 2) a (*stm_drv_ioctl) stub.
> The private field would be filled by the registrant and left alone by
> the generic-stm core.  When parsing the commands in
> "stm_char_ioctl()", data->stm_drv_ioctl(private, cmd, arg) could be
> called to let architecture specific drivers deal with it.  That way
> applications can deal with a single configuration file descriptor.

Indeed, good idea.

>
>> +};
>> +
>> +int stm_register_device(struct device *parent, struct stm_data *stm_data,
>> +                       struct module *owner);
>> +void stm_unregister_device(struct stm_data *stm_data);
>> +
>> +struct stm_source_device;
>> +
>> +/**
>> + * struct stm_source_data - STM source device description and callbacks
>> + * @name:      device name, will be used for policy lookup
>> + * @src:       internal structure, only used by stm class code
>> + * @nr_chans:  number of channels to allocate
>> + * @link:      called when STM device gets linked to this source
>> + * @unlink:    called when STH device is about to be unlinked
>> + *
>> + * Fill in this structure before calling stm_source_register_device() to
>> + * register a source device. Also pass it to unregister and write calls.
>> + */
>> +struct stm_source_data {
>> +       const char              *name;
>> +       struct stm_source_device *src;
>> +       unsigned int            percpu;
>> +       unsigned int            nr_chans;
>> +       int                     (*link)(struct stm_source_data *data);
>> +       void                    (*unlink)(struct stm_source_data *data);
>> +};
>> +
>
> I didn't get the linking/unlinking process - it is also not clear as
> to why we need struct stm_data and struct stm_source_data.  More
> explanation on this would be good.

My idea is that connecting stm to stm_source device is done via sysfs
attribute write like so:

$ echo dummy_stm > /sys/class/stm_source/stm_console/stm_source_link

This will result in stm_source's .link getting called, at which point
stm_console, for example, will do a register_console().

stm_data is there for stm devices to pass their parameters and callbacks
to stm_register_device(); stm_source data is that for stm_source
devices. I'll try to be more elaborate about these in my followup.

>> +int stm_source_register_device(struct device *parent,
>> +                              struct stm_source_data *data);
>> +void stm_source_unregister_device(struct stm_source_data *data);
>> +
>> +int stm_source_write(struct stm_source_data *data, unsigned int chan,
>> +                    const char *buf, size_t count);
>> +
>> +#endif /* _STM_H_ */
>> diff --git a/include/uapi/linux/stm.h b/include/uapi/linux/stm.h
>> new file mode 100644
>> index 0000000000..042b58b53b
>> --- /dev/null
>> +++ b/include/uapi/linux/stm.h
>> @@ -0,0 +1,47 @@
>> +/*
>> + * System Trace Module (STM) userspace interfaces
>> + * Copyright (c) 2014, Intel Corporation.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + *
>> + * STM class implements generic infrastructure for  System Trace Module devices
>> + * as defined in MIPI STPv2 specification.
>> + */
>> +
>> +#ifndef _UAPI_LINUX_STM_H
>> +#define _UAPI_LINUX_STM_H
>> +
>> +/**
>> + * struct stp_policy_id - identification for the STP policy
>> + * @size:      size of the structure including real id[] length
>> + * @master:    assigned master
>> + * @channel:   first assigned channel
>> + * @width:     number of requested channels
>> + * @id:                identification string
>> + *
>> + * User must calculate the total size of the structure and put it into
>> + * @size field, fill out the @id and desired @width. In return, kernel
>> + * fills out @master, @channel and @width.
>> + */
>> +struct stp_policy_id {
>> +       __u32           size;
>> +       __u16           master;
>> +       __u16           channel;
>> +       __u16           width;
>> +       /* padding */
>> +       __u16           __reserved_0;
>> +       __u32           __reserved_1;
>> +       char            id[0];
>> +};
>> +
>> +#define STP_POLICY_ID_SET      _IOWR('%', 0, struct stp_policy_id)
>> +#define STP_POLICY_ID_GET      _IOR('%', 1, struct stp_policy_id)
>> +
>> +#endif /* _UAPI_LINUX_STM_H */
>> --
>> 2.1.4
>>
>
> Aside from the above points I think this is all very good work.  The
> patchset should likely be broken up in two sets, one for the generic
> STM architecture wrapper and another for the Intel TH part.  That way
> things can be worked on independently.

The Intel TH part is very much dependent on the STM part; I could split
out the stm-dependant patch from Intel TH, but that just seems to make
it more complex for me and reviewers. I'd prefer to keep them together
unless there are strong objections.

> On my side I will get a prototype going that folds the current
> coresight-stm driver in this new mindset - I'll let you know how
> things go.

Thanks a lot!

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