On 06/03/2019 22:57, Mathieu Poirier wrote:

Set the proper bit in the configuration register when contextID tracing has been requested by user space. That way PE_CONTEXT elements are generated by the tracers when a process is installed on a CPU.

Signed-off-by: Mathieu Poirier mathieu.poirier@linaro.org

drivers/hwtracing/coresight/coresight-etm-perf.c | 2 ++ drivers/hwtracing/coresight/coresight-etm4x.c | 5 +++++ include/linux/coresight-pmu.h | 2 ++ tools/include/linux/coresight-pmu.h | 2 ++ 4 files changed, 11 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 25ae56e924bb..bbfed70b3402 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -29,6 +29,7 @@ static DEFINE_PER_CPU(struct coresight_device *, csdev_src); /* ETMv3.5/PTM's ETMCR is 'config' */ PMU_FORMAT_ATTR(cycacc, "config:" __stringify(ETM_OPT_CYCACC)); +PMU_FORMAT_ATTR(contextid, "config:" __stringify(ETM_OPT_CTXTID)); PMU_FORMAT_ATTR(timestamp, "config:" __stringify(ETM_OPT_TS)); PMU_FORMAT_ATTR(retstack, "config:" __stringify(ETM_OPT_RETSTK)); /* Sink ID - same for all ETMs */ @@ -36,6 +37,7 @@ PMU_FORMAT_ATTR(sinkid, "config2:0-31"); static struct attribute *etm_config_formats_attr[] = { &format_attr_cycacc.attr,

• &format_attr_contextid.attr, &format_attr_timestamp.attr, &format_attr_retstack.attr, &format_attr_sinkid.attr,

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c index 08ce37c9475d..f067f7ee08c7 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.c +++ b/drivers/hwtracing/coresight/coresight-etm4x.c @@ -239,6 +239,11 @@ static int etm4_parse_event_config(struct etmv4_drvdata *drvdata, if (attr->config & BIT(ETM_OPT_TS)) /* bit[11], Global timestamp tracing bit */ config->cfg |= BIT(11);

• if (attr->config & BIT(ETM_OPT_CTXTID))

• /* bit[6], Context ID tracing bit */
  

• config->cfg |= BIT(6);
  

nit: Could we use the ETM4_CFG_BIT_CTXTID here rather than hard coding the value we already define ?

Otherwise:

Reviewed-by: Suzuki K Poulose suzuki.poulose@arm.com

Btw, someday, we could make this an array of attr->config bits defs and their corresponding drvdata->config bit defs and be done with it, rather than adding separate checks like.

• /* return stack - enable if selected and supported */ if ((attr->config & BIT(ETM_OPT_RETSTK)) && drvdata->retstack) /* bit[12], Return stack enable bit */

diff --git a/include/linux/coresight-pmu.h b/include/linux/coresight-pmu.h index a1a959ba24ff..b0e35eec6499 100644 --- a/include/linux/coresight-pmu.h +++ b/include/linux/coresight-pmu.h @@ -12,11 +12,13 @@ /* ETMv3.5/PTM's ETMCR config bit */ #define ETM_OPT_CYCACC 12 +#define ETM_OPT_CTXTID 14 #define ETM_OPT_TS 28 #define ETM_OPT_RETSTK 29 /* ETMv4 CONFIGR programming bits for the ETM OPTs */ #define ETM4_CFG_BIT_CYCACC 4 +#define ETM4_CFG_BIT_CTXTID 6 #define ETM4_CFG_BIT_TS 11 #define ETM4_CFG_BIT_RETSTK 12 diff --git a/tools/include/linux/coresight-pmu.h b/tools/include/linux/coresight-pmu.h index a1a959ba24ff..b0e35eec6499 100644 --- a/tools/include/linux/coresight-pmu.h +++ b/tools/include/linux/coresight-pmu.h @@ -12,11 +12,13 @@ /* ETMv3.5/PTM's ETMCR config bit */ #define ETM_OPT_CYCACC 12 +#define ETM_OPT_CTXTID 14 #define ETM_OPT_TS 28 #define ETM_OPT_RETSTK 29 /* ETMv4 CONFIGR programming bits for the ETM OPTs */ #define ETM4_CFG_BIT_CYCACC 4 +#define ETM4_CFG_BIT_CTXTID 6 #define ETM4_CFG_BIT_TS 11 #define ETM4_CFG_BIT_RETSTK 12

nit: Should this be really exposed to the perf tool ? The tool must always rely on the "format" strings, isn't it ? That way, we are free to change the kernel definitions and get the tool working everywhere ?

Cheers Suzuki

On 07/03/2019 17:53, Mathieu Poirier wrote:

...

...

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c index 08ce37c9475d..f067f7ee08c7 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.c +++ b/drivers/hwtracing/coresight/coresight-etm4x.c @@ -239,6 +239,11 @@ static int etm4_parse_event_config(struct etmv4_drvdata *drvdata, if (attr->config & BIT(ETM_OPT_TS)) /* bit[11], Global timestamp tracing bit */ config->cfg |= BIT(11);

• if (attr->config & BIT(ETM_OPT_CTXTID))
  

•         /* bit[6], Context ID tracing bit */
  

•         config->cfg |= BIT(6);
  

nit: Could we use the ETM4_CFG_BIT_CTXTID here rather than hard coding the value we already define ?

We certainly can.

...

Otherwise:

Reviewed-by: Suzuki K Poulose suzuki.poulose@arm.com

Btw, someday, we could make this an array of attr->config bits defs and their corresponding drvdata->config bit defs and be done with it, rather than adding separate checks like.

A conversion must be made because configuration bits differ between ETMv3.x and ETMv4. Since ETMv3.x came first I kept the bits specified for that architecture but it is only a matter of time before that strategy doesn't work anymore. I envision a time in the not so distant future where new options will come out with configuration bits completely orthogonal to what we currently have. When that happens user space will have to set attr->config for each ETM flavour, essentially removing the need for bit conversion. At that point attr->config and drvdata->config will be the same for each ETM architecture.

...

...

diff --git a/tools/include/linux/coresight-pmu.h b/tools/include/linux/coresight-pmu.h index a1a959ba24ff..b0e35eec6499 100644 --- a/tools/include/linux/coresight-pmu.h +++ b/tools/include/linux/coresight-pmu.h @@ -12,11 +12,13 @@

/* ETMv3.5/PTM's ETMCR config bit */ #define ETM_OPT_CYCACC 12 +#define ETM_OPT_CTXTID 14 #define ETM_OPT_TS 28 #define ETM_OPT_RETSTK 29

/* ETMv4 CONFIGR programming bits for the ETM OPTs */ #define ETM4_CFG_BIT_CYCACC 4 +#define ETM4_CFG_BIT_CTXTID 6 #define ETM4_CFG_BIT_TS 11 #define ETM4_CFG_BIT_RETSTK 12

nit: Should this be really exposed to the perf tool ? The tool must always rely on the "format" strings, isn't it ? That way, we are free to change the kernel definitions and get the tool working everywhere ?

We don't have a choice to copy coresight-pmu.h over to the tools' side. Otherwise there is a script that compares the perf headers with the kernel headers that complains bitterly, something that makes Acme very unhappy.

Ok. But, my question is, why do the perf tool need to know this from the header file. AFAICS, all of this must be exposed via sysfs format. We could certainly remove the CFG bits.

Cheers Suzuki

On Fri, 8 Mar 2019 at 06:54, Mike Leach mike.leach@linaro.org wrote:

On Wed, 6 Mar 2019 at 22:58, Mathieu Poirier mathieu.poirier@linaro.org wrote:

...

Configure timestamps to be emitted at regular intervals in the trace stream to temporally correlate instructions executed on different CPUs.

Signed-off-by: Mathieu Poirier mathieu.poirier@linaro.org

drivers/hwtracing/coresight/coresight-etm4x.c | 109 +++++++++++++++++- 1 file changed, 106 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c index f067f7ee08c7..2c29d6743fee 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.c +++ b/drivers/hwtracing/coresight/coresight-etm4x.c @@ -138,8 +138,11 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) drvdata->base + TRCCNTVRn(i)); }

•   /* Resource selector pair 0 is always implemented and reserved */
  

•   for (i = 0; i < drvdata->nr_resource * 2; i++)
  

•   /*
  

•    * Resource selector pair 0 is always implemented and reserved.  As
  

•    * such start at 2.
  

•    */
  

•   for (i = 2; i < drvdata->nr_resource * 2; i++)
            writel_relaxed(config->res_ctrl[i],
                           drvdata->base + TRCRSCTLRn(i));
  
  

@@ -201,6 +204,91 @@ static void etm4_enable_hw_smp_call(void *info) arg->rc = etm4_enable_hw(arg->drvdata); }

+/*

• • The goal of function etm4_config_timestamp_event() is to configure a
• • counter that will tell the tracer to emit a timestamp packet when it
• • reaches zero. This is done in order to get a more fine grained idea
• • of when instructions are executed so that they can be correlated
• • with execution on other CPUs.
• • To do this the counter itself is configured to self reload and
• • TRCRSCTLR1 (always true) used to get the counter to decrement. From
• • there a resource selector is configured with the counter and the
• • timestamp control register to use the resource selector to trigger the
• • event that will insert a timestamp packet in the stream.
• */

+static int etm4_config_timestamp_event(struct etmv4_drvdata *drvdata) +{

•   int i, ret = -EINVAL;
  

•   int counter, rselector;
  

•   u32 val = 0;
  

•   struct etmv4_config *config = &drvdata->config;
  

•   /* No point in trying if we don't have at least one counter */
  

•   if (!drvdata->nr_cntr)
  

•           goto out;
  

•   /* Find a counter that hasn't been initialised */
  

•   for (i = 0; i < drvdata->nr_cntr; i++)
  

•           if (config->cntr_val[i] == 0)
  

•                   break;
  

•   /* Remember what counter we used */
  

•   counter = 1 << i;
  

•   /* All the counters have been configured already, bail out */
  

•   if (i == drvdata->nr_cntr) {
  

•           pr_err("%s: no available counter found\n", __func__);
  

•           goto out;
  

•   }
  

•   /*
  

•    * Initialise original and reload counter value to the smallest
  

•    * possible value in order to get as much precision as we can.
  

•    */
  

•   config->cntr_val[i] = 1;
  

•   config->cntrldvr[i] = 1;
  

Careful here - need to ensure that such a small gap between TS does not flood the output with timestamps and reduce the amount of usable trace. ETMv4 will delay outputting TS if doing so will cause an overflow - so may reduce the effective TS frequency anyway. Once we are happy the mechanism is right, then may need to play with these figures to get the right balance between precision and quality.

I toyed with these values for a while... From the testing I did on my side the above values yield one timestamp every 2 or 3 range packets when the system is not busy. When CPUs are 100% occupied there can be several hundreds of range packet generated between timestamps, hence going for the absolute minimum. That being said I'm definitely open to adopting a different configuration if we find something better.

Thanks, Mathieu

Mike

...

•   /* Set the trace counter control register */
  

•   val =  0x1 << 16        |  /* Bit 16, reload counter automatically */
  

•          0x0 << 7         |  /* Select single resource selector */
  

•          0x1;                /* Resource selector 1, i.e always true */
  

•   config->cntr_ctrl[i] = val;
  

•   /*
  

•    * Searching for an available resource selector to use, starting at
  

•    * '2' since every implementation has at least 2 resource selector.
  

•    * ETMIDR4 gives the number of resource selector _pairs_,
  

•    * hence multiply by 2.
  

•    */
  

•   for (i = 2; i < drvdata->nr_resource * 2; i++)
  

•           if (!config->res_ctrl[i])
  

•                   break;
  

•   /* Remember what resource selector we used */
  

•   rselector = i;
  

•   if (i == drvdata->nr_resource * 2) {
  

•           pr_err("%s: no available resource selector found\n", __func__);
  

•           goto out;
  

•   }
  

•   val = 0x2 << 16         | /* Group 0b0010 - Counter and sequencers */
  

•         counter << 0;       /* Counter to use */
  

•   config->res_ctrl[i] = val;
  

•   val = 0x0 << 7          | /* Select single resource selector */
  

•         rselector;          /* Resource selector */
  

•   config->ts_ctrl = val;
  

•   ret = 0;
  

+out:

•   return ret;
  

+}

static int etm4_parse_event_config(struct etmv4_drvdata *drvdata, struct perf_event *event) { @@ -236,9 +324,24 @@ static int etm4_parse_event_config(struct etmv4_drvdata *drvdata, /* TRM: Must program this for cycacc to work */ config->ccctlr = ETM_CYC_THRESHOLD_DEFAULT; }

•   if (attr->config & BIT(ETM_OPT_TS))
  

•   if (attr->config & BIT(ETM_OPT_TS)) {
  

•           /*
  

•            * Configure timestamps to be emitted at regular intervals in
  

•            * order to correlate instructions executed on different CPUs
  

•            * (CPU-wide trace scenarios).
  

•            */
  

•           ret = etm4_config_timestamp_event(drvdata);
  

•           /*
  

•            * No need to go further if timestamp intervals can't
  

•            * be configured.
  

•            */
  

•           if (ret)
  

•                   goto out;
  

•           /* bit[11], Global timestamp tracing bit */
            config->cfg |= BIT(11);
  

•   }
  
    if (attr->config & BIT(ETM_OPT_CTXTID))
            /* bit[6], Context ID tracing bit */
  

-- 2.17.1

---

CoreSight mailing list CoreSight@lists.linaro.org https://lists.linaro.org/mailman/listinfo/coresight

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

Hey Leo,

On Sun, 17 Mar 2019 at 00:30, Leo Yan leo.yan@linaro.org wrote:

On Wed, Mar 06, 2019 at 03:57:35PM -0700, Mathieu Poirier wrote:

...

Configure timestamps to be emitted at regular intervals in the trace stream to temporally correlate instructions executed on different CPUs.

Signed-off-by: Mathieu Poirier mathieu.poirier@linaro.org

drivers/hwtracing/coresight/coresight-etm4x.c | 109 +++++++++++++++++- 1 file changed, 106 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c index f067f7ee08c7..2c29d6743fee 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.c +++ b/drivers/hwtracing/coresight/coresight-etm4x.c @@ -138,8 +138,11 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) drvdata->base + TRCCNTVRn(i)); }

• /* Resource selector pair 0 is always implemented and reserved */
  

• for (i = 0; i < drvdata->nr_resource * 2; i++)
  

• /*
  

•  * Resource selector pair 0 is always implemented and reserved.  As
  

•  * such start at 2.
  

•  */
  

• for (i = 2; i < drvdata->nr_resource * 2; i++)
          writel_relaxed(config->res_ctrl[i],
                         drvdata->base + TRCRSCTLRn(i));
  
  

@@ -201,6 +204,91 @@ static void etm4_enable_hw_smp_call(void *info) arg->rc = etm4_enable_hw(arg->drvdata); }

+/*

• • The goal of function etm4_config_timestamp_event() is to configure a
• • counter that will tell the tracer to emit a timestamp packet when it
• • reaches zero. This is done in order to get a more fine grained idea
• • of when instructions are executed so that they can be correlated
• • with execution on other CPUs.
• • To do this the counter itself is configured to self reload and
• • TRCRSCTLR1 (always true) used to get the counter to decrement. From
• • there a resource selector is configured with the counter and the
• • timestamp control register to use the resource selector to trigger the
• • event that will insert a timestamp packet in the stream.
• */

+static int etm4_config_timestamp_event(struct etmv4_drvdata *drvdata) +{

• int i, ret = -EINVAL;
  

• int counter, rselector;
  

• u32 val = 0;
  

• struct etmv4_config *config = &drvdata->config;
  

• /* No point in trying if we don't have at least one counter */
  

• if (!drvdata->nr_cntr)
  

•         goto out;
  

• /* Find a counter that hasn't been initialised */
  

• for (i = 0; i < drvdata->nr_cntr; i++)
  

•         if (config->cntr_val[i] == 0)
  

•                 break;
  

• /* Remember what counter we used */
  

• counter = 1 << i;
  

• /* All the counters have been configured already, bail out */
  

• if (i == drvdata->nr_cntr) {
  

•         pr_err("%s: no available counter found\n", __func__);
  

•         goto out;
  

• }
  

• /*
  

•  * Initialise original and reload counter value to the smallest
  

•  * possible value in order to get as much precision as we can.
  

•  */
  

• config->cntr_val[i] = 1;
  

• config->cntrldvr[i] = 1;
  

Maybe this variable is defined by old code, but show use the more consistent naming like: config->cntr_ldvr?

Perhaps yes... But it would require a patch on its own, something that is at the very bottom of the priority list.

...

• /* Set the trace counter control register */
  

• val =  0x1 << 16        |  /* Bit 16, reload counter automatically */
  

•        0x0 << 7         |  /* Select single resource selector */
  

•        0x1;                /* Resource selector 1, i.e always true */
  

• config->cntr_ctrl[i] = val;
  

• /*
  

•  * Searching for an available resource selector to use, starting at
  

•  * '2' since every implementation has at least 2 resource selector.
  

•  * ETMIDR4 gives the number of resource selector _pairs_,
  

•  * hence multiply by 2.
  

•  */
  

• for (i = 2; i < drvdata->nr_resource * 2; i++)
  

•         if (!config->res_ctrl[i])
  

•                 break;
  

• /* Remember what resource selector we used */
  

• rselector = i;
  

• if (i == drvdata->nr_resource * 2) {
  

•         pr_err("%s: no available resource selector found\n", __func__);
  

•         goto out;
  

• }
  

If there ahve no available resource selector and 'goto' out tag, it will not reset the values for counter registers. Do you think for the failure case, we should clean below fields:

config->cntr_val[i] = 0; config->cntrldvr[i] = 0; config->cntr_ctrl[i] = 0;

You are correct - I will fix this.

...

• val = 0x2 << 16         | /* Group 0b0010 - Counter and sequencers */
  

•       counter << 0;       /* Counter to use */
  

When read the ETMv4 spec, the counter value are only set in bits 0 to 3, so should reserve for other fields?

This is a concern I looked into a while back. The other bits are labeled "RES0", which is defined in the TRM's glossary as:

"A reserved bit or field with Should-Be-Zero-or-Preserved (SBZP) behavior".

As such we should be fine with just writing '0' to the RES0 bits.

...

• config->res_ctrl[i] = val;
  

• val = 0x0 << 7          | /* Select single resource selector */
  

•       rselector;          /* Resource selector */
  

• config->ts_ctrl = val;
  

• ret = 0;
  

+out:

• return ret;
  

+}

static int etm4_parse_event_config(struct etmv4_drvdata *drvdata, struct perf_event *event) { @@ -236,9 +324,24 @@ static int etm4_parse_event_config(struct etmv4_drvdata *drvdata, /* TRM: Must program this for cycacc to work */ config->ccctlr = ETM_CYC_THRESHOLD_DEFAULT; }

• if (attr->config & BIT(ETM_OPT_TS))
  

• if (attr->config & BIT(ETM_OPT_TS)) {
  

•         /*
  

•          * Configure timestamps to be emitted at regular intervals in
  

•          * order to correlate instructions executed on different CPUs
  

•          * (CPU-wide trace scenarios).
  

•          */
  

•         ret = etm4_config_timestamp_event(drvdata);
  

Use this method, I can see now it only allocates counter but there have no flow to release counter; so after run for several times with perf command, will it fail to allocate any counter and resource selector?

Very well spotted. I will fix this.

...

•         /*
  

•          * No need to go further if timestamp intervals can't
  

•          * be configured.
  

•          */
  

•         if (ret)
  

•                 goto out;
  

•         /* bit[11], Global timestamp tracing bit */
          config->cfg |= BIT(11);
  

• }
  
  if (attr->config & BIT(ETM_OPT_CTXTID))
          /* bit[6], Context ID tracing bit */
  

-- 2.17.1

On Mon, Mar 18, 2019 at 02:48:55PM -0600, Mathieu Poirier wrote:

...

...

...

{ @@ -236,9 +324,24 @@ static int etm4_parse_event_config(struct etmv4_drvdata *drvdata, /* TRM: Must program this for cycacc to work */ config->ccctlr = ETM_CYC_THRESHOLD_DEFAULT; }

• if (attr->config & BIT(ETM_OPT_TS))
  

• if (attr->config & BIT(ETM_OPT_TS)) {
  

•         /*
  

•          * Configure timestamps to be emitted at regular intervals in
  

•          * order to correlate instructions executed on different CPUs
  

•          * (CPU-wide trace scenarios).
  

•          */
  

•         ret = etm4_config_timestamp_event(drvdata);
  

Use this method, I can see now it only allocates counter but there have no flow to release counter; so after run for several times with perf command, will it fail to allocate any counter and resource selector?

Very well spotted. I will fix this.

Turns out function etm4_parse_event_config() already clears out specifics from the previous session and starts with a clean slate.

You are right, sorry for noise.

Thanks, Leo Yan

[1]. https://elixir.bootlin.com/linux/v5.1-rc1/source/drivers/hwtracing/coresight...

Hi Mathieu,

On 06/03/2019 22:57, Mathieu Poirier wrote:

In preparation to handle device reference counting inside of the sink drivers, add a return code to the sink::disable() operation so that proper action can be taken if a sink has not been disabled.

Signed-off-by: Mathieu Poirier mathieu.poirier@linaro.org

drivers/hwtracing/coresight/coresight-etb10.c | 3 ++- drivers/hwtracing/coresight/coresight-tmc-etf.c | 5 +++-- drivers/hwtracing/coresight/coresight-tmc-etr.c | 5 +++-- drivers/hwtracing/coresight/coresight-tpiu.c | 3 ++- drivers/hwtracing/coresight/coresight.c | 6 +++++- include/linux/coresight.h | 2 +- 6 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c index 105782ea64c7..71c2a3cdb866 100644 --- a/drivers/hwtracing/coresight/coresight-etb10.c +++ b/drivers/hwtracing/coresight/coresight-etb10.c @@ -325,7 +325,7 @@ static void etb_disable_hw(struct etb_drvdata *drvdata) coresight_disclaim_device(drvdata->base); } -static void etb_disable(struct coresight_device *csdev) +static int etb_disable(struct coresight_device *csdev) { struct etb_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); unsigned long flags; @@ -340,6 +340,7 @@ static void etb_disable(struct coresight_device *csdev) spin_unlock_irqrestore(&drvdata->spinlock, flags); dev_dbg(drvdata->dev, "ETB disabled\n");

• return 0; }

static void *etb_alloc_buffer(struct coresight_device *csdev, int cpu, diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index a5f053f2db2c..d4213e7c2c45 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -273,7 +273,7 @@ static int tmc_enable_etf_sink(struct coresight_device *csdev, return 0; } -static void tmc_disable_etf_sink(struct coresight_device *csdev) +static int tmc_disable_etf_sink(struct coresight_device *csdev) { unsigned long flags; struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); @@ -281,7 +281,7 @@ static void tmc_disable_etf_sink(struct coresight_device *csdev) spin_lock_irqsave(&drvdata->spinlock, flags); if (drvdata->reading) { spin_unlock_irqrestore(&drvdata->spinlock, flags);

• return;
  

• return -EBUSY;
  

...

-static void tmc_disable_etr_sink(struct coresight_device *csdev) +static int tmc_disable_etr_sink(struct coresight_device *csdev) { unsigned long flags; struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); @@ -1400,7 +1400,7 @@ static void tmc_disable_etr_sink(struct coresight_device *csdev) spin_lock_irqsave(&drvdata->spinlock, flags); if (drvdata->reading) { spin_unlock_irqrestore(&drvdata->spinlock, flags);

• return;
  

• return -EBUSY;
  

This patch as such is fine. But I think we can improve the handling here.

I agree that these error codes are appropriate. However, I think we should still modify the mode to disabled for both the etf/etr to make sure the device is kept disabled after we finished reading. As the user has disabled it and thinks that it has completed successfully.

So, I think when we find that the sink buffer is being read, we may :

1) Set the mode to disabled, to allow the unprepare routine to keep this disabled.

2) Return 0, to indicate the operation will be acknowledged and no need to redo the operation.

What do you think ?

Suzuki

Hi Mathieu,

...

...

-static void tmc_disable_etr_sink(struct coresight_device *csdev) +static int tmc_disable_etr_sink(struct coresight_device *csdev) { unsigned long flags; struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); @@ -1400,7 +1400,7 @@ static void tmc_disable_etr_sink(struct coresight_device *csdev) spin_lock_irqsave(&drvdata->spinlock, flags); if (drvdata->reading) { spin_unlock_irqrestore(&drvdata->spinlock, flags);

•         return;
  

•         return -EBUSY;
  

This patch as such is fine. But I think we can improve the handling here.

I agree that these error codes are appropriate. However, I think we should still modify the mode to disabled for both the etf/etr to make sure the device is kept disabled after we finished reading. As the user has disabled it and thinks that it has completed successfully.

So, I think when we find that the sink buffer is being read, we may :

1. Set the mode to disabled, to allow the unprepare routine to keep this

disabled.

2. Return 0, to indicate the operation will be acknowledged and no need

to redo the operation.

What do you think ?

We could do that but after looking at the ETR code (I haven't looked at ETF) the following comes to mind:

1. Disabling the ETR when a read is already happening means we call

__tmc_etr_disable_hw() twice, that is once in tmc_read_prepare_etr() and another time in tmc_disable_etr_sink() as part of tmc_etr_disable_hw(). I looked at the code and this _should_ be fine. To do things properly we'd need to split tmc_etr_disable_hw() in two, hence creating 3 different levels of ETR disable, which I'm not a fan of.

So the point is, if it is being read, we simply set the mode to DISABLED and return success here. That implies, we don't do the __tmc_etr_disable_hw() as the hw is already disabled. Now, the read_unprepare_etr() could leave the ETR disabled, since the mode has been changed already.

2. Another problem I see is that by disabling the sink while a read

operation is pending, we also set the mode to CS_MODE_DISABLED and decrement csdev->refcnt. That means another session (sysfs or perf) is free to use the sink and do all the required re-initialisation it needs. I think this should be fine but a lot more scrutiny would need to be invested if we are to go down that route. Thanks to you I found a bug in tmc_disable_etf_sink()[1] and tmc_disable_etr_sink()[2] - testing for drvdata->reading should be done before decrementing the refcount.

But then we do check if it is being read before we allow enabling the hardware. So, we are covered. If we don't, we must do that if we are going to enable PERF mode.

3. I've always seen sysfs operations being done consecutively, i.e one

after the other. If a user is reading from a sink it is not expected they would issue a concurrent disable operation.

So long story short I think it is possible but I'm not sure it it worth the investment. I'm also not sure it should be rolled in this set. Anyways, your turn to let me know what you think.

I am fine with this being delayed into separate patch.

Cheers Suzuki

On 06/03/2019 22:57, Mathieu Poirier wrote:

When operating in CPU-wide mode with an N:1 source/sink HW topology, multiple CPUs can access a sink concurrently. As such reference counting needs to happen when the device's spinlock is held to avoid racing with other operations (start(), update(), stop()).

I think this patch is trying to address the following issue. i.e, we get the refcount a bit late, after we have enabled the sink, and that could make use loose some sessions, if another session drops the refcount before the another one gets it.

i.e,

Session A Session B ----- -------

enable_sink atomic_inc(refcount) = 1

... atomic_dec(refcount) = 0 enable_sink if (refcount == 0) disable_sink atomic_inc() could we have this in the commit description.

Signed-off-by: Mathieu Poirier mathieu.poirier@linaro.org

drivers/hwtracing/coresight/coresight-etb10.c | 18 ++++++++++-- .../hwtracing/coresight/coresight-tmc-etf.c | 21 +++++++++++--- .../hwtracing/coresight/coresight-tmc-etr.c | 19 +++++++++++-- drivers/hwtracing/coresight/coresight-tpiu.c | 6 +++- drivers/hwtracing/coresight/coresight.c | 28 +++++++++---------- 5 files changed, 66 insertions(+), 26 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c index 71c2a3cdb866..dcc550507f70 100644 --- a/drivers/hwtracing/coresight/coresight-etb10.c +++ b/drivers/hwtracing/coresight/coresight-etb10.c @@ -5,6 +5,7 @@

• Description: CoreSight Embedded Trace Buffer driver

*/ +#include <linux/atomic.h> #include <linux/kernel.h> #include <linux/init.h> #include <linux/types.h> @@ -160,12 +161,16 @@ static int etb_enable_sysfs(struct coresight_device *csdev) } /* Nothing to do, the tracer is already enabled. */

• if (drvdata->mode == CS_MODE_SYSFS)

• if (drvdata->mode == CS_MODE_SYSFS) {

• atomic_inc(csdev->refcnt);
  

  goto out;
• }

ret = etb_enable_hw(drvdata);

• if (!ret)

• if (!ret) { drvdata->mode = CS_MODE_SYSFS;

• atomic_inc(csdev->refcnt);
  

• }

out:

Could we consolidate the above two cases and move it here at out ?;

if (!ret) atomic_inc(csdev->refcnt);

spin_unlock_irqrestore(&drvdata->spinlock, flags); @@ -196,8 +201,10 @@ static int etb_enable_perf(struct coresight_device *csdev, void *data) goto out; ret = etb_enable_hw(drvdata);

• if (!ret)

• if (!ret) { drvdata->mode = CS_MODE_PERF;

• atomic_inc(csdev->refcnt);
  

• }

out: spin_unlock_irqrestore(&drvdata->spinlock, flags); @@ -332,6 +339,11 @@ static int etb_disable(struct coresight_device *csdev) spin_lock_irqsave(&drvdata->spinlock, flags);

• if (atomic_dec_return(csdev->refcnt)) {

• spin_unlock_irqrestore(&drvdata->spinlock, flags);
  

• return -EBUSY;
  

• }
• /* Disable the ETB only if it needs to */ if (drvdata->mode != CS_MODE_DISABLED) { etb_disable_hw(drvdata);

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index d4213e7c2c45..b6d0c010eec1 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -4,6 +4,7 @@

• Author: Mathieu Poirier mathieu.poirier@linaro.org

*/

..

/* * If drvdata::buf isn't NULL, memory was allocated for a previous @@ -200,11 +203,13 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev) } ret = tmc_etb_enable_hw(drvdata);

• if (!ret)

• if (!ret) { drvdata->mode = CS_MODE_SYSFS;

• else

• atomic_inc(csdev->refcnt);
  

• } else { /* Free up the buffer if we failed to enable */ used = false;
• } out:

Same as above here.

spin_unlock_irqrestore(&drvdata->spinlock, flags); @@ -239,8 +244,10 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data) if (ret) break; ret = tmc_etb_enable_hw(drvdata);

• if (!ret)
  

• if (!ret) {
  drvdata->mode = CS_MODE_PERF;
  

• 	atomic_inc(csdev->refcnt);
  

• }
  

  } while (0); spin_unlock_irqrestore(&drvdata->spinlock, flags);

@@ -279,6 +286,12 @@ static int tmc_disable_etf_sink(struct coresight_device *csdev) struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); spin_lock_irqsave(&drvdata->spinlock, flags);

• if (atomic_dec_return(csdev->refcnt)) {

• spin_unlock_irqrestore(&drvdata->spinlock, flags);
  

• return -EBUSY;
  

• }
• if (drvdata->reading) { spin_unlock_irqrestore(&drvdata->spinlock, flags); return -EBUSY;

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index 33501777038a..cbb17b2c53b5 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -4,6 +4,7 @@

• Author: Mathieu Poirier mathieu.poirier@linaro.org

*/ +#include <linux/atomic.h> #include <linux/coresight.h> #include <linux/dma-mapping.h> #include <linux/iommu.h> @@ -1124,8 +1125,10 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) * sink is already enabled no memory is needed and the HW need not be * touched, even if the buffer size has changed. */

• if (drvdata->mode == CS_MODE_SYSFS)

• if (drvdata->mode == CS_MODE_SYSFS) {

• atomic_inc(csdev->refcnt);
  

  goto out;
• }

/* * If we don't have a buffer or it doesn't match the requested size, @@ -1138,8 +1141,10 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) } ret = tmc_etr_enable_hw(drvdata, drvdata->sysfs_buf);

• if (!ret)

• if (!ret) { drvdata->mode = CS_MODE_SYSFS;

• atomic_inc(csdev->refcnt);
  

• } out: spin_unlock_irqrestore(&drvdata->spinlock, flags);

@@ -1370,8 +1375,10 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data) etr_perf->head = PERF_IDX2OFF(handle->head, etr_perf); drvdata->perf_data = etr_perf; rc = tmc_etr_enable_hw(drvdata, etr_perf->etr_buf);

• if (!rc)

• if (!rc) { drvdata->mode = CS_MODE_PERF;

• atomic_inc(csdev->refcnt);
  

• }

unlock_out: spin_unlock_irqrestore(&drvdata->spinlock, flags); @@ -1398,6 +1405,12 @@ static int tmc_disable_etr_sink(struct coresight_device *csdev) struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); spin_lock_irqsave(&drvdata->spinlock, flags);

• if (atomic_dec_return(csdev->refcnt)) {

• spin_unlock_irqrestore(&drvdata->spinlock, flags);
  

• return -EBUSY;
  

• }
• if (drvdata->reading) { spin_unlock_irqrestore(&drvdata->spinlock, flags); return -EBUSY;

diff --git a/drivers/hwtracing/coresight/coresight-tpiu.c b/drivers/hwtracing/coresight/coresight-tpiu.c index 0d13da1b9df1..7acbeffcc137 100644 --- a/drivers/hwtracing/coresight/coresight-tpiu.c +++ b/drivers/hwtracing/coresight/coresight-tpiu.c @@ -5,6 +5,7 @@

• Description: CoreSight Trace Port Interface Unit driver

*/ +#include <linux/atomic.h> #include <linux/kernel.h> #include <linux/init.h> #include <linux/device.h> @@ -73,7 +74,7 @@ static int tpiu_enable(struct coresight_device *csdev, u32 mode, void *__unused) struct tpiu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); tpiu_enable_hw(drvdata);

• atomic_inc(csdev->refcnt); dev_dbg(drvdata->dev, "TPIU enabled\n"); return 0; }

@@ -98,6 +99,9 @@ static int tpiu_disable(struct coresight_device *csdev) { struct tpiu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);

• if (atomic_dec_return(csdev->refcnt))

• return -EBUSY;
  

• tpiu_disable_hw(drvdata);

dev_dbg(drvdata->dev, "TPIU disabled\n"); diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c index 13eda4693f81..19ba121d7451 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -225,14 +225,13 @@ static int coresight_enable_sink(struct coresight_device *csdev, * We need to make sure the "new" session is compatible with the * existing "mode" of operation. */

• if (sink_ops(csdev)->enable) {

• ret = sink_ops(csdev)->enable(csdev, mode, data);
  

• if (ret)
  

• 	return ret;
  

• csdev->enable = true;
  

• }

• if (!sink_ops(csdev)->enable)

• return -EINVAL;
  

• atomic_inc(csdev->refcnt);

• ret = sink_ops(csdev)->enable(csdev, mode, data);
• if (ret)

• return ret;
  

• csdev->enable = true;

return 0; } @@ -241,14 +240,13 @@ static void coresight_disable_sink(struct coresight_device *csdev) { int ret;

• if (atomic_dec_return(csdev->refcnt) == 0) {

• if (sink_ops(csdev)->disable) {
  

• 	ret = sink_ops(csdev)->disable(csdev);
  

• 	if (ret)
  

• 		return;
  

• 	csdev->enable = false;
  

• }
  

• }

• if (!sink_ops(csdev)->disable)

• return;
  

• ret = sink_ops(csdev)->disable(csdev);
• if (ret)

• return;
  

• csdev->enable = false; }

static int coresight_enable_link(struct coresight_device *csdev,

Otherwise looks fine to me.

Suzuki

On Fri, Mar 15, 2019 at 05:19:30PM +0000, Suzuki K Poulose wrote:

Hi Mathieu,

On 06/03/2019 22:57, Mathieu Poirier wrote:

...

Function free_event_data() is already busy and is bound to become worse with the addition of CPU-wide trace scenarios. As such spin off a new function to strickly take care of the sink buffers.

Signed-off-by: Mathieu Poirier mathieu.poirier@linaro.org

.../hwtracing/coresight/coresight-etm-perf.c | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index b8ca3800b56b..806b3dd5872d 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -120,22 +120,34 @@ static int etm_event_init(struct perf_event *event) return ret; } +static void free_sink_buffer(struct etm_event_data *event_data) +{

• int cpu;
• cpumask_t *mask = &event_data->mask;
• struct coresight_device *sink;
• if (WARN_ON(cpumask_empty(mask)))

• return;
  

• if (!event_data->snk_config)

• return;
  

I think it may be a good idea to keep the check as they were. i.e, we check for the snk_config, followed by empty cpumask. It is perfectly valid for an event_data to have an empty sink_config and and empty cpumask.

I must admit you lost me here. Here in order to proceed we need a event_data->snk_config and a mask. In fact not having a ->snk_config should also be a WARN_ON() since etm_setup_aux() ensures one is valid. As for having an empty cpumask, that can't happen either since it is explicitly set in alloc_event_data().

With that:

Reviewed-by: Suzuki K Poulose suzuki.poulose@arm.com

On Tue, 19 Mar 2019 at 04:00, Suzuki K Poulose suzuki.poulose@arm.com wrote:

Hi Mathieu,

On 06/03/2019 22:57, Mathieu Poirier wrote:

...

In preparation to support CPU-wide trace scenarios, add the notion of process ID to ETR devices so that memory buffers can be shared between events.

The PID only matters to the perf buffers. And etr_buf is really a representation of the ETR Hw buffer and I prefer to keep it that way. Please could we have all the "perf" related book keeping in the etr_perf_buf, which holds the perf session specific data ?

From a quick look at the code it might streamline the allocation/free

process. I'll let you know if I run into problems but for now I'm pretty sure it is possible.

As for enforcing the FLAT mode in the later series, we don't necessarily need to force it for double buffering. Even our SG mode uses double buffering. Even otherwise, we may be able to force a mode, than passing down perf specific data to the generic ETR buffer.

The patches to force the flat mode was to make sure things are covered when the ETR improves and we are able to use the perf ring buffer properly. I'm happy to drop them (I never really liked them).

...

Signed-off-by: Mathieu Poirier mathieu.poirier@linaro.org

.../hwtracing/coresight/coresight-tmc-etr.c | 26 +++++++++++++++---- drivers/hwtracing/coresight/coresight-tmc.h | 6 +++++ 2 files changed, 27 insertions(+), 5 deletions(-)

...

...

diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index 487c53701e9c..d3657acb6cbf 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -9,6 +9,8 @@

#include <linux/dma-mapping.h> #include <linux/miscdevice.h> +#include <linux/refcount.h> +#include <linux/types.h>

#define TMC_RSZ 0x004 #define TMC_STS 0x00c @@ -133,6 +135,8 @@ struct etr_buf_operations;

/**

• struct etr_buf - Details of the buffer used by ETR

• • @pid : The pid this etr_buf belongs to.
• • @refcount : Number of sources currently using this etr_buf.
  • @mode : Mode of the ETR buffer, contiguous, Scatter Gather etc.
  • @full : Trace data overflow
  • @size : Size of the buffer.

@@ -143,6 +147,8 @@ struct etr_buf_operations;

• @private : Backend specific information for the buf

*/ struct etr_buf {

• pid_t                           pid;
  

• refcount_t                      refcount;
  

As mentioned above, those are better suited for etr_perf_buf.

Thoughts ?

Suzuki

On 22/03/2019 19:14, Mathieu Poirier wrote:

On Tue, 19 Mar 2019 at 04:00, Suzuki K Poulose suzuki.poulose@arm.com wrote:

...

Hi Mathieu,

On 06/03/2019 22:57, Mathieu Poirier wrote:

...

In preparation to support CPU-wide trace scenarios, add the notion of process ID to ETR devices so that memory buffers can be shared between events.

The PID only matters to the perf buffers. And etr_buf is really a representation of the ETR Hw buffer and I prefer to keep it that way. Please could we have all the "perf" related book keeping in the etr_perf_buf, which holds the perf session specific data ?

As for enforcing the FLAT mode in the later series, we don't necessarily need to force it for double buffering. Even our SG mode uses double buffering. Even otherwise, we may be able to force a mode, than passing down perf specific data to the generic ETR buffer.

...

Signed-off-by: Mathieu Poirier mathieu.poirier@linaro.org

.../hwtracing/coresight/coresight-tmc-etr.c | 26 +++++++++++++++---- drivers/hwtracing/coresight/coresight-tmc.h | 6 +++++ 2 files changed, 27 insertions(+), 5 deletions(-)

...

...

diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index 487c53701e9c..d3657acb6cbf 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -9,6 +9,8 @@

#include <linux/dma-mapping.h> #include <linux/miscdevice.h> +#include <linux/refcount.h> +#include <linux/types.h>

#define TMC_RSZ 0x004 #define TMC_STS 0x00c @@ -133,6 +135,8 @@ struct etr_buf_operations;

/** * struct etr_buf - Details of the buffer used by ETR

• • @pid : The pid this etr_buf belongs to.
• • @refcount : Number of sources currently using this etr_buf.
    • @mode : Mode of the ETR buffer, contiguous, Scatter Gather etc.
    • @full : Trace data overflow
    • @size : Size of the buffer.

@@ -143,6 +147,8 @@ struct etr_buf_operations; * @private : Backend specific information for the buf */ struct etr_buf {

• pid_t                           pid;
  

• refcount_t                      refcount;
  

As mentioned above, those are better suited for etr_perf_buf.

Thoughts ?

Going back on this, I definitely agree with you that pid should be in the etr_perf_buf structure. On the flip side I think refcount should stay in etr_buf as it counts the number of users of this buffer.

I see, you're right, since we could be creating multiple output handles and thus aux-pages per event ? Did I miss something ?

Cheers Suzuki

On 25/03/2019 15:08, Mathieu Poirier wrote:

On Mon, 25 Mar 2019 at 05:49, Suzuki K Poulose suzuki.poulose@arm.com wrote:

...

On 22/03/2019 19:14, Mathieu Poirier wrote:

...

On Tue, 19 Mar 2019 at 04:00, Suzuki K Poulose suzuki.poulose@arm.com wrote:

...

Hi Mathieu,

On 06/03/2019 22:57, Mathieu Poirier wrote:

...

In preparation to support CPU-wide trace scenarios, add the notion of process ID to ETR devices so that memory buffers can be shared between events.

The PID only matters to the perf buffers. And etr_buf is really a representation of the ETR Hw buffer and I prefer to keep it that way. Please could we have all the "perf" related book keeping in the etr_perf_buf, which holds the perf session specific data ?

As for enforcing the FLAT mode in the later series, we don't necessarily need to force it for double buffering. Even our SG mode uses double buffering. Even otherwise, we may be able to force a mode, than passing down perf specific data to the generic ETR buffer.

...

Signed-off-by: Mathieu Poirier mathieu.poirier@linaro.org

.../hwtracing/coresight/coresight-tmc-etr.c   | 26 +++++++++++++++----
drivers/hwtracing/coresight/coresight-tmc.h   |  6 +++++
2 files changed, 27 insertions(+), 5 deletions(-)

...

...

diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index 487c53701e9c..d3657acb6cbf 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -9,6 +9,8 @@

#include <linux/dma-mapping.h>
#include <linux/miscdevice.h>

+#include <linux/refcount.h> +#include <linux/types.h>

#define TMC_RSZ                     0x004
#define TMC_STS                     0x00c

@@ -133,6 +135,8 @@ struct etr_buf_operations;

/**
 * struct etr_buf - Details of the buffer used by ETR

• • @pid : The pid this etr_buf belongs to.
• • @refcount : Number of sources currently using this etr_buf.
    • @mode : Mode of the ETR buffer, contiguous, Scatter Gather etc.
    • @full : Trace data overflow
    • @size : Size of the buffer.

@@ -143,6 +147,8 @@ struct etr_buf_operations; * @private : Backend specific information for the buf */ struct etr_buf {

• pid_t                           pid;
  

• refcount_t                      refcount;
  

As mentioned above, those are better suited for etr_perf_buf.

Thoughts ?

Going back on this, I definitely agree with you that pid should be in the etr_perf_buf structure. On the flip side I think refcount should stay in etr_buf as it counts the number of users of this buffer.

I see, you're right, since we could be creating multiple output handles and thus aux-pages per event ? Did I miss something ?

In CPU-wide mode a struct etr_perf_buffer is instantiated for each CPU that is part of the trace session. If the HW enacts an 1:1 topology each etr_perf_buffer has its own etr_buf, otherwise the etr_buf is shared between etr_perf_buffer, hence the need to keep the refcount at that level.

Oh ! Thats nice and makes perfect sense. So we are covered for the 1:1 topology.

I'll spin another revision of this set today. After that I'm charging in your ACPI work.

Thanks. Np, take your time.

Cheers Suzuki

On Wed, 20 Mar 2019 at 08:16, Suzuki K Poulose suzuki.poulose@arm.com wrote:

Hi Mathieu,

On 06/03/2019 22:57, Mathieu Poirier wrote:

...

This patch adds support for CPU-wide trace scenarios by making sure that only the sources monitoring the same process have access to a common sink. Because the sink is shared between sources, the first source to use the sink switches it on while the last one does the cleanup. Any attempt to modify the HW is overlooked for as long as more than one source is using a sink.

Signed-off-by: Mathieu Poirier mathieu.poirier@linaro.org

.../hwtracing/coresight/coresight-tmc-etr.c | 39 ++++++++++++++++--- drivers/hwtracing/coresight/coresight-tmc.c | 2 + drivers/hwtracing/coresight/coresight-tmc.h | 3 ++ 3 files changed, 39 insertions(+), 5 deletions(-)

...

@@ -1463,10 +1468,32 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data) goto unlock_out; }

• /* Get a handle on the pid of the process to monitor */
  

• pid = etr_perf->etr_buf->pid;
  

• /* Do not proceed if this device is associated with another session */
  

• if (drvdata->pid != -1 && drvdata->pid != pid) {
  

•         rc = -EBUSY;
  

•         goto unlock_out;
  

• }
  

• etr_perf->head = PERF_IDX2OFF(handle->head, etr_perf);
  drvdata->perf_data = etr_perf;
  

• /*
  

•  * No HW configuration is needed if the sink is already in
  

•  * use for this session.
  

•  */
  

• if (drvdata->pid == pid) {
  

•         drvdata->mode = CS_MODE_PERF;
  

minor nit: This is not needed ^^. Otherwise, looks good to me.

An astute observation - I will also fix it for ETF and ETB.

Mathieu

Suzuki

On Mon, 25 Mar 2019 at 05:44, Suzuki K Poulose suzuki.poulose@arm.com wrote:

Hi Mathieu,

On 06/03/2019 22:57, Mathieu Poirier wrote:

...

When working with CPU-wide scenarios and the HW enacts an N:1 source/sink topology the sink is shared between source and needs to use double buffering. As such introduce a new function to easily determine if a sink is shared by multiple source.

Signed-off-by: Mathieu Poirier mathieu.poirier@linaro.org

drivers/hwtracing/coresight/coresight-priv.h | 1 + drivers/hwtracing/coresight/coresight.c | 25 ++++++++++++++++++++ 2 files changed, 26 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index e0684d06e9ee..5d7213561e94 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -146,6 +146,7 @@ static inline void coresight_write_reg_pair(void __iomem *addr, u64 val,

void coresight_disable_path(struct list_head *path); int coresight_enable_path(struct list_head *path, u32 mode, void *sink_data); +int coresight_sink_is_shared(struct list_head *path); struct coresight_device *coresight_get_sink(struct list_head *path); struct coresight_device *coresight_get_enabled_sink(bool reset); struct coresight_device *coresight_get_sink_by_id(u32 id); diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c index 19ba121d7451..d74b2f621885 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -483,6 +483,31 @@ int coresight_enable_path(struct list_head *path, u32 mode, void *sink_data) goto out; }

+int coresight_sink_is_shared(struct list_head *path) +{

• enum coresight_dev_type type;
  

• enum coresight_dev_subtype_link subtype;
  

• struct coresight_node *nd;
  

• struct coresight_device *csdev;
  

• if (!path)
  

•         return -EINVAL;
  

• /* Go through each of the entries in the path */
  

• list_for_each_entry(nd, path, link) {
  

•         csdev = nd->csdev;
  

•         type = csdev->type;
  

•         subtype = csdev->subtype.link_subtype;
  

•         /* If we find a funnel the sink is shared */
  

•         if (type == CORESIGHT_DEV_TYPE_LINK &&
  

•             subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG)
  

•                 return 1;
  

One caveat with this logic is that, we have systems where a funnel appears in the path, whose input ports are not connected, implying they may not be shared after all (e.g, imx7s in arm32). It may be a good idea to add (csdev->nr_inport > 1) check to the list, if we figure out that it is a funnel.

You are 100% right.

Otherwise, looks good to me.

Suzuki

Mathieu,

On 06/03/2019 22:57, Mathieu Poirier wrote:

This patch makes the ETR component aware of the topology the HW is enacting so that it can make choices about the memory mode to use when working in CPU-wide scenarios.

Signed-off-by: Mathieu Poirier mathieu.poirier@linaro.org

.../hwtracing/coresight/coresight-tmc-etr.c | 19 +++++++++++++++++++ drivers/hwtracing/coresight/coresight-tmc.c | 2 ++ drivers/hwtracing/coresight/coresight-tmc.h | 2 ++ 3 files changed, 23 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index d8c71d22bdef..eace816ff042 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1276,6 +1276,22 @@ tmc_etr_setup_perf_buf(struct tmc_drvdata *drvdata, int node, pid_t pid, return etr_perf; } +static int tmc_etr_is_shared(struct tmc_drvdata *drvdata,

• 	     struct list_head *path)
  

+{

• int shared;
• /* The HW topology is static, no need to check this twice */
• if (drvdata->shared >= 0)

• return 0;
  

• shared = coresight_sink_is_shared(path);
• if (shared == -EINVAL)

• return shared;
  

• drvdata->shared = shared;
• return 0;

+} static void *tmc_alloc_etr_buffer(struct list_head *path, int cpu, pid_t pid, void **pages, int nr_pages, @@ -1291,6 +1307,9 @@ static void *tmc_alloc_etr_buffer(struct list_head *path, int cpu, drvdata = dev_get_drvdata(csdev->dev.parent);

• if (tmc_etr_is_shared(drvdata, path))

• return NULL;
  

So, we don't support tracing if the ETR is shared ? I am failing to understand the rationale of this whole "shared" check. Please could you elaborate ? And may be even add a comment here ?

Cheers Suzuki