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:
Hi Suzuki,
On 2024-10-18 at 15:16:17, Suzuki K Poulose (suzuki.poulose@arm.com) wrote:
On 17/10/2024 12:40, Linu Cherian wrote:
On 2024-10-03 at 18:55:54, Suzuki K Poulose (suzuki.poulose@arm.com) wrote:
Hi Linu
On 16/09/2024 11:34, Linu Cherian wrote:
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.
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 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.
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.
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.
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.