linux-kernel - Re: [PATCH v2 2/3] coresight: tmc: Update error logging in tmc common functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAJ9a7VjCK+FGB+EMbcgaRfkv13TYYgBNHTG+AGp=52dZ7EfQAQ@mail.gmail.com>
Date: Tue, 17 Dec 2024 11:41:11 +0000
From: Mike Leach <mike.leach@...aro.org>
To: Suzuki K Poulose <suzuki.poulose@....com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	coresight@...ts.linaro.org, james.clark@...aro.org, 
	alexander.shishkin@...ux.intel.com
Subject: Re: [PATCH v2 2/3] coresight: tmc: Update error logging in tmc common functions

Hi Suzuki,

On Mon, 16 Dec 2024 at 09:33, Suzuki K Poulose <suzuki.poulose@....com> wrote:
>
> On 13/12/2024 14:49, Mike Leach wrote:
> > Enhance the error logging in the tmc_wait_for_tmcready() and
> > tmc_flush_and_stop() to print key tmc  register values on error
> > conditions to improve hardware debug information.
> >
> > Signed-off-by: Mike Leach <mike.leach@...aro.org>
> > ---
> >   .../hwtracing/coresight/coresight-tmc-core.c  | 37 +++++++++++++++----
> >   drivers/hwtracing/coresight/coresight-tmc.h   |  2 +-
> >   2 files changed, 30 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
> > index e9876252a789..1a9299adae93 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
> > @@ -34,25 +34,36 @@ DEFINE_CORESIGHT_DEVLIST(etb_devs, "tmc_etb");
> >   DEFINE_CORESIGHT_DEVLIST(etf_devs, "tmc_etf");
> >   DEFINE_CORESIGHT_DEVLIST(etr_devs, "tmc_etr");
> >
> > +#define TMC_WAIT_READY_FMT_STR "timeout while waiting for TMC to be Ready [STS=0x%04x]\n"
> > +
> >   int tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
> >   {
> >       struct coresight_device *csdev = drvdata->csdev;
> >       struct csdev_access *csa = &csdev->access;
> > +     u32 tmc_sts = 0;
> >
> >       /* Ensure formatter, unformatter and hardware fifo are empty */
> > -     if (coresight_timeout(csa, TMC_STS, TMC_STS_TMCREADY_BIT, 1)) {
> > -             dev_err(&csdev->dev,
> > -                     "timeout while waiting for TMC to be Ready\n");
> > +     if (coresight_timeout_retval(csa, TMC_STS, TMC_STS_TMCREADY_BIT, 1,
> > +                                  &tmc_sts)) {
> > +             dev_err(&csdev->dev, TMC_WAIT_READY_FMT_STR, tmc_sts);
> >               return -EBUSY;
> >       }
> >       return 0;
> >   }
> >
> > -void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
> > +int tmc_flush_and_stop(struct tmc_drvdata *drvdata)
> >   {
> >       struct coresight_device *csdev = drvdata->csdev;
> >       struct csdev_access *csa = &csdev->access;
> > -     u32 ffcr;
> > +     u32 ffcr, ffsr, tmc_sts;
> > +     int rc = 0;
> > +
> > +     /* note any MemErr present when stopping TMC */
> > +     tmc_sts = readl_relaxed(drvdata->base + TMC_STS);
> > +     if (tmc_sts & TMC_STS_MEMERR)
> > +             dev_err(&csdev->dev,
> > +                     "MemErr detected before Manual Flush; STS[0x%02x]\n",
> > +                     tmc_sts);
> >
> >       ffcr = readl_relaxed(drvdata->base + TMC_FFCR);
> >       ffcr |= TMC_FFCR_STOP_ON_FLUSH;
> > @@ -60,12 +71,22 @@ void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
> >       ffcr |= BIT(TMC_FFCR_FLUSHMAN_BIT);
> >       writel_relaxed(ffcr, drvdata->base + TMC_FFCR);
> >       /* Ensure flush completes */
> > -     if (coresight_timeout(csa, TMC_FFCR, TMC_FFCR_FLUSHMAN_BIT, 0)) {
> > +     if (coresight_timeout_retval(csa, TMC_FFCR, TMC_FFCR_FLUSHMAN_BIT, 0,
> > +                                  &ffcr)) {
> > +             ffsr = readl_relaxed(drvdata->base + TMC_FFSR);
> >               dev_err(&csdev->dev,
> > -             "timeout while waiting for completion of Manual Flush\n");
> > +                     "timeout while waiting for completion of Manual Flush\n");
> > +             dev_err(&csdev->dev,
> > +                     "regs: FFCR[0x%02x] FFSR[0x%02x] STS[0x%02x]\n",
> > +                     ffcr, ffsr, tmc_sts);
> > +             rc = -EBUSY;
> >       }
> >
> > -     tmc_wait_for_tmcready(drvdata);
> > +     if (tmc_wait_for_tmcready(drvdata)) {
> > +             dev_err(&csdev->dev, "TMC ready error after Manual flush\n");
> > +             rc = -EBUSY;
> > +     }
> > +     return rc;
> >   }
>
> All of this looks good to me. Do we need to move the MemErr check post
> the flush ? There is a potential chance of hitting a MemERR on flushing
> and we could miss reproting those ones ?
>

We need the one before - if there is already a Memerr then this will
affect the flush operation - on Memerr the formatter will have stopped
etc. But no harm in putting another check afterwards.

Mike

> Suzuki
>
> >   void tmc_enable_hw(struct tmc_drvdata *drvdata)
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> > index 2671926be62a..34513f07c3aa 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc.h
> > +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> > @@ -259,7 +259,7 @@ struct tmc_sg_table {
> >
> >   /* Generic functions */
> >   int tmc_wait_for_tmcready(struct tmc_drvdata *drvdata);
> > -void tmc_flush_and_stop(struct tmc_drvdata *drvdata);
> > +int tmc_flush_and_stop(struct tmc_drvdata *drvdata);
> >   void tmc_enable_hw(struct tmc_drvdata *drvdata);
> >   void tmc_disable_hw(struct tmc_drvdata *drvdata);
> >   u32 tmc_get_memwidth_mask(struct tmc_drvdata *drvdata);
>


-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK