Hi Jay,
On Mon, Dec 20, 2021 at 10:32:21AM +0800, Jiankang Chen wrote:
[...]
> > > +module_init(tmc_init);
> > Should invoke tmc_init() in an ealier phase so it can prepare device class
> > prior to the TMC devices registration? In other words, I think here
> > should be:
> >
> > subsys_initcall(tmc_init);
> >
> Your suggestion is very good. I will revise it according to your suggestion;
As I clarified for Mathieu's concerning in another reply, please ignore
this comment.
Thanks,
Leo
On 12/16/21 10:22, Daniel Thompson wrote:
> On Wed, Dec 15, 2021 at 04:03:53PM +0000, carsten.haitzler(a)foss.arm.com wrote:
>> From: Carsten Haitzler <carsten.haitzler(a)arm.com>
>>
>> You edit your scripts in the tests and end up with your usual shell
>> backup files with ~ or .bak or something else at the end, but then your
>> next perf test run wants to run the backups too. You might also have perf
>> .data files in the directory or something else undesireable as well. You end
>> up chasing which test is the one you edited and the backup and have to keep
>> removing all the backup files, so automatically skip any files that are
>> not plain *.sh scripts to limit the time wasted in chasing ghosts.
>>
>> Signed-off-by: Carsten Haitzler <carsten.haitzler(a)arm.com>
>
> Why require both executable *and* endswith('.sh')?
Paranoia. :) Making sure we really only run things that are meant to be
run and avoid other junk/tmp/whatever files.
>> ---
>> tools/perf/tests/builtin-test.c | 15 ++++++++++++++-
>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
>> index ece272b55587..849737ead9fd 100644
>> --- a/tools/perf/tests/builtin-test.c
>> +++ b/tools/perf/tests/builtin-test.c
>> @@ -297,7 +297,20 @@ static const char *shell_test__description(char *description, size_t size,
>> for (int __i = 0; __i < nr && (ent = entlist[__i]); __i++) \
>> if (!is_directory(base, ent) && \
>> is_executable_file(base, ent) && \
>> - ent->d_name[0] != '.')
>> + ent->d_name[0] != '.' && \
>> + (shell_file_is_sh(ent->d_name) == 0))
>> +
>> +static int shell_file_is_sh(const char *file)
>> +{
>> + const char *ext;
>> +
>> + ext = strchr(file, '.');
>
> Shouldn't this be strrchr()?
Oh indeed probably should be. My bad. Nothing uses a dot inside the
filename yet. I can fix that - will wait for the rest to come in before
doing an update
On Wed, Dec 15, 2021 at 02:09:12PM -0500, Richard Guy Briggs wrote:
> On 2021-12-14 17:35, Paul Moore wrote:
> > On Wed, Dec 8, 2021 at 3:33 AM Leo Yan <leo.yan(a)linaro.org> wrote:
> > >
> > > Replace open code with task_is_in_init_pid_ns() for checking root PID
> > > namespace.
> > >
> > > Signed-off-by: Leo Yan <leo.yan(a)linaro.org>
> > > ---
> > > kernel/audit.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > I'm not sure how necessary this is, but it looks correct to me.
>
> I had the same thought. I looks correct to me. I could see the value
> if it permitted init_pid_ns to not be global.
Just for a background info, we need to check root PID namespace in some
drivers [1], to avoid introducing more open codes, we decided to refactor
with helper task_is_in_init_pid_ns().
[1] https://lore.kernel.org/lkml/20211213121323.1887180-1-leo.yan@linaro.org/
> > Acked-by: Paul Moore <paul(a)paul-moore.com>
>
> Reviewed-by: Richard Guy Briggs <rgb(a)redhat.com>
Thanks for review, Paul and Richard.
Leo
From: Carsten Haitzler <carsten.haitzler(a)arm.com>
Perf test's shell runner will just run everything in the tests
directory (as long as it's not another directory or does not begin
with a dot), but sometimes you find files in there that are not shell
scripts - perf.data output for example if you do some testing and then
the next time you run perf test it tries to run these. Check the files
are executable so they are actually intended to be test scripts and
not just some "random junk" files there.
Signed-off-by: Carsten Haitzler <carsten.haitzler(a)arm.com>
---
tools/perf/tests/builtin-test.c | 2 +-
tools/perf/util/path.c | 12 ++++++++++++
tools/perf/util/path.h | 1 +
3 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index c4b888f18e9c..1a7e21e5acf1 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -512,7 +512,7 @@ static const char *shell_test__description(char *description, size_t size,
#define for_each_shell_test(dir, base, ent) \
while ((ent = readdir(dir)) != NULL) \
- if (!is_directory(base, ent) && ent->d_name[0] != '.')
+ if (!is_directory(base, ent) && is_executable_file(base, ent) && ent->d_name[0] != '.')
static const char *shell_tests__dir(char *path, size_t size)
{
diff --git a/tools/perf/util/path.c b/tools/perf/util/path.c
index caed0336429f..7dde8c230ae8 100644
--- a/tools/perf/util/path.c
+++ b/tools/perf/util/path.c
@@ -92,3 +92,15 @@ bool is_directory(const char *base_path, const struct dirent *dent)
return S_ISDIR(st.st_mode);
}
+
+bool is_executable_file(const char *base_path, const struct dirent *dent)
+{
+ char path[PATH_MAX];
+ struct stat st;
+
+ sprintf(path, "%s/%s", base_path, dent->d_name);
+ if (stat(path, &st))
+ return false;
+
+ return !S_ISDIR(st.st_mode) && (st.st_mode & S_IXUSR);
+}
diff --git a/tools/perf/util/path.h b/tools/perf/util/path.h
index 083429b7efa3..d94902c22222 100644
--- a/tools/perf/util/path.h
+++ b/tools/perf/util/path.h
@@ -12,5 +12,6 @@ int path__join3(char *bf, size_t size, const char *path1, const char *path2, con
bool is_regular_file(const char *file);
bool is_directory(const char *base_path, const struct dirent *dent);
+bool is_executable_file(const char *base_path, const struct dirent *dent);
#endif /* _PERF_PATH_H */
--
2.32.0
Hi Mao,
On Thu, Dec 09, 2021 at 10:15:35PM +0800, Mao Jinlong 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(a)quicinc.com>
> ---
> drivers/hwtracing/coresight/coresight-core.c | 76 +++++++-------------
> 1 file changed, 26 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 8a18c71df37a..cc6b6cabf85f 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
> + * 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,13 @@ 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. */
> + path = idr_find(&path_idr, hash);
> + if (path == NULL)
> + return;
Please add a dev_err() here as this should really not be happening.
>
> + idr_remove(&path_idr, hash);
> coresight_disable_path(path);
> coresight_release_path(path);
>
> @@ -1779,6 +1754,7 @@ static int __init coresight_init(void)
>
> static void __exit coresight_exit(void)
> {
> + idr_destroy(&path_idr);
As far as I can tell this isn't needed.
> cscfg_exit();
> etm_perf_exit();
> bus_unregister(&coresight_bustype);
> --
> 2.17.1
>