On Tue, 4 Sep 2018 13:45:48 +0300 mike@perception-point.io wrote:
+++ b/drivers/hwtracing/coresight/coresight-etm-api.c @@ -0,0 +1,284 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- ETM kernel interface driver
- Author: Mike Bazov mike@perception-point.io
- This program is free software; you can redistribute it and/or modify it
- under the terms of the GNU General Public License version 2 as published by
- the Free Software Foundation.
- This program is distributed in the hope that 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.
- You should have received a copy of the GNU General Public License along with
- this program. If not, see http://www.gnu.org/licenses/.
- */
The SPDX license identifier above makes this full license text redundant.
+int coresight_etm_play(struct coresight_session *session,
struct etm_config *config)
I thought 'record' would be more appropriate terminology here, to match the perf 'record' command. Perf doesn't have a 'play' command, or, rather, if it did, it would (re-)'play' the previously recorded data like 'perf report' and 'perf script' do.
+{
- struct coresight_device *source = NULL;
- if (!session)
return -EINVAL;
If session isn't being used, can we avoid passing it in in the first place?
- source = per_cpu(csdev_src, smp_processor_id());
- if (!source)
return -ENXIO;
Sure would be nice to be more verbose here, with a dev_err, for example.
- return source_ops(source)->enable(source, config, CS_MODE_API);
+}
Hmm, if this function results in a source enable callback, should it not be called play, nor record, rather something-enable?
+EXPORT_SYMBOL_GPL(coresight_etm_play);
+void coresight_etm_pause(struct coresight_session *session) +{
- struct coresight_device *source = NULL;
- if (!session)
return;- source = per_cpu(csdev_src, smp_processor_id());
- if (!source)
return;- source_ops(source)->disable(source, NULL);
+} +EXPORT_SYMBOL_GPL(coresight_etm_pause);
+ssize_t coresight_etm_get_trace(struct coresight_session *session,
loff_t poss, size_t len, char **buf)+{
- struct coresight_device *sink = NULL;
- if (!session)
return -EINVAL;- sink = coresight_get_sink(session->path[0]);
- if (!sink)
return -ENXIO;- return sink_ops(sink)->get_trace(sink, CS_MODE_API, poss, len, buf);
+}
Same comments apply to the last two functions.
+++ b/include/linux/coresight.h @@ -156,6 +156,7 @@ struct coresight_connection {
- @activated: 'true' only if a _sink_ has been activated. A sink can be activated but not yet enabled. Enabling for a _sink_ happens when a source has been selected for that it.
*/
- @used: used only in a _sink_, 1 if the sink is used by an API session.
struct coresight_device { struct coresight_connection *conns; @@ -169,6 +170,7 @@ struct coresight_device { bool orphan; bool enable; /* true only if configured as part of a path */ bool activated; /* true only if a sink is part of a path */
- atomic_t used; /* 1 if used by a coresight api trace session */
};
I'm not too familiar with these flags, but it seems to be their data types should match, if something, e.g., 'activated', can't already be used to imply the strict "API"-specific definition 'used' seems to be making here?
+/**
- coresight_etm_destroy_session: destroy a coresight session.
- @session: the coresight session.
- */
+extern void coresight_etm_destroy_session(struct coresight_session *session);
+/**
- coresight_etm_enable: make the source start playing trace data. the source
^^^^^^^^^^^^^^^^^^^^
- is configured according to the config argument that is provided.
- @session: the coresight session.
- @config: the etm configuration
- Returns 0 on success, <0 on failure.
- */
+extern int coresight_etm_play(struct coresight_session *session,
^^^^^^^^^^^^^^^^^^
struct etm_config *config);
The comment function name doesn't match the name in the function declaration, and in fact reinforces the naming question I had above: these functions really enable and disable, so let's not reinvent their names to some foreign play and pause terminology.
+/**
- coresight_etm_disable: pause the source from playing trace data.
- @session: the coresight session.
- */
+extern void coresight_etm_pause(struct coresight_session *session);
same here: pause is (was?) disable.
This was just a couple of things that caught my eye, and I thought I'd bring up, it's by no means a complete review of the patchseries.
Kim