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

[Linu Cherian <lcherian@marvell.com>]

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.