Hi Suzuki,
On 2024-10-29 at 11:51:12, Linu Cherian (lcherian@xxxxxxxxxxx) wrote:
> Hi Suzuki,
>
> On 2024-10-21 at 18:10:40, Linu Cherian (lcherian@xxxxxxxxxxx) wrote:
> > Hi Suzuki,
> >
> > On 2024-10-18 at 15:16:17, Suzuki K Poulose (suzuki.poulose@xxxxxxx) wrote:
> > > On 17/10/2024 12:40, Linu Cherian wrote:
> > > > On 2024-10-03 at 18:55:54, Suzuki K Poulose (suzuki.poulose@xxxxxxx) 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@xxxxxxxxxxx>
> > > > > > Signed-off-by: Tanmay Jagdale <tanmay@xxxxxxxxxxx>
> > > > > > Signed-off-by: Linu Cherian <lcherian@xxxxxxxxxxx>
> > > > > > ---
> > > > > > 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.
