On 05/06/2024 17:09, James Clark 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.
>>
>> * 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@xxxxxxxxxxx>
>> Signed-off-by: Tanmay Jagdale <tanmay@xxxxxxxxxxx>
>> Signed-off-by: Linu Cherian <lcherian@xxxxxxxxxxx>
>> ---
>> 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;
>> + 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;
>> + 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;
>> + }
>> +
>> + /* 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);
>> +
>> +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);
>> +
>> + 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);
>> +}
>> +
>> +static int tmc_crashdata_release(struct inode *inode, struct file *file)
>> +{
>> + int ret = 0;
>> + struct tmc_drvdata *drvdata = container_of(file->private_data,
>> + struct tmc_drvdata,
>> + crashdev);
>> +
>> + ret = tmc_read_unprepare(drvdata);
>> +
>> + coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED);
>> +
>> + dev_dbg(&drvdata->csdev->dev, "%s: released\n", __func__);
>> + return ret;
>> +}
>> +
>> +static const struct file_operations tmc_crashdata_fops = {
>> + .owner = THIS_MODULE,
>> + .open = tmc_crashdata_open,
>> + .read = tmc_crashdata_read,
>> + .release = tmc_crashdata_release,
>> + .llseek = no_llseek,
>> +};
>> +
>> static enum tmc_mem_intf_width tmc_get_memwidth(u32 devid)
>> {
>> enum tmc_mem_intf_width memwidth;
>> @@ -542,6 +662,18 @@ static u32 tmc_etr_get_max_burst_size(struct device *dev)
>> return burst_size;
>> }
>>
>> +static void register_crash_dev_interface(struct tmc_drvdata *drvdata,
>> + const char *name)
>> +{
>> + drvdata->crashdev.name =
>> + devm_kasprintf(&drvdata->csdev->dev, GFP_KERNEL, "%s_%s", "crash", name);
>> + drvdata->crashdev.minor = MISC_DYNAMIC_MINOR;
>> + drvdata->crashdev.fops = &tmc_crashdata_fops;
>> + if (misc_register(&drvdata->crashdev))
>> + dev_dbg(&drvdata->csdev->dev,
>> + "Failed to setup user interface for crashdata\n");
>> +}
>> +
>> static int __tmc_probe(struct device *dev, struct resource *res)
>> {
>> int ret = 0;
>> @@ -642,8 +774,13 @@ static int __tmc_probe(struct device *dev, struct resource *res)
>> drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
>> drvdata->miscdev.fops = &tmc_fops;
>> ret = misc_register(&drvdata->miscdev);
>> - if (ret)
>> + if (ret) {
>> coresight_unregister(drvdata->csdev);
>> + goto out;
>> + }
>> +
>> + if (is_tmc_reserved_region_valid(dev))
>> + register_crash_dev_interface(drvdata, desc.name);
>
> I think this would be better if it checked the CRC of the metadata in
> the same way it does before reading the file.
>
> Now we have two forms of "region valid", one that's any non-zero value,
> and the other "really valid" one. And because we don't check the CRC
> here we register a device that can't be used.
>
> I found it a bit confusing because without enabling debug prints I
> didn't know why the file couldn't be read. So I wasn't sure if it was
> because it wasn't valid or some other reason.
>
> I also wasn't able to get a valid region after booting the crash kernel.
> But maybe the memory isn't preserved across the reboot on my Juno, so I
> don't think that's necessarily an issue?
Ok so I double checked by writing 0x123456 into the reserved region and
confirmed that it _is_ preserved when booting the panic kernel on my
Juno. So I'm not sure why I wasn't able to read out the crash dump.
I did see the "success" message from tmc_panic_sync_etr() at least some
of the times, although I do remember it not printing out every time. I
don't know if this is just an issue with outputting to serial after a
panic or something else was going on?
Did you ever see the success message not print out? Or not able to read
back the data when you were testing it?
