| Message ID | 20250226-trace-noc-driver-v2-3-8afc6584afc5@quicinc.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | coresight: Add Coresight Trace NOC driver | expand |
On Wed, Feb 26, 2025 at 07:05:52PM +0800, Yuanfang Zhang wrote: > > Two nodes for configure flush are added here: > 1. flush_req: write 1 to initiates a flush sequence. > > 2. flush_state: read this node to get flush status. 0: sequence in > progress; 1: sequence has been completed. > > Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com> > --- > drivers/hwtracing/coresight/coresight-tnoc.c | 73 ++++++++++++++++++++++++++++ > drivers/hwtracing/coresight/coresight-tnoc.h | 4 ++ > 2 files changed, 77 insertions(+) > > diff --git a/drivers/hwtracing/coresight/coresight-tnoc.c b/drivers/hwtracing/coresight/coresight-tnoc.c > index fad8e61f05ef25989aba1be342c547f835e8953a..20231f28ddcb6a60d9b3c1ca3e0ca4d731dac39c 100644 > --- a/drivers/hwtracing/coresight/coresight-tnoc.c > +++ b/drivers/hwtracing/coresight/coresight-tnoc.c > @@ -16,6 +16,78 @@ > #include "coresight-tnoc.h" > #include "coresight-trace-id.h" > > +static ssize_t flush_req_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent); > + struct coresight_device *csdev = drvdata->csdev; > + unsigned long val; > + u32 reg; > + > + if (kstrtoul(buf, 10, &val)) > + return -EINVAL; > + > + if (val != 1) > + return -EINVAL; > + > + spin_lock(&drvdata->spinlock); > + if (csdev->refcnt == 0) { > + spin_unlock(&drvdata->spinlock); > + return -EPERM; > + } > + > + reg = readl_relaxed(drvdata->base + TRACE_NOC_CTRL); > + reg = reg | TRACE_NOC_CTRL_FLUSHREQ; > + writel_relaxed(reg, drvdata->base + TRACE_NOC_CTRL); How can userspace determine when to trigger a flush? Generally, a driver kicks off a flush operation for a hardware before reading data from buffer or when disable a link path. I don't know the hardware mechanism of TNOC, but seems to me, it does not make sense to let the userspace to trigger a hardware flush, given the userspace has no knowledge for device's state. Furthermore, based on my understanding for patch 02 and 03, the working flow is also concerned me. IIUC, you want to use the driver to create a linkage and then use userspace program to poll state and trigger flushing. Could you explain why use this way for managing the device? Thanks, Leo > + > + spin_unlock(&drvdata->spinlock); > + > + return size; > +} > +static DEVICE_ATTR_WO(flush_req); > + > +/* > + * flush-sequence status: > + * value 0: sequence in progress; > + * value 1: sequence has been completed. > + */ > +static ssize_t flush_status_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent); > + struct coresight_device *csdev = drvdata->csdev; > + u32 val; > + > + spin_lock(&drvdata->spinlock); > + if (csdev->refcnt == 0) { > + spin_unlock(&drvdata->spinlock); > + return -EPERM; > + } > + > + val = readl_relaxed(drvdata->base + TRACE_NOC_CTRL); > + spin_unlock(&drvdata->spinlock); > + return sysfs_emit(buf, "%lu\n", BMVAL(val, 2, 2)); > +} > +static DEVICE_ATTR_RO(flush_status); > + > +static struct attribute *trace_noc_attrs[] = { > + &dev_attr_flush_req.attr, > + &dev_attr_flush_status.attr, > + NULL, > +}; > + > +static struct attribute_group trace_noc_attr_grp = { > + .attrs = trace_noc_attrs, > +}; > + > +static const struct attribute_group *trace_noc_attr_grps[] = { > + &trace_noc_attr_grp, > + NULL, > +}; > + > static void trace_noc_enable_hw(struct trace_noc_drvdata *drvdata) > { > u32 val; > @@ -142,6 +214,7 @@ static int trace_noc_probe(struct amba_device *adev, const struct amba_id *id) > return ret; > > desc.ops = &trace_noc_cs_ops; > + desc.groups = trace_noc_attr_grps; > desc.type = CORESIGHT_DEV_TYPE_LINK; > desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_MERG; > desc.pdata = adev->dev.platform_data; > diff --git a/drivers/hwtracing/coresight/coresight-tnoc.h b/drivers/hwtracing/coresight/coresight-tnoc.h > index b6bd1ef659897d8e0994c5e8514e8cbdd16eebd8..d0fe8f52709ff4147d66dbf90987595012cfaa4e 100644 > --- a/drivers/hwtracing/coresight/coresight-tnoc.h > +++ b/drivers/hwtracing/coresight/coresight-tnoc.h > @@ -10,6 +10,10 @@ > > /* Enable generation of output ATB traffic.*/ > #define TRACE_NOC_CTRL_PORTEN BIT(0) > +/* Writing 1 to initiate a flush sequence.*/ > +#define TRACE_NOC_CTRL_FLUSHREQ BIT(1) > +/* 0: sequence in progress; 1: sequence has been completed.*/ > +#define TRACE_NOC_CTRL_FLUSHSTATUS BIT(2) > /* Writing 1 to issue a FREQ or FREQ_TS packet*/ > #define TRACE_NOC_CTRL_FREQTSREQ BIT(5) > /* Sets the type of issued ATB FLAG packets. 0: 'FLAG' packets; 1: 'FLAG_TS' packets.*/ > > -- > 2.34.1 > > _______________________________________________ > CoreSight mailing list -- coresight@lists.linaro.org > To unsubscribe send an email to coresight-leave@lists.linaro.org
On 2/28/2025 12:23 AM, Leo Yan wrote: > On Wed, Feb 26, 2025 at 07:05:52PM +0800, Yuanfang Zhang wrote: >> >> Two nodes for configure flush are added here: >> 1. flush_req: write 1 to initiates a flush sequence. >> >> 2. flush_state: read this node to get flush status. 0: sequence in >> progress; 1: sequence has been completed. >> >> Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com> >> --- >> drivers/hwtracing/coresight/coresight-tnoc.c | 73 ++++++++++++++++++++++++++++ >> drivers/hwtracing/coresight/coresight-tnoc.h | 4 ++ >> 2 files changed, 77 insertions(+) >> >> diff --git a/drivers/hwtracing/coresight/coresight-tnoc.c b/drivers/hwtracing/coresight/coresight-tnoc.c >> index fad8e61f05ef25989aba1be342c547f835e8953a..20231f28ddcb6a60d9b3c1ca3e0ca4d731dac39c 100644 >> --- a/drivers/hwtracing/coresight/coresight-tnoc.c >> +++ b/drivers/hwtracing/coresight/coresight-tnoc.c >> @@ -16,6 +16,78 @@ >> #include "coresight-tnoc.h" >> #include "coresight-trace-id.h" >> >> +static ssize_t flush_req_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, >> + size_t size) >> +{ >> + struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent); >> + struct coresight_device *csdev = drvdata->csdev; >> + unsigned long val; >> + u32 reg; >> + >> + if (kstrtoul(buf, 10, &val)) >> + return -EINVAL; >> + >> + if (val != 1) >> + return -EINVAL; >> + >> + spin_lock(&drvdata->spinlock); >> + if (csdev->refcnt == 0) { >> + spin_unlock(&drvdata->spinlock); >> + return -EPERM; >> + } >> + >> + reg = readl_relaxed(drvdata->base + TRACE_NOC_CTRL); >> + reg = reg | TRACE_NOC_CTRL_FLUSHREQ; >> + writel_relaxed(reg, drvdata->base + TRACE_NOC_CTRL); > > How can userspace determine when to trigger a flush? It can be triggered under any circumstances. > > Generally, a driver kicks off a flush operation for a hardware before > reading data from buffer or when disable a link path. I don't know the > hardware mechanism of TNOC, but seems to me, it does not make sense to > let the userspace to trigger a hardware flush, given the userspace has > no knowledge for device's state. TNOC supports the aforementioned flush operation, and it also adds this flush functionality, allowing users to set the flush themselves. > > Furthermore, based on my understanding for patch 02 and 03, the working > flow is also concerned me. IIUC, you want to use the driver to create > a linkage and then use userspace program to poll state and trigger > flushing. Could you explain why use this way for managing the device? > TNOC support flush just like other links. This interface simply provides customers with an additional option to trigger the flush. > Thanks, > Leo > >> + >> + spin_unlock(&drvdata->spinlock); >> + >> + return size; >> +} >> +static DEVICE_ATTR_WO(flush_req); >> + >> +/* >> + * flush-sequence status: >> + * value 0: sequence in progress; >> + * value 1: sequence has been completed. >> + */ >> +static ssize_t flush_status_show(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent); >> + struct coresight_device *csdev = drvdata->csdev; >> + u32 val; >> + >> + spin_lock(&drvdata->spinlock); >> + if (csdev->refcnt == 0) { >> + spin_unlock(&drvdata->spinlock); >> + return -EPERM; >> + } >> + >> + val = readl_relaxed(drvdata->base + TRACE_NOC_CTRL); >> + spin_unlock(&drvdata->spinlock); >> + return sysfs_emit(buf, "%lu\n", BMVAL(val, 2, 2)); >> +} >> +static DEVICE_ATTR_RO(flush_status); >> + >> +static struct attribute *trace_noc_attrs[] = { >> + &dev_attr_flush_req.attr, >> + &dev_attr_flush_status.attr, >> + NULL, >> +}; >> + >> +static struct attribute_group trace_noc_attr_grp = { >> + .attrs = trace_noc_attrs, >> +}; >> + >> +static const struct attribute_group *trace_noc_attr_grps[] = { >> + &trace_noc_attr_grp, >> + NULL, >> +}; >> + >> static void trace_noc_enable_hw(struct trace_noc_drvdata *drvdata) >> { >> u32 val; >> @@ -142,6 +214,7 @@ static int trace_noc_probe(struct amba_device *adev, const struct amba_id *id) >> return ret; >> >> desc.ops = &trace_noc_cs_ops; >> + desc.groups = trace_noc_attr_grps; >> desc.type = CORESIGHT_DEV_TYPE_LINK; >> desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_MERG; >> desc.pdata = adev->dev.platform_data; >> diff --git a/drivers/hwtracing/coresight/coresight-tnoc.h b/drivers/hwtracing/coresight/coresight-tnoc.h >> index b6bd1ef659897d8e0994c5e8514e8cbdd16eebd8..d0fe8f52709ff4147d66dbf90987595012cfaa4e 100644 >> --- a/drivers/hwtracing/coresight/coresight-tnoc.h >> +++ b/drivers/hwtracing/coresight/coresight-tnoc.h >> @@ -10,6 +10,10 @@ >> >> /* Enable generation of output ATB traffic.*/ >> #define TRACE_NOC_CTRL_PORTEN BIT(0) >> +/* Writing 1 to initiate a flush sequence.*/ >> +#define TRACE_NOC_CTRL_FLUSHREQ BIT(1) >> +/* 0: sequence in progress; 1: sequence has been completed.*/ >> +#define TRACE_NOC_CTRL_FLUSHSTATUS BIT(2) >> /* Writing 1 to issue a FREQ or FREQ_TS packet*/ >> #define TRACE_NOC_CTRL_FREQTSREQ BIT(5) >> /* Sets the type of issued ATB FLAG packets. 0: 'FLAG' packets; 1: 'FLAG_TS' packets.*/ >> >> -- >> 2.34.1 >> >> _______________________________________________ >> CoreSight mailing list -- coresight@lists.linaro.org >> To unsubscribe send an email to coresight-leave@lists.linaro.org
On Thu, Mar 06, 2025 at 04:39:27PM +0800, Yuanfang Zhang wrote: [...] > >> +static ssize_t flush_req_store(struct device *dev, > >> + struct device_attribute *attr, > >> + const char *buf, > >> + size_t size) > >> +{ ... > >> + reg = readl_relaxed(drvdata->base + TRACE_NOC_CTRL); > >> + reg = reg | TRACE_NOC_CTRL_FLUSHREQ; > >> + writel_relaxed(reg, drvdata->base + TRACE_NOC_CTRL); > > > > How can userspace determine when to trigger a flush? > It can be triggered under any circumstances. > > > > Generally, a driver kicks off a flush operation for a hardware before > > reading data from buffer or when disable a link path. I don't know the > > hardware mechanism of TNOC, but seems to me, it does not make sense to > > let the userspace to trigger a hardware flush, given the userspace has > > no knowledge for device's state. > > TNOC supports the aforementioned flush operation, and it also adds this > flush functionality, allowing users to set the flush themselves. I am still not convinced for providing knobs to allow userspace to directly control hardware. A low level driver should have sufficient information to know when and how it triggers a flush. E.g., CoreSight ETF (coresight-tmc-etf.c) can act as a link, in this case, it calls the tmc_flush_and_stop() function to flush its buffer when it is stopped. A flushing is triggered when a session is terminated (either is a perf session or a Sysfs session). Why not TNOC driver do the flushing same as other drivers? It can flush the data before a hardware link is to be disabled. I don't think flush operations are required at any time. Seems to me, exposing APIs to userspace for flushing operations also will introduce potential security risk. A malicious software might attack system with triggering tons of flushing in short time. > > Furthermore, based on my understanding for patch 02 and 03, the working > > flow is also concerned me. IIUC, you want to use the driver to create > > a linkage and then use userspace program to poll state and trigger > > flushing. Could you explain why use this way for managing the device? > > > TNOC support flush just like other links. This interface simply provides > customers with an additional option to trigger the flush. This is not true for Arm CoreSight components. My understanding is Arm CoreSight drivers never provides an API to userspace to manually trigger flush operations. Thanks, Leo
diff --git a/drivers/hwtracing/coresight/coresight-tnoc.c b/drivers/hwtracing/coresight/coresight-tnoc.c index fad8e61f05ef25989aba1be342c547f835e8953a..20231f28ddcb6a60d9b3c1ca3e0ca4d731dac39c 100644 --- a/drivers/hwtracing/coresight/coresight-tnoc.c +++ b/drivers/hwtracing/coresight/coresight-tnoc.c @@ -16,6 +16,78 @@ #include "coresight-tnoc.h" #include "coresight-trace-id.h" +static ssize_t flush_req_store(struct device *dev, + struct device_attribute *attr, + const char *buf, + size_t size) +{ + struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent); + struct coresight_device *csdev = drvdata->csdev; + unsigned long val; + u32 reg; + + if (kstrtoul(buf, 10, &val)) + return -EINVAL; + + if (val != 1) + return -EINVAL; + + spin_lock(&drvdata->spinlock); + if (csdev->refcnt == 0) { + spin_unlock(&drvdata->spinlock); + return -EPERM; + } + + reg = readl_relaxed(drvdata->base + TRACE_NOC_CTRL); + reg = reg | TRACE_NOC_CTRL_FLUSHREQ; + writel_relaxed(reg, drvdata->base + TRACE_NOC_CTRL); + + spin_unlock(&drvdata->spinlock); + + return size; +} +static DEVICE_ATTR_WO(flush_req); + +/* + * flush-sequence status: + * value 0: sequence in progress; + * value 1: sequence has been completed. + */ +static ssize_t flush_status_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent); + struct coresight_device *csdev = drvdata->csdev; + u32 val; + + spin_lock(&drvdata->spinlock); + if (csdev->refcnt == 0) { + spin_unlock(&drvdata->spinlock); + return -EPERM; + } + + val = readl_relaxed(drvdata->base + TRACE_NOC_CTRL); + spin_unlock(&drvdata->spinlock); + return sysfs_emit(buf, "%lu\n", BMVAL(val, 2, 2)); +} +static DEVICE_ATTR_RO(flush_status); + +static struct attribute *trace_noc_attrs[] = { + &dev_attr_flush_req.attr, + &dev_attr_flush_status.attr, + NULL, +}; + +static struct attribute_group trace_noc_attr_grp = { + .attrs = trace_noc_attrs, +}; + +static const struct attribute_group *trace_noc_attr_grps[] = { + &trace_noc_attr_grp, + NULL, +}; + static void trace_noc_enable_hw(struct trace_noc_drvdata *drvdata) { u32 val; @@ -142,6 +214,7 @@ static int trace_noc_probe(struct amba_device *adev, const struct amba_id *id) return ret; desc.ops = &trace_noc_cs_ops; + desc.groups = trace_noc_attr_grps; desc.type = CORESIGHT_DEV_TYPE_LINK; desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_MERG; desc.pdata = adev->dev.platform_data; diff --git a/drivers/hwtracing/coresight/coresight-tnoc.h b/drivers/hwtracing/coresight/coresight-tnoc.h index b6bd1ef659897d8e0994c5e8514e8cbdd16eebd8..d0fe8f52709ff4147d66dbf90987595012cfaa4e 100644 --- a/drivers/hwtracing/coresight/coresight-tnoc.h +++ b/drivers/hwtracing/coresight/coresight-tnoc.h @@ -10,6 +10,10 @@ /* Enable generation of output ATB traffic.*/ #define TRACE_NOC_CTRL_PORTEN BIT(0) +/* Writing 1 to initiate a flush sequence.*/ +#define TRACE_NOC_CTRL_FLUSHREQ BIT(1) +/* 0: sequence in progress; 1: sequence has been completed.*/ +#define TRACE_NOC_CTRL_FLUSHSTATUS BIT(2) /* Writing 1 to issue a FREQ or FREQ_TS packet*/ #define TRACE_NOC_CTRL_FREQTSREQ BIT(5) /* Sets the type of issued ATB FLAG packets. 0: 'FLAG' packets; 1: 'FLAG_TS' packets.*/
Two nodes for configure flush are added here: 1. flush_req: write 1 to initiates a flush sequence. 2. flush_state: read this node to get flush status. 0: sequence in progress; 1: sequence has been completed. Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com> --- drivers/hwtracing/coresight/coresight-tnoc.c | 73 ++++++++++++++++++++++++++++ drivers/hwtracing/coresight/coresight-tnoc.h | 4 ++ 2 files changed, 77 insertions(+)