Re: [PATCH v10 5/8] coresight: tmc: Add support for reading crash data

Hi Suzuki,

On 2024-10-29 at 11:51:12, Linu Cherian (lcherian@marvell.com) wrote:

Hi Suzuki,

On 2024-10-21 at 18:10:40, Linu Cherian (lcherian@marvell.com) wrote:

quoted

Hi Suzuki,

On 2024-10-18 at 15:16:17, Suzuki K Poulose (suzuki.poulose@arm.com) wrote:

quoted

On 17/10/2024 12:40, Linu Cherian wrote:

quoted

On 2024-10-03 at 18:55:54, Suzuki K Poulose (suzuki.poulose@arm.com) wrote:

quoted

Hi Linu

On 16/09/2024 11:34, Linu Cherian wrote:

quoted

* Add support for reading crashdata using special device files.
    The special device files /dev/crash_tmc_xxx would be available
    for read file operation only when the crash data is valid.

* 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

There are some comments below, please take a look.

quoted

Signed-off-by: Anil Kumar Reddy <redacted>
Signed-off-by: Tanmay Jagdale <redacted>
Signed-off-by: Linu Cherian <redacted>
---
Changelog from v9:
- Removed READ_CRASHDATA mode meant for special casing crashdata read
- Added new fields full, len, offset to struct tmc_resrv_buf

Why do we need "full" ? See more on that below.

quoted

    so as to have a common read function for ETR and ETF
- Introduced read file operation, tmc_crashdata_read
    specific to crashdata reads common for both ETR and ETF
- Introduced is_tmc_crashdata_valid function
    Special device file /dev/crash_tmc_xxx will be available only when
    crashdata is valid.
- Version checks added to crashdata validity checks
- Mark crashdata as invalid when user starts tracing with ETR sink in
    "resrv" buffer mode

   .../hwtracing/coresight/coresight-tmc-core.c  | 206 +++++++++++++++++-
   .../hwtracing/coresight/coresight-tmc-etf.c   |  36 +++
   .../hwtracing/coresight/coresight-tmc-etr.c   |  63 ++++++
   drivers/hwtracing/coresight/coresight-tmc.h   |  18 +-
   include/linux/coresight.h                     |  12 +
   5 files changed, 333 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
index 54bf8ae2bff8..47b6b3f88750 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-core.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-core.c

@@ -105,6 +105,125 @@ u32 tmc_get_memwidth_mask(struct tmc_drvdata *drvdata)
   	return mask;
   }
+bool is_tmc_crashdata_valid(struct tmc_drvdata *drvdata)
+{
+	struct tmc_crash_metadata *mdata;
+
+	if (!tmc_has_reserved_buffer(drvdata) ||
+	    !tmc_has_crash_mdata_buffer(drvdata))
+		return false;
+
+	mdata = drvdata->crash_mdata.vaddr;
+
+	/* Check version match */
+	if (mdata->version != CS_CRASHDATA_VERSION)
+		return false;
+
+	/* 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");
+		return false;
+	}
+	/* 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");
+		return false;
+	}
+	/* Check for valid metadata */
+	if (!mdata->valid) {

minor nit: This could be checked right after the VERSION and we verify
the CRC anyway later and thus could skip all the CRC calculations if
!valid.

Ack.

quoted

quoted

+		dev_dbg(&drvdata->csdev->dev,
+			"Data invalid in tmc crash metadata\n");
+		return false;
+	}
+
+	return true;
+}
+
+int tmc_read_prepare_crashdata(struct tmc_drvdata *drvdata)
+{
+	int ret = 0;
+	unsigned long flags;
+	struct tmc_crash_metadata *mdata;
+	struct coresight_device *csdev = drvdata->csdev;
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+
+	if (!is_tmc_crashdata_valid(drvdata)) {
+		ret = -ENXIO;
+		goto out;
+	}
+
+	mdata = drvdata->crash_mdata.vaddr;
+	/*
+	 * 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 (drvdata->resrv_buf.paddr != mdata->trace_paddr) {
+		dev_dbg(&csdev->dev, "Trace buffer address of previous boot invalid\n");

Couldn't this be made part of the "is_tmc_crashdata_valid()" and not
repeated everytime we do the read ? Surely, this can't change after
boot.

Ack. Will move.

quoted

quoted

+		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);
+
+	drvdata->reading = true;

Why are we dealing with drvdata->reading ? That is supposed to be only
for the normal trace reading ?

Ack. Will remove, we dont need this.

Looked into this further and it seems like, retaining the
drvdata->reading flags would be useful to avoid simultaneous crashdata reads
and regular trace capture sessions using reserve buffers.

Something like this,

+static int buf_mode_set_resrv(struct tmc_drvdata *drvdata)
+{
+	unsigned long flags;
+	int err = 0;
+
+	/* Ensure there are no active crashdata read sessions */
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+	if (!drvdata->reading) {
+		tmc_crashdata_set_invalid(drvdata);
+		drvdata->etr_mode = ETR_MODE_RESRV;
+
+	} else
+		err = -EINVAL;
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+	return err;
+}
+
 static ssize_t buf_mode_preferred_store(struct device *dev,
 					  struct device_attribute *attr,
 					  const char *buf, size_t size)
 {
 	struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent);
 	struct etr_buf_hw buf_hw;
 
 	get_etr_buf_hw(dev, &buf_hw);

@@ -2039,7 +2050,7 @@ static ssize_t buf_mode_preferred_store(struct device *dev,
 	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;
+		return buf_mode_set_resrv(drvdata);
 	else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_AUTO]))
 		drvdata->etr_mode = ETR_MODE_AUTO;
 	else

Was acutally calling the tmc_crashdata_set_invalid in the wrong place in v10 patch.