Hi James,
> -----Original Message-----
> From: James Clark <james.clark@xxxxxxx>
> Sent: Friday, May 31, 2024 3:33 PM
> To: Linu Cherian <lcherian@xxxxxxxxxxx>
> 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>; suzuki.poulose@xxxxxxx; mike.leach@xxxxxxxxxx
> Subject: [EXTERNAL] Re: [PATCH v8 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 31/05/2024 05:27, 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 v7:
> > * Moved crash dev registration into new function,
> > register_crash_dev_interface
> > * Removed redundant variable trace_addr in
> > tmc_etr_setup_crashdata_buf
> >
> > .../coresight/coresight-etm4x-core.c | 1 +
> > .../hwtracing/coresight/coresight-tmc-core.c | 148 ++++++++++++++++-
> > .../hwtracing/coresight/coresight-tmc-etf.c | 73 +++++++++
> > .../hwtracing/coresight/coresight-tmc-etr.c | 152 +++++++++++++++++-
> > drivers/hwtracing/coresight/coresight-tmc.h | 11 +-
> > include/linux/coresight.h | 13 ++
> > 6 files changed, 391 insertions(+), 7 deletions(-)
> >
>
> [...]
>
> > +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 amba_device *adev, const struct amba_id
> > *id) {
> > int ret = 0;
> > @@ -619,6 +752,10 @@ 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))
> > + register_crash_dev_interface(drvdata, desc.name);
> > +
>
> Now that you've added an extra step to the probe you need to add a "goto
> out" when the previous step fails:
>
> if (ret) {
> coresight_unregister(drvdata->csdev);
> + goto out;
> }
>
> That seems to be the pattern in the rest of the function. Otherwise you
> might register the interface on no device.
>
> There's also a conflict here and in some other places. Since this has to go
> through the coresight branch you should base it off of coresight-next.
>
Ack.
> > out:
> > return ret;
> > }
> > @@ -630,7 +767,8 @@ static void tmc_shutdown(struct amba_device
> *adev)
> >
> > spin_lock_irqsave(&drvdata->spinlock, flags);
> >
> > - if (coresight_get_mode(drvdata->csdev) == CS_MODE_DISABLED)
> > + if ((coresight_get_mode(drvdata->csdev) == CS_MODE_DISABLED)
> ||
> > + (coresight_get_mode(drvdata->csdev) ==
> CS_MODE_READ_CRASHDATA))
> > goto out;
> >
> > if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) diff --git
> > a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > index f9569585e9f8..655c0c0ba54b 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > @@ -657,6 +657,56 @@ static int tmc_panic_sync_etf(struct
> coresight_device *csdev)
> > return 0;
> > }
> >
>
> [...]
>
> > @@ -725,6 +789,7 @@ int tmc_read_prepare_etb(struct tmc_drvdata
> *drvdata)
> > __tmc_etb_disable_hw(drvdata);
> > }
> >
> > +mode_valid:
> > drvdata->reading = true;
> > out:
> > spin_unlock_irqrestore(&drvdata->spinlock, flags); @@ -744,8
> +809,16
> > @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
> > drvdata->config_type != TMC_CONFIG_TYPE_ETF))
> > return -EINVAL;
> >
> > +
>
> Whitespace change. There are a couple of other minor checkpatch style
> warnings.
Ack.
>
> The rest looks good to me. All the tests are passing. I still think we should all
> the kself test for this mode, but that can be done later.
Sure. Will add kself tests for sysfs mode and send it as separate patches.
