* [PATCH 1/2] coresight: cti: Fix error handling in probe
2020-07-01 16:08 [PATCH 0/2] coresight: Fixes for v5.8-rc4 Mathieu Poirier
@ 2020-07-01 16:08 ` Mathieu Poirier
2020-07-01 16:08 ` [PATCH 2/2] coresight: etmv4: Fix CPU power management setup in probe() function Mathieu Poirier
1 sibling, 0 replies; 4+ messages in thread
From: Mathieu Poirier @ 2020-07-01 16:08 UTC (permalink / raw)
To: gregkh; +Cc: linux-arm-kernel, dan.carpenter, mike.leach
From: Dan Carpenter <dan.carpenter@oracle.com>
There were a couple problems with error handling in the probe function:
1) If the "drvdata" allocation failed then it lead to a NULL
dereference.
2) On several error paths we decremented "nr_cti_cpu" before it was
incremented which lead to a reference counting bug.
There were also some parts of the error handling which were not bugs but
were messy. The error handling was confusing to read. It printed some
unnecessary error messages.
The simplest way to fix these problems was to create a cti_pm_setup()
function that did all the power management setup in one go. That way
when we call cti_pm_release() we don't have to deal with the
complications of a partially configured power management config.
I reversed the "if (drvdata->ctidev.cpu >= 0)" condition in
cti_pm_release() so that it mirros the new cti_pm_setup() function.
Fixes: 6a0953ce7de9 ("coresight: cti: Add CPU idle pm notifer to CTI devices")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Mike Leach <mike.leach@linaro.org>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
drivers/hwtracing/coresight/coresight-cti.c | 96 ++++++++++++---------
1 file changed, 54 insertions(+), 42 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c
index 40387d58c8e7..3ccc703dc940 100644
--- a/drivers/hwtracing/coresight/coresight-cti.c
+++ b/drivers/hwtracing/coresight/coresight-cti.c
@@ -747,17 +747,50 @@ static int cti_dying_cpu(unsigned int cpu)
return 0;
}
+static int cti_pm_setup(struct cti_drvdata *drvdata)
+{
+ int ret;
+
+ if (drvdata->ctidev.cpu == -1)
+ return 0;
+
+ if (nr_cti_cpu)
+ goto done;
+
+ cpus_read_lock();
+ ret = cpuhp_setup_state_nocalls_cpuslocked(
+ CPUHP_AP_ARM_CORESIGHT_CTI_STARTING,
+ "arm/coresight_cti:starting",
+ cti_starting_cpu, cti_dying_cpu);
+ if (ret) {
+ cpus_read_unlock();
+ return ret;
+ }
+
+ ret = cpu_pm_register_notifier(&cti_cpu_pm_nb);
+ cpus_read_unlock();
+ if (ret) {
+ cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_CTI_STARTING);
+ return ret;
+ }
+
+done:
+ nr_cti_cpu++;
+ cti_cpu_drvdata[drvdata->ctidev.cpu] = drvdata;
+
+ return 0;
+}
+
/* release PM registrations */
static void cti_pm_release(struct cti_drvdata *drvdata)
{
- if (drvdata->ctidev.cpu >= 0) {
- if (--nr_cti_cpu == 0) {
- cpu_pm_unregister_notifier(&cti_cpu_pm_nb);
+ if (drvdata->ctidev.cpu == -1)
+ return;
- cpuhp_remove_state_nocalls(
- CPUHP_AP_ARM_CORESIGHT_CTI_STARTING);
- }
- cti_cpu_drvdata[drvdata->ctidev.cpu] = NULL;
+ cti_cpu_drvdata[drvdata->ctidev.cpu] = NULL;
+ if (--nr_cti_cpu == 0) {
+ cpu_pm_unregister_notifier(&cti_cpu_pm_nb);
+ cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_CTI_STARTING);
}
}
@@ -823,19 +856,14 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id)
/* driver data*/
drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
- if (!drvdata) {
- ret = -ENOMEM;
- dev_info(dev, "%s, mem err\n", __func__);
- goto err_out;
- }
+ if (!drvdata)
+ return -ENOMEM;
/* Validity for the resource is already checked by the AMBA core */
base = devm_ioremap_resource(dev, res);
- if (IS_ERR(base)) {
- ret = PTR_ERR(base);
- dev_err(dev, "%s, remap err\n", __func__);
- goto err_out;
- }
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+
drvdata->base = base;
dev_set_drvdata(dev, drvdata);
@@ -854,8 +882,7 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id)
pdata = coresight_cti_get_platform_data(dev);
if (IS_ERR(pdata)) {
dev_err(dev, "coresight_cti_get_platform_data err\n");
- ret = PTR_ERR(pdata);
- goto err_out;
+ return PTR_ERR(pdata);
}
/* default to powered - could change on PM notifications */
@@ -867,35 +894,20 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id)
drvdata->ctidev.cpu);
else
cti_desc.name = coresight_alloc_device_name(&cti_sys_devs, dev);
- if (!cti_desc.name) {
- ret = -ENOMEM;
- goto err_out;
- }
+ if (!cti_desc.name)
+ return -ENOMEM;
/* setup CPU power management handling for CPU bound CTI devices. */
- if (drvdata->ctidev.cpu >= 0) {
- cti_cpu_drvdata[drvdata->ctidev.cpu] = drvdata;
- if (!nr_cti_cpu++) {
- cpus_read_lock();
- ret = cpuhp_setup_state_nocalls_cpuslocked(
- CPUHP_AP_ARM_CORESIGHT_CTI_STARTING,
- "arm/coresight_cti:starting",
- cti_starting_cpu, cti_dying_cpu);
-
- if (!ret)
- ret = cpu_pm_register_notifier(&cti_cpu_pm_nb);
- cpus_read_unlock();
- if (ret)
- goto err_out;
- }
- }
+ ret = cti_pm_setup(drvdata);
+ if (ret)
+ return ret;
/* create dynamic attributes for connections */
ret = cti_create_cons_sysfs(dev, drvdata);
if (ret) {
dev_err(dev, "%s: create dynamic sysfs entries failed\n",
cti_desc.name);
- goto err_out;
+ goto pm_release;
}
/* set up coresight component description */
@@ -908,7 +920,7 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id)
drvdata->csdev = coresight_register(&cti_desc);
if (IS_ERR(drvdata->csdev)) {
ret = PTR_ERR(drvdata->csdev);
- goto err_out;
+ goto pm_release;
}
/* add to list of CTI devices */
@@ -927,7 +939,7 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id)
dev_info(&drvdata->csdev->dev, "CTI initialized\n");
return 0;
-err_out:
+pm_release:
cti_pm_release(drvdata);
return ret;
}
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH 2/2] coresight: etmv4: Fix CPU power management setup in probe() function
2020-07-01 16:08 [PATCH 0/2] coresight: Fixes for v5.8-rc4 Mathieu Poirier
2020-07-01 16:08 ` [PATCH 1/2] coresight: cti: Fix error handling in probe Mathieu Poirier
@ 2020-07-01 16:08 ` Mathieu Poirier
2020-07-01 16:45 ` Greg KH
1 sibling, 1 reply; 4+ messages in thread
From: Mathieu Poirier @ 2020-07-01 16:08 UTC (permalink / raw)
To: gregkh; +Cc: linux-arm-kernel, dan.carpenter, mike.leach
From: Mike Leach <mike.leach@linaro.org>
The current probe() function calls a pair of cpuhp_xxx API functions to
setup CPU hotplug handling. The hotplug lock is held for the duration of
the two calls and other CPU related code using cpus_read_lock() /
cpus_read_unlock() calls.
The problem is that on error states, goto: statements bypass the
cpus_read_unlock() call. This code has increased in complexity as the
driver has developed.
This patch introduces a pair of helper functions etm4_pm_setup_cpuslocked()
and etm4_pm_clear() which correct the issues above and group the PM code a
little better.
The two functions etm4_cpu_pm_register() and etm4_cpu_pm_unregister() are
dropped as these call cpu_pm_register_notifier() / ..unregister_notifier()
dependent on CONFIG_CPU_PM - but this define is used to nop these functions
out in the pm headers - so the wrapper functions are superfluous.
Fixes: f188b5e76aae ("coresight: etm4x: Save/restore state across CPU low power states")
Fixes: e9f5d63f84fe ("hwtracing/coresight-etm4x: Use cpuhp_setup_state_nocalls_cpuslocked()")
Fixes: 58eb457be028 ("etm4x: Convert to hotplug state machine")
Signed-off-by: Mike Leach <mike.leach@linaro.org>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
drivers/hwtracing/coresight/coresight-etm4x.c | 82 ++++++++++++-------
1 file changed, 53 insertions(+), 29 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
index 82fc2fab072a..2d732af8b3e7 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x.c
@@ -1398,18 +1398,57 @@ static struct notifier_block etm4_cpu_pm_nb = {
.notifier_call = etm4_cpu_pm_notify,
};
-static int etm4_cpu_pm_register(void)
+/* Setup PM. Called with cpus locked. Deals with error conditions and counts */
+static int etm4_pm_setup_cpuslocked(void)
{
- if (IS_ENABLED(CONFIG_CPU_PM))
- return cpu_pm_register_notifier(&etm4_cpu_pm_nb);
+ int ret;
- return 0;
+ if (etm4_count++)
+ return 0;
+
+ ret = cpu_pm_register_notifier(&etm4_cpu_pm_nb);
+ if (ret)
+ goto reduce_count;
+
+ ret = cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ARM_CORESIGHT_STARTING,
+ "arm/coresight4:starting",
+ etm4_starting_cpu, etm4_dying_cpu);
+
+ if (ret)
+ goto unregister_notifier;
+
+ ret = cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ONLINE_DYN,
+ "arm/coresight4:online",
+ etm4_online_cpu, NULL);
+
+ /* HP dyn state ID returned in ret on success */
+ if (ret > 0) {
+ hp_online = ret;
+ return 0;
+ }
+
+ /* failed dyn state - remove others */
+ cpuhp_remove_state_nocalls_cpuslocked(CPUHP_AP_ARM_CORESIGHT_STARTING);
+
+unregister_notifier:
+ cpu_pm_unregister_notifier(&etm4_cpu_pm_nb);
+
+reduce_count:
+ --etm4_count;
+ return ret;
}
-static void etm4_cpu_pm_unregister(void)
+static void etm4_pm_clear(void)
{
- if (IS_ENABLED(CONFIG_CPU_PM))
- cpu_pm_unregister_notifier(&etm4_cpu_pm_nb);
+ if (--etm4_count != 0)
+ return;
+
+ cpu_pm_unregister_notifier(&etm4_cpu_pm_nb);
+ cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);
+ if (hp_online) {
+ cpuhp_remove_state_nocalls(hp_online);
+ hp_online = 0;
+ }
}
static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
@@ -1466,24 +1505,15 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
etm4_init_arch_data, drvdata, 1))
dev_err(dev, "ETM arch init failed\n");
- if (!etm4_count++) {
- cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ARM_CORESIGHT_STARTING,
- "arm/coresight4:starting",
- etm4_starting_cpu, etm4_dying_cpu);
- ret = cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ONLINE_DYN,
- "arm/coresight4:online",
- etm4_online_cpu, NULL);
- if (ret < 0)
- goto err_arch_supported;
- hp_online = ret;
+ ret = etm4_pm_setup_cpuslocked();
+ cpus_read_unlock();
- ret = etm4_cpu_pm_register();
- if (ret)
- goto err_arch_supported;
+ /* etm4_pm_setup_cpuslocked() does its own cleanup - exit on error */
+ if (ret) {
+ etmdrvdata[drvdata->cpu] = NULL;
+ return ret;
}
- cpus_read_unlock();
-
if (etm4_arch_supported(drvdata->arch) == false) {
ret = -EINVAL;
goto err_arch_supported;
@@ -1530,13 +1560,7 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
err_arch_supported:
etmdrvdata[drvdata->cpu] = NULL;
- if (--etm4_count == 0) {
- etm4_cpu_pm_unregister();
-
- cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);
- if (hp_online)
- cpuhp_remove_state_nocalls(hp_online);
- }
+ etm4_pm_clear();
return ret;
}
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 4+ messages in thread