Re: [PATCH v3 01/10] Use IDR to maintain all the enabled sources' paths.

[Mike Leach <mike.leach@linaro.org>]

Hi,

On Wed, 9 Feb 2022 at 10:57, Mao Jinlong <quic_jinlmao@quicinc.com> wrote:
>
> Use hash length of the source's device name to map to the pointer
> of the enabled path. Using IDR will be more efficient than using
> the list. And there could be other sources except STM and CPU etms
> in the new HWs. It is better to maintain all the paths together.
>
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>  drivers/hwtracing/coresight/coresight-core.c | 76 +++++++-------------
>  1 file changed, 27 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 88653d1c06a4..a90097f88425 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -7,6 +7,7 @@
>  #include <linux/init.h>
>  #include <linux/types.h>
>  #include <linux/device.h>
> +#include <linux/idr.h>
>  #include <linux/io.h>
>  #include <linux/err.h>
>  #include <linux/export.h>
> @@ -26,6 +27,12 @@
>  static DEFINE_MUTEX(coresight_mutex);
>  static DEFINE_PER_CPU(struct coresight_device *, csdev_sink);
>
> +/*
> + * Use IDR to map the hash length of the source's device name
> + * to the pointer of path for the source
> + */
> +static DEFINE_IDR(path_idr);
> +
>  /**
>   * struct coresight_node - elements of a path, from source to sink
>   * @csdev:     Address of an element.
> @@ -36,20 +43,6 @@ struct coresight_node {
>         struct list_head link;
>  };
>
> -/*
> - * When operating Coresight drivers from the sysFS interface, only a single
> - * path can exist from a tracer (associated to a CPU) to a sink.
> - */
> -static DEFINE_PER_CPU(struct list_head *, tracer_path);
> -
> -/*
> - * 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;
> -
>  /*
>   * When losing synchronisation a new barrier packet needs to be inserted at the
>   * beginning of the data collected in a buffer.  That way the decoder knows that
> @@ -1088,10 +1081,11 @@ static int coresight_validate_source(struct coresight_device *csdev,
>
>  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;
> +       u32 hash;
>
>         subtype = csdev->subtype.source_subtype;
>
> @@ -1133,26 +1127,14 @@ 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;
> -       }
> -
> +       /*
> +        * Use the hash length of source's device name as ID
Slightly confusing comment:
hashlen_string creates a hash and a length for the string. We are
using the hash here so comment should be:

"Use the hash of source's device name as ID"

> +        * and map the ID to the pointer of the path.
> +        */
> +       hash = hashlen_hash(hashlen_string(NULL, dev_name(&csdev->dev)));
> +       ret = idr_alloc_u32(&path_idr, path, &hash, hash, GFP_KERNEL);
> +       if (ret)
> +               goto err_source;
>  out:
>         mutex_unlock(&coresight_mutex);
>         return ret;
> @@ -1168,8 +1150,9 @@ EXPORT_SYMBOL_GPL(coresight_enable);
>
>  void coresight_disable(struct coresight_device *csdev)
>  {
> -       int cpu, ret;
> +       int ret;
>         struct list_head *path = NULL;
> +       u32 hash;
>
>         mutex_lock(&coresight_mutex);
>
> @@ -1180,21 +1163,16 @@ 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;
> +       hash = hashlen_hash(hashlen_string(NULL, dev_name(&csdev->dev)));
> +       /* Find the path by the hash length. */

same comment as above re hash  / hash length

> +       path = idr_find(&path_idr, hash);
> +       if (path == NULL) {
> +               dev_info(&csdev->dev, "Path is not found for %s\n",
> +                               dev_name(&csdev->dev));

pr_err() is used in other parts of this file to print error messages.


> +               return;

this skips the mutex_unlock(). use goto out;


>         }
>
> +       idr_remove(&path_idr, hash);
>         coresight_disable_path(path);
>         coresight_release_path(path);
>
> --
> 2.17.1
>
Regards

Mike

-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK