Hi James,
-----Original Message----- From: James Clark james.clark@arm.com Sent: Wednesday, August 16, 2023 10:35 PM To: Linu Cherian lcherian@marvell.com; suzuki.poulose@arm.com; mike.leach@linaro.org; mathieu.poirier@linaro.org Cc: coresight@lists.linaro.org; Anil Kumar Reddy H areddy3@marvell.com; Tanmay Jagdale tanmay@marvell.com; George Cherian gcherian@marvell.com Subject: [EXT] Re: [RFC PATCH v2 5/7] coresight: tmc: Add support for reading tracedata from previous boot
External Email
On 13/07/2023 14:47, Linu Cherian wrote:
- Introduce a new mode CS_MODE_READ_PREVBOOT for reading
tracedata
captured in previous boot.
Add special handlers for preparing ETR/ETF for this special mode
User can read the trace data as below
For example, for reading trace data from tmc_etf sink
cd /sys/bus/coresight/devices/tmc_etfXX/
Change mode to READ_PREVBOOT
#echo 1 > read_prevboot
Dump trace buffer data to a file,
#dd if=/dev/tmc_etrXX of=~/cstrace.bin
Reset back to normal mode
#echo 0 > read_prevboot
Signed-off-by: Anil Kumar Reddy areddy3@marvell.com Signed-off-by: Tanmay Jagdale tanmay@marvell.com Signed-off-by: Linu Cherian lcherian@marvell.com
drivers/hwtracing/coresight/coresight-priv.h | 1 + .../hwtracing/coresight/coresight-tmc-core.c | 38 ++++- .../hwtracing/coresight/coresight-tmc-etf.c | 85 ++++++++++ .../hwtracing/coresight/coresight-tmc-etr.c | 154 +++++++++++++++++- drivers/hwtracing/coresight/coresight-tmc.h | 2 + 5 files changed, 278 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index 595ce5862056..2b8b83e72849 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -86,6 +86,7 @@ enum cs_mode { CS_MODE_DISABLED, CS_MODE_SYSFS, CS_MODE_PERF,
- CS_MODE_READ_PREVBOOT, /* Trace data from previous boot */
This causes an unhandled case error in etm4_disable() if you have CONFIG_WERROR=y
Will fix.
};
/** diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index 67a4bc6f4bff..2f97bb4f5760 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -152,6 +152,10 @@ static int tmc_open(struct inode *inode, struct file
*file)
struct tmc_drvdata *drvdata = container_of(file->private_data, struct tmc_drvdata,
miscdev);
- /* Advertise if we are opening with a special mode */
- if (drvdata->mode == CS_MODE_READ_PREVBOOT)
dev_info(&drvdata->csdev->dev, "TMC read mode for
previous
+boot\n");
I think dev_info might be too high a log level for this because it's something that's supposed to happen. You could do dev_info_once, but that's probably not that useful. So dev_dbg might be better. There are some other new dev_infos that have been added in other commits that also look like they might print multiple times.
Okay will change this. Will revisit the other dev_dbg cases.
ret = tmc_read_prepare(drvdata); if (ret) return ret; @@ -330,9 +334,40 @@ static ssize_t buffer_size_store(struct device *dev,
static DEVICE_ATTR_RW(buffer_size);
+static ssize_t read_prevboot_show(struct device *dev,
struct device_attribute *attr, char *buf) {
- struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent);
- return sprintf(buf, "%#x\n", drvdata->size); }
Should this not show drvdata->mode == CS_MODE_READ_PREVBOOT, rather than the size of the buffer? The size is already available through buffer_size, but there is no way to get the mode that it's in.
Yeah. Will fix this.
+static ssize_t read_prevboot_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t size)
+{
- int ret;
- unsigned long val;
- struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent);
- ret = kstrtoul(buf, 0, &val);
- if (ret)
return ret;
- if (val)
drvdata->mode = CS_MODE_READ_PREVBOOT;
- else
drvdata->mode = CS_MODE_DISABLED;
I saw there were some more thorough checks for changing the mode, for example etb_enable_sysfs(). It might be worth doing something similar here or making a re-usable mode changing function that does the appropriate checks, plus locking.
Ack. Will ensure necessary checks.
- return size;
+}
+static DEVICE_ATTR_RW(read_prevboot);
static struct attribute *coresight_tmc_attrs[] = { &dev_attr_trigger_cntr.attr, &dev_attr_buffer_size.attr,
- &dev_attr_read_prevboot.attr, NULL,
};
@@ -632,7 +667,8 @@ static void tmc_shutdown(struct amba_device
*adev)
spin_lock_irqsave(&drvdata->spinlock, flags);
- if (drvdata->mode == CS_MODE_DISABLED)
if (drvdata->mode == CS_MODE_DISABLED ||
drvdata->mode == CS_MODE_READ_PREVBOOT)
goto out;
if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) diff --git
a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index 63f59ea1508e..434b1586af54 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -664,6 +664,85 @@ const struct coresight_ops tmc_etf_cs_ops = { .panic_ops = &tmc_etf_sync_ops, };
+static int tmc_etb_setup_prevboot_buf(struct tmc_drvdata *drvdata) {
- u64 trace_addr;
- struct tmc_register_snapshot *reg_ptr;
- reg_ptr = drvdata->metadata.vaddr;
- trace_addr = reg_ptr->trc_addr | ((u64)reg_ptr->trc_addrhi << 32);
- drvdata->buf = memremap(trace_addr, reg_ptr->size,
MEMREMAP_WC);
- if (IS_ERR(drvdata->buf))
return -ENOMEM;
- drvdata->len = reg_ptr->size;
- return 0;
+}
+static void tmc_etb_free_prevboot_buf(struct tmc_drvdata *drvdata) {
- void *buf = drvdata->buf;
- if (!buf)
return;
- memunmap(buf);
- drvdata->buf = NULL;
+}
+static int tmc_etb_prepare_prevboot(struct tmc_drvdata *drvdata) {
- int ret = 0;
- unsigned long flags;
- struct tmc_register_snapshot *reg_ptr;
- spin_lock_irqsave(&drvdata->spinlock, flags);
- if (drvdata->reading) {
ret = -EBUSY;
goto out;
- }
- if (!drvdata->metadata.vaddr) {
ret = -ENOMEM;
goto out;
- }
- reg_ptr = drvdata->metadata.vaddr;
- if (!reg_ptr->valid) {
dev_err(&drvdata->csdev->dev,
"Invalid metadata captured from previous boot\n");
ret = -EINVAL;
goto out;
- }
- ret = tmc_etb_setup_prevboot_buf(drvdata);
- if (ret)
goto out;
- if (reg_ptr->sts & 0x1)
coresight_insert_barrier_packet(drvdata->buf);
- drvdata->reading = true;
+out:
- spin_unlock_irqrestore(&drvdata->spinlock, flags);
- return ret;
+}
+static int tmc_etb_unprepare_prevboot(struct tmc_drvdata *drvdata) {
- unsigned long flags;
- spin_lock_irqsave(&drvdata->spinlock, flags);
- tmc_etb_free_prevboot_buf(drvdata);
- drvdata->reading = false;
- spin_unlock_irqrestore(&drvdata->spinlock, flags);
- return 0;
+}
int tmc_read_prepare_etb(struct tmc_drvdata *drvdata) { enum tmc_mode mode; @@ -675,6 +754,9 @@ int tmc_read_prepare_etb(struct tmc_drvdata
*drvdata)
drvdata->config_type != TMC_CONFIG_TYPE_ETF)) return -EINVAL;
- if (drvdata->mode == CS_MODE_READ_PREVBOOT)
return tmc_etb_prepare_prevboot(drvdata);
Can you move this inside the spinlock below, and remove the spinlock from tmc_etb_prepare_prevboot()? Otherwise it looks a bit like it's a special case that is supposed to happen outside of the lock until you go and look inside the function, but then all the locking code is just repeated.
I think the same applies for the ETR one.
Ack.
spin_lock_irqsave(&drvdata->spinlock, flags);
if (drvdata->reading) { @@ -724,6 +806,9 @@ int tmc_read_unprepare_etb(struct tmc_drvdata
*drvdata)
drvdata->config_type != TMC_CONFIG_TYPE_ETF)) return -EINVAL;
if (drvdata->mode == CS_MODE_READ_PREVBOOT)
return tmc_etb_unprepare_prevboot(drvdata);
spin_lock_irqsave(&drvdata->spinlock, flags);
/* Re-enable the TMC if need be */
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index a99b3e9fb9ec..6e8d4069a3fa 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1159,7 +1159,12 @@ ssize_t tmc_etr_get_sysfs_trace(struct tmc_drvdata *drvdata, { s64 offset; ssize_t actual = len;
- struct etr_buf *etr_buf = drvdata->sysfs_buf;
struct etr_buf *etr_buf;
if (drvdata->mode == CS_MODE_READ_PREVBOOT)
etr_buf = drvdata->prevboot_buf;
else
etr_buf = drvdata->sysfs_buf;
if (pos + actual > etr_buf->len) actual = etr_buf->len - pos;
@@ -1843,6 +1848,147 @@ const struct coresight_ops tmc_etr_cs_ops = { .panic_ops = &tmc_etr_sync_ops, };
+static int tmc_etr_setup_prevboot_buf(struct tmc_drvdata *drvdata) {
- int rc = 0;
- u64 trace_addr;
- struct etr_buf *etr_buf;
- struct etr_flat_buf *resrv_buf;
- struct tmc_register_snapshot *reg_ptr;
- etr_buf = kzalloc(sizeof(*etr_buf), GFP_KERNEL);
- if (!etr_buf) {
rc = -ENOMEM;
goto out;
- }
- etr_buf->size = drvdata->size;
- resrv_buf = kzalloc(sizeof(*resrv_buf), GFP_KERNEL);
- if (!resrv_buf) {
rc = -ENOMEM;
goto rmem_err;
- }
- reg_ptr = drvdata->metadata.vaddr;
- trace_addr = reg_ptr->trc_addr | ((u64)reg_ptr->trc_addrhi << 32);
- resrv_buf->vaddr = memremap(trace_addr, reg_ptr->size * 4,
MEMREMAP_WC);
- if (IS_ERR(drvdata->buf)) {
rc = -ENOMEM;
goto map_err;
- }
- resrv_buf->size = etr_buf->size;
- resrv_buf->dev = &drvdata->csdev->dev;
- etr_buf->hwaddr = trace_addr;
- etr_buf->mode = ETR_MODE_RESRV;
- etr_buf->private = resrv_buf;
- etr_buf->ops = etr_buf_ops[ETR_MODE_RESRV];
- drvdata->prevboot_buf = etr_buf;
- return 0;
+map_err:
- kfree(resrv_buf);
+rmem_err:
- kfree(etr_buf);
+out:
- return rc;
+}
+static int tmc_etr_sync_prevboot_buf(struct tmc_drvdata *drvdata) {
- u32 status;
- u64 rrp, rwp, dba;
- struct tmc_register_snapshot *reg_ptr;
- struct etr_buf *etr_buf = drvdata->prevboot_buf;
- reg_ptr = drvdata->metadata.vaddr;
- rrp = reg_ptr->rrp | ((u64)reg_ptr->rrphi << 32);
- rwp = reg_ptr->rwp | ((u64)reg_ptr->rwphi << 32);
- dba = reg_ptr->dba | ((u64)reg_ptr->dbahi << 32);
- status = reg_ptr->sts;
- etr_buf->full = !!(status & TMC_STS_FULL);
- /* Sync the buffer pointers */
- etr_buf->offset = rrp - dba;
- if (etr_buf->full)
etr_buf->len = etr_buf->size;
- else
etr_buf->len = rwp - rrp;
- return 0;
+}
+static void tmc_etr_free_prevboot_buf(struct tmc_drvdata *drvdata) {
- void *buf = drvdata->prevboot_buf;
- if (!buf)
return;
- memunmap(buf);
- drvdata->prevboot_buf = NULL;
+}
+static int tmc_etr_prepare_prevboot(struct tmc_drvdata *drvdata) {
- struct tmc_register_snapshot *reg_ptr;
- unsigned long flags;
- int ret = 0;
- spin_lock_irqsave(&drvdata->spinlock, flags);
- if (drvdata->reading) {
ret = -EBUSY;
goto out;
- }
- if (!drvdata->metadata.vaddr) {
ret = -ENOMEM;
goto out;
- }
- reg_ptr = drvdata->metadata.vaddr;
- if (!reg_ptr->valid) {
dev_err(&drvdata->csdev->dev,
"Invalid metadata captured from previous boot\n");
ret = -EINVAL;
goto out;
- }
- ret = tmc_etr_setup_prevboot_buf(drvdata);
- if (ret)
goto out;
- tmc_etr_sync_prevboot_buf(drvdata);
- if (reg_ptr->sts & 0x1)
coresight_insert_barrier_packet(drvdata->buf);
- drvdata->reading = true;
+out:
- spin_unlock_irqrestore(&drvdata->spinlock, flags);
- return ret;
+}
There seems to be mainly duplicated code between tmc_etr_prepare_prevboot() and tmc_etb_prepare_prevboot(). Is there any way of factoring out the common stuff and including some specialised callback in the middle like:
const struct coresight_ops tmc_etf_cs_ops = { .sink_ops = &tmc_etf_sink_ops, .link_ops = &tmc_etf_link_ops, .panic_ops = &tmc_etf_sync_ops,
.prepare_prevboot = &tmc_etb_prepare_prevboot,
};
Okay, will do.
+static int tmc_etr_unprepare_prevboot(struct tmc_drvdata *drvdata) {
- unsigned long flags;
- spin_lock_irqsave(&drvdata->spinlock, flags);
- tmc_etr_free_prevboot_buf(drvdata);
- drvdata->reading = false;
- spin_unlock_irqrestore(&drvdata->spinlock, flags);
- return 0;
+}
int tmc_read_prepare_etr(struct tmc_drvdata *drvdata) { int ret = 0; @@ -1852,6 +1998,9 @@ int tmc_read_prepare_etr(struct tmc_drvdata
*drvdata)
if (WARN_ON_ONCE(drvdata->config_type !=
TMC_CONFIG_TYPE_ETR))
return -EINVAL;
- if (drvdata->mode == CS_MODE_READ_PREVBOOT)
return tmc_etr_prepare_prevboot(drvdata);
- spin_lock_irqsave(&drvdata->spinlock, flags); if (drvdata->reading) { ret = -EBUSY;
@@ -1888,6 +2037,9 @@ int tmc_read_unprepare_etr(struct tmc_drvdata
*drvdata)
if (WARN_ON_ONCE(drvdata->config_type !=
TMC_CONFIG_TYPE_ETR))
return -EINVAL;
if (drvdata->mode == CS_MODE_READ_PREVBOOT)
return tmc_etr_unprepare_prevboot(drvdata);
spin_lock_irqsave(&drvdata->spinlock, flags);
/* RE-enable the TMC if need be */
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index 8ebf624b26d9..64d80d95e5eb 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -217,6 +217,7 @@ struct tmc_resrv_buf {
- @idr_mutex: Access serialisation for idr.
- @sysfs_buf: SYSFS buffer for ETR.
- @perf_buf: PERF buffer for ETR.
*/
- @prevboot_buf: Previous boot buffer for ETR
- @resrv_buf: Reserved Memory for trace data buffer. Used by ETR/ETF.
- @metadata: Reserved memory for metadata. Used by ETR/ETF.
@@ -243,6 +244,7 @@ struct tmc_drvdata { struct mutex idr_mutex; struct etr_buf *sysfs_buf; struct etr_buf *perf_buf;
- struct etr_buf *prevboot_buf;
Why does ETR have this new member, but ETB uses the existing *buf for the previous buffer?
Since there was a separate etr buf for each mode, thought it was more inline with the current style of implementation. Basically the setting up of buffer is different for PREVBOOT mode. Ie. Initialization of sysf_buf involves, memory allocation, dma mapping etc while for the prevboot_buf, we just need to do the memremap.
Will add necessary comments for this.
Maybe a comment here or where it is used would help.
struct tmc_resrv_buf resrv_buf; struct tmc_resrv_buf metadata; };