[PATCH 0/5] Several updates for PTT driver

[PATCH 0/5] Several updates for PTT driver

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

This series contains several updates for PTT driver:
- Disable interrupt when trace stops, reverse to what we do in trace start
- Always handle the interrupt in hardirq context
- Optimize the AUX buffer handling to make consumer have more time to process
  the data
- Since we're a uncore PMU so block any task attach operation
- Add a dummy pmu::read() callback since the perf core may use

Junhao He (1):
  hwtracing: hisi_ptt: Add dummy callback pmu::read()

Yicong Yang (4):
  hwtracing: hisi_ptt: Disable interrupt after trace end
  hwtracing: hisi_ptt: Handle the interrupt in hardirq context
  hwtracing: hisi_ptt: Optimize the trace data committing
  hwtracing: hisi_ptt: Don't try to attach a task

 drivers/hwtracing/ptt/hisi_ptt.c | 33 +++++++++++++++++++++-----------
 drivers/hwtracing/ptt/hisi_ptt.h |  1 +
 2 files changed, 23 insertions(+), 11 deletions(-)

-- 
2.24.0

[PATCH 1/5] hwtracing: hisi_ptt: Disable interrupt after trace end

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

On trace end we disable the hardware but leave the interrupt
unmasked. Mask the interrupt to make the process reverse to
the start. No actual issue since hardware should send no
interrupt after disabled.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/hwtracing/ptt/hisi_ptt.c | 4 ++++
 drivers/hwtracing/ptt/hisi_ptt.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c
index ba081b6d2435..5f0a8e653ae8 100644
--- a/drivers/hwtracing/ptt/hisi_ptt.c
+++ b/drivers/hwtracing/ptt/hisi_ptt.c
@@ -183,6 +183,10 @@ static void hisi_ptt_wait_dma_reset_done(struct hisi_ptt *hisi_ptt)
 static void hisi_ptt_trace_end(struct hisi_ptt *hisi_ptt)
 {
 	writel(0, hisi_ptt->iobase + HISI_PTT_TRACE_CTRL);
+
+	/* Mask the interrupt on the end */
+	writel(HISI_PTT_TRACE_INT_MASK_ALL, hisi_ptt->iobase + HISI_PTT_TRACE_INT_MASK);
+
 	hisi_ptt->trace_ctrl.started = false;
 }
 
diff --git a/drivers/hwtracing/ptt/hisi_ptt.h b/drivers/hwtracing/ptt/hisi_ptt.h
index e17f045d7e72..46030aa88081 100644
--- a/drivers/hwtracing/ptt/hisi_ptt.h
+++ b/drivers/hwtracing/ptt/hisi_ptt.h
@@ -47,6 +47,7 @@
 #define HISI_PTT_TRACE_INT_STAT		0x0890
 #define   HISI_PTT_TRACE_INT_STAT_MASK	GENMASK(3, 0)
 #define HISI_PTT_TRACE_INT_MASK		0x0894
+#define   HISI_PTT_TRACE_INT_MASK_ALL	GENMASK(3, 0)
 #define HISI_PTT_TUNING_INT_STAT	0x0898
 #define   HISI_PTT_TUNING_INT_STAT_MASK	BIT(0)
 #define HISI_PTT_TRACE_WR_STS		0x08a0
-- 
2.24.0

Re: [PATCH 1/5] hwtracing: hisi_ptt: Disable interrupt after trace end

[Jonathan Cameron]

On Wed, 9 Aug 2023 16:18:21 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> On trace end we disable the hardware but leave the interrupt
> unmasked. Mask the interrupt to make the process reverse to
> the start. No actual issue since hardware should send no
> interrupt after disabled.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/hwtracing/ptt/hisi_ptt.c | 4 ++++
>  drivers/hwtracing/ptt/hisi_ptt.h | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c
> index ba081b6d2435..5f0a8e653ae8 100644
> --- a/drivers/hwtracing/ptt/hisi_ptt.c
> +++ b/drivers/hwtracing/ptt/hisi_ptt.c
> @@ -183,6 +183,10 @@ static void hisi_ptt_wait_dma_reset_done(struct hisi_ptt *hisi_ptt)
>  static void hisi_ptt_trace_end(struct hisi_ptt *hisi_ptt)
>  {
>  	writel(0, hisi_ptt->iobase + HISI_PTT_TRACE_CTRL);
> +
> +	/* Mask the interrupt on the end */
> +	writel(HISI_PTT_TRACE_INT_MASK_ALL, hisi_ptt->iobase + HISI_PTT_TRACE_INT_MASK);
> +
>  	hisi_ptt->trace_ctrl.started = false;
>  }
>  
> diff --git a/drivers/hwtracing/ptt/hisi_ptt.h b/drivers/hwtracing/ptt/hisi_ptt.h
> index e17f045d7e72..46030aa88081 100644
> --- a/drivers/hwtracing/ptt/hisi_ptt.h
> +++ b/drivers/hwtracing/ptt/hisi_ptt.h
> @@ -47,6 +47,7 @@
>  #define HISI_PTT_TRACE_INT_STAT		0x0890
>  #define   HISI_PTT_TRACE_INT_STAT_MASK	GENMASK(3, 0)
>  #define HISI_PTT_TRACE_INT_MASK		0x0894
> +#define   HISI_PTT_TRACE_INT_MASK_ALL	GENMASK(3, 0)
>  #define HISI_PTT_TUNING_INT_STAT	0x0898
>  #define   HISI_PTT_TUNING_INT_STAT_MASK	BIT(0)
>  #define HISI_PTT_TRACE_WR_STS		0x08a0

[PATCH 2/5] hwtracing: hisi_ptt: Handle the interrupt in hardirq context

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

Handle the trace interrupt in the hardirq context, make sure the irq
core won't threaded it by declaring IRQF_NO_THREAD and userspace won't
balance it by declaring IRQF_NOBALANCING. Otherwise we may violate the
synchronization requirements of the perf core, referenced to the
change of arm-ccn PMU commit 0811ef7e2f54 ("bus: arm-ccn: fix PMU interrupt flags").

In the interrupt handler we mainly doing 2 things:
- Copy the data from the local DMA buffer to the AUX buffer
- Commit the data in the AUX buffer

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/hwtracing/ptt/hisi_ptt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c
index 5f0a8e653ae8..89638ed96cf9 100644
--- a/drivers/hwtracing/ptt/hisi_ptt.c
+++ b/drivers/hwtracing/ptt/hisi_ptt.c
@@ -346,9 +346,9 @@ static int hisi_ptt_register_irq(struct hisi_ptt *hisi_ptt)
 		return ret;
 
 	hisi_ptt->trace_irq = pci_irq_vector(pdev, HISI_PTT_TRACE_DMA_IRQ);
-	ret = devm_request_threaded_irq(&pdev->dev, hisi_ptt->trace_irq,
-					NULL, hisi_ptt_isr, 0,
-					DRV_NAME, hisi_ptt);
+	ret = devm_request_irq(&pdev->dev, hisi_ptt->trace_irq, hisi_ptt_isr,
+				IRQF_NOBALANCING | IRQF_NO_THREAD, DRV_NAME,
+				hisi_ptt);
 	if (ret) {
 		pci_err(pdev, "failed to request irq %d, ret = %d\n",
 			hisi_ptt->trace_irq, ret);
-- 
2.24.0

Re: [PATCH 2/5] hwtracing: hisi_ptt: Handle the interrupt in hardirq context

[Jonathan Cameron]

On Wed, 9 Aug 2023 16:18:22 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> Handle the trace interrupt in the hardirq context, make sure the irq
> core won't threaded it by declaring IRQF_NO_THREAD and userspace won't
> balance it by declaring IRQF_NOBALANCING. Otherwise we may violate the
> synchronization requirements of the perf core, referenced to the
> change of arm-ccn PMU commit 0811ef7e2f54 ("bus: arm-ccn: fix PMU interrupt flags").
> 
> In the interrupt handler we mainly doing 2 things:
> - Copy the data from the local DMA buffer to the AUX buffer
> - Commit the data in the AUX buffer
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/hwtracing/ptt/hisi_ptt.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c
> index 5f0a8e653ae8..89638ed96cf9 100644
> --- a/drivers/hwtracing/ptt/hisi_ptt.c
> +++ b/drivers/hwtracing/ptt/hisi_ptt.c
> @@ -346,9 +346,9 @@ static int hisi_ptt_register_irq(struct hisi_ptt *hisi_ptt)
>  		return ret;
>  
>  	hisi_ptt->trace_irq = pci_irq_vector(pdev, HISI_PTT_TRACE_DMA_IRQ);
> -	ret = devm_request_threaded_irq(&pdev->dev, hisi_ptt->trace_irq,
> -					NULL, hisi_ptt_isr, 0,
> -					DRV_NAME, hisi_ptt);
> +	ret = devm_request_irq(&pdev->dev, hisi_ptt->trace_irq, hisi_ptt_isr,
> +				IRQF_NOBALANCING | IRQF_NO_THREAD, DRV_NAME,
> +				hisi_ptt);
>  	if (ret) {
>  		pci_err(pdev, "failed to request irq %d, ret = %d\n",
>  			hisi_ptt->trace_irq, ret);

[PATCH 3/5] hwtracing: hisi_ptt: Optimize the trace data committing

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

Currently during the PTT trace, we'll only commit the data
to the perf core when its full, which means after 4 interrupts
and totally 16MiB data while the AUX buffer is 16MiB length.
Then the userspace gets notified and handle the data. The driver
cannot apply a new AUX buffer immediately until the committed data
are handled and there's enough room in the buffer again.

This patch tries to optimize this by commit the data in every
interrupts in a 4MiB granularity. Then the userspace can have
enough time to consume the data and there's always enough room
in the AUX buffer.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/hwtracing/ptt/hisi_ptt.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c
index 89638ed96cf9..3aba7bb6e548 100644
--- a/drivers/hwtracing/ptt/hisi_ptt.c
+++ b/drivers/hwtracing/ptt/hisi_ptt.c
@@ -274,15 +274,14 @@ static int hisi_ptt_update_aux(struct hisi_ptt *hisi_ptt, int index, bool stop)
 	buf->pos += size;
 
 	/*
-	 * Just commit the traced data if we're going to stop. Otherwise if the
-	 * resident AUX buffer cannot contain the data of next trace buffer,
-	 * apply a new one.
+	 * Always commit the data to the AUX buffer in time to make sure
+	 * userspace got enough time to consume the data.
+	 *
+	 * If we're not going to stop, apply a new one and check whether
+	 * there's enough room for the next trace.
 	 */
-	if (stop) {
-		perf_aux_output_end(handle, buf->pos);
-	} else if (buf->length - buf->pos < HISI_PTT_TRACE_BUF_SIZE) {
-		perf_aux_output_end(handle, buf->pos);
-
+	perf_aux_output_end(handle, size);
+	if (!stop) {
 		buf = perf_aux_output_begin(handle, event);
 		if (!buf)
 			return -EINVAL;
-- 
2.24.0

Re: [PATCH 3/5] hwtracing: hisi_ptt: Optimize the trace data committing

[Jonathan Cameron]

On Wed, 9 Aug 2023 16:18:23 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> Currently during the PTT trace, we'll only commit the data
> to the perf core when its full, which means after 4 interrupts
> and totally 16MiB data while the AUX buffer is 16MiB length.
> Then the userspace gets notified and handle the data. The driver
> cannot apply a new AUX buffer immediately until the committed data
> are handled and there's enough room in the buffer again.
> 
> This patch tries to optimize this by commit the data in every
> interrupts in a 4MiB granularity. Then the userspace can have
> enough time to consume the data and there's always enough room
> in the AUX buffer.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/hwtracing/ptt/hisi_ptt.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c
> index 89638ed96cf9..3aba7bb6e548 100644
> --- a/drivers/hwtracing/ptt/hisi_ptt.c
> +++ b/drivers/hwtracing/ptt/hisi_ptt.c
> @@ -274,15 +274,14 @@ static int hisi_ptt_update_aux(struct hisi_ptt *hisi_ptt, int index, bool stop)
>  	buf->pos += size;
>  
>  	/*
> -	 * Just commit the traced data if we're going to stop. Otherwise if the
> -	 * resident AUX buffer cannot contain the data of next trace buffer,
> -	 * apply a new one.
> +	 * Always commit the data to the AUX buffer in time to make sure
> +	 * userspace got enough time to consume the data.
> +	 *
> +	 * If we're not going to stop, apply a new one and check whether
> +	 * there's enough room for the next trace.
>  	 */
> -	if (stop) {
> -		perf_aux_output_end(handle, buf->pos);
> -	} else if (buf->length - buf->pos < HISI_PTT_TRACE_BUF_SIZE) {
> -		perf_aux_output_end(handle, buf->pos);
> -
> +	perf_aux_output_end(handle, size);
> +	if (!stop) {
>  		buf = perf_aux_output_begin(handle, event);
>  		if (!buf)
>  			return -EINVAL;

[PATCH 4/5] hwtracing: hisi_ptt: Don't try to attach a task

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

PTT is an uncore PMU and shouldn't be attached to any task. Block
the usage in pmu::event_init().

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/hwtracing/ptt/hisi_ptt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c
index 3aba7bb6e548..916686c6a9c8 100644
--- a/drivers/hwtracing/ptt/hisi_ptt.c
+++ b/drivers/hwtracing/ptt/hisi_ptt.c
@@ -1003,6 +1003,9 @@ static int hisi_ptt_pmu_event_init(struct perf_event *event)
 		return -EOPNOTSUPP;
 	}
 
+	if (event->attach_state & PERF_ATTACH_TASK)
+		return -EOPNOTSUPP;
+
 	if (event->attr.type != hisi_ptt->hisi_ptt_pmu.type)
 		return -ENOENT;
 
-- 
2.24.0

Re: [PATCH 4/5] hwtracing: hisi_ptt: Don't try to attach a task

[Jonathan Cameron]

On Wed, 9 Aug 2023 16:18:24 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> PTT is an uncore PMU and shouldn't be attached to any task. Block
> the usage in pmu::event_init().
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/hwtracing/ptt/hisi_ptt.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c
> index 3aba7bb6e548..916686c6a9c8 100644
> --- a/drivers/hwtracing/ptt/hisi_ptt.c
> +++ b/drivers/hwtracing/ptt/hisi_ptt.c
> @@ -1003,6 +1003,9 @@ static int hisi_ptt_pmu_event_init(struct perf_event *event)
>  		return -EOPNOTSUPP;
>  	}
>  
> +	if (event->attach_state & PERF_ATTACH_TASK)
> +		return -EOPNOTSUPP;
> +
>  	if (event->attr.type != hisi_ptt->hisi_ptt_pmu.type)
>  		return -ENOENT;
>  

[PATCH 5/5] hwtracing: hisi_ptt: Add dummy callback pmu::read()

[Yicong Yang]

From: Junhao He <hejunhao3@huawei.com>

When start trace with perf option "-C $cpu" and immediately stop it
with SIGTERM or others, the perf core will invoke pmu::read() while
the driver doesn't implement it. Add a dummy pmu::read() to avoid
any issues.

Signed-off-by: Junhao He <hejunhao3@huawei.com>
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/hwtracing/ptt/hisi_ptt.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c
index 916686c6a9c8..7392e039b8c1 100644
--- a/drivers/hwtracing/ptt/hisi_ptt.c
+++ b/drivers/hwtracing/ptt/hisi_ptt.c
@@ -1184,6 +1184,10 @@ static void hisi_ptt_pmu_del(struct perf_event *event, int flags)
 	hisi_ptt_pmu_stop(event, PERF_EF_UPDATE);
 }
 
+static void hisi_ptt_pmu_read(struct perf_event *event)
+{
+}
+
 static void hisi_ptt_remove_cpuhp_instance(void *hotplug_node)
 {
 	cpuhp_state_remove_instance_nocalls(hisi_ptt_pmu_online, hotplug_node);
@@ -1227,6 +1231,7 @@ static int hisi_ptt_register_pmu(struct hisi_ptt *hisi_ptt)
 		.stop		= hisi_ptt_pmu_stop,
 		.add		= hisi_ptt_pmu_add,
 		.del		= hisi_ptt_pmu_del,
+		.read		= hisi_ptt_pmu_read,
 	};
 
 	reg = readl(hisi_ptt->iobase + HISI_PTT_LOCATION);
-- 
2.24.0

Re: [PATCH 5/5] hwtracing: hisi_ptt: Add dummy callback pmu::read()

[Jonathan Cameron]

On Wed, 9 Aug 2023 16:18:25 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Junhao He <hejunhao3@huawei.com>
> 
> When start trace with perf option "-C $cpu" and immediately stop it
> with SIGTERM or others, the perf core will invoke pmu::read() while
> the driver doesn't implement it. Add a dummy pmu::read() to avoid
> any issues.
> 
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/hwtracing/ptt/hisi_ptt.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c
> index 916686c6a9c8..7392e039b8c1 100644
> --- a/drivers/hwtracing/ptt/hisi_ptt.c
> +++ b/drivers/hwtracing/ptt/hisi_ptt.c
> @@ -1184,6 +1184,10 @@ static void hisi_ptt_pmu_del(struct perf_event *event, int flags)
>  	hisi_ptt_pmu_stop(event, PERF_EF_UPDATE);
>  }
>  
> +static void hisi_ptt_pmu_read(struct perf_event *event)
> +{
> +}
> +
>  static void hisi_ptt_remove_cpuhp_instance(void *hotplug_node)
>  {
>  	cpuhp_state_remove_instance_nocalls(hisi_ptt_pmu_online, hotplug_node);
> @@ -1227,6 +1231,7 @@ static int hisi_ptt_register_pmu(struct hisi_ptt *hisi_ptt)
>  		.stop		= hisi_ptt_pmu_stop,
>  		.add		= hisi_ptt_pmu_add,
>  		.del		= hisi_ptt_pmu_del,
> +		.read		= hisi_ptt_pmu_read,
>  	};
>  
>  	reg = readl(hisi_ptt->iobase + HISI_PTT_LOCATION);