Hi James,
> -----Original Message-----
> From: James Clark <james.clark@xxxxxxx>
> Sent: Friday, April 12, 2024 3:36 PM
> To: Linu Cherian <lcherian@xxxxxxxxxxx>; Suzuki K Poulose
> <suzuki.poulose@xxxxxxx>
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; coresight@xxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> krzysztof.kozlowski+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; Sunil Kovvuri Goutham
> <sgoutham@xxxxxxxxxxx>; George Cherian <gcherian@xxxxxxxxxxx>; Anil
> Kumar Reddy H <areddy3@xxxxxxxxxxx>; Tanmay Jagdale
> <tanmay@xxxxxxxxxxx>; mike.leach@xxxxxxxxxx; leo.yan@xxxxxxxxxx
> Subject: [EXTERNAL] Re: [PATCH v7 5/7] coresight: tmc: Add support for
> reading crash data
>
> Prioritize security for external emails: Confirm sender and content safety
> before clicking links or opening attachments
>
> ----------------------------------------------------------------------
>
>
> On 07/03/2024 03:36, 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 v6:
> > * Removed read_prevboot flag in sysfs
> > * Added special device files for reading crashdata
> > * Renamed CS mode READ_PREVBOOT to READ_CRASHDATA
> > * Setting the READ_CRASHDATA mode is done as part of file open.
> >
>
> [...]
>
> > @@ -619,6 +740,19 @@ static int tmc_probe(struct amba_device *adev,
> const struct amba_id *id)
> > coresight_unregister(drvdata->csdev);
> > else
> > pm_runtime_put(&adev->dev);
> > +
> > + if (!is_tmc_reserved_region_valid(dev))
> > + goto out;
> > +
> > + drvdata->crashdev.name =
> > + devm_kasprintf(dev, GFP_KERNEL, "%s_%s", "crash",
> desc.name);
> > + drvdata->crashdev.minor = MISC_DYNAMIC_MINOR;
> > + drvdata->crashdev.fops = &tmc_crashdata_fops;
> > + ret = misc_register(&drvdata->crashdev);
> > + if (ret)
> > + pr_err("%s: Failed to setup dev interface for crashdata\n",
> > + desc.name);
> > +
>
> Is this all optional after the is_tmc_reserved_region_valid()? Skipping to out
> seems to be more like an error condition, but in this case it's not? Having it
> like this makes it more difficult to add extra steps to the probe function. You
> could move it to a function and flip the condition which would be clearer:
>
Ack.
> if (is_tmc_reserved_region_valid(dev))
> register_crash_dev_interface(drvdata);
>
>
> > out:
> > return ret;
> > }
>
> [...]
>
> >
> > +static int tmc_etr_setup_crashdata_buf(struct tmc_drvdata *drvdata) {
> > + int rc = 0;
> > + u64 trace_addr;
> > + struct etr_buf *etr_buf;
> > + struct etr_flat_buf *resrv_buf;
> > + struct tmc_crash_metadata *mdata;
> > + struct device *dev = &drvdata->csdev->dev;
> > +
> > + mdata = drvdata->crash_mdata.vaddr;
> > +
> > + etr_buf = kzalloc(sizeof(*etr_buf), GFP_ATOMIC);
> > + if (!etr_buf) {
> > + rc = -ENOMEM;
> > + goto out;
> > + }
> > + etr_buf->size = drvdata->crash_tbuf.size;
> > +
> > + resrv_buf = kzalloc(sizeof(*resrv_buf), GFP_ATOMIC);
> > + if (!resrv_buf) {
> > + rc = -ENOMEM;
> > + goto rmem_err;
> > + }
> > +
> > + /*
> > + * 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 (is_tmc_reserved_region_valid(dev->parent)
> > + && (drvdata->crash_tbuf.paddr == mdata->trc_paddr))
> > + resrv_buf->vaddr = drvdata->crash_tbuf.vaddr;
> > + else {
> > + dev_dbg(dev, "Trace buffer address of previous boot
> invalid\n");
> > + rc = -EINVAL;
> > + goto map_err;
> > + }
> > +
> > + resrv_buf->size = etr_buf->size;
> > + resrv_buf->dev = &drvdata->csdev->dev;
> > + etr_buf->hwaddr = trace_addr;
>
> trace_addr is uninitialised at this point. If you pull the latest coresight branch
> we enabled some extra compiler warnings which catch this.
>
> I assume it's supposed to be mdata->trc_paddr?
Since no DMA operation happens here, its supposed to be kept NULL.
Since it was redundant for crash data read operation, missed catching this. Will fix this.
>
> Is there some kind of test that could be added that could have caught this?
> Maybe skip the full reboot flow but just enable the feature and see if we can
> read from the buffer.
As pointed above this field is redundant for crashdata read mode. So its not a functionality breakage as such.
>
> Or even just toggling the mode on and off via sysfs. We're running the Perf
> and kselftests on Juno internally so I can add a reserved region to the Juno DT
> and make sure this stays working.
Did you meant adding a kselftest for tracing and reading back tracedata in sysfs mode using the reserved region ?
Thanks
Linu Cherian.
