Re: [RFC PATCH v2 5/7] coresight: tmc: Add support for reading tracedata from previous boot

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

  1. cd /sys/bus/coresight/devices/tmc_etfXX/

  2. Change mode to READ_PREVBOOT

     #echo 1 > read_prevboot

  3. Dump trace buffer data to a file,

     #dd if=/dev/tmc_etrXX of=~/cstrace.bin

  4. 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

}; /** 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.

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.

+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.

• 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.

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, };

+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?

Maybe a comment here or where it is used would help.

struct tmc_resrv_buf resrv_buf; struct tmc_resrv_buf metadata; };