Re: [PATCH] coresight: etm4x: lazily allocate memory for save_state

[Andrew Murray <andrew.murray@arm.com>]

On Tue, Jul 23, 2019 at 10:05:37AM +0100, Suzuki K Poulose wrote:
> 
> 
> On 22/07/2019 21:32, Mathieu Poirier wrote:
> > Hi Andrew,
> > 
> > Sorry for the late reply - you patch got lost under the pile.
> > 
> > On Fri, 12 Jul 2019 at 09:01, Andrew Murray <andrew.murray@arm.com> wrote:
> > > 
> > > I had intended to lazily allocate memory for the save_state structure when
> > > it is first used. Following is a patch that I will squash into "[PATCH v3 5/6]
> > > coresight: etm4x: save/restore state across CPU low power states" on my
> > > next respin. I thought I'd share it here to get some feedback along with
> > > the rest of v3.
> > > 
> > > Thanks,
> > > 
> > > Andrew Murray
> > > 
> > > ---
> > >   drivers/hwtracing/coresight/coresight-etm4x.c | 14 +++++++++++---
> > >   drivers/hwtracing/coresight/coresight-etm4x.h |  2 +-
> > >   2 files changed, 12 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
> > > index b0bd8158bf13..cd02372194bc 100644
> > > --- a/drivers/hwtracing/coresight/coresight-etm4x.c
> > > +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
> > > @@ -1112,6 +1112,13 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
> > >          struct etmv4_save_state *state;
> > >          struct device *etm_dev = &drvdata->csdev->dev;
> > > 
> > > +       if (!drvdata->save_state) {
> > > +               drvdata->save_state = devm_kmalloc(etm_dev,
> > > +                               sizeof(struct etmv4_save_state), GFP_KERNEL);
> > 
> > GFP_KERNEL may sleep and will not work in the context where
> > etm4_cpu_save() is called.
> 
> Thats right and it is not worth making this GFP_ATOMIC either. We could simply
> decide this at probe time or when the save_restore is modified dynamically via
> callbacks.

I think it is simpler to change this to GFP_ATOMIC and leave it where it is.

As pm_save_enable can change at run time, we can't rely solely on allocating
memory for this at probe time. We'd have to allocate memory in two places 1)
a module_parm_cb callback for when the pm_save_enable parameter changes at
run-time and 2) at probe time to find out the initial value of the
pm_save_enable which may be set by kernel command line.

As the module_parm_cb callback is file-static we wouldn't know which drvdata
to allocate the memory for. We could allocate it for any etmdrvdata member
that isn't NULL - but this seems to add a lot of complexity - is this worth
it to avoid a GFP_ATOMIC allocation? (If we fail the allocation we can return
NOTIFY_BAD and stop the PM event.)

Thanks,

Andrew Murray

> 
> Suzuki
> 
> > 
> > Thanks,
> > Mathieu
> > 
> > > +               if (!drvdata->save_state)
> > > +                       return -ENOMEM;
> > > +       }
> > > +
> > >          /*
> > >           * As recommended by 3.4.1 ("The procedure when powering down the PE")
> > >           * of ARM IHI 0064D
> > > @@ -1134,7 +1141,7 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
> > >                  goto out;
> > >          }
> > > 
> > > -       state = &drvdata->save_state;
> > > +       state = drvdata->save_state;
> > > 
> > >          state->trcprgctlr = readl(drvdata->base + TRCPRGCTLR);
> > >          state->trcprocselr = readl(drvdata->base + TRCPROCSELR);
> > > @@ -1234,9 +1241,10 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
> > >   static void etm4_cpu_restore(struct etmv4_drvdata *drvdata)
> > >   {
> > >          int i;
> > > -       struct etmv4_save_state *state;
> > > +       struct etmv4_save_state *state = drvdata->save_state;
> > > 
> > > -       state = &drvdata->save_state;
> > > +       if (WARN_ON_ONCE(!state))
> > > +               return;
> > > 
> > >          CS_UNLOCK(drvdata->base);
> > > 
> > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> > > index c31634c64f87..a70cafbbb8cf 100644
> > > --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> > > +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> > > @@ -441,7 +441,7 @@ struct etmv4_drvdata {
> > >          bool                            atbtrig;
> > >          bool                            lpoverride;
> > >          struct etmv4_config             config;
> > > -       struct etmv4_save_state         save_state;
> > > +       struct etmv4_save_state         *save_state;
> > >          bool                            state_needs_restore;
> > >   };
> > > 
> > > --
> > > 2.21.0
> > > 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel