| Message ID | 20250401180708.385396-6-leo.yan@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Arm CoreSight: Support AUX pause and resume | expand |
Hi Leo, On Tue, 1 Apr 2025 at 19:07, Leo Yan <leo.yan@arm.com> wrote: > > The buffer update callbacks disable the sink before syncing data but > misses to re-enable it afterward. This is fine in the general flow, > because the sink will be re-enabled the next time the PMU event is > activated. > > However, during AUX pause and resume, if the sink is disabled in the > buffer update callback, there is no chance to re-enable it when AUX > resumes. > > To address this, the callbacks now check the event state > 'event->hw.state'. If the event is an active state (0), the sink is > re-enabled. > > For the TMC ETR driver, buffer updates are not fully protected by > the driver's spinlock. In this case, the sink is not re-enabled if its > reference counter is 0, in order to avoid race conditions where the sink > may have been completely disabled. > > Signed-off-by: Leo Yan <leo.yan@arm.com> > --- > drivers/hwtracing/coresight/coresight-tmc-etf.c | 9 +++++++++ > drivers/hwtracing/coresight/coresight-tmc-etr.c | 10 ++++++++++ > 2 files changed, 19 insertions(+) > > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c > index d858740001c2..7584cc03d8e6 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c > @@ -482,6 +482,7 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev, > unsigned long offset, to_read = 0, flags; > struct cs_buffers *buf = sink_config; > struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > + struct perf_event *event = handle->event; > > if (!buf) > return 0; > @@ -586,6 +587,14 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev, > * is expected by the perf ring buffer. > */ > CS_LOCK(drvdata->base); > + > + /* > + * If the event is active, it is triggered during an AUX pause. > + * Re-enable the sink so that it is ready when AUX resume is invoked. > + */ > + if (!event->hw.state) > + __tmc_etb_enable_hw(drvdata); > + Think that the refcnt should be checked here too. Does the ETB case need to be handled? - somewhat confusingly the coresight-tmc-etf.c file handles both ETF and ETB. Regards Mike > out: > raw_spin_unlock_irqrestore(&drvdata->spinlock, flags); > > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c > index 76a8cb29b68a..8923fbc6e1a0 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c > @@ -1636,6 +1636,7 @@ tmc_update_etr_buffer(struct coresight_device *csdev, > struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > struct etr_perf_buffer *etr_perf = config; > struct etr_buf *etr_buf = etr_perf->etr_buf; > + struct perf_event *event = handle->event; > > raw_spin_lock_irqsave(&drvdata->spinlock, flags); > > @@ -1705,6 +1706,15 @@ tmc_update_etr_buffer(struct coresight_device *csdev, > */ > smp_wmb(); > > + /* > + * If the event is active, it is triggered during an AUX pause. > + * Re-enable the sink so that it is ready when AUX resume is invoked. > + */ > + raw_spin_lock_irqsave(&drvdata->spinlock, flags); > + if (csdev->refcnt && !event->hw.state) > + __tmc_etr_enable_hw(drvdata); > + raw_spin_unlock_irqrestore(&drvdata->spinlock, flags); > + > out: > /* > * Don't set the TRUNCATED flag in snapshot mode because 1) the > -- > 2.34.1 > -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
Hi Mike, On Wed, Apr 02, 2025 at 04:05:10PM +0100, Mike Leach wrote: [...] > > @@ -482,6 +482,7 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev, > > unsigned long offset, to_read = 0, flags; > > struct cs_buffers *buf = sink_config; > > struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > > + struct perf_event *event = handle->event; > > > > if (!buf) > > return 0; > > @@ -586,6 +587,14 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev, > > * is expected by the perf ring buffer. > > */ > > CS_LOCK(drvdata->base); > > + > > + /* > > + * If the event is active, it is triggered during an AUX pause. > > + * Re-enable the sink so that it is ready when AUX resume is invoked. > > + */ > > + if (!event->hw.state) > > + __tmc_etb_enable_hw(drvdata); > > + > > Think that the refcnt should be checked here too. No, ETF driver uses spinlock to guard the entire region for checking refcnt and updating buffer, here it is still in the same critical region. This is why the checking refcnt is not needed. > Does the ETB case need to be handled? - somewhat confusingly the > coresight-tmc-etf.c file handles both ETF and ETB. ETF is for the link mode, and ETB is for sink. Updating buffer is only for sink mode, this is why here I use __tmc_etb_enable_hw(). Does it make sense? I also have a question for the paired operations (this is applied for both ETF and ETR drivers). Now the flow is: tmc_update_etf_buffer() { tmc_flush_and_stop(); update buffer; __tmc_etb_enable_hw(); } The operations are not paired between tmc_flush_and_stop() and __tmc_etb_enable_hw(). The tmc_flush_and_stop() function only controls the TMC_FFCR register. I'm not sure whether I need to extract the TMC_FFCR operations from __tmc_etb_enable_hw() to use them for recovery in the update buffer. Or do you think re-enabling the hardware in this patch is the safer approach? Thanks, Leo
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index d858740001c2..7584cc03d8e6 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -482,6 +482,7 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev, unsigned long offset, to_read = 0, flags; struct cs_buffers *buf = sink_config; struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); + struct perf_event *event = handle->event; if (!buf) return 0; @@ -586,6 +587,14 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev, * is expected by the perf ring buffer. */ CS_LOCK(drvdata->base); + + /* + * If the event is active, it is triggered during an AUX pause. + * Re-enable the sink so that it is ready when AUX resume is invoked. + */ + if (!event->hw.state) + __tmc_etb_enable_hw(drvdata); + out: raw_spin_unlock_irqrestore(&drvdata->spinlock, flags); diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index 76a8cb29b68a..8923fbc6e1a0 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1636,6 +1636,7 @@ tmc_update_etr_buffer(struct coresight_device *csdev, struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); struct etr_perf_buffer *etr_perf = config; struct etr_buf *etr_buf = etr_perf->etr_buf; + struct perf_event *event = handle->event; raw_spin_lock_irqsave(&drvdata->spinlock, flags); @@ -1705,6 +1706,15 @@ tmc_update_etr_buffer(struct coresight_device *csdev, */ smp_wmb(); + /* + * If the event is active, it is triggered during an AUX pause. + * Re-enable the sink so that it is ready when AUX resume is invoked. + */ + raw_spin_lock_irqsave(&drvdata->spinlock, flags); + if (csdev->refcnt && !event->hw.state) + __tmc_etr_enable_hw(drvdata); + raw_spin_unlock_irqrestore(&drvdata->spinlock, flags); + out: /* * Don't set the TRUNCATED flag in snapshot mode because 1) the
The buffer update callbacks disable the sink before syncing data but misses to re-enable it afterward. This is fine in the general flow, because the sink will be re-enabled the next time the PMU event is activated. However, during AUX pause and resume, if the sink is disabled in the buffer update callback, there is no chance to re-enable it when AUX resumes. To address this, the callbacks now check the event state 'event->hw.state'. If the event is an active state (0), the sink is re-enabled. For the TMC ETR driver, buffer updates are not fully protected by the driver's spinlock. In this case, the sink is not re-enabled if its reference counter is 0, in order to avoid race conditions where the sink may have been completely disabled. Signed-off-by: Leo Yan <leo.yan@arm.com> --- drivers/hwtracing/coresight/coresight-tmc-etf.c | 9 +++++++++ drivers/hwtracing/coresight/coresight-tmc-etr.c | 10 ++++++++++ 2 files changed, 19 insertions(+)