Message-ID: <20240711054932.GA264176@hyd1403.caveonetworks.com>
Date: Thu, 11 Jul 2024 11:19:32 +0530
From: Linu Cherian <lcherian@...vell.com>
To: Suzuki K Poulose <suzuki.poulose@....com>
CC: <mike.leach@...aro.org>, <james.clark@....com>,
        <linux-arm-kernel@...ts.infradead.org>, <coresight@...ts.linaro.org>,
        <linux-kernel@...r.kernel.org>, <robh+dt@...nel.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
        <devicetree@...r.kernel.org>, <sgoutham@...vell.com>,
        <gcherian@...vell.com>, Anil Kumar Reddy <areddy3@...vell.com>,
        Tanmay Jagdale <tanmay@...vell.com>
Subject: Re: [PATCH v9 5/7] coresight: tmc: Add support for reading crash data

Hi Suzuki,

On 2024-07-03 at 15:09:42, Suzuki K Poulose (suzuki.poulose@....com) wrote:
> On 03/07/2024 05:29, Linu Cherian wrote:
> > 
> > Hi Suzuki,
> > 
> > Sorry for the late reply.
> > 
> > On 2024-06-21 at 16:24:45, Suzuki K Poulose (suzuki.poulose@....com) wrote:
> > > On 20/06/2024 02:48, Linu Cherian wrote:
> > > > On 2024-06-10 at 22:04:16, Suzuki K Poulose (suzuki.poulose@....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.
> > 
> > Okay Agree. I misunderstood your previous comment.
> > 
> > 
> > > 
> > > > 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.
> > 
> > Agree.
> > 
> > > 
> > > > 
> > > > > 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 ?
> > > 
> > 
> > Only trace data would be required similar to how the sysfs reads are
> > done. Currently metadata is consumed only by the driver at the time of
> > buffer read for calculating offsets etc.
> 
> How about, other info: e.g., was there a flush failure ? The trace data may
> not be reliable if there was flush failure. We could leave it to the
> user to figure out from the trace read ?
> 

Just trying to understand, only the last few trace data bytes left over in internal buffer
would be lost. Correct ? OR Do you think there is a possiblity of more serious corruption
to make the whole trace data unreliable with flush failure(w.r.t ETR sink case). 

But regardless of how flush failure affects the trace data, agree that it would be better to
give the user the complete picture of trace data status. Either we can export the complete
register dump(from metadata) through sysfs or we could just print a
warning to the user indicating that there was a flush failure at the
time of crashdata read by the user. 

Would be simpler to add the flush failure notification warning.


> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > > * 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@...vell.com>
> > > > > > Signed-off-by: Tanmay Jagdale <tanmay@...vell.com>
> > > > > > Signed-off-by: Linu Cherian <lcherian@...vell.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.
> > 
> > Okay. Got it. Will rework accordingly.
> > 
> > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > +
> > > > > > +	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.
> > > 
> > 
> > The difference that i was trying to point out was on the offset part.
> > For ETB sink mode, the start offset is always 0 and for ETR its based on RWP
> > and hence the changes. So my understanding is that we still need to have
> > seperate functions for tmc_etb/etr_get_sysfs_trace while reading from
> > the reserved buffer area. CMIIWH.
> 
> We don't. We simply use a single function:
> 
> /*
>  * Read from crash_buf @size bytes from @offset and copy to @target_buf
>  * @mdata describes the metadata for the crash buffer.
>  *
>  */
> tmc_read_crash_buf(crash_buf, mdata, offset, size, target_buf)
> {
> 	real_offset = mdata.first_byte; // RRP for ETR. 0 for ETB
> 	real_size = mdata.size; // Size for the total buffer.
> 
> 	to_read = min(size, real_size - real_offset);
> 
> 	copy_from(crash_buf + real_offset), to_read bytes => target_buf
> }
> 

Okay, will rework for a common function that takes care of the offset related
changes as suggested.

> Suzuki
> 
> 
> 

Powered by blists - more mailing lists