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