| Message ID | 20190712150056.15775-1-andrew.murray@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: etm4x: lazily allocate memory for save_state | expand |
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. 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 >
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. 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 >>
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 > > >
On 26/07/2019 12:24, Andrew Murray wrote: > 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 Or we could make it static. You either need it on the system, irrespective of what else happens or you don't need it at all. I agree that it helps with testing. > 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.) Ok, fair enough. If we are going for the GFP_ATOMIC allocation, please could we release it once we have restored ? We don't want to be holding onto the ATOMIC allocated memory forever. Cheers Suzuki
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); + 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; };