Hi Jie
On Wed, 5 Mar 2025 at 13:27, Jie Gan <quic_jiegan@xxxxxxxxxxx> wrote:
>
>
>
> On 3/5/2025 7:07 PM, Mike Leach wrote:
> > Hi,
> >
> > On Mon, 3 Mar 2025 at 03:30, Jie Gan <quic_jiegan@xxxxxxxxxxx> wrote:
> >>
> >> Add 'trace_id' function pointer in coresight_ops. It's responsible for retrieving
> >> the device's trace ID.
> >>
> >> Co-developed-by: James Clark <james.clark@xxxxxxxxxx>
> >> Signed-off-by: James Clark <james.clark@xxxxxxxxxx>
> >> Reviewed-by: James Clark <james.clark@xxxxxxxxxx>
> >> Signed-off-by: Jie Gan <quic_jiegan@xxxxxxxxxxx>
> >> ---
> >> drivers/hwtracing/coresight/coresight-core.c | 30 +++++++++++++++++++
> >> drivers/hwtracing/coresight/coresight-dummy.c | 13 +++++++-
> >> .../coresight/coresight-etm3x-core.c | 1 +
> >> .../coresight/coresight-etm4x-core.c | 1 +
> >> drivers/hwtracing/coresight/coresight-stm.c | 11 +++++++
> >> drivers/hwtracing/coresight/coresight-tpda.c | 11 +++++++
> >> include/linux/coresight.h | 5 ++++
> >> 7 files changed, 71 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> >> index ab55e10d4b79..32aa07f4f8c1 100644
> >> --- a/drivers/hwtracing/coresight/coresight-core.c
> >> +++ b/drivers/hwtracing/coresight/coresight-core.c
> >> @@ -24,6 +24,7 @@
> >> #include "coresight-etm-perf.h"
> >> #include "coresight-priv.h"
> >> #include "coresight-syscfg.h"
> >> +#include "coresight-trace-id.h"
> >>
> >> /*
> >> * Mutex used to lock all sysfs enable and disable actions and loading and
> >> @@ -1557,6 +1558,35 @@ void coresight_remove_driver(struct amba_driver *amba_drv,
> >> }
> >> EXPORT_SYMBOL_GPL(coresight_remove_driver);
> >>
> >> +int coresight_etm_get_trace_id(struct coresight_device *csdev, enum cs_mode mode,
> >> + struct coresight_device *sink)
> >> +{
> >> + int trace_id;
> >> + int cpu = source_ops(csdev)->cpu_id(csdev);
> >> +
> >
> > This is a global funciton so need to check that this csdev is a
> > source,. and does provide a cpu function before calling it.
> >
>
> Hi Mike,
>
> I put this function here because it's required by etm3x and etm4x. It's
> intended to be called only by ETM devices, which are definitely source
> devices and have a cpu function.
>
I fully understand the intent, but for a function that can be accessed
from anywhere, it is safer to validate input rather than assume any
caller will always respect the input conditions.
Lots of other places in the coresight drivers check that these
functions exist before calling them.
Regards
Mike
> Jie
>
> >> + switch (mode) {
> >> + case CS_MODE_SYSFS:
> >> + trace_id = coresight_trace_id_get_cpu_id(cpu);
> >> + break;
> >> + case CS_MODE_PERF:
> >> + if (WARN_ON(!sink))
> >> + return -EINVAL;
> >> +
> >> + trace_id = coresight_trace_id_get_cpu_id_map(cpu, &sink->perf_sink_id_map);
> >> + break;
> >> + default:
> >> + trace_id = -EINVAL;
> >> + break;
> >> + }
> >> +
> >> + if (!IS_VALID_CS_TRACE_ID(trace_id))
> >> + dev_err(&csdev->dev,
> >> + "Failed to allocate trace ID on CPU%d\n", cpu);
> >> +
> >> + return trace_id;
> >> +}
> >> +EXPORT_SYMBOL_GPL(coresight_etm_get_trace_id);
> >> +
> >> MODULE_LICENSE("GPL v2");
> >> MODULE_AUTHOR("Pratik Patel <pratikp@xxxxxxxxxxxxxx>");
> >> MODULE_AUTHOR("Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>");
> >> diff --git a/drivers/hwtracing/coresight/coresight-dummy.c b/drivers/hwtracing/coresight/coresight-dummy.c
> >> index 9be53be8964b..b5692ba358c1 100644
> >> --- a/drivers/hwtracing/coresight/coresight-dummy.c
> >> +++ b/drivers/hwtracing/coresight/coresight-dummy.c
> >> @@ -41,6 +41,16 @@ static void dummy_source_disable(struct coresight_device *csdev,
> >> dev_dbg(csdev->dev.parent, "Dummy source disabled\n");
> >> }
> >>
> >> +static int dummy_source_trace_id(struct coresight_device *csdev, __maybe_unused enum cs_mode mode,
> >> + __maybe_unused struct coresight_device *sink)
> >> +{
> >> + struct dummy_drvdata *drvdata;
> >> +
> >> + drvdata = dev_get_drvdata(csdev->dev.parent);
> >> +
> >> + return drvdata->traceid;
> >> +}
> >> +
> >> static int dummy_sink_enable(struct coresight_device *csdev, enum cs_mode mode,
> >> void *data)
> >> {
> >> @@ -62,7 +72,8 @@ static const struct coresight_ops_source dummy_source_ops = {
> >> };
> >>
> >> static const struct coresight_ops dummy_source_cs_ops = {
> >> - .source_ops = &dummy_source_ops,
> >> + .trace_id = dummy_source_trace_id,
> >> + .source_ops = &dummy_source_ops,
> >> };
> >>
> >> static const struct coresight_ops_sink dummy_sink_ops = {
> >> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> >> index c103f4c70f5d..c1dda4bc4a2f 100644
> >> --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
> >> +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> >> @@ -704,6 +704,7 @@ static const struct coresight_ops_source etm_source_ops = {
> >> };
> >>
> >> static const struct coresight_ops etm_cs_ops = {
> >> + .trace_id = coresight_etm_get_trace_id,
> >> .source_ops = &etm_source_ops,
> >> };
> >>
> >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> >> index 2c1a60577728..cfd116b87460 100644
> >> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> >> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> >> @@ -1067,6 +1067,7 @@ static const struct coresight_ops_source etm4_source_ops = {
> >> };
> >>
> >> static const struct coresight_ops etm4_cs_ops = {
> >> + .trace_id = coresight_etm_get_trace_id,
> >> .source_ops = &etm4_source_ops,
> >> };
> >>
> >> diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
> >> index b581a30a1cd9..aca25b5e3be2 100644
> >> --- a/drivers/hwtracing/coresight/coresight-stm.c
> >> +++ b/drivers/hwtracing/coresight/coresight-stm.c
> >> @@ -281,12 +281,23 @@ static void stm_disable(struct coresight_device *csdev,
> >> }
> >> }
> >>
> >> +static int stm_trace_id(struct coresight_device *csdev, __maybe_unused enum cs_mode mode,
> >> + __maybe_unused struct coresight_device *sink)
> >> +{
> >> + struct stm_drvdata *drvdata;
> >> +
> >> + drvdata = dev_get_drvdata(csdev->dev.parent);
> >> +
> >> + return drvdata->traceid;
> >> +}
> >> +
> >> static const struct coresight_ops_source stm_source_ops = {
> >> .enable = stm_enable,
> >> .disable = stm_disable,
> >> };
> >>
> >> static const struct coresight_ops stm_cs_ops = {
> >> + .trace_id = stm_trace_id,
> >> .source_ops = &stm_source_ops,
> >> };
> >>
> >> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
> >> index 573da8427428..94c2201fc8d3 100644
> >> --- a/drivers/hwtracing/coresight/coresight-tpda.c
> >> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
> >> @@ -241,12 +241,23 @@ static void tpda_disable(struct coresight_device *csdev,
> >> dev_dbg(drvdata->dev, "TPDA inport %d disabled\n", in->dest_port);
> >> }
> >>
> >> +static int tpda_trace_id(struct coresight_device *csdev, __maybe_unused enum cs_mode mode,
> >> + __maybe_unused struct coresight_device *sink)
> >> +{
> >> + struct tpda_drvdata *drvdata;
> >> +
> >> + drvdata = dev_get_drvdata(csdev->dev.parent);
> >> +
> >> + return drvdata->atid;
> >> +}
> >> +
> >> static const struct coresight_ops_link tpda_link_ops = {
> >> .enable = tpda_enable,
> >> .disable = tpda_disable,
> >> };
> >>
> >> static const struct coresight_ops tpda_cs_ops = {
> >> + .trace_id = tpda_trace_id,
> >> .link_ops = &tpda_link_ops,
> >> };
> >>
> >> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> >> index c7cd5886c908..ce9a5e71b261 100644
> >> --- a/include/linux/coresight.h
> >> +++ b/include/linux/coresight.h
> >> @@ -335,6 +335,7 @@ enum cs_mode {
> >> CS_MODE_PERF,
> >> };
> >>
> >> +#define coresight_ops(csdev) csdev->ops
> >> #define source_ops(csdev) csdev->ops->source_ops
> >> #define sink_ops(csdev) csdev->ops->sink_ops
> >> #define link_ops(csdev) csdev->ops->link_ops
> >> @@ -421,6 +422,8 @@ struct coresight_ops_panic {
> >> };
> >>
> >> struct coresight_ops {
> >> + int (*trace_id)(struct coresight_device *csdev, enum cs_mode mode,
> >> + struct coresight_device *sink);
> >> const struct coresight_ops_sink *sink_ops;
> >> const struct coresight_ops_link *link_ops;
> >> const struct coresight_ops_source *source_ops;
> >> @@ -709,4 +712,6 @@ int coresight_init_driver(const char *drv, struct amba_driver *amba_drv,
> >>
> >> void coresight_remove_driver(struct amba_driver *amba_drv,
> >> struct platform_driver *pdev_drv);
> >> +int coresight_etm_get_trace_id(struct coresight_device *csdev, enum cs_mode mode,
> >> + struct coresight_device *sink);
> >> #endif /* _LINUX_COREISGHT_H */
> >> --
> >> 2.34.1
> >>
> >
> >
>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
