Hi Linu

On 05/06/2024 09:17, Linu Cherian wrote:

Add support to use reserved memory for coresight ETR trace buffer.

Introduce a new ETR buffer mode called ETR_MODE_RESRV, which becomes available when ETR device tree node is supplied with a valid reserved memory region.

ETR_MODE_RESRV can be selected only by explicit user request.

$ echo resrv >/sys/bus/coresight/devices/tmc_etr<N>/buf_mode_preferred

Signed-off-by: Anil Kumar Reddy areddy3@marvell.com Signed-off-by: Linu Cherian lcherian@marvell.com Reviewed-by: James Clark james.clark@arm.com

Some minor comments below.

---

Changelog from v8: Added Reviewed-by tag.

.../hwtracing/coresight/coresight-tmc-core.c | 47 +++++++++++ .../hwtracing/coresight/coresight-tmc-etr.c | 79 +++++++++++++++++++ drivers/hwtracing/coresight/coresight-tmc.h | 27 +++++++ 3 files changed, 153 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index 741ce1748e75..6beb69d74d0a 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -23,6 +23,7 @@ #include <linux/spinlock.h> #include <linux/pm_runtime.h> #include <linux/of.h> +#include <linux/of_address.h> #include <linux/coresight.h> #include <linux/amba/bus.h> #include <linux/platform_device.h> @@ -400,6 +401,50 @@ static inline bool tmc_etr_has_non_secure_access(struct tmc_drvdata *drvdata) static const struct amba_id tmc_ids[]; +static struct device_node *tmc_get_region_byname(struct device_node *node,

• 				 char *name)
  

+{

• int index;
• index = of_property_match_string(node, "memory-region-names", name);
• if (index < 0)

• return ERR_PTR(-ENODEV);
  

• return of_parse_phandle(node, "memory-region", index);

+}

+static void tmc_get_reserved_region(struct device *parent) +{

• struct tmc_drvdata *drvdata = dev_get_drvdata(parent);
• struct device_node *node;
• struct resource res;
• int rc;
• node = tmc_get_region_byname(parent->of_node, "tracedata");
• if (IS_ERR_OR_NULL(node)) {

• dev_dbg(parent, "No reserved trace buffer specified\n");
  

• return;
  

• }
• rc = of_address_to_resource(node, 0, &res);

This sequence is repeated again in the metadat area, where :

1. We get region by name. 2. Read the resource details.

Could we not make "the helper" do both and move the "of_node_put()" inside the helper ?

static int of_tmc_get_reserved_resource_by_name(struct device *dev, const char *name, struct resource *res) {

• int index, rc = -ENODEV;
• struct device_node *node;

if (!is_of_node(dev_fwnode(dev->fwnode))) return -ENODEV;

• index = of_property_match_string(dev->of_node,

"memory-region-names", name);

• if (index < 0)

• return rc;
  

• node = of_parse_phandle(node, "memory-region", index);

if (!node) return rc;

if (!of_address_to_resource(node, 0, res) && res->start != 0 && resource_size(res) != 0) rc = 0; of_node_put(node); return rc; }

• of_node_put(node);
• if (rc || res.start == 0 || resource_size(&res) == 0) {

• dev_err(parent, "Reserved trace buffer memory is invalid\n");
  

• return;
  

• }

and the caller does :

if (of_tmc_get_reserved_area_by_name(parent, "tracedata", &res)) return;

• drvdata->crash_tbuf.vaddr = memremap(res.start,

• 				resource_size(&res),
  

• 				MEMREMAP_WC);
  

• if (IS_ERR_OR_NULL(drvdata->crash_tbuf.vaddr)) {

• dev_err(parent, "Reserved trace buffer mapping failed\n");
  

• return;
  

• }
• drvdata->crash_tbuf.paddr = res.start;
• drvdata->crash_tbuf.size = resource_size(&res);

+}

• /* Detect and initialise the capabilities of a TMC ETR */ static int tmc_etr_setup_caps(struct device *parent, u32 devid, struct csdev_access *access)

@@ -510,6 +555,8 @@ static int __tmc_probe(struct device *dev, struct resource *res) drvdata->size = readl_relaxed(drvdata->base + TMC_RSZ) * 4; }

• tmc_get_reserved_region(dev);
• desc.dev = dev;

switch (drvdata->config_type) { diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index e75428fa1592..041c428dd7cd 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -30,6 +30,7 @@ struct etr_buf_hw { bool has_iommu; bool has_etr_sg; bool has_catu;

• bool has_resrv; };

/* @@ -694,6 +695,75 @@ static const struct etr_buf_operations etr_flat_buf_ops = { .get_data = tmc_etr_get_data_flat_buf, }; +/*

• • tmc_etr_alloc_resrv_buf: Allocate a contiguous DMA buffer from reserved region.
• */

+static int tmc_etr_alloc_resrv_buf(struct tmc_drvdata *drvdata,

• 		  struct etr_buf *etr_buf, int node,
  

• 		  void **pages)
  

+{

• struct etr_flat_buf *resrv_buf;
• struct device *real_dev = drvdata->csdev->dev.parent;
• /* We cannot reuse existing pages for resrv buf */
• if (pages)

• return -EINVAL;
  

• resrv_buf = kzalloc(sizeof(*resrv_buf), GFP_KERNEL);
• if (!resrv_buf)

• return -ENOMEM;
  

• resrv_buf->daddr = dma_map_resource(real_dev, drvdata->crash_tbuf.paddr,

• 			   drvdata->crash_tbuf.size,
  

• 			   DMA_FROM_DEVICE, 0);
  

• if (dma_mapping_error(real_dev, resrv_buf->daddr)) {

• dev_err(real_dev, "failed to map source buffer address\n");
  

• kfree(resrv_buf);
  

• return -ENOMEM;
  

• }
• resrv_buf->vaddr = drvdata->crash_tbuf.vaddr;
• resrv_buf->size = etr_buf->size = drvdata->crash_tbuf.size;
• resrv_buf->dev = &drvdata->csdev->dev;
• etr_buf->hwaddr = resrv_buf->daddr;
• etr_buf->mode = ETR_MODE_RESRV;
• etr_buf->private = resrv_buf;
• return 0;

+}

+static void tmc_etr_free_resrv_buf(struct etr_buf *etr_buf) +{

• struct etr_flat_buf *resrv_buf = etr_buf->private;
• if (resrv_buf && resrv_buf->daddr) {

• struct device *real_dev = resrv_buf->dev->parent;
  

• dma_unmap_resource(real_dev, resrv_buf->daddr,
  

• 		resrv_buf->size, DMA_FROM_DEVICE, 0);
  

• }
• kfree(resrv_buf);

+}

+static void tmc_etr_sync_resrv_buf(struct etr_buf *etr_buf, u64 rrp, u64 rwp) +{

• /*

• * Adjust the buffer to point to the beginning of the trace data
  

• * and update the available trace data.
  

• */
  

• etr_buf->offset = rrp - etr_buf->hwaddr;
• if (etr_buf->full)

• etr_buf->len = etr_buf->size;
  

• else

• etr_buf->len = rwp - rrp;
  

+}

+static const struct etr_buf_operations etr_resrv_buf_ops = {

• .alloc = tmc_etr_alloc_resrv_buf,
• .free = tmc_etr_free_resrv_buf,
• .sync = tmc_etr_sync_resrv_buf,
• .get_data = tmc_etr_get_data_flat_buf,

+};

• /*
  • tmc_etr_alloc_sg_buf: Allocate an SG buf @etr_buf. Setup the parameters
  • appropriately.

@@ -800,6 +870,7 @@ static const struct etr_buf_operations *etr_buf_ops[] = { [ETR_MODE_FLAT] = &etr_flat_buf_ops, [ETR_MODE_ETR_SG] = &etr_sg_buf_ops, [ETR_MODE_CATU] = NULL,

• [ETR_MODE_RESRV] = &etr_resrv_buf_ops };

void tmc_etr_set_catu_ops(const struct etr_buf_operations *catu) @@ -825,6 +896,7 @@ static inline int tmc_etr_mode_alloc_buf(int mode, case ETR_MODE_FLAT: case ETR_MODE_ETR_SG: case ETR_MODE_CATU:

• case ETR_MODE_RESRV: if (etr_buf_ops[mode] && etr_buf_ops[mode]->alloc) rc = etr_buf_ops[mode]->alloc(drvdata, etr_buf, node, pages);

@@ -843,6 +915,7 @@ static void get_etr_buf_hw(struct device *dev, struct etr_buf_hw *buf_hw) buf_hw->has_iommu = iommu_get_domain_for_dev(dev->parent); buf_hw->has_etr_sg = tmc_etr_has_cap(drvdata, TMC_ETR_SG); buf_hw->has_catu = !!tmc_etr_get_catu_device(drvdata);

• buf_hw->has_resrv = is_tmc_reserved_region_valid(dev->parent);

Minor nit: In line with the other helpers, please could we rename this to :

tmc_has_reserved_buffer(drvdata) ?

We already have access to drvdata and we can pass that directly instead of passing the dev->parent and then fetch drvdata again.

} static bool etr_can_use_flat_mode(struct etr_buf_hw *buf_hw, ssize_t etr_buf_size) @@ -1830,6 +1903,7 @@ static const char *const buf_modes_str[] = { [ETR_MODE_FLAT] = "flat", [ETR_MODE_ETR_SG] = "tmc-sg", [ETR_MODE_CATU] = "catu",

• [ETR_MODE_RESRV] = "resrv", [ETR_MODE_AUTO] = "auto", };

@@ -1848,6 +1922,9 @@ static ssize_t buf_modes_available_show(struct device *dev, if (buf_hw.has_catu) size += sysfs_emit_at(buf, size, "%s ", buf_modes_str[ETR_MODE_CATU]);

• if (buf_hw.has_resrv)

• size += sysfs_emit_at(buf, size, "%s ", buf_modes_str[ETR_MODE_RESRV]);
  

• size += sysfs_emit_at(buf, size, "\n"); return size; }

@@ -1875,6 +1952,8 @@ static ssize_t buf_mode_preferred_store(struct device *dev, drvdata->etr_mode = ETR_MODE_ETR_SG; else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_CATU]) && buf_hw.has_catu) drvdata->etr_mode = ETR_MODE_CATU;

• else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_RESRV]) && buf_hw.has_resrv)

• drvdata->etr_mode = ETR_MODE_RESRV;
  

  else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_AUTO])) drvdata->etr_mode = ETR_MODE_AUTO; else

diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index c77763b49de0..c23dc9917ab9 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -135,6 +135,7 @@ enum etr_mode { ETR_MODE_FLAT, /* Uses contiguous flat buffer */ ETR_MODE_ETR_SG, /* Uses in-built TMC ETR SG mechanism */ ETR_MODE_CATU, /* Use SG mechanism in CATU */

• ETR_MODE_RESRV, /* Use reserved region contiguous buffer */ ETR_MODE_AUTO, /* Use the default mechanism */ };

@@ -164,6 +165,17 @@ struct etr_buf { void *private; }; +/**

• • @paddr : Start address of reserved memory region.
• • @vaddr : Corresponding CPU virtual address.
• • @size : Size of reserved memory region.
• */

+struct tmc_resrv_buf {

• phys_addr_t paddr;
• void *vaddr;
• size_t size;

+};

• /**
  • struct tmc_drvdata - specifics associated to an TMC component
  • @pclk: APB clock if present, otherwise NULL

@@ -188,6 +200,10 @@ struct etr_buf {

• @idr_mutex: Access serialisation for idr.
• @sysfs_buf: SYSFS buffer for ETR.
• @perf_buf: PERF buffer for ETR.

• • @crash_tbuf: Used by ETR as hardware trace buffer and for trace data

• • retention (after crash) only when ETR_MODE_RESRV buffer
    

• • mode is enabled. Used by ETF for trace data retention
    

• • (after crash) by default.
    

Minor nit: This need not be called crash_tbuf, could instead be resrv_buf as we use it everywhere.

Suzuki

*/ struct tmc_drvdata { struct clk *pclk; @@ -213,6 +229,7 @@ struct tmc_drvdata { struct mutex idr_mutex; struct etr_buf *sysfs_buf; struct etr_buf *perf_buf;

• struct tmc_resrv_buf crash_tbuf; };

struct etr_buf_operations { @@ -330,6 +347,16 @@ tmc_sg_table_buf_size(struct tmc_sg_table *sg_table) return (unsigned long)sg_table->data_pages.nr_pages << PAGE_SHIFT; } +static inline bool is_tmc_reserved_region_valid(struct device *dev) +{

• struct tmc_drvdata *drvdata = dev_get_drvdata(dev);
• if (drvdata->crash_tbuf.paddr &&

• drvdata->crash_tbuf.size)
  

• return true;
  

• return false;

+}

• struct coresight_device *tmc_etr_get_catu_device(struct tmc_drvdata *drvdata);

void tmc_etr_set_catu_ops(const struct etr_buf_operations *catu);

On 2024-06-05 at 21:39:17, James Clark (james.clark@arm.com) wrote:

On 05/06/2024 09:17, Linu Cherian wrote:

...

Panic callback handlers allows coresight device drivers to sync relevant trace data and trace metadata to reserved memory regions so that they can be retrieved later in the subsequent boot or in the crashdump kernel.

Signed-off-by: Linu Cherian lcherian@marvell.com Reviewed-by: James Clark james.clark@arm.com

---

Changelog from v8: Added Reviewed-by tag.

drivers/hwtracing/coresight/coresight-core.c | 37 ++++++++++++++++++++ include/linux/coresight.h | 12 +++++++ 2 files changed, 49 insertions(+)

[...]

...

static int __init coresight_init(void) { int ret; @@ -1377,6 +1408,10 @@ static int __init coresight_init(void) if (ret) goto exit_bus_unregister;

• /* Register function to be called for panic */
• ret = atomic_notifier_chain_register(&panic_notifier_list,

• 			     &coresight_notifier);
  

ret isn't checked here

Ack.

---

linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On 05/06/2024 09:17, Linu Cherian wrote:

• Get reserved region from device tree node for metadata
• Define metadata format for TMC
• Add TMC ETR panic sync handler that syncs register snapshot to metadata region
• Add TMC ETF panic sync handler that syncs register snapshot to metadata region and internal SRAM to reserved trace buffer region.

Signed-off-by: Linu Cherian lcherian@marvell.com Reviewed-by: James Clark james.clark@arm.com

---

Changelog from v8: Added Reviewed-by tag.

.../hwtracing/coresight/coresight-tmc-core.c | 25 +++++++ .../hwtracing/coresight/coresight-tmc-etf.c | 72 +++++++++++++++++++ .../hwtracing/coresight/coresight-tmc-etr.c | 70 ++++++++++++++++++ drivers/hwtracing/coresight/coresight-tmc.h | 45 +++++++++++- 4 files changed, 211 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index 6beb69d74d0a..daad08bc693d 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -443,6 +443,31 @@ static void tmc_get_reserved_region(struct device *parent) drvdata->crash_tbuf.paddr = res.start; drvdata->crash_tbuf.size = resource_size(&res);

• /* Metadata region */
• node = tmc_get_region_byname(parent->of_node, "metadata");
• if (IS_ERR_OR_NULL(node)) {

• dev_dbg(parent, "No metadata memory-region specified\n");
  

• return;
  

• }
• rc = of_address_to_resource(node, 0, &res);
• of_node_put(node);
• if (rc || res.start == 0 || resource_size(&res) == 0) {

• dev_err(parent, "Metadata memory is invalid\n");
  

• return;
  

• }
• drvdata->crash_mdata.vaddr = memremap(res.start,

• 			       resource_size(&res),
  

• 			       MEMREMAP_WC);
  

• if (IS_ERR_OR_NULL(drvdata->crash_mdata.vaddr)) {

• dev_err(parent, "Metadata memory mapping failed\n");
  

• return;
  

• }
• drvdata->crash_mdata.paddr = res.start;
• drvdata->crash_mdata.size = resource_size(&res); }

/* Detect and initialise the capabilities of a TMC ETR */ diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index d4f641cd9de6..f9569585e9f8 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -590,6 +590,73 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev, return to_read; } +static int tmc_panic_sync_etf(struct coresight_device *csdev) +{

• u32 val;
• struct csdev_access *csa;
• struct tmc_crash_metadata *mdata;
• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• csa = &drvdata->csdev->access;
• /* Make sure we have valid reserved memory */
• if (!is_tmc_reserved_region_valid(csdev->dev.parent))

• return 0;
  

• mdata = (struct tmc_crash_metadata *)drvdata->crash_mdata.vaddr;
• mdata->valid = false;
• CS_UNLOCK(drvdata->base);
• /* Proceed only if ETF is enabled or configured as sink */
• val = readl(drvdata->base + TMC_CTL);
• if (!(val & TMC_CTL_CAPT_EN))

• goto out;
  

• val = readl(drvdata->base + TMC_MODE);
• if (val != TMC_MODE_CIRCULAR_BUFFER)

• goto out;
  

• val = readl(drvdata->base + TMC_FFSR);
• /* Do manual flush and stop only if its not auto-stopped */
• if (!(val & TMC_FFSR_FT_STOPPED)) {

• dev_info(&csdev->dev,
  

• 	 "%s: Triggering manual flush\n", __func__);
  

• tmc_flush_and_stop(drvdata);
  

• } else

• tmc_wait_for_tmcready(drvdata);
  

• /* Sync registers from hardware to metadata region */
• mdata->sts = csdev_access_relaxed_read32(csa, TMC_STS);
• mdata->trc_paddr = drvdata->crash_tbuf.paddr;
• /* Sync Internal SRAM to reserved trace buffer region */
• drvdata->buf = drvdata->crash_tbuf.vaddr;
• tmc_etb_dump_hw(drvdata);
• /* Store as per RSZ register convention */
• mdata->size = drvdata->len >> 2;
• /*

• * Make sure all previous writes are completed,
  

• * before we mark valid
  

• */
  

• dsb(sy);
• mdata->valid = true;
• /*

• * Below order need to maintained, since crc of metadata
  

• * is dependent on first
  

• */
  

• mdata->crc32_tdata = find_crash_tracedata_crc(drvdata, mdata);
• mdata->crc32_mdata = find_crash_metadata_crc(mdata);
• tmc_disable_hw(drvdata);
• dev_info(&csdev->dev, "%s: success\n", __func__);

+out:

• CS_UNLOCK(drvdata->base);
• return 0;

+}

• static const struct coresight_ops_sink tmc_etf_sink_ops = { .enable = tmc_enable_etf_sink, .disable = tmc_disable_etf_sink,

@@ -603,6 +670,10 @@ static const struct coresight_ops_link tmc_etf_link_ops = { .disable = tmc_disable_etf_link, }; +static const struct coresight_ops_panic tmc_etf_sync_ops = {

• .sync = tmc_panic_sync_etf,

+};

• const struct coresight_ops tmc_etb_cs_ops = { .sink_ops = &tmc_etf_sink_ops, };

@@ -610,6 +681,7 @@ const struct coresight_ops tmc_etb_cs_ops = { 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, };

int tmc_read_prepare_etb(struct tmc_drvdata *drvdata) diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index 041c428dd7cd..be1079e8fd64 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1813,6 +1813,71 @@ static int tmc_disable_etr_sink(struct coresight_device *csdev) return 0; } +static int tmc_panic_sync_etr(struct coresight_device *csdev) +{

• u32 val;
• struct csdev_access *csa;
• struct tmc_crash_metadata *mdata;
• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• csa = &drvdata->csdev->access;
• if (!drvdata->etr_buf)

• return 0;
  

• /* Being in RESRV mode implies valid reserved memory as well */
• if (drvdata->etr_buf->mode != ETR_MODE_RESRV)

• return 0;
  

• mdata = (struct tmc_crash_metadata *)drvdata->crash_mdata.vaddr;
• mdata->valid = false;
• CS_UNLOCK(drvdata->base);
• /* Proceed only if ETR is enabled */
• val = readl(drvdata->base + TMC_CTL);
• if (!(val & TMC_CTL_CAPT_EN))

• goto out;
  

• val = readl(drvdata->base + TMC_FFSR);
• /* Do manual flush and stop only if its not auto-stopped */
• if (!(val & TMC_FFSR_FT_STOPPED)) {

• dev_info(&csdev->dev,
  

• 	 "%s: Triggering manual flush\n", __func__);
  

• tmc_flush_and_stop(drvdata);
  

• } else

• tmc_wait_for_tmcready(drvdata);
  

• /* Sync registers from hardware to metadata region */
• mdata->size = csdev_access_relaxed_read32(csa, TMC_RSZ);
• mdata->sts = csdev_access_relaxed_read32(csa, TMC_STS);
• mdata->rrp = tmc_read_rrp(drvdata);
• mdata->rwp = tmc_read_rwp(drvdata);
• mdata->dba = tmc_read_dba(drvdata);
• mdata->trc_paddr = drvdata->crash_tbuf.paddr;
• /*

• * Make sure all previous writes are completed,
  

• * before we mark valid
  

• */
  

• dsb(sy);
• mdata->valid = true;
• /*

• * Below order need to maintained, since crc of metadata
  

• * is dependent on first
  

• */
  

• mdata->crc32_tdata = find_crash_tracedata_crc(drvdata, mdata);
• mdata->crc32_mdata = find_crash_metadata_crc(mdata);
• tmc_disable_hw(drvdata);
• dev_info(&csdev->dev, "%s: success\n", __func__);

+out:

• CS_UNLOCK(drvdata->base);
• return 0;

+}

• static const struct coresight_ops_sink tmc_etr_sink_ops = { .enable = tmc_enable_etr_sink, .disable = tmc_disable_etr_sink,

@@ -1821,8 +1886,13 @@ static const struct coresight_ops_sink tmc_etr_sink_ops = { .free_buffer = tmc_free_etr_buffer, }; +static const struct coresight_ops_panic tmc_etr_sync_ops = {

• .sync = tmc_panic_sync_etr,

+};

• const struct coresight_ops tmc_etr_cs_ops = { .sink_ops = &tmc_etr_sink_ops,
• .panic_ops = &tmc_etr_sync_ops, };

int tmc_read_prepare_etr(struct tmc_drvdata *drvdata) diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index c23dc9917ab9..35beee53584a 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -12,6 +12,7 @@ #include <linux/miscdevice.h> #include <linux/mutex.h> #include <linux/refcount.h> +#include <linux/crc32.h> #define TMC_RSZ 0x004 #define TMC_STS 0x00c @@ -76,6 +77,9 @@ #define TMC_AXICTL_AXCACHE_OS (0xf << 2) #define TMC_AXICTL_ARCACHE_OS (0xf << 16) +/* TMC_FFSR - 0x300 */ +#define TMC_FFSR_FT_STOPPED BIT(1)

• /* TMC_FFCR - 0x304 */ #define TMC_FFCR_FLUSHMAN_BIT 6 #define TMC_FFCR_EN_FMT BIT(0)

@@ -131,6 +135,21 @@ enum tmc_mem_intf_width { #define CORESIGHT_SOC_600_ETR_CAPS \ (TMC_ETR_SAVE_RESTORE | TMC_ETR_AXI_ARCACHE)

+/* TMC metadata region for ETR and ETF configurations */

I strongly think we should version the data layout to handle the future changes better (if at all).

+struct tmc_crash_metadata {

• uint32_t crc32_mdata; /* crc of metadata */
• uint32_t crc32_tdata; /* crc of tracedata */

uint32_t version; /* Version of the structure = 1 */

• uint32_t valid; /* Indicate if this ETF/ETR was enabled */
• uint32_t size; /* Ram Size register */

Please save the size in bytes and also rename it :

uint32_t trace_size; /* Trace size in Bytes */

• uint32_t sts; /* Status register */
• uint32_t reserved32[3];
• uint64_t rrp; /* Ram Read pointer register */
• uint64_t rwp; /* Ram Write pointer register */

• uint64_t dba; /* Data buffer address register */

Is this field useful ? And we store RRP/RWP relative to the DBA ? Could we instead :

1. Drop DBA 2. Store RRP and RWP as offsets from DBA. Or even convert them to the actual PADDRs relative to the trc_paddr.

DBA could be a "DMA" Address and not necessarily the PA Address. We already have the trc_paddr below. (And for ETF, we already copy the buffer to the reserved buffer). So all the user needs to know is where the pointers are within the buffer. Having them relative to the "actual" location of the buffer is much useful than basing it on some unusable base address.

• uint64_t trc_paddr; /* Phys address of trace buffer */

s/trc/trace

Move RRP and RWP, after the above field.

For the sake of completeness, you are also missing :

1) FFCR register => That tells you whether the Formatting was enabled or not (among other things) ? Though we always enable it, its good to capture it, if we ever decide to turn off the formatting.

2) MODE => Which mode was selected. Again, CIRCULAR_BUFFER for now, but lets seal it for the future, so that you can infer the trace buffer correctly with RRP/RWP.

3) And may be FFSR, just in case the flush never completed and the data is not reliable ?

• uint64_t reserved64[3];

+};

• enum etr_mode { ETR_MODE_FLAT, /* Uses contiguous flat buffer */ ETR_MODE_ETR_SG, /* Uses in-built TMC ETR SG mechanism */

@@ -204,6 +223,8 @@ struct tmc_resrv_buf {

• retention (after crash) only when ETR_MODE_RESRV buffer
  

• mode is enabled. Used by ETF for trace data retention
  

• (after crash) by default.
  

• • @crash_mdata: Reserved memory for storing tmc crash metadata.

• •  Used by ETR/ETF.
    

  */ struct tmc_drvdata { struct clk *pclk;

@@ -230,6 +251,7 @@ struct tmc_drvdata { struct etr_buf *sysfs_buf; struct etr_buf *perf_buf; struct tmc_resrv_buf crash_tbuf;

• struct tmc_resrv_buf crash_mdata; };

struct etr_buf_operations { @@ -352,11 +374,32 @@ static inline bool is_tmc_reserved_region_valid(struct device *dev) struct tmc_drvdata *drvdata = dev_get_drvdata(dev); if (drvdata->crash_tbuf.paddr &&

• drvdata->crash_tbuf.size)
  

• drvdata->crash_tbuf.size &&
  

• drvdata->crash_mdata.paddr &&
  

• drvdata->crash_mdata.size)
  

Why do we need to tie the "reserved" region to metdata region availability ? It is perfectly possible for another usecase to dedicate a buffer for trace and use it without metadata ?

Suzuki

return true;

return false; } +static inline uint32_t find_crash_metadata_crc(struct tmc_crash_metadata *md) +{

• unsigned long crc_size;
• crc_size = sizeof(struct tmc_crash_metadata) -

• offsetof(struct tmc_crash_metadata, crc32_tdata);
  

• return crc32_le(0, (void *)&md->crc32_tdata, crc_size);

+}

+static inline uint32_t find_crash_tracedata_crc(struct tmc_drvdata *drvdata,

• 				struct tmc_crash_metadata *md)
  

+{

• unsigned long crc_size;
• /* Take CRC of configured buffer size to keep it simple */
• crc_size = md->size << 2;
• return crc32_le(0, (void *)drvdata->crash_tbuf.vaddr, crc_size);

+}

• struct coresight_device *tmc_etr_get_catu_device(struct tmc_drvdata *drvdata);

void tmc_etr_set_catu_ops(const struct etr_buf_operations *catu);

On 20/06/2024 05:10, Linu Cherian wrote:

On 2024-06-10 at 18:32:33, Suzuki K Poulose (suzuki.poulose@arm.com) wrote:

...

On 05/06/2024 09:17, Linu Cherian wrote:

...

• Get reserved region from device tree node for metadata
• Define metadata format for TMC
• Add TMC ETR panic sync handler that syncs register snapshot to metadata region
• Add TMC ETF panic sync handler that syncs register snapshot to metadata region and internal SRAM to reserved trace buffer region.

Signed-off-by: Linu Cherian lcherian@marvell.com Reviewed-by: James Clark james.clark@arm.com

---

Changelog from v8: Added Reviewed-by tag.

.../hwtracing/coresight/coresight-tmc-core.c | 25 +++++++ .../hwtracing/coresight/coresight-tmc-etf.c | 72 +++++++++++++++++++ .../hwtracing/coresight/coresight-tmc-etr.c | 70 ++++++++++++++++++ drivers/hwtracing/coresight/coresight-tmc.h | 45 +++++++++++- 4 files changed, 211 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index 6beb69d74d0a..daad08bc693d 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -443,6 +443,31 @@ static void tmc_get_reserved_region(struct device *parent) drvdata->crash_tbuf.paddr = res.start; drvdata->crash_tbuf.size = resource_size(&res);

• /* Metadata region */
• node = tmc_get_region_byname(parent->of_node, "metadata");
• if (IS_ERR_OR_NULL(node)) {

• dev_dbg(parent, "No metadata memory-region specified\n");
  

• return;
  

• }
• rc = of_address_to_resource(node, 0, &res);
• of_node_put(node);
• if (rc || res.start == 0 || resource_size(&res) == 0) {

• dev_err(parent, "Metadata memory is invalid\n");
  

• return;
  

• }
• drvdata->crash_mdata.vaddr = memremap(res.start,

• 			       resource_size(&res),
  

• 			       MEMREMAP_WC);
  

• if (IS_ERR_OR_NULL(drvdata->crash_mdata.vaddr)) {

• dev_err(parent, "Metadata memory mapping failed\n");
  

• return;
  

• }
• drvdata->crash_mdata.paddr = res.start;
• drvdata->crash_mdata.size = resource_size(&res); } /* Detect and initialise the capabilities of a TMC ETR */

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index d4f641cd9de6..f9569585e9f8 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -590,6 +590,73 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev, return to_read; } +static int tmc_panic_sync_etf(struct coresight_device *csdev) +{

• u32 val;
• struct csdev_access *csa;
• struct tmc_crash_metadata *mdata;
• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• csa = &drvdata->csdev->access;
• /* Make sure we have valid reserved memory */
• if (!is_tmc_reserved_region_valid(csdev->dev.parent))

• return 0;
  

• mdata = (struct tmc_crash_metadata *)drvdata->crash_mdata.vaddr;
• mdata->valid = false;
• CS_UNLOCK(drvdata->base);
• /* Proceed only if ETF is enabled or configured as sink */
• val = readl(drvdata->base + TMC_CTL);
• if (!(val & TMC_CTL_CAPT_EN))

• goto out;
  

• val = readl(drvdata->base + TMC_MODE);
• if (val != TMC_MODE_CIRCULAR_BUFFER)

• goto out;
  

• val = readl(drvdata->base + TMC_FFSR);
• /* Do manual flush and stop only if its not auto-stopped */
• if (!(val & TMC_FFSR_FT_STOPPED)) {

• dev_info(&csdev->dev,
  

• 	 "%s: Triggering manual flush\n", __func__);
  

• tmc_flush_and_stop(drvdata);
  

• } else

• tmc_wait_for_tmcready(drvdata);
  

• /* Sync registers from hardware to metadata region */
• mdata->sts = csdev_access_relaxed_read32(csa, TMC_STS);
• mdata->trc_paddr = drvdata->crash_tbuf.paddr;
• /* Sync Internal SRAM to reserved trace buffer region */
• drvdata->buf = drvdata->crash_tbuf.vaddr;
• tmc_etb_dump_hw(drvdata);
• /* Store as per RSZ register convention */
• mdata->size = drvdata->len >> 2;
• /*

• * Make sure all previous writes are completed,
  

• * before we mark valid
  

• */
  

• dsb(sy);
• mdata->valid = true;
• /*

• * Below order need to maintained, since crc of metadata
  

• * is dependent on first
  

• */
  

• mdata->crc32_tdata = find_crash_tracedata_crc(drvdata, mdata);
• mdata->crc32_mdata = find_crash_metadata_crc(mdata);
• tmc_disable_hw(drvdata);
• dev_info(&csdev->dev, "%s: success\n", __func__);

+out:

• CS_UNLOCK(drvdata->base);
• return 0;

+}

• static const struct coresight_ops_sink tmc_etf_sink_ops = { .enable = tmc_enable_etf_sink, .disable = tmc_disable_etf_sink,

@@ -603,6 +670,10 @@ static const struct coresight_ops_link tmc_etf_link_ops = { .disable = tmc_disable_etf_link, }; +static const struct coresight_ops_panic tmc_etf_sync_ops = {

• .sync = tmc_panic_sync_etf,

+};

• const struct coresight_ops tmc_etb_cs_ops = { .sink_ops = &tmc_etf_sink_ops, };

@@ -610,6 +681,7 @@ const struct coresight_ops tmc_etb_cs_ops = { 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, }; int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index 041c428dd7cd..be1079e8fd64 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1813,6 +1813,71 @@ static int tmc_disable_etr_sink(struct coresight_device *csdev) return 0; } +static int tmc_panic_sync_etr(struct coresight_device *csdev) +{

• u32 val;
• struct csdev_access *csa;
• struct tmc_crash_metadata *mdata;
• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• csa = &drvdata->csdev->access;
• if (!drvdata->etr_buf)

• return 0;
  

• /* Being in RESRV mode implies valid reserved memory as well */
• if (drvdata->etr_buf->mode != ETR_MODE_RESRV)

• return 0;
  

• mdata = (struct tmc_crash_metadata *)drvdata->crash_mdata.vaddr;
• mdata->valid = false;
• CS_UNLOCK(drvdata->base);
• /* Proceed only if ETR is enabled */
• val = readl(drvdata->base + TMC_CTL);
• if (!(val & TMC_CTL_CAPT_EN))

• goto out;
  

• val = readl(drvdata->base + TMC_FFSR);
• /* Do manual flush and stop only if its not auto-stopped */
• if (!(val & TMC_FFSR_FT_STOPPED)) {

• dev_info(&csdev->dev,
  

• 	 "%s: Triggering manual flush\n", __func__);
  

• tmc_flush_and_stop(drvdata);
  

• } else

• tmc_wait_for_tmcready(drvdata);
  

• /* Sync registers from hardware to metadata region */
• mdata->size = csdev_access_relaxed_read32(csa, TMC_RSZ);
• mdata->sts = csdev_access_relaxed_read32(csa, TMC_STS);
• mdata->rrp = tmc_read_rrp(drvdata);
• mdata->rwp = tmc_read_rwp(drvdata);
• mdata->dba = tmc_read_dba(drvdata);
• mdata->trc_paddr = drvdata->crash_tbuf.paddr;
• /*

• * Make sure all previous writes are completed,
  

• * before we mark valid
  

• */
  

• dsb(sy);
• mdata->valid = true;
• /*

• * Below order need to maintained, since crc of metadata
  

• * is dependent on first
  

• */
  

• mdata->crc32_tdata = find_crash_tracedata_crc(drvdata, mdata);
• mdata->crc32_mdata = find_crash_metadata_crc(mdata);
• tmc_disable_hw(drvdata);
• dev_info(&csdev->dev, "%s: success\n", __func__);

+out:

• CS_UNLOCK(drvdata->base);
• return 0;

+}

• static const struct coresight_ops_sink tmc_etr_sink_ops = { .enable = tmc_enable_etr_sink, .disable = tmc_disable_etr_sink,

@@ -1821,8 +1886,13 @@ static const struct coresight_ops_sink tmc_etr_sink_ops = { .free_buffer = tmc_free_etr_buffer, }; +static const struct coresight_ops_panic tmc_etr_sync_ops = {

• .sync = tmc_panic_sync_etr,

+};

• const struct coresight_ops tmc_etr_cs_ops = { .sink_ops = &tmc_etr_sink_ops,
• .panic_ops = &tmc_etr_sync_ops, }; int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)

diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index c23dc9917ab9..35beee53584a 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -12,6 +12,7 @@ #include <linux/miscdevice.h> #include <linux/mutex.h> #include <linux/refcount.h> +#include <linux/crc32.h> #define TMC_RSZ 0x004 #define TMC_STS 0x00c @@ -76,6 +77,9 @@ #define TMC_AXICTL_AXCACHE_OS (0xf << 2) #define TMC_AXICTL_ARCACHE_OS (0xf << 16) +/* TMC_FFSR - 0x300 */ +#define TMC_FFSR_FT_STOPPED BIT(1)

• /* TMC_FFCR - 0x304 */ #define TMC_FFCR_FLUSHMAN_BIT 6 #define TMC_FFCR_EN_FMT BIT(0)

@@ -131,6 +135,21 @@ enum tmc_mem_intf_width { #define CORESIGHT_SOC_600_ETR_CAPS \ (TMC_ETR_SAVE_RESTORE | TMC_ETR_AXI_ARCACHE)

...

+/* TMC metadata region for ETR and ETF configurations */

I strongly think we should version the data layout to handle the future changes better (if at all).

...

...

+struct tmc_crash_metadata {

• uint32_t crc32_mdata; /* crc of metadata */
• uint32_t crc32_tdata; /* crc of tracedata */

uint32_t version; /* Version of the structure = 1 */

Ack.

...

...

• uint32_t valid; /* Indicate if this ETF/ETR was enabled */
• uint32_t size; /* Ram Size register */

Please save the size in bytes and also rename it :

uint32_t trace_size; /* Trace size in Bytes */

The idea was to only take a register snapshot by kernel/firmware at the time of panic/watchdog reset and thought that keeping that panic handler/watchdog reset handler without any conversions would be better when external entities like firmware is involved.

Please let me know if you still prefer adding this field.

...

...

• uint32_t sts; /* Status register */
• uint32_t reserved32[3];
• uint64_t rrp; /* Ram Read pointer register */
• uint64_t rwp; /* Ram Write pointer register */

...

• uint64_t dba; /* Data buffer address register */

Is this field useful ? And we store RRP/RWP relative to the DBA ? Could

RRP/RWP/DBA fields are just register snapshots. Yes we use the DBA to calculate offsets. ie. etr_buf->offset = rrp - dba;

...

we instead :

1. Drop DBA
2. Store RRP and RWP as offsets from DBA. Or even convert them to the

actual PADDRs relative to the trc_paddr.

Converting to actual PADDRs would be difficult in case of watchdog reset scenarios where firmware is involved.

...

DBA could be a "DMA" Address and not necessarily the PA Address. We already have the trc_paddr below. (And for ETF, we already copy the buffer to the reserved buffer). So all the user needs to know is where the pointers are within the buffer. Having them relative to the "actual" location of the buffer is much useful than basing it on some unusable base address.

As pointed out above, the idea was to only take a register snapshot without any conversions. Please let me know if you still prefer dropping the DBA.

...

...

• uint64_t trc_paddr; /* Phys address of trace buffer */

s/trc/trace

Move RRP and RWP, after the above field.

For the sake of completeness, you are also missing :

1. FFCR register => That tells you whether the Formatting was enabled or not

(among other things) ? Though we always enable it, its good to capture it, if we ever decide to turn off the formatting.

2. MODE => Which mode was selected. Again, CIRCULAR_BUFFER for now,

but lets seal it for the future, so that you can infer the trace buffer correctly with RRP/RWP.

3. And may be FFSR, just in case the flush never completed and the

data is not reliable ?

Sure will add the above three fields.

...

...

• uint64_t reserved64[3];

+};

...

• enum etr_mode { ETR_MODE_FLAT, /* Uses contiguous flat buffer */ ETR_MODE_ETR_SG, /* Uses in-built TMC ETR SG mechanism */

@@ -204,6 +223,8 @@ struct tmc_resrv_buf {

• retention (after crash) only when ETR_MODE_RESRV buffer
  

• mode is enabled. Used by ETF for trace data retention
  

• (after crash) by default.
  

• • @crash_mdata: Reserved memory for storing tmc crash metadata.

• •  Used by ETR/ETF.
    

  */ struct tmc_drvdata { struct clk *pclk;

@@ -230,6 +251,7 @@ struct tmc_drvdata { struct etr_buf *sysfs_buf; struct etr_buf *perf_buf; struct tmc_resrv_buf crash_tbuf;

• struct tmc_resrv_buf crash_mdata; }; struct etr_buf_operations {

@@ -352,11 +374,32 @@ static inline bool is_tmc_reserved_region_valid(struct device *dev) struct tmc_drvdata *drvdata = dev_get_drvdata(dev); if (drvdata->crash_tbuf.paddr &&

• drvdata->crash_tbuf.size)
  

• drvdata->crash_tbuf.size &&
  

• drvdata->crash_mdata.paddr &&
  

• drvdata->crash_mdata.size)
  

Why do we need to tie the "reserved" region to metdata region availability ? It is perfectly possible for another usecase to dedicate a buffer for trace and use it without metadata ?

Okay that can be reworked.

Wouldn't that be the same as the old sysfs mode? Without the metadata does it do anything different functionally? I'm wondering if it's worth the extra complexity to implement and test two modes (with and without metadata).

James

On 05/06/2024 09:17, Linu Cherian wrote:

• Introduce a new mode CS_MODE_READ_CRASHDATA for reading trace captured in previous crash/watchdog reset.

• Add special device files for reading ETR/ETF crash data.

• User can read the crash data as below

  For example, for reading crash data from tmc_etf sink

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

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

---

Changelog from v8:

• Added missing exit path in __tmc_probe
• Few whitespace fixes and a checkpatch fix.

.../coresight/coresight-etm4x-core.c | 1 + .../hwtracing/coresight/coresight-tmc-core.c | 150 ++++++++++++++++- .../hwtracing/coresight/coresight-tmc-etf.c | 72 +++++++++ .../hwtracing/coresight/coresight-tmc-etr.c | 151 +++++++++++++++++- drivers/hwtracing/coresight/coresight-tmc.h | 11 +- include/linux/coresight.h | 13 ++ 6 files changed, 390 insertions(+), 8 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index a0bdfabddbc6..7924883476c6 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1011,6 +1011,7 @@ static void etm4_disable(struct coresight_device *csdev, switch (mode) { case CS_MODE_DISABLED:

• case CS_MODE_READ_CRASHDATA: break; case CS_MODE_SYSFS: etm4_disable_sysfs(csdev);

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index daad08bc693d..0c145477ba66 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -106,6 +106,60 @@ u32 tmc_get_memwidth_mask(struct tmc_drvdata *drvdata) return mask; } +int tmc_read_prepare_crashdata(struct tmc_drvdata *drvdata) +{

• int ret = 0;
• struct tmc_crash_metadata *mdata;
• struct coresight_device *csdev = drvdata->csdev;
• if (!drvdata->crash_mdata.vaddr) {

• ret = -ENOMEM;
  

• goto out;
  

• }
• mdata = drvdata->crash_mdata.vaddr;
• /* Check data integrity of metadata */
• if (mdata->crc32_mdata != find_crash_metadata_crc(mdata)) {

• dev_dbg(&drvdata->csdev->dev,
  

• 	"CRC mismatch in tmc crash metadata\n");
  

• ret = -EINVAL;
  

• goto out;
  

• }
• /* Check data integrity of tracedata */
• if (mdata->crc32_tdata != find_crash_tracedata_crc(drvdata, mdata)) {

• dev_dbg(&drvdata->csdev->dev,
  

• 	"CRC mismatch in tmc crash tracedata\n");
  

• ret = -EINVAL;
  

• goto out;
  

• }
• /* Check for valid metadata */
• if (!mdata->valid) {

• dev_dbg(&drvdata->csdev->dev,
  

• 	"Data invalid in tmc crash metadata\n");
  

• ret = -EINVAL;
  

• goto out;
  

• }
• /* Sink specific crashdata mode preparation */
• ret = crashdata_ops(csdev)->prepare(csdev);
• if (ret)

• goto out;
  

• if (mdata->sts & 0x1)

• coresight_insert_barrier_packet(drvdata->buf);
  

+out:

• return ret;

+}

+int tmc_read_unprepare_crashdata(struct tmc_drvdata *drvdata) +{

• struct coresight_device *csdev = drvdata->csdev;
• /* Sink specific crashdata mode preparation */
• return crashdata_ops(csdev)->unprepare(csdev);

+}

static int tmc_read_prepare(struct tmc_drvdata *drvdata) { int ret = 0; @@ -156,6 +210,9 @@ static int tmc_open(struct inode *inode, struct file *file) struct tmc_drvdata *drvdata = container_of(file->private_data, struct tmc_drvdata, miscdev);

• if (coresight_get_mode(drvdata->csdev) == CS_MODE_READ_CRASHDATA)

• return -EBUSY;
  

• ret = tmc_read_prepare(drvdata); if (ret) return ret;

@@ -180,13 +237,12 @@ static inline ssize_t tmc_get_sysfs_trace(struct tmc_drvdata *drvdata, return -EINVAL; } -static ssize_t tmc_read(struct file *file, char __user *data, size_t len,

• 	loff_t *ppos)
  

+static ssize_t tmc_read_common(struct tmc_drvdata *drvdata, char __user *data,

• 	       size_t len, loff_t *ppos)
  

{ char *bufp; ssize_t actual;

• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata, miscdev);
  

• actual = tmc_get_sysfs_trace(drvdata, *ppos, len, &bufp); if (actual <= 0) return 0;

@@ -203,6 +259,15 @@ static ssize_t tmc_read(struct file *file, char __user *data, size_t len, return actual; } +static ssize_t tmc_read(struct file *file, char __user *data, size_t len,

• 	loff_t *ppos)
  

+{

• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata, miscdev);
  

• return tmc_read_common(drvdata, data, len, ppos);

+}

static int tmc_release(struct inode *inode, struct file *file) { int ret; @@ -225,6 +290,61 @@ static const struct file_operations tmc_fops = { .llseek = no_llseek, }; +static int tmc_crashdata_open(struct inode *inode, struct file *file) +{

• int ret;
• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata,
  

• 				   crashdev);
  

• if (!coresight_take_mode(drvdata->csdev, CS_MODE_READ_CRASHDATA))

• return -EBUSY;
  

• ret = tmc_read_prepare(drvdata);
• if (ret) {

• coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED);
  

• return ret;
  

• }
• nonseekable_open(inode, file);
• dev_dbg(&drvdata->csdev->dev, "%s: successfully opened\n", __func__);
• return 0;

+}

+static ssize_t tmc_crashdata_read(struct file *file, char __user *data,

• 		  size_t len, loff_t *ppos)
  

+{

• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata,
  

• 				   crashdev);
  

• return tmc_read_common(drvdata, data, len, ppos);

+}

+static int tmc_crashdata_release(struct inode *inode, struct file *file) +{

• int ret = 0;
• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata,
  

• 				   crashdev);
  

• ret = tmc_read_unprepare(drvdata);
• coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED);
• dev_dbg(&drvdata->csdev->dev, "%s: released\n", __func__);
• return ret;

+}

+static const struct file_operations tmc_crashdata_fops = {

• .owner = THIS_MODULE,
• .open = tmc_crashdata_open,
• .read = tmc_crashdata_read,
• .release = tmc_crashdata_release,
• .llseek = no_llseek,

+};

static enum tmc_mem_intf_width tmc_get_memwidth(u32 devid) { enum tmc_mem_intf_width memwidth; @@ -542,6 +662,18 @@ static u32 tmc_etr_get_max_burst_size(struct device *dev) return burst_size; } +static void register_crash_dev_interface(struct tmc_drvdata *drvdata,

• 			 const char *name)
  

+{

• drvdata->crashdev.name =

• devm_kasprintf(&drvdata->csdev->dev, GFP_KERNEL, "%s_%s", "crash", name);
  

• drvdata->crashdev.minor = MISC_DYNAMIC_MINOR;
• drvdata->crashdev.fops = &tmc_crashdata_fops;
• if (misc_register(&drvdata->crashdev))

• dev_dbg(&drvdata->csdev->dev,
  

• 	"Failed to setup user interface for crashdata\n");
  

+}

static int __tmc_probe(struct device *dev, struct resource *res) { int ret = 0; @@ -642,8 +774,13 @@ static int __tmc_probe(struct device *dev, struct resource *res) drvdata->miscdev.minor = MISC_DYNAMIC_MINOR; drvdata->miscdev.fops = &tmc_fops; ret = misc_register(&drvdata->miscdev);

• if (ret)

• if (ret) { coresight_unregister(drvdata->csdev);

• goto out;
  

• }
• if (is_tmc_reserved_region_valid(dev))

• register_crash_dev_interface(drvdata, desc.name);
  

I think this would be better if it checked the CRC of the metadata in the same way it does before reading the file.

Now we have two forms of "region valid", one that's any non-zero value, and the other "really valid" one. And because we don't check the CRC here we register a device that can't be used.

I found it a bit confusing because without enabling debug prints I didn't know why the file couldn't be read. So I wasn't sure if it was because it wasn't valid or some other reason.

I also wasn't able to get a valid region after booting the crash kernel. But maybe the memory isn't preserved across the reboot on my Juno, so I don't think that's necessarily an issue?

Hi James,

-----Original Message----- From: James Clark james.clark@arm.com Sent: Friday, June 7, 2024 7:48 PM To: Linu Cherian lcherian@marvell.com Cc: linux-arm-kernel@lists.infradead.org; coresight@lists.linaro.org; linux- kernel@vger.kernel.org; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; devicetree@vger.kernel.org; Sunil Kovvuri Goutham sgoutham@marvell.com; George Cherian gcherian@marvell.com; Anil Kumar Reddy H areddy3@marvell.com; Tanmay Jagdale tanmay@marvell.com; suzuki.poulose@arm.com; mike.leach@linaro.org Subject: [EXTERNAL] Re: [PATCH v9 5/7] coresight: tmc: Add support for reading crash data

Prioritize security for external emails: Confirm sender and content safety before clicking links or opening attachments

---

On 05/06/2024 17:09, James Clark wrote:

...

On 05/06/2024 09:17, Linu Cherian wrote:

...

• Introduce a new mode CS_MODE_READ_CRASHDATA for reading trace captured in previous crash/watchdog reset.

• Add special device files for reading ETR/ETF crash data.

• User can read the crash data as below

  For example, for reading crash data from tmc_etf sink

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

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

---

Changelog from v8:

• Added missing exit path in __tmc_probe
• Few whitespace fixes and a checkpatch fix.

.../coresight/coresight-etm4x-core.c | 1 + .../hwtracing/coresight/coresight-tmc-core.c | 150 ++++++++++++++++- .../hwtracing/coresight/coresight-tmc-etf.c | 72 +++++++++ .../hwtracing/coresight/coresight-tmc-etr.c | 151 +++++++++++++++++- drivers/hwtracing/coresight/coresight-tmc.h | 11 +- include/linux/coresight.h | 13 ++ 6 files changed, 390 insertions(+), 8 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index a0bdfabddbc6..7924883476c6 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1011,6 +1011,7 @@ static void etm4_disable(struct coresight_device *csdev,

switch (mode) { case CS_MODE_DISABLED:

• case CS_MODE_READ_CRASHDATA: break; case CS_MODE_SYSFS: etm4_disable_sysfs(csdev);

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index daad08bc693d..0c145477ba66 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -106,6 +106,60 @@ u32 tmc_get_memwidth_mask(struct

tmc_drvdata *drvdata)

...

...

return mask; }

+int tmc_read_prepare_crashdata(struct tmc_drvdata *drvdata) {

• int ret = 0;
• struct tmc_crash_metadata *mdata;
• struct coresight_device *csdev = drvdata->csdev;
• if (!drvdata->crash_mdata.vaddr) {

• ret = -ENOMEM;
  

• goto out;
  

• }
• mdata = drvdata->crash_mdata.vaddr;
• /* Check data integrity of metadata */
• if (mdata->crc32_mdata != find_crash_metadata_crc(mdata)) {

• dev_dbg(&drvdata->csdev->dev,
  

• 	"CRC mismatch in tmc crash metadata\n");
  

• ret = -EINVAL;
  

• goto out;
  

• }
• /* Check data integrity of tracedata */
• if (mdata->crc32_tdata != find_crash_tracedata_crc(drvdata, mdata))

{

...

...

• dev_dbg(&drvdata->csdev->dev,
  

• 	"CRC mismatch in tmc crash tracedata\n");
  

• ret = -EINVAL;
  

• goto out;
  

• }
• /* Check for valid metadata */
• if (!mdata->valid) {

• dev_dbg(&drvdata->csdev->dev,
  

• 	"Data invalid in tmc crash metadata\n");
  

• ret = -EINVAL;
  

• goto out;
  

• }
• /* Sink specific crashdata mode preparation */
• ret = crashdata_ops(csdev)->prepare(csdev);
• if (ret)

• goto out;
  

• if (mdata->sts & 0x1)

• coresight_insert_barrier_packet(drvdata->buf);
  

+out:

• return ret;

+}

+int tmc_read_unprepare_crashdata(struct tmc_drvdata *drvdata) {

• struct coresight_device *csdev = drvdata->csdev;
• /* Sink specific crashdata mode preparation */
• return crashdata_ops(csdev)->unprepare(csdev);

+}

static int tmc_read_prepare(struct tmc_drvdata *drvdata) { int ret = 0; @@ -156,6 +210,9 @@ static int tmc_open(struct inode *inode, struct file

*file)

...

...

struct tmc_drvdata *drvdata = container_of(file->private_data, struct tmc_drvdata,

miscdev);

...

...

• if (coresight_get_mode(drvdata->csdev) ==

CS_MODE_READ_CRASHDATA)

...

...

• return -EBUSY;
  

• ret = tmc_read_prepare(drvdata); if (ret) return ret;

@@ -180,13 +237,12 @@ static inline ssize_t tmc_get_sysfs_trace(struct

tmc_drvdata *drvdata,

...

...

return -EINVAL; }

-static ssize_t tmc_read(struct file *file, char __user *data, size_t len,

• 	loff_t *ppos)
  

+static ssize_t tmc_read_common(struct tmc_drvdata *drvdata, char

__user *data,

...

...

• 	       size_t len, loff_t *ppos)
  

{ char *bufp; ssize_t actual;

• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata,
  

miscdev);

...

...

• actual = tmc_get_sysfs_trace(drvdata, *ppos, len, &bufp); if (actual <= 0) return 0;

@@ -203,6 +259,15 @@ static ssize_t tmc_read(struct file *file, char

__user *data, size_t len,

...

...

return actual; }

+static ssize_t tmc_read(struct file *file, char __user *data, size_t len,

• 	loff_t *ppos)
  

+{

• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata,
  

miscdev);

...

...

• return tmc_read_common(drvdata, data, len, ppos); }

static int tmc_release(struct inode *inode, struct file *file) { int ret; @@ -225,6 +290,61 @@ static const struct file_operations tmc_fops = { .llseek = no_llseek, };

+static int tmc_crashdata_open(struct inode *inode, struct file +*file) {

• int ret;
• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata,
  

• 				   crashdev);
  

• if (!coresight_take_mode(drvdata->csdev,

CS_MODE_READ_CRASHDATA))

...

...

• return -EBUSY;
  

• ret = tmc_read_prepare(drvdata);
• if (ret) {

• coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED);
  

• return ret;
  

• }
• nonseekable_open(inode, file);
• dev_dbg(&drvdata->csdev->dev, "%s: successfully opened\n",

__func__);

...

...

• return 0;

+}

+static ssize_t tmc_crashdata_read(struct file *file, char __user *data,

• 		  size_t len, loff_t *ppos)
  

+{

• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata,
  

• 				   crashdev);
  

• return tmc_read_common(drvdata, data, len, ppos); }

+static int tmc_crashdata_release(struct inode *inode, struct file +*file) {

• int ret = 0;
• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata,
  

• 				   crashdev);
  

• ret = tmc_read_unprepare(drvdata);
• coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED);
• dev_dbg(&drvdata->csdev->dev, "%s: released\n", __func__);
• return ret;

+}

+static const struct file_operations tmc_crashdata_fops = {

• .owner = THIS_MODULE,
• .open = tmc_crashdata_open,
• .read = tmc_crashdata_read,
• .release = tmc_crashdata_release,
• .llseek = no_llseek,

+};

static enum tmc_mem_intf_width tmc_get_memwidth(u32 devid) { enum tmc_mem_intf_width memwidth; @@ -542,6 +662,18 @@ static u32 tmc_etr_get_max_burst_size(struct

device *dev)

...

...

return burst_size; }

+static void register_crash_dev_interface(struct tmc_drvdata *drvdata,

• 			 const char *name)
  

+{

• drvdata->crashdev.name =

• devm_kasprintf(&drvdata->csdev->dev, GFP_KERNEL,
  

"%s_%s", "crash", name);

...

...

• drvdata->crashdev.minor = MISC_DYNAMIC_MINOR;
• drvdata->crashdev.fops = &tmc_crashdata_fops;
• if (misc_register(&drvdata->crashdev))

• dev_dbg(&drvdata->csdev->dev,
  

• 	"Failed to setup user interface for crashdata\n"); }
  

static int __tmc_probe(struct device *dev, struct resource *res) { int ret = 0; @@ -642,8 +774,13 @@ static int __tmc_probe(struct device *dev, struct

resource *res)

...

...

drvdata->miscdev.minor = MISC_DYNAMIC_MINOR; drvdata->miscdev.fops = &tmc_fops; ret = misc_register(&drvdata->miscdev);

• if (ret)

• if (ret) { coresight_unregister(drvdata->csdev);

• goto out;
  

• }
• if (is_tmc_reserved_region_valid(dev))

• register_crash_dev_interface(drvdata, desc.name);
  

I think this would be better if it checked the CRC of the metadata in the same way it does before reading the file.

Now we have two forms of "region valid", one that's any non-zero value, and the other "really valid" one. And because we don't check the CRC here we register a device that can't be used.

I found it a bit confusing because without enabling debug prints I didn't know why the file couldn't be read. So I wasn't sure if it was because it wasn't valid or some other reason.

I also wasn't able to get a valid region after booting the crash kernel. But maybe the memory isn't preserved across the reboot on my Juno, so I don't think that's necessarily an issue?

Ok so I double checked by writing 0x123456 into the reserved region and confirmed that it _is_ preserved when booting the panic kernel on my Juno. So I'm not sure why I wasn't able to read out the crash dump.

I did see the "success" message from tmc_panic_sync_etr() at least some of the times, although I do remember it not printing out every time. I don't know if this is just an issue with outputting to serial after a panic or something else was going on?

Did you ever see the success message not print out? Or not able to read back the data when you were testing it?

During my testing, success messages were not print out only in two cases, a. resrv buf mode is not enabled for ETR #echo "resrv" > /sys/bus/coresight/devices/tmc_etr0/buf_mode_preferred b. ETR is not in enabled state ie. Neither perf session nor syfs trace session is not active at the time of kernel panic.

If success message is not printed, then definitely a valid snapshot is not taken at the time of panic.

On iterations where success is printed, do you see any debug messages from tmc_read_prepare_crashdata at the time of reading /dev/crash_tmc_etrXX indicating invalid snapshot ?

Thanks.

On 05/06/2024 09:17, Linu Cherian wrote:

• Introduce a new mode CS_MODE_READ_CRASHDATA for reading trace captured in previous crash/watchdog reset.

I am still not convinced if we really need this new mode. We should :

1) Register the new misc device to expose the "reserved" saved trace data, only when the metadata is crc checked and it indicates "valid" trace data.

2) Always allow reading from the "reserved" buffer when the above file is opened. At open(), once again check if the mdata.valid is true (see below), if not, return -EIO.

3) Whenever we use the buffer in "RESERVE" for a session, also set the mdata.valid == false. This would prevent any further read from the /dev/crash_xxx device. Which makes sense, as the data is lost.

This way, you don't need to dance with a new mode always provide the reserve buffer contents, if it is valid (with an ongoing RESERVE mode) invalidating the buffer and the change is much much simpler.

• Add special device files for reading ETR/ETF crash data.

• User can read the crash data as below

  For example, for reading crash data from tmc_etf sink

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

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

---

Changelog from v8:

• Added missing exit path in __tmc_probe

• Few whitespace fixes and a checkpatch fix.

  .../coresight/coresight-etm4x-core.c | 1 + .../hwtracing/coresight/coresight-tmc-core.c | 150 ++++++++++++++++- .../hwtracing/coresight/coresight-tmc-etf.c | 72 +++++++++ .../hwtracing/coresight/coresight-tmc-etr.c | 151 +++++++++++++++++- drivers/hwtracing/coresight/coresight-tmc.h | 11 +- include/linux/coresight.h | 13 ++ 6 files changed, 390 insertions(+), 8 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index a0bdfabddbc6..7924883476c6 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1011,6 +1011,7 @@ static void etm4_disable(struct coresight_device *csdev, switch (mode) { case CS_MODE_DISABLED:

• case CS_MODE_READ_CRASHDATA: break; case CS_MODE_SYSFS: etm4_disable_sysfs(csdev);

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index daad08bc693d..0c145477ba66 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -106,6 +106,60 @@ u32 tmc_get_memwidth_mask(struct tmc_drvdata *drvdata) return mask; } +int tmc_read_prepare_crashdata(struct tmc_drvdata *drvdata) +{

• int ret = 0;
• struct tmc_crash_metadata *mdata;
• struct coresight_device *csdev = drvdata->csdev;
• if (!drvdata->crash_mdata.vaddr) {

• ret = -ENOMEM;
  

• goto out;
  

• }
• mdata = drvdata->crash_mdata.vaddr;
• /* Check data integrity of metadata */
• if (mdata->crc32_mdata != find_crash_metadata_crc(mdata)) {

• dev_dbg(&drvdata->csdev->dev,
  

• 	"CRC mismatch in tmc crash metadata\n");
  

• ret = -EINVAL;
  

After the comments above, if at all we retain this, please use -EIO

• goto out;
  

• }
• /* Check data integrity of tracedata */
• if (mdata->crc32_tdata != find_crash_tracedata_crc(drvdata, mdata)) {

• dev_dbg(&drvdata->csdev->dev,
  

• 	"CRC mismatch in tmc crash tracedata\n");
  

• ret = -EINVAL;
  

Same here, -EIO

• goto out;
  

• }
• /* Check for valid metadata */
• if (!mdata->valid) {

• dev_dbg(&drvdata->csdev->dev,
  

• 	"Data invalid in tmc crash metadata\n");
  

• ret = -EINVAL;
  

• goto out;
  

• }

-ENXIO

• /* Sink specific crashdata mode preparation */
• ret = crashdata_ops(csdev)->prepare(csdev);
• if (ret)

• goto out;
  

• if (mdata->sts & 0x1)

If at all we need this, please use TMC_STS_FULL instead of hard coding numbers.

• coresight_insert_barrier_packet(drvdata->buf);
  

+out:

• return ret;

+}

+int tmc_read_unprepare_crashdata(struct tmc_drvdata *drvdata) +{

• struct coresight_device *csdev = drvdata->csdev;
• /* Sink specific crashdata mode preparation */
• return crashdata_ops(csdev)->unprepare(csdev);

+}

• static int tmc_read_prepare(struct tmc_drvdata *drvdata) { int ret = 0;

@@ -156,6 +210,9 @@ static int tmc_open(struct inode *inode, struct file *file) struct tmc_drvdata *drvdata = container_of(file->private_data, struct tmc_drvdata, miscdev);

• if (coresight_get_mode(drvdata->csdev) == CS_MODE_READ_CRASHDATA)

• return -EBUSY;
  

• ret = tmc_read_prepare(drvdata); if (ret) return ret;

@@ -180,13 +237,12 @@ static inline ssize_t tmc_get_sysfs_trace(struct tmc_drvdata *drvdata, return -EINVAL; } -static ssize_t tmc_read(struct file *file, char __user *data, size_t len,

• 	loff_t *ppos)
  

+static ssize_t tmc_read_common(struct tmc_drvdata *drvdata, char __user *data,

• 	       size_t len, loff_t *ppos)
  

  { char *bufp; ssize_t actual;

• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata, miscdev);
  

• actual = tmc_get_sysfs_trace(drvdata, *ppos, len, &bufp); if (actual <= 0) return 0;

@@ -203,6 +259,15 @@ static ssize_t tmc_read(struct file *file, char __user *data, size_t len, return actual; } +static ssize_t tmc_read(struct file *file, char __user *data, size_t len,

• 	loff_t *ppos)
  

+{

• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata, miscdev);
  

• return tmc_read_common(drvdata, data, len, ppos);

+}

• static int tmc_release(struct inode *inode, struct file *file) { int ret;

@@ -225,6 +290,61 @@ static const struct file_operations tmc_fops = { .llseek = no_llseek, }; +static int tmc_crashdata_open(struct inode *inode, struct file *file) +{

• int ret;
• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata,
  

• 				   crashdev);
  

• if (!coresight_take_mode(drvdata->csdev, CS_MODE_READ_CRASHDATA))

• return -EBUSY;
  

• ret = tmc_read_prepare(drvdata);
• if (ret) {

• coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED);
  

• return ret;
  

• }
• nonseekable_open(inode, file);

As mentioned in the beginning, please use "mdata.valid" to deny any open requests and that way you could simplify most of the code and make a single function to handle all of the crash device handling.

• dev_dbg(&drvdata->csdev->dev, "%s: successfully opened\n", __func__);
• return 0;

+}

+static ssize_t tmc_crashdata_read(struct file *file, char __user *data,

• 		  size_t len, loff_t *ppos)
  

+{

• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata,
  

• 				   crashdev);
  

• return tmc_read_common(drvdata, data, len, ppos);

Simply provide the reserve buffer data, instead of creating ETB / ETR specific handling.

+}

+static int tmc_crashdata_release(struct inode *inode, struct file *file) +{

• int ret = 0;
• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata,
  

• 				   crashdev);
  

• ret = tmc_read_unprepare(drvdata);
• coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED);
• dev_dbg(&drvdata->csdev->dev, "%s: released\n", __func__);
• return ret;

+}

+static const struct file_operations tmc_crashdata_fops = {

• .owner = THIS_MODULE,
• .open = tmc_crashdata_open,
• .read = tmc_crashdata_read,
• .release = tmc_crashdata_release,
• .llseek = no_llseek,

+};

• static enum tmc_mem_intf_width tmc_get_memwidth(u32 devid) { enum tmc_mem_intf_width memwidth;

@@ -542,6 +662,18 @@ static u32 tmc_etr_get_max_burst_size(struct device *dev) return burst_size; } +static void register_crash_dev_interface(struct tmc_drvdata *drvdata,

• 			 const char *name)
  

+{

• drvdata->crashdev.name =

• devm_kasprintf(&drvdata->csdev->dev, GFP_KERNEL, "%s_%s", "crash", name);
  

• drvdata->crashdev.minor = MISC_DYNAMIC_MINOR;
• drvdata->crashdev.fops = &tmc_crashdata_fops;
• if (misc_register(&drvdata->crashdev))

• dev_dbg(&drvdata->csdev->dev,
  

• 	"Failed to setup user interface for crashdata\n");
  

+}

• static int __tmc_probe(struct device *dev, struct resource *res) { int ret = 0;

@@ -642,8 +774,13 @@ static int __tmc_probe(struct device *dev, struct resource *res) drvdata->miscdev.minor = MISC_DYNAMIC_MINOR; drvdata->miscdev.fops = &tmc_fops; ret = misc_register(&drvdata->miscdev);

• if (ret)

• if (ret) { coresight_unregister(drvdata->csdev);

• goto out;
  

• }
• if (is_tmc_reserved_region_valid(dev))

• register_crash_dev_interface(drvdata, desc.name);
  

Please could we only expose this when the reserved region has valid metadata, crc checked etc ? That way the users don't have to read it to figure out if it is valid or not. (Please note we need to use the valid bit check to avoid reading over-written data).

Suzuki

out: return ret; } @@ -672,7 +809,8 @@ static void tmc_shutdown(struct amba_device *adev) spin_lock_irqsave(&drvdata->spinlock, flags);

• if (coresight_get_mode(drvdata->csdev) == CS_MODE_DISABLED)

• if ((coresight_get_mode(drvdata->csdev) == CS_MODE_DISABLED) ||

•    (coresight_get_mode(drvdata->csdev) == CS_MODE_READ_CRASHDATA))
  

  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 f9569585e9f8..a8cabbf6679b 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -657,6 +657,56 @@ static int tmc_panic_sync_etf(struct coresight_device *csdev) return 0; } +static int tmc_etb_setup_crashdata_buf(struct tmc_drvdata *drvdata) +{

• unsigned long size;
• struct tmc_crash_metadata *mdata;
• struct device *dev = &drvdata->csdev->dev;
• mdata = drvdata->crash_mdata.vaddr;
• size = mdata->size << 2;
• /*

• * Buffer address given by metadata for retrieval of trace data
  

• * from previous boot is expected to be same as the reserved
  

• * trace buffer memory region provided through DTS
  

• */
  

• if (is_tmc_reserved_region_valid(dev->parent)

•    && (drvdata->crash_tbuf.paddr == mdata->trc_paddr))
  

• drvdata->buf = drvdata->crash_tbuf.vaddr;
  

• else {

• dev_dbg(dev, "Trace buffer address of previous boot invalid\n");
  

• return -EINVAL;
  

• }
• drvdata->len = size;
• return 0;

+}

+static void tmc_etb_free_crashdata_buf(struct tmc_drvdata *drvdata) +{

• void *buf = drvdata->buf;
• if (!buf)

• return;
  

• drvdata->buf = NULL;

+}

+static int tmc_etb_prepare_crashdata(struct coresight_device *csdev) +{

• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• return tmc_etb_setup_crashdata_buf(drvdata);

+}

+static int tmc_etb_unprepare_crashdata(struct coresight_device *csdev) +{

• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• tmc_etb_free_crashdata_buf(drvdata);
• return 0;

+}

• static const struct coresight_ops_sink tmc_etf_sink_ops = { .enable = tmc_enable_etf_sink, .disable = tmc_disable_etf_sink,

@@ -674,6 +724,11 @@ static const struct coresight_ops_panic tmc_etf_sync_ops = { .sync = tmc_panic_sync_etf, }; +static const struct coresight_ops_crashdata tmc_etf_crashdata_ops = {

• .prepare = tmc_etb_prepare_crashdata,
• .unprepare = tmc_etb_unprepare_crashdata,

+};

• const struct coresight_ops tmc_etb_cs_ops = { .sink_ops = &tmc_etf_sink_ops, };

@@ -682,6 +737,7 @@ 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,

• .crashdata_ops = &tmc_etf_crashdata_ops, };

int tmc_read_prepare_etb(struct tmc_drvdata *drvdata) @@ -702,6 +758,14 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata) goto out; }

• if (coresight_get_mode(drvdata->csdev) == CS_MODE_READ_CRASHDATA) {

• ret = tmc_read_prepare_crashdata(drvdata);
  

• if (ret)
  

• 	goto out;
  

• else
  

• 	goto mode_valid;
  

• }
• /* Don't interfere if operated from Perf */ if (coresight_get_mode(drvdata->csdev) == CS_MODE_PERF) { ret = -EINVAL;

@@ -725,6 +789,7 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata) __tmc_etb_disable_hw(drvdata); } +mode_valid: drvdata->reading = true; out: spin_unlock_irqrestore(&drvdata->spinlock, flags); @@ -746,6 +811,13 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata) spin_lock_irqsave(&drvdata->spinlock, flags);

• if (coresight_get_mode(drvdata->csdev) == CS_MODE_READ_CRASHDATA) {

• tmc_read_unprepare_crashdata(drvdata);
  

• drvdata->reading = false;
  

• spin_unlock_irqrestore(&drvdata->spinlock, flags);
  

• return 0;
  

• }
• /* Re-enable the TMC if need be */ if (coresight_get_mode(drvdata->csdev) == CS_MODE_SYSFS) { /* There is no point in reading a TMC in HW FIFO mode */

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index be1079e8fd64..b12ae7cc3372 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1160,7 +1160,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 (coresight_get_mode(drvdata->csdev) == CS_MODE_READ_CRASHDATA)

• etr_buf = drvdata->sysfs_crash_buf;
  

• else

• etr_buf = drvdata->sysfs_buf;
  

if (pos + actual > etr_buf->len) actual = etr_buf->len - pos; @@ -1878,6 +1883,128 @@ static int tmc_panic_sync_etr(struct coresight_device *csdev) return 0; } +static int tmc_etr_setup_crashdata_buf(struct tmc_drvdata *drvdata) +{

• int rc = 0;
• struct etr_buf *etr_buf;
• struct etr_flat_buf *resrv_buf;
• struct tmc_crash_metadata *mdata;
• struct device *dev = &drvdata->csdev->dev;
• mdata = drvdata->crash_mdata.vaddr;
• etr_buf = kzalloc(sizeof(*etr_buf), GFP_ATOMIC);
• if (!etr_buf) {

• rc = -ENOMEM;
  

• goto out;
  

• }
• etr_buf->size = drvdata->crash_tbuf.size;
• resrv_buf = kzalloc(sizeof(*resrv_buf), GFP_ATOMIC);
• if (!resrv_buf) {

• rc = -ENOMEM;
  

• goto rmem_err;
  

• }
• /*

• * Buffer address given by metadata for retrieval of trace data
  

• * from previous boot is expected to be same as the reserved
  

• * trace buffer memory region provided through DTS
  

• */
  

• if (is_tmc_reserved_region_valid(dev->parent)

•    && (drvdata->crash_tbuf.paddr == mdata->trc_paddr))
  

• resrv_buf->vaddr = drvdata->crash_tbuf.vaddr;
  

• else {

• dev_dbg(dev, "Trace buffer address of previous boot invalid\n");
  

• rc = -EINVAL;
  

• goto map_err;
  

• }
• resrv_buf->size = etr_buf->size;
• resrv_buf->dev = &drvdata->csdev->dev;
• etr_buf->mode = ETR_MODE_RESRV;
• etr_buf->private = resrv_buf;
• etr_buf->ops = etr_buf_ops[ETR_MODE_RESRV];
• drvdata->sysfs_crash_buf = etr_buf;
• return 0;

+map_err:

• kfree(resrv_buf);

+rmem_err:

• kfree(etr_buf);

+out:

• return rc;

+}

+static int tmc_etr_sync_crashdata_buf(struct tmc_drvdata *drvdata) +{

• u32 status;
• u64 rrp, rwp, dba;
• struct tmc_crash_metadata *mdata;
• struct etr_buf *etr_buf = drvdata->sysfs_crash_buf;
• mdata = drvdata->crash_mdata.vaddr;
• rrp = mdata->rrp;
• rwp = mdata->rwp;
• dba = mdata->dba;
• status = mdata->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;
  

• /* Additional sanity checks for validating metadata */
• if ((etr_buf->offset > etr_buf->size) ||

•    (etr_buf->len > etr_buf->size)) {
  

• dev_dbg(&drvdata->csdev->dev,
  

• 	"Offset and length invalid in tmc crash metadata\n");
  

• return -EINVAL;
  

• }
• return 0;

+}

+static void tmc_etr_free_crashdata_buf(struct tmc_drvdata *drvdata) +{

• struct etr_buf *etr_buf = drvdata->sysfs_crash_buf;
• if (!etr_buf)

• return;
  

• drvdata->sysfs_crash_buf = NULL;

+}

+static int tmc_etr_prepare_crashdata(struct coresight_device *csdev) +{

• int ret = 0;
• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• ret = tmc_etr_setup_crashdata_buf(drvdata);
• if (ret)

• goto out;
  

• ret = tmc_etr_sync_crashdata_buf(drvdata);

+out:

• return ret;

+}

+static int tmc_etr_unprepare_crashdata(struct coresight_device *csdev) +{

• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• tmc_etr_free_crashdata_buf(drvdata);
• return 0;

+}

• static const struct coresight_ops_sink tmc_etr_sink_ops = { .enable = tmc_enable_etr_sink, .disable = tmc_disable_etr_sink,

@@ -1890,9 +2017,15 @@ static const struct coresight_ops_panic tmc_etr_sync_ops = { .sync = tmc_panic_sync_etr, }; +static const struct coresight_ops_crashdata tmc_etr_crashdata_ops = {

• .prepare = tmc_etr_prepare_crashdata,
• .unprepare = tmc_etr_unprepare_crashdata,

+};

• const struct coresight_ops tmc_etr_cs_ops = { .sink_ops = &tmc_etr_sink_ops, .panic_ops = &tmc_etr_sync_ops,
• .crashdata_ops = &tmc_etr_crashdata_ops, };

int tmc_read_prepare_etr(struct tmc_drvdata *drvdata) @@ -1910,6 +2043,14 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata) goto out; }

• if (coresight_get_mode(drvdata->csdev) == CS_MODE_READ_CRASHDATA) {

• ret = tmc_read_prepare_crashdata(drvdata);
  

• if (ret)
  

• 	goto out;
  

• else
  

• 	goto mode_valid;
  

• }
• /*
  • We can safely allow reads even if the ETR is operating in PERF mode,
  • since the sysfs session is captured in mode specific data.

@@ -1924,6 +2065,7 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata) if (coresight_get_mode(drvdata->csdev) == CS_MODE_SYSFS) __tmc_etr_disable_hw(drvdata); +mode_valid: drvdata->reading = true; out: spin_unlock_irqrestore(&drvdata->spinlock, flags); @@ -1942,6 +2084,12 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata) spin_lock_irqsave(&drvdata->spinlock, flags);

• if (coresight_get_mode(drvdata->csdev) == CS_MODE_READ_CRASHDATA) {

• sysfs_buf = drvdata->sysfs_crash_buf;
  

• tmc_read_unprepare_crashdata(drvdata);
  

• goto out;
  

• }
• /* RE-enable the TMC if need be */ if (coresight_get_mode(drvdata->csdev) == CS_MODE_SYSFS) { /*

@@ -1959,6 +2107,7 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata) drvdata->sysfs_buf = NULL; } +out: drvdata->reading = false; spin_unlock_irqrestore(&drvdata->spinlock, flags); diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index 35beee53584a..6102eea3fc79 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -201,6 +201,8 @@ struct tmc_resrv_buf {

• @base: memory mapped base address for this component.
• @csdev: component vitals needed by the framework.
• @miscdev: specifics to handle "/dev/xyz.tmc" entry.

• • @crashdev: specifics to handle "/dev/crash_tmc_xyz" entry for reading

• • crash tracedata.
    

  • @spinlock: only one at a time pls.
  • @pid: Process ID of the process being monitored by the session

  • that is using this component.
    

@@ -219,7 +221,10 @@ struct tmc_resrv_buf {

• @idr_mutex: Access serialisation for idr.
• @sysfs_buf: SYSFS buffer for ETR.
• @perf_buf: PERF buffer for ETR.

• • @crash_tbuf: Used by ETR as hardware trace buffer and for trace data

• • @sysfs_crash_buf: Sysfs crashdata buffer for ETR. This is a special purpose

• • buffer that is used only for mapping the trace buffer from
    

• • previous crash and not for capturing trace.
    

• • @crash_tbuf: Used by ETR as hardware trace buffer and for trace data

  • retention (after crash) only when ETR_MODE_RESRV buffer
    

  • mode is enabled. Used by ETF for trace data retention
    

  • (after crash) by default.
    

@@ -231,6 +236,7 @@ struct tmc_drvdata { void __iomem *base; struct coresight_device *csdev; struct miscdevice miscdev;

• struct miscdevice crashdev; spinlock_t spinlock; pid_t pid; bool reading;

@@ -250,6 +256,7 @@ struct tmc_drvdata { struct mutex idr_mutex; struct etr_buf *sysfs_buf; struct etr_buf *perf_buf;

• struct etr_buf *sysfs_crash_buf; struct tmc_resrv_buf crash_tbuf; struct tmc_resrv_buf crash_mdata; };

@@ -301,6 +308,8 @@ void tmc_flush_and_stop(struct tmc_drvdata *drvdata); void tmc_enable_hw(struct tmc_drvdata *drvdata); void tmc_disable_hw(struct tmc_drvdata *drvdata); u32 tmc_get_memwidth_mask(struct tmc_drvdata *drvdata); +int tmc_read_prepare_crashdata(struct tmc_drvdata *drvdata); +int tmc_read_unprepare_crashdata(struct tmc_drvdata *drvdata); /* ETB/ETF functions */ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata); diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 6aa54cdb66a2..a7c9fe27ef40 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -309,6 +309,7 @@ enum cs_mode { CS_MODE_DISABLED, CS_MODE_SYSFS, CS_MODE_PERF,

• CS_MODE_READ_CRASHDATA, /* Trace data from previous crash */ };

#define source_ops(csdev) csdev->ops->source_ops @@ -317,6 +318,7 @@ enum cs_mode { #define helper_ops(csdev) csdev->ops->helper_ops #define ect_ops(csdev) csdev->ops->ect_ops #define panic_ops(csdev) csdev->ops->panic_ops +#define crashdata_ops(csdev) csdev->ops->crashdata_ops /**

• struct coresight_ops_sink - basic operations for a sink

@@ -396,12 +398,23 @@ struct coresight_ops_panic { int (*sync)(struct coresight_device *csdev); }; +/**

• • struct coresight_ops_crashdata - Generic device ops for crashdata mode
• • @prepare : Preparation for reading crashdata mode
• */

+struct coresight_ops_crashdata {

• int (*prepare)(struct coresight_device *csdev);
• int (*unprepare)(struct coresight_device *csdev);

+};

• struct coresight_ops { const struct coresight_ops_sink *sink_ops; const struct coresight_ops_link *link_ops; const struct coresight_ops_source *source_ops; const struct coresight_ops_helper *helper_ops; const struct coresight_ops_panic *panic_ops;
• const struct coresight_ops_crashdata *crashdata_ops; };

static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa,

On 20/06/2024 02:48, Linu Cherian wrote:

On 2024-06-10 at 22:04:16, Suzuki K Poulose (suzuki.poulose@arm.com) wrote:

...

On 05/06/2024 09:17, Linu Cherian wrote:

...

• Introduce a new mode CS_MODE_READ_CRASHDATA for reading trace captured in previous crash/watchdog reset.

I am still not convinced if we really need this new mode. We should :

1. Register the new misc device to expose the "reserved" saved trace

data, only when the metadata is crc checked and it indicates "valid" trace data.

Ack.

...

2. Always allow reading from the "reserved" buffer when the above file

is opened. At open(), once again check if the mdata.valid is true (see below), if not, return -EIO.

3. Whenever we use the buffer in "RESERVE" for a session, also set the

mdata.valid == false. This would prevent any further read from the /dev/crash_xxx device. Which makes sense, as the data is lost.

Well, how about the below changes a. Clear the valid bit only upon user request so that, user is free to do multiple reads of crashdata buffer.

I don't see the need for it. With the proposed changes as long as we don't overwrite the trace using RESERVE mode, the user can read it and they don't need to explicitly clear the field.

The current changes do not prevent the user from overwriting the buffer any way using the RESERVE mode. And we can get rid of the "READ CRASH" mode.

Clearing the mdata.valid can be triggered using a write to the file like echo 0 > /dev/crash_tmc_etxx.

b. In order to prevent the buffer overwrite, when mdata.valid = true * ETR sink mode: dont allow the user to change the buffer mode to "RESERVE" * ETB sink mode: dont copy panic trace data to reserve buffer during kernel panic.

Why ? It is the users responsibility (I would add inittab job to capture the trace) to collect the trace, before you start using the "reserve" mode.

You may additionally prevent the mode change to "RESERVE" if someone has opened the /dev/crashxxx.

...

This way, you don't need to dance with a new mode always provide the reserve buffer contents, if it is valid (with an ongoing RESERVE mode) invalidating the buffer and the change is much much simpler.

In general, we tried to reuse the existing normal trace buffer read operations for the crash data reads as well. Hence we need a way to special case the crash data reads in few places like for eg. tmc_etr/etb_get_sysfs_trace.

I understand, but that is unnecessarily complicating the existing framework.

Its not clear to me if you are suggesting to write parallel independent functions to manage the crash device reads OR its just about relying on drvdata->crash_mdata.valid to special case the common read functions insead of csdev->mode ? Please clarify.

Use parallel independent functions to read from crash device. No new mode. All we need to do when someone opens the /dev/crash--:

1) See if the metdata.valid == true 2) Block further "mode" changes to RESERV_BUF. Hold a refcount 3) Provide the buffer from crash buffer. 4) Upon close()=> drop the refcount from (2)

Related question, does anyone need the "metdata" ? e.g. flush status ? Or do we think that only the trace data is required ?

...

...

• Add special device files for reading ETR/ETF crash data.

• User can read the crash data as below

  For example, for reading crash data from tmc_etf sink

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

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

---

Changelog from v8:

• Added missing exit path in __tmc_probe

• Few whitespace fixes and a checkpatch fix.

  .../coresight/coresight-etm4x-core.c | 1 + .../hwtracing/coresight/coresight-tmc-core.c | 150 ++++++++++++++++- .../hwtracing/coresight/coresight-tmc-etf.c | 72 +++++++++ .../hwtracing/coresight/coresight-tmc-etr.c | 151 +++++++++++++++++- drivers/hwtracing/coresight/coresight-tmc.h | 11 +- include/linux/coresight.h | 13 ++ 6 files changed, 390 insertions(+), 8 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index a0bdfabddbc6..7924883476c6 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1011,6 +1011,7 @@ static void etm4_disable(struct coresight_device *csdev, switch (mode) { case CS_MODE_DISABLED:

• case CS_MODE_READ_CRASHDATA: break; case CS_MODE_SYSFS: etm4_disable_sysfs(csdev);

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index daad08bc693d..0c145477ba66 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -106,6 +106,60 @@ u32 tmc_get_memwidth_mask(struct tmc_drvdata *drvdata) return mask; } +int tmc_read_prepare_crashdata(struct tmc_drvdata *drvdata) +{

• int ret = 0;
• struct tmc_crash_metadata *mdata;
• struct coresight_device *csdev = drvdata->csdev;
• if (!drvdata->crash_mdata.vaddr) {

• ret = -ENOMEM;
  

• goto out;
  

• }
• mdata = drvdata->crash_mdata.vaddr;
• /* Check data integrity of metadata */
• if (mdata->crc32_mdata != find_crash_metadata_crc(mdata)) {

• dev_dbg(&drvdata->csdev->dev,
  

• 	"CRC mismatch in tmc crash metadata\n");
  

• ret = -EINVAL;
  

After the comments above, if at all we retain this, please use -EIO

Ack.

...

...

• goto out;
  

• }
• /* Check data integrity of tracedata */
• if (mdata->crc32_tdata != find_crash_tracedata_crc(drvdata, mdata)) {

• dev_dbg(&drvdata->csdev->dev,
  

• 	"CRC mismatch in tmc crash tracedata\n");
  

• ret = -EINVAL;
  

Same here, -EIO

Ack.

...

...

• goto out;
  

• }
• /* Check for valid metadata */
• if (!mdata->valid) {

• dev_dbg(&drvdata->csdev->dev,
  

• 	"Data invalid in tmc crash metadata\n");
  

• ret = -EINVAL;
  

• goto out;
  

• }

-ENXIO

Ack.

...

...

• /* Sink specific crashdata mode preparation */
• ret = crashdata_ops(csdev)->prepare(csdev);
• if (ret)

• goto out;
  

• if (mdata->sts & 0x1)

If at all we need this, please use TMC_STS_FULL instead of hard coding numbers.

Ack.

...

...

• coresight_insert_barrier_packet(drvdata->buf);
  

+out:

• return ret;

+}

+int tmc_read_unprepare_crashdata(struct tmc_drvdata *drvdata) +{

• struct coresight_device *csdev = drvdata->csdev;
• /* Sink specific crashdata mode preparation */
• return crashdata_ops(csdev)->unprepare(csdev);

+}

• static int tmc_read_prepare(struct tmc_drvdata *drvdata) { int ret = 0;

@@ -156,6 +210,9 @@ static int tmc_open(struct inode *inode, struct file *file) struct tmc_drvdata *drvdata = container_of(file->private_data, struct tmc_drvdata, miscdev);

• if (coresight_get_mode(drvdata->csdev) == CS_MODE_READ_CRASHDATA)

• return -EBUSY;
  

• ret = tmc_read_prepare(drvdata); if (ret) return ret;

@@ -180,13 +237,12 @@ static inline ssize_t tmc_get_sysfs_trace(struct tmc_drvdata *drvdata, return -EINVAL; } -static ssize_t tmc_read(struct file *file, char __user *data, size_t len,

• 	loff_t *ppos)
  

+static ssize_t tmc_read_common(struct tmc_drvdata *drvdata, char __user *data,

• 	       size_t len, loff_t *ppos)
  

  { char *bufp; ssize_t actual;

• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata, miscdev);
  

• actual = tmc_get_sysfs_trace(drvdata, *ppos, len, &bufp); if (actual <= 0) return 0;

@@ -203,6 +259,15 @@ static ssize_t tmc_read(struct file *file, char __user *data, size_t len, return actual; } +static ssize_t tmc_read(struct file *file, char __user *data, size_t len,

• 	loff_t *ppos)
  

+{

• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata, miscdev);
  

• return tmc_read_common(drvdata, data, len, ppos);

+}

• static int tmc_release(struct inode *inode, struct file *file) { int ret;

@@ -225,6 +290,61 @@ static const struct file_operations tmc_fops = { .llseek = no_llseek, }; +static int tmc_crashdata_open(struct inode *inode, struct file *file) +{

• int ret;
• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata,
  

• 				   crashdev);
  

• if (!coresight_take_mode(drvdata->csdev, CS_MODE_READ_CRASHDATA))

• return -EBUSY;
  

• ret = tmc_read_prepare(drvdata);
• if (ret) {

• coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED);
  

• return ret;
  

• }
• nonseekable_open(inode, file);

As mentioned in the beginning, please use "mdata.valid" to deny any open requests and that way you could simplify most of the code and make a single function to handle all of the crash device handling.

Did you meant using drvdata->crash_mdata.valid to special case common functions instead of relying on csdev->mode ? Please clarify.

You may reuse "code" for reading from a buffer. But please do not use a mode to switch buffer. Instead, you could refactor common "code" to a function and pass appropriate buffer to the function.

...

...

• dev_dbg(&drvdata->csdev->dev, "%s: successfully opened\n", __func__);
• return 0;

+}

+static ssize_t tmc_crashdata_read(struct file *file, char __user *data,

• 		  size_t len, loff_t *ppos)
  

+{

• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata,
  

• 				   crashdev);
  

• return tmc_read_common(drvdata, data, len, ppos);

Simply provide the reserve buffer data, instead of creating ETB / ETR specific handling.

The differences between tmc_etb/etr_get_sysfs_trace applies to crash data reads as well. So its not clear to me why we dont need ETB/ETR specific handling.

My point is they all read from the "drvdata->crashbuf" isn't ? If you have a crash_buf_fops, that would apply to ETB/ETR/ETFs as they all have the data stored in the reserved buffer area. It doesn't matter for crash buf. For sysfs trace it is different, because the trace data is at different places. ETB has in internal SRAM, ETR has in RAM etc.

Suzuki

...

...

+}

+static int tmc_crashdata_release(struct inode *inode, struct file *file) +{

• int ret = 0;
• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata,
  

• 				   crashdev);
  

• ret = tmc_read_unprepare(drvdata);
• coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED);
• dev_dbg(&drvdata->csdev->dev, "%s: released\n", __func__);
• return ret;

+}

+static const struct file_operations tmc_crashdata_fops = {

• .owner = THIS_MODULE,
• .open = tmc_crashdata_open,
• .read = tmc_crashdata_read,
• .release = tmc_crashdata_release,
• .llseek = no_llseek,

+};

• static enum tmc_mem_intf_width tmc_get_memwidth(u32 devid) { enum tmc_mem_intf_width memwidth;

@@ -542,6 +662,18 @@ static u32 tmc_etr_get_max_burst_size(struct device *dev) return burst_size; } +static void register_crash_dev_interface(struct tmc_drvdata *drvdata,

• 			 const char *name)
  

+{

• drvdata->crashdev.name =

• devm_kasprintf(&drvdata->csdev->dev, GFP_KERNEL, "%s_%s", "crash", name);
  

• drvdata->crashdev.minor = MISC_DYNAMIC_MINOR;
• drvdata->crashdev.fops = &tmc_crashdata_fops;
• if (misc_register(&drvdata->crashdev))

• dev_dbg(&drvdata->csdev->dev,
  

• 	"Failed to setup user interface for crashdata\n");
  

+}

• static int __tmc_probe(struct device *dev, struct resource *res) { int ret = 0;

@@ -642,8 +774,13 @@ static int __tmc_probe(struct device *dev, struct resource *res) drvdata->miscdev.minor = MISC_DYNAMIC_MINOR; drvdata->miscdev.fops = &tmc_fops; ret = misc_register(&drvdata->miscdev);

• if (ret)

• if (ret) { coresight_unregister(drvdata->csdev);

• goto out;
  

• }
• if (is_tmc_reserved_region_valid(dev))

• register_crash_dev_interface(drvdata, desc.name);
  

Please could we only expose this when the reserved region has valid metadata, crc checked etc ? That way the users don't have to read it to figure out if it is valid or not. (Please note we need to use the valid bit check to avoid reading over-written data).

Sure. Will take care.

Thanks. Linu Cherian.

...

Suzuki

...

out: return ret; } @@ -672,7 +809,8 @@ static void tmc_shutdown(struct amba_device *adev) spin_lock_irqsave(&drvdata->spinlock, flags);

• if (coresight_get_mode(drvdata->csdev) == CS_MODE_DISABLED)

• if ((coresight_get_mode(drvdata->csdev) == CS_MODE_DISABLED) ||

•    (coresight_get_mode(drvdata->csdev) == CS_MODE_READ_CRASHDATA))
  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 f9569585e9f8..a8cabbf6679b 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -657,6 +657,56 @@ static int tmc_panic_sync_etf(struct coresight_device *csdev) return 0; } +static int tmc_etb_setup_crashdata_buf(struct tmc_drvdata *drvdata) +{

• unsigned long size;
• struct tmc_crash_metadata *mdata;
• struct device *dev = &drvdata->csdev->dev;
• mdata = drvdata->crash_mdata.vaddr;
• size = mdata->size << 2;
• /*

• * Buffer address given by metadata for retrieval of trace data
  

• * from previous boot is expected to be same as the reserved
  

• * trace buffer memory region provided through DTS
  

• */
  

• if (is_tmc_reserved_region_valid(dev->parent)

•    && (drvdata->crash_tbuf.paddr == mdata->trc_paddr))
  

• drvdata->buf = drvdata->crash_tbuf.vaddr;
  

• else {

• dev_dbg(dev, "Trace buffer address of previous boot invalid\n");
  

• return -EINVAL;
  

• }
• drvdata->len = size;
• return 0;

+}

+static void tmc_etb_free_crashdata_buf(struct tmc_drvdata *drvdata) +{

• void *buf = drvdata->buf;
• if (!buf)

• return;
  

• drvdata->buf = NULL;

+}

+static int tmc_etb_prepare_crashdata(struct coresight_device *csdev) +{

• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• return tmc_etb_setup_crashdata_buf(drvdata);

+}

+static int tmc_etb_unprepare_crashdata(struct coresight_device *csdev) +{

• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• tmc_etb_free_crashdata_buf(drvdata);
• return 0;

+}

• static const struct coresight_ops_sink tmc_etf_sink_ops = { .enable = tmc_enable_etf_sink, .disable = tmc_disable_etf_sink,

@@ -674,6 +724,11 @@ static const struct coresight_ops_panic tmc_etf_sync_ops = { .sync = tmc_panic_sync_etf, }; +static const struct coresight_ops_crashdata tmc_etf_crashdata_ops = {

• .prepare = tmc_etb_prepare_crashdata,
• .unprepare = tmc_etb_unprepare_crashdata,

+};

• const struct coresight_ops tmc_etb_cs_ops = { .sink_ops = &tmc_etf_sink_ops, };

@@ -682,6 +737,7 @@ 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,

• .crashdata_ops = &tmc_etf_crashdata_ops, }; int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)

@@ -702,6 +758,14 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata) goto out; }

• if (coresight_get_mode(drvdata->csdev) == CS_MODE_READ_CRASHDATA) {

• ret = tmc_read_prepare_crashdata(drvdata);
  

• if (ret)
  

• 	goto out;
  

• else
  

• 	goto mode_valid;
  

• }
• /* Don't interfere if operated from Perf */ if (coresight_get_mode(drvdata->csdev) == CS_MODE_PERF) { ret = -EINVAL;

@@ -725,6 +789,7 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata) __tmc_etb_disable_hw(drvdata); } +mode_valid: drvdata->reading = true; out: spin_unlock_irqrestore(&drvdata->spinlock, flags); @@ -746,6 +811,13 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata) spin_lock_irqsave(&drvdata->spinlock, flags);

• if (coresight_get_mode(drvdata->csdev) == CS_MODE_READ_CRASHDATA) {

• tmc_read_unprepare_crashdata(drvdata);
  

• drvdata->reading = false;
  

• spin_unlock_irqrestore(&drvdata->spinlock, flags);
  

• return 0;
  

• }
• /* Re-enable the TMC if need be */ if (coresight_get_mode(drvdata->csdev) == CS_MODE_SYSFS) { /* There is no point in reading a TMC in HW FIFO mode */

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index be1079e8fd64..b12ae7cc3372 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1160,7 +1160,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 (coresight_get_mode(drvdata->csdev) == CS_MODE_READ_CRASHDATA)

• etr_buf = drvdata->sysfs_crash_buf;
  

• else

• etr_buf = drvdata->sysfs_buf;
  

  if (pos + actual > etr_buf->len) actual = etr_buf->len - pos;

@@ -1878,6 +1883,128 @@ static int tmc_panic_sync_etr(struct coresight_device *csdev) return 0; } +static int tmc_etr_setup_crashdata_buf(struct tmc_drvdata *drvdata) +{

• int rc = 0;
• struct etr_buf *etr_buf;
• struct etr_flat_buf *resrv_buf;
• struct tmc_crash_metadata *mdata;
• struct device *dev = &drvdata->csdev->dev;
• mdata = drvdata->crash_mdata.vaddr;
• etr_buf = kzalloc(sizeof(*etr_buf), GFP_ATOMIC);
• if (!etr_buf) {

• rc = -ENOMEM;
  

• goto out;
  

• }
• etr_buf->size = drvdata->crash_tbuf.size;
• resrv_buf = kzalloc(sizeof(*resrv_buf), GFP_ATOMIC);
• if (!resrv_buf) {

• rc = -ENOMEM;
  

• goto rmem_err;
  

• }
• /*

• * Buffer address given by metadata for retrieval of trace data
  

• * from previous boot is expected to be same as the reserved
  

• * trace buffer memory region provided through DTS
  

• */
  

• if (is_tmc_reserved_region_valid(dev->parent)

•    && (drvdata->crash_tbuf.paddr == mdata->trc_paddr))
  

• resrv_buf->vaddr = drvdata->crash_tbuf.vaddr;
  

• else {

• dev_dbg(dev, "Trace buffer address of previous boot invalid\n");
  

• rc = -EINVAL;
  

• goto map_err;
  

• }
• resrv_buf->size = etr_buf->size;
• resrv_buf->dev = &drvdata->csdev->dev;
• etr_buf->mode = ETR_MODE_RESRV;
• etr_buf->private = resrv_buf;
• etr_buf->ops = etr_buf_ops[ETR_MODE_RESRV];
• drvdata->sysfs_crash_buf = etr_buf;
• return 0;

+map_err:

• kfree(resrv_buf);

+rmem_err:

• kfree(etr_buf);

+out:

• return rc;

+}

+static int tmc_etr_sync_crashdata_buf(struct tmc_drvdata *drvdata) +{

• u32 status;
• u64 rrp, rwp, dba;
• struct tmc_crash_metadata *mdata;
• struct etr_buf *etr_buf = drvdata->sysfs_crash_buf;
• mdata = drvdata->crash_mdata.vaddr;
• rrp = mdata->rrp;
• rwp = mdata->rwp;
• dba = mdata->dba;
• status = mdata->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;
  

• /* Additional sanity checks for validating metadata */
• if ((etr_buf->offset > etr_buf->size) ||

•    (etr_buf->len > etr_buf->size)) {
  

• dev_dbg(&drvdata->csdev->dev,
  

• 	"Offset and length invalid in tmc crash metadata\n");
  

• return -EINVAL;
  

• }
• return 0;

+}

+static void tmc_etr_free_crashdata_buf(struct tmc_drvdata *drvdata) +{

• struct etr_buf *etr_buf = drvdata->sysfs_crash_buf;
• if (!etr_buf)

• return;
  

• drvdata->sysfs_crash_buf = NULL;

+}

+static int tmc_etr_prepare_crashdata(struct coresight_device *csdev) +{

• int ret = 0;
• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• ret = tmc_etr_setup_crashdata_buf(drvdata);
• if (ret)

• goto out;
  

• ret = tmc_etr_sync_crashdata_buf(drvdata);

+out:

• return ret;

+}

+static int tmc_etr_unprepare_crashdata(struct coresight_device *csdev) +{

• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• tmc_etr_free_crashdata_buf(drvdata);
• return 0;

+}

• static const struct coresight_ops_sink tmc_etr_sink_ops = { .enable = tmc_enable_etr_sink, .disable = tmc_disable_etr_sink,

@@ -1890,9 +2017,15 @@ static const struct coresight_ops_panic tmc_etr_sync_ops = { .sync = tmc_panic_sync_etr, }; +static const struct coresight_ops_crashdata tmc_etr_crashdata_ops = {

• .prepare = tmc_etr_prepare_crashdata,
• .unprepare = tmc_etr_unprepare_crashdata,

+};

• const struct coresight_ops tmc_etr_cs_ops = { .sink_ops = &tmc_etr_sink_ops, .panic_ops = &tmc_etr_sync_ops,
• .crashdata_ops = &tmc_etr_crashdata_ops, }; int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)

@@ -1910,6 +2043,14 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata) goto out; }

• if (coresight_get_mode(drvdata->csdev) == CS_MODE_READ_CRASHDATA) {

• ret = tmc_read_prepare_crashdata(drvdata);
  

• if (ret)
  

• 	goto out;
  

• else
  

• 	goto mode_valid;
  

• }
• /*
  • We can safely allow reads even if the ETR is operating in PERF mode,
  • since the sysfs session is captured in mode specific data.

@@ -1924,6 +2065,7 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata) if (coresight_get_mode(drvdata->csdev) == CS_MODE_SYSFS) __tmc_etr_disable_hw(drvdata); +mode_valid: drvdata->reading = true; out: spin_unlock_irqrestore(&drvdata->spinlock, flags); @@ -1942,6 +2084,12 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata) spin_lock_irqsave(&drvdata->spinlock, flags);

• if (coresight_get_mode(drvdata->csdev) == CS_MODE_READ_CRASHDATA) {

• sysfs_buf = drvdata->sysfs_crash_buf;
  

• tmc_read_unprepare_crashdata(drvdata);
  

• goto out;
  

• }
• /* RE-enable the TMC if need be */ if (coresight_get_mode(drvdata->csdev) == CS_MODE_SYSFS) { /*

@@ -1959,6 +2107,7 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata) drvdata->sysfs_buf = NULL; } +out: drvdata->reading = false; spin_unlock_irqrestore(&drvdata->spinlock, flags); diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index 35beee53584a..6102eea3fc79 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -201,6 +201,8 @@ struct tmc_resrv_buf { * @base: memory mapped base address for this component. * @csdev: component vitals needed by the framework. * @miscdev: specifics to handle "/dev/xyz.tmc" entry.

• • @crashdev: specifics to handle "/dev/crash_tmc_xyz" entry for reading

• • crash tracedata.
    

    • @spinlock: only one at a time pls.
    • @pid: Process ID of the process being monitored by the session

    • that is using this component.
      

@@ -219,7 +221,10 @@ struct tmc_resrv_buf { * @idr_mutex: Access serialisation for idr. * @sysfs_buf: SYSFS buffer for ETR. * @perf_buf: PERF buffer for ETR.

• • @crash_tbuf: Used by ETR as hardware trace buffer and for trace data

• • @sysfs_crash_buf: Sysfs crashdata buffer for ETR. This is a special purpose

• • buffer that is used only for mapping the trace buffer from
    

• • previous crash and not for capturing trace.
    

• • @crash_tbuf: Used by ETR as hardware trace buffer and for trace data

    • retention (after crash) only when ETR_MODE_RESRV buffer
      

    • mode is enabled. Used by ETF for trace data retention
      

    • (after crash) by default.
      

@@ -231,6 +236,7 @@ struct tmc_drvdata { void __iomem *base; struct coresight_device *csdev; struct miscdevice miscdev;

• struct miscdevice crashdev; spinlock_t spinlock; pid_t pid; bool reading;

@@ -250,6 +256,7 @@ struct tmc_drvdata { struct mutex idr_mutex; struct etr_buf *sysfs_buf; struct etr_buf *perf_buf;

• struct etr_buf *sysfs_crash_buf; struct tmc_resrv_buf crash_tbuf; struct tmc_resrv_buf crash_mdata; };

@@ -301,6 +308,8 @@ void tmc_flush_and_stop(struct tmc_drvdata *drvdata); void tmc_enable_hw(struct tmc_drvdata *drvdata); void tmc_disable_hw(struct tmc_drvdata *drvdata); u32 tmc_get_memwidth_mask(struct tmc_drvdata *drvdata); +int tmc_read_prepare_crashdata(struct tmc_drvdata *drvdata); +int tmc_read_unprepare_crashdata(struct tmc_drvdata *drvdata); /* ETB/ETF functions */ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata); diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 6aa54cdb66a2..a7c9fe27ef40 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -309,6 +309,7 @@ enum cs_mode { CS_MODE_DISABLED, CS_MODE_SYSFS, CS_MODE_PERF,

• CS_MODE_READ_CRASHDATA, /* Trace data from previous crash */ }; #define source_ops(csdev) csdev->ops->source_ops

@@ -317,6 +318,7 @@ enum cs_mode { #define helper_ops(csdev) csdev->ops->helper_ops #define ect_ops(csdev) csdev->ops->ect_ops #define panic_ops(csdev) csdev->ops->panic_ops +#define crashdata_ops(csdev) csdev->ops->crashdata_ops /** * struct coresight_ops_sink - basic operations for a sink @@ -396,12 +398,23 @@ struct coresight_ops_panic { int (*sync)(struct coresight_device *csdev); }; +/**

• • struct coresight_ops_crashdata - Generic device ops for crashdata mode
• • @prepare : Preparation for reading crashdata mode
• */

+struct coresight_ops_crashdata {

• int (*prepare)(struct coresight_device *csdev);
• int (*unprepare)(struct coresight_device *csdev);

+};

• struct coresight_ops { const struct coresight_ops_sink *sink_ops; const struct coresight_ops_link *link_ops; const struct coresight_ops_source *source_ops; const struct coresight_ops_helper *helper_ops; const struct coresight_ops_panic *panic_ops;
• const struct coresight_ops_crashdata *crashdata_ops; }; static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa,

---

linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel