lkml.org 
[lkml]   [2021]   [Oct]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 01/10] coresight: add support to enable more coresight paths
    On Thu, Oct 21, 2021 at 03:38:47PM +0800, Tao Zhang wrote:
    > Current coresight implementation only supports enabling source
    > ETMs or STM. This patch adds support to enable more kinds of
    > coresight source to sink paths. We build a path from source to
    > sink when any source is enabled and store it in a list. When the
    > source is disabled, we fetch the corresponding path from the list
    > and decrement the refcount on each device in the path. The device
    > is disabled if the refcount reaches zero. Don't store path to
    > coresight data structure of source to avoid unnecessary change to
    > ABI.
    > Since some targets may have coresight sources other than STM and
    > ETMs, we need to add this change to support these coresight
    > devices.
    >
    > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
    > Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
    > ---
    > drivers/hwtracing/coresight/coresight-core.c | 100 +++++++++++--------
    > 1 file changed, 56 insertions(+), 44 deletions(-)
    >
    > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
    > index 8a18c71df37a..1e621d61307a 100644
    > --- a/drivers/hwtracing/coresight/coresight-core.c
    > +++ b/drivers/hwtracing/coresight/coresight-core.c
    > @@ -37,18 +37,16 @@ struct coresight_node {
    > };
    >
    > /*
    > - * When operating Coresight drivers from the sysFS interface, only a single
    > - * path can exist from a tracer (associated to a CPU) to a sink.
    > + * struct coresight_path - path from source to sink
    > + * @path: Address of path list.
    > + * @link: hook to the list.
    > */
    > -static DEFINE_PER_CPU(struct list_head *, tracer_path);
    > +struct coresight_path {
    > + struct list_head *path;
    > + struct list_head link;
    > +};

    For sources associated with a CPU, like ETMs, having a per-cpu way of storing
    paths is a definite advantage and should be kept that way.

    >
    > -/*
    > - * As of this writing only a single STM can be found in CS topologies. Since
    > - * there is no way to know if we'll ever see more and what kind of
    > - * configuration they will enact, for the time being only define a single path
    > - * for STM.
    > - */
    > -static struct list_head *stm_path;
    > +static LIST_HEAD(cs_active_paths);

    Then there are sources that aren't associated with a CPU like STMs and TPDMs.
    Perhaps using an IDR or the hash of the device name as a key to a hashing
    vector would be better than doing a sequential search, especially as the
    list of devices is bound to increase over time.

    >
    > /*
    > * When losing synchronisation a new barrier packet needs to be inserted at the
    > @@ -354,6 +352,7 @@ static void coresight_disable_sink(struct coresight_device *csdev)
    > if (ret)
    > return;
    > coresight_control_assoc_ectdev(csdev, false);
    > + csdev->activated = false;

    I don't see why this is needed and without proper documentation there is no way
    for me to guess the logic behind the change. The ->activated flag should be
    manipulated from the command line interface only.

    > csdev->enable = false;
    > }
    >
    > @@ -590,6 +589,20 @@ int coresight_enable_path(struct list_head *path, u32 mode, void *sink_data)
    > goto out;
    > }
    >
    > +static struct coresight_device *coresight_get_source(struct list_head *path)
    > +{
    > + struct coresight_device *csdev;
    > +
    > + if (!path)
    > + return NULL;
    > +
    > + csdev = list_first_entry(path, struct coresight_node, link)->csdev;
    > + if (csdev->type != CORESIGHT_DEV_TYPE_SOURCE)
    > + return NULL;
    > +
    > + return csdev;
    > +}
    > +
    > struct coresight_device *coresight_get_sink(struct list_head *path)
    > {
    > struct coresight_device *csdev;
    > @@ -1086,9 +1099,23 @@ static int coresight_validate_source(struct coresight_device *csdev,
    > return 0;
    > }
    >
    > +static int coresight_store_path(struct list_head *path)
    > +{
    > + struct coresight_path *node;
    > +
    > + node = kzalloc(sizeof(struct coresight_path), GFP_KERNEL);
    > + if (!node)
    > + return -ENOMEM;
    > +
    > + node->path = path;
    > + list_add(&node->link, &cs_active_paths);
    > +
    > + return 0;
    > +}
    > +
    > int coresight_enable(struct coresight_device *csdev)
    > {
    > - int cpu, ret = 0;
    > + int ret = 0;
    > struct coresight_device *sink;
    > struct list_head *path;
    > enum coresight_dev_subtype_source subtype;
    > @@ -1133,25 +1160,9 @@ int coresight_enable(struct coresight_device *csdev)
    > if (ret)
    > goto err_source;
    >
    > - switch (subtype) {
    > - case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC:
    > - /*
    > - * When working from sysFS it is important to keep track
    > - * of the paths that were created so that they can be
    > - * undone in 'coresight_disable()'. Since there can only
    > - * be a single session per tracer (when working from sysFS)
    > - * a per-cpu variable will do just fine.
    > - */
    > - cpu = source_ops(csdev)->cpu_id(csdev);
    > - per_cpu(tracer_path, cpu) = path;
    > - break;
    > - case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE:
    > - stm_path = path;
    > - break;
    > - default:
    > - /* We can't be here */
    > - break;
    > - }
    > + ret = coresight_store_path(path);
    > + if (ret)
    > + goto err_source;
    >
    > out:
    > mutex_unlock(&coresight_mutex);
    > @@ -1168,8 +1179,11 @@ EXPORT_SYMBOL_GPL(coresight_enable);
    >
    > void coresight_disable(struct coresight_device *csdev)
    > {
    > - int cpu, ret;
    > + int ret;
    > struct list_head *path = NULL;
    > + struct coresight_path *cspath = NULL;
    > + struct coresight_path *cspath_next = NULL;
    > + struct coresight_device *src_csdev = NULL;
    >
    > mutex_lock(&coresight_mutex);
    >
    > @@ -1180,20 +1194,18 @@ void coresight_disable(struct coresight_device *csdev)
    > if (!csdev->enable || !coresight_disable_source(csdev))
    > goto out;
    >
    > - switch (csdev->subtype.source_subtype) {
    > - case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC:
    > - cpu = source_ops(csdev)->cpu_id(csdev);
    > - path = per_cpu(tracer_path, cpu);
    > - per_cpu(tracer_path, cpu) = NULL;
    > - break;
    > - case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE:
    > - path = stm_path;
    > - stm_path = NULL;
    > - break;
    > - default:
    > - /* We can't be here */
    > - break;
    > + list_for_each_entry_safe(cspath, cspath_next, &cs_active_paths, link) {
    > + src_csdev = coresight_get_source(cspath->path);
    > + if (!src_csdev)
    > + continue;
    > + if (src_csdev == csdev) {
    > + path = cspath->path;
    > + list_del(&cspath->link);
    > + kfree(cspath);

    See my comment above - I agree that sources _not_ associated with a CPU should
    be handled differently. CPU bound sources should be kept untouched.

    That is all the time I had for today, I will continue tomorrow.

    Thanks,
    Mathieu

    > + }
    > }
    > + if (path == NULL)
    > + goto out;
    >
    > coresight_disable_path(path);
    > coresight_release_path(path);
    > --
    > 2.17.1
    >

    \
     
     \ /
      Last update: 2021-10-28 20:08    [W:3.433 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site