Skip to contentSkip to conversation

drivers/hwtracing/coresight/coresight-cti.c:862 cti_probe() error: we previously assumed 'drvdata' could be null (see line 759)

kbuild Superseded Dan Carpenter 4 participants 11 messages Jun 12, 2020 – Jun 29, 2020

Subsystems (3)
Kernel janitors Arm/CoreSight framework and drivers Hardware tracing facilities
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

Hi Mike,

Here is the buggy line:

861 err_out:

 862  	cti_pm_release(drvdata);
                       ^^^^^^^

To me it's a red flag any time there is a label called "out:". The label should say what is being released like "goto unregister_notifier;". The style of error handling here is called a "free everything function" and it is the most error prone style of error handling.

A better way to write error handling is to track the most recently allocated resource and free it with a well named goto.

	a = alloc();
	if (!a)
		return -ENOMEM;

	b = alloc();
	if (!b) {
		ret = -ENOMEM;
		goto free_a;
	}

	c = alloc();
	if (!c) {
		ret = -ENOMEM;
		goto free_b;

	...
	return 0;

free_b:

	free(b);
free_a:
	free(a);

The advantage of this is that 1) You only have to track the most recent allocation. 2) You can easily verify that the most recent allocation is freed. 3) Now you can create a free function by copy and pasting and adding a free(c);

void my_free(struct whatever *p)

{
	free(c);
	free(b);
	free(a);
}

This style uses about the same number of lines of code because although we duplicate the free(b) and free(a), we can remove some if statements so it ends up being about the same.

The main problem with free everything function is that they free things which have not been allocated. I have added more explanation at the bottom of this bug report.

I am also sending a patch which hopefully is clear but I can't actually compile it. :(

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head:   b791d1bdf9212d944d749a5c7ff6febdba241771
commit: e9b880581d555c8f7b58c7d19cc3f8f9016a1b5f coresight: cti: Add CPU Hotplug handling to CTI driver
config: arm-randconfig-m031-20200612 (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>
Reported-by Dan Carpenter <[email protected]>

smatch warnings:
drivers/hwtracing/coresight/coresight-cti.c:862 cti_probe() error: we previously assumed 'drvdata' could be null (see line 759)

# https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e9b880581d555c8f7b58c7d19cc3f8f9016a1b5f
git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git remote update linus
git checkout e9b880581d555c8f7b58c7d19cc3f8f9016a1b5f
vim +/drvdata +862 drivers/hwtracing/coresight/coresight-cti.c

835d722ba10ac92 Mike Leach 2020-03-20  747  static int cti_probe(struct amba_device *adev, const struct amba_id *id)
835d722ba10ac92 Mike Leach 2020-03-20  748  {
835d722ba10ac92 Mike Leach 2020-03-20  749  	int ret = 0;
835d722ba10ac92 Mike Leach 2020-03-20  750  	void __iomem *base;
835d722ba10ac92 Mike Leach 2020-03-20  751  	struct device *dev = &adev->dev;
835d722ba10ac92 Mike Leach 2020-03-20  752  	struct cti_drvdata *drvdata = NULL;
835d722ba10ac92 Mike Leach 2020-03-20  753  	struct coresight_desc cti_desc;
835d722ba10ac92 Mike Leach 2020-03-20  754  	struct coresight_platform_data *pdata = NULL;
835d722ba10ac92 Mike Leach 2020-03-20  755  	struct resource *res = &adev->res;
835d722ba10ac92 Mike Leach 2020-03-20  756  
835d722ba10ac92 Mike Leach 2020-03-20  757  	/* driver data*/
835d722ba10ac92 Mike Leach 2020-03-20  758  	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
835d722ba10ac92 Mike Leach 2020-03-20 @759  	if (!drvdata) {
835d722ba10ac92 Mike Leach 2020-03-20  760  		ret = -ENOMEM;
835d722ba10ac92 Mike Leach 2020-03-20  761  		dev_info(dev, "%s, mem err\n", __func__);

No need to print an error message for kmalloc() failures. It already has a stack trace built in.

835d722ba10ac92 Mike Leach 2020-03-20  762  		goto err_out;
                                                        ^^^^^^^^^^^^
835d722ba10ac92 Mike Leach 2020-03-20  763  	}
835d722ba10ac92 Mike Leach 2020-03-20  764  
835d722ba10ac92 Mike Leach 2020-03-20  765  	/* Validity for the resource is already checked by the AMBA core */
835d722ba10ac92 Mike Leach 2020-03-20  766  	base = devm_ioremap_resource(dev, res);
835d722ba10ac92 Mike Leach 2020-03-20  767  	if (IS_ERR(base)) {
835d722ba10ac92 Mike Leach 2020-03-20  768  		ret = PTR_ERR(base);
835d722ba10ac92 Mike Leach 2020-03-20  769  		dev_err(dev, "%s, remap err\n", __func__);

At this point "drvdata->ctidev.cpu" is zero.

835d722ba10ac92 Mike Leach 2020-03-20  770  		goto err_out;
835d722ba10ac92 Mike Leach 2020-03-20  771  	}
835d722ba10ac92 Mike Leach 2020-03-20  772  	drvdata->base = base;
835d722ba10ac92 Mike Leach 2020-03-20  773  
835d722ba10ac92 Mike Leach 2020-03-20  774  	dev_set_drvdata(dev, drvdata);
835d722ba10ac92 Mike Leach 2020-03-20  775  
835d722ba10ac92 Mike Leach 2020-03-20  776  	/* default CTI device info  */
835d722ba10ac92 Mike Leach 2020-03-20  777  	drvdata->ctidev.cpu = -1;
                                                ^^^^^^^^^^^^^^^^^^^^^^^^
835d722ba10ac92 Mike Leach 2020-03-20  778  	drvdata->ctidev.nr_trig_con = 0;
835d722ba10ac92 Mike Leach 2020-03-20  779  	drvdata->ctidev.ctm_id = 0;
835d722ba10ac92 Mike Leach 2020-03-20  780  	INIT_LIST_HEAD(&drvdata->ctidev.trig_cons);
835d722ba10ac92 Mike Leach 2020-03-20  781  
835d722ba10ac92 Mike Leach 2020-03-20  782  	spin_lock_init(&drvdata->spinlock);
835d722ba10ac92 Mike Leach 2020-03-20  783  
835d722ba10ac92 Mike Leach 2020-03-20  784  	/* initialise CTI driver config values */
835d722ba10ac92 Mike Leach 2020-03-20  785  	cti_set_default_config(dev, drvdata);
835d722ba10ac92 Mike Leach 2020-03-20  786  
835d722ba10ac92 Mike Leach 2020-03-20  787  	pdata = coresight_cti_get_platform_data(dev);
                                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This function sets drvdata->ctidev.cpu on some success paths and also on certain failure paths.

835d722ba10ac92 Mike Leach 2020-03-20  788  	if (IS_ERR(pdata)) {
835d722ba10ac92 Mike Leach 2020-03-20  789  		dev_err(dev, "coresight_cti_get_platform_data err\n");
835d722ba10ac92 Mike Leach 2020-03-20  790  		ret =  PTR_ERR(pdata);
835d722ba10ac92 Mike Leach 2020-03-20  791  		goto err_out;
835d722ba10ac92 Mike Leach 2020-03-20  792  	}
835d722ba10ac92 Mike Leach 2020-03-20  793  
835d722ba10ac92 Mike Leach 2020-03-20  794  	/* default to powered - could change on PM notifications */
835d722ba10ac92 Mike Leach 2020-03-20  795  	drvdata->config.hw_powered = true;
835d722ba10ac92 Mike Leach 2020-03-20  796  
835d722ba10ac92 Mike Leach 2020-03-20  797  	/* set up device name - will depend if cpu bound or otherwise */
835d722ba10ac92 Mike Leach 2020-03-20  798  	if (drvdata->ctidev.cpu >= 0)
835d722ba10ac92 Mike Leach 2020-03-20  799  		cti_desc.name = devm_kasprintf(dev, GFP_KERNEL, "cti_cpu%d",
835d722ba10ac92 Mike Leach 2020-03-20  800  					       drvdata->ctidev.cpu);
835d722ba10ac92 Mike Leach 2020-03-20  801  	else
835d722ba10ac92 Mike Leach 2020-03-20  802  		cti_desc.name = coresight_alloc_device_name(&cti_sys_devs, dev);
835d722ba10ac92 Mike Leach 2020-03-20  803  	if (!cti_desc.name) {
835d722ba10ac92 Mike Leach 2020-03-20  804  		ret = -ENOMEM;
835d722ba10ac92 Mike Leach 2020-03-20  805  		goto err_out;
835d722ba10ac92 Mike Leach 2020-03-20  806  	}
835d722ba10ac92 Mike Leach 2020-03-20  807  
e9b880581d555c8 Mike Leach 2020-05-18  808  	/* setup CPU power management handling for CPU bound CTI devices. */
e9b880581d555c8 Mike Leach 2020-05-18  809  	if (drvdata->ctidev.cpu >= 0) {
e9b880581d555c8 Mike Leach 2020-05-18  810  		cti_cpu_drvdata[drvdata->ctidev.cpu] = drvdata;
e9b880581d555c8 Mike Leach 2020-05-18  811  		if (!nr_cti_cpu++) {
                                                             ^^^^^^^^^^^^
e9b880581d555c8 Mike Leach 2020-05-18  812  			cpus_read_lock();
e9b880581d555c8 Mike Leach 2020-05-18  813  			ret = cpuhp_setup_state_nocalls_cpuslocked(
e9b880581d555c8 Mike Leach 2020-05-18  814  				CPUHP_AP_ARM_CORESIGHT_CTI_STARTING,
e9b880581d555c8 Mike Leach 2020-05-18  815  				"arm/coresight_cti:starting",
e9b880581d555c8 Mike Leach 2020-05-18  816  				cti_starting_cpu, cti_dying_cpu);
e9b880581d555c8 Mike Leach 2020-05-18  817  
e9b880581d555c8 Mike Leach 2020-05-18  818  			cpus_read_unlock();
e9b880581d555c8 Mike Leach 2020-05-18  819  			if (ret)
e9b880581d555c8 Mike Leach 2020-05-18  820  				goto err_out;
e9b880581d555c8 Mike Leach 2020-05-18  821  		}
e9b880581d555c8 Mike Leach 2020-05-18  822  	}
e9b880581d555c8 Mike Leach 2020-05-18  823  
3c5597e398124e6 Mike Leach 2020-03-20  824  	/* create dynamic attributes for connections */
3c5597e398124e6 Mike Leach 2020-03-20  825  	ret = cti_create_cons_sysfs(dev, drvdata);
3c5597e398124e6 Mike Leach 2020-03-20  826  	if (ret) {
3c5597e398124e6 Mike Leach 2020-03-20  827  		dev_err(dev, "%s: create dynamic sysfs entries failed\n",
3c5597e398124e6 Mike Leach 2020-03-20  828  			cti_desc.name);
3c5597e398124e6 Mike Leach 2020-03-20  829  		goto err_out;
3c5597e398124e6 Mike Leach 2020-03-20  830  	}
3c5597e398124e6 Mike Leach 2020-03-20  831  
835d722ba10ac92 Mike Leach 2020-03-20  832  	/* set up coresight component description */
835d722ba10ac92 Mike Leach 2020-03-20  833  	cti_desc.pdata = pdata;
835d722ba10ac92 Mike Leach 2020-03-20  834  	cti_desc.type = CORESIGHT_DEV_TYPE_ECT;
835d722ba10ac92 Mike Leach 2020-03-20  835  	cti_desc.subtype.ect_subtype = CORESIGHT_DEV_SUBTYPE_ECT_CTI;
835d722ba10ac92 Mike Leach 2020-03-20  836  	cti_desc.ops = &cti_ops;
3c5597e398124e6 Mike Leach 2020-03-20  837  	cti_desc.groups = drvdata->ctidev.con_groups;
835d722ba10ac92 Mike Leach 2020-03-20  838  	cti_desc.dev = dev;
835d722ba10ac92 Mike Leach 2020-03-20  839  	drvdata->csdev = coresight_register(&cti_desc);
835d722ba10ac92 Mike Leach 2020-03-20  840  	if (IS_ERR(drvdata->csdev)) {
835d722ba10ac92 Mike Leach 2020-03-20  841  		ret = PTR_ERR(drvdata->csdev);
835d722ba10ac92 Mike Leach 2020-03-20  842  		goto err_out;
835d722ba10ac92 Mike Leach 2020-03-20  843  	}
835d722ba10ac92 Mike Leach 2020-03-20  844  
835d722ba10ac92 Mike Leach 2020-03-20  845  	/* add to list of CTI devices */
835d722ba10ac92 Mike Leach 2020-03-20  846  	mutex_lock(&ect_mutex);
835d722ba10ac92 Mike Leach 2020-03-20  847  	list_add(&drvdata->node, &ect_net);
177af8285b59a38 Mike Leach 2020-03-20  848  	/* set any cross references */
177af8285b59a38 Mike Leach 2020-03-20  849  	cti_update_conn_xrefs(drvdata);
835d722ba10ac92 Mike Leach 2020-03-20  850  	mutex_unlock(&ect_mutex);
835d722ba10ac92 Mike Leach 2020-03-20  851  
835d722ba10ac92 Mike Leach 2020-03-20  852  	/* set up release chain */
835d722ba10ac92 Mike Leach 2020-03-20  853  	drvdata->csdev_release = drvdata->csdev->dev.release;
835d722ba10ac92 Mike Leach 2020-03-20  854  	drvdata->csdev->dev.release = cti_device_release;
835d722ba10ac92 Mike Leach 2020-03-20  855  
835d722ba10ac92 Mike Leach 2020-03-20  856  	/* all done - dec pm refcount */
835d722ba10ac92 Mike Leach 2020-03-20  857  	pm_runtime_put(&adev->dev);
835d722ba10ac92 Mike Leach 2020-03-20  858  	dev_info(&drvdata->csdev->dev, "CTI initialized\n");
835d722ba10ac92 Mike Leach 2020-03-20  859  	return 0;
835d722ba10ac92 Mike Leach 2020-03-20  860  
835d722ba10ac92 Mike Leach 2020-03-20  861  err_out:
e9b880581d555c8 Mike Leach 2020-05-18 @862  	cti_pm_release(drvdata);
                                                               ^^^^^^^
835d722ba10ac92 Mike Leach 2020-03-20  863  	return ret;
835d722ba10ac92 Mike Leach 2020-03-20  864  }
   750  /* release PM registrations */
   751  static void cti_pm_release(struct cti_drvdata *drvdata)
   752  {
   753          if (drvdata->ctidev.cpu >= 0) {
                    ^^^^^^^

We are dereferencing this when it wasn't allocated.

   754                  if (--nr_cti_cpu == 0) {
                            ^^^^^^^^^^^^

If devm_kasprintf() fails then we are decrementing this when it wasn't incremented so now it can be negative.

   755                          cpu_pm_unregister_notifier(&cti_cpu_pm_nb);

If the cpu_pm_register_notifier() fails then we are unregistering this when it wasn't registered. It turns out this is harmless but if we only free things which have been allocated then it becomes a lot easier to audit the code.

756
757 cpuhp_remove_state_nocalls(
758 CPUHP_AP_ARM_CORESIGHT_CTI_STARTING);

If cpuhp_setup_state_nocalls_cpuslocked() failed then this wasn't allocated. I believe this is harmless.

   759                  }
   760                  cti_cpu_drvdata[drvdata->ctidev.cpu] = NULL;
   761          }
   762  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

_______________________________________________
kbuild mailing list -- kbuild(a)lists.01.org
To unsubscribe send an email to kbuild-leave(a)lists.01.org

author
#1 lore raw

PATCH 1/1 coresight: cti: Fix error handling in probe

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 <[email protected]>

---
Please note!!! I cannot compile this patch. Mike can you review it?

 drivers/hwtracing/coresight/coresight-cti.c | 96 ++++++++++++---------
 1 file changed, 54 insertions(+), 42 deletions(-)
drivers/hwtracing/coresight/coresight-cti.c +54 -42
index 40387d58c8e7..d2da5bf9f552 100644
@@ -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] = drvdata;
+	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;
 }
Signature · 1 line
2.27.0
#2 lore raw

AW: [PATCH] coresight: cti: Fix error handling in probe

Hi Dan,

nit picking in cti_pm_release()

IMHO this should be done in 2 steps:

      if (--nr_cti_cpu == 0)

->
--nr_cti_cpu ;
if ( nr_cti_cpu == 0)

the decrement is easy to miss (what i did first).

yes, i noticed that it is also in the original code and it is not that important but while you are here ...

jm2c,
re,
wh
________________________________________
Von: [email protected] <[email protected]> im Auftrag von Dan Carpenter <[email protected]>
Gesendet: Freitag, 12. Juni 2020 14:11:33
An: Mike Leach
Cc: Mathieu Poirier; Suzuki K Poulose; Alexander Shishkin; Greg Kroah-Hartman; [email protected]; [email protected]; [email protected]
Betreff: [PATCH] coresight: cti: Fix error handling in probe

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 <[email protected]>

---
Please note!!! I cannot compile this patch. Mike can you review it?

 drivers/hwtracing/coresight/coresight-cti.c | 96 ++++++++++++---------
 1 file changed, 54 insertions(+), 42 deletions(-)
drivers/hwtracing/coresight/coresight-cti.c +54 -42
index 40387d58c8e7..d2da5bf9f552 100644
@@ -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] = drvdata;
+       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.27.0

author
#3 lore raw

Re: [PATCH] coresight: cti: Fix error handling in probe

Quoted 9 lines · Walter Harms

Hi Dan,

nit picking in cti_pm_release()

IMHO this should be done in 2 steps:

      if (--nr_cti_cpu = 0)

->
--nr_cti_cpu ;
if ( nr_cti_cpu = 0)

The first way is sort of the more canonical way to write it... By far.

regards,
carpenter

author
#4 lore raw

Re: [PATCH] coresight: cti: Fix error handling in probe

In reply to Dan Carpenter

drivers/hwtracing/coresight/coresight-cti.c static int cti_dying_cpu(unsigned int cpu) · line 760

Quoted 11 lines · Dan Carpenter
drivers/hwtracing/coresight/coresight-cti.c +11 -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();

        ^^^^^^^^^^^^^^^^

One thing which I do wonder is why we have locking here but not in the cti_pm_release() function. That was how the original code was so the patch doesn't change anything, but I am curious.

Quoted 32 lines
drivers/hwtracing/coresight/coresight-cti.c +25 -3
+	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;

drivers/hwtracing/coresight/coresight-cti.c static int cti_dying_cpu(unsigned int cpu) · line 795

Quoted 10 lines
drivers/hwtracing/coresight/coresight-cti.c +4 -4
-			cpuhp_remove_state_nocalls(
-				CPUHP_AP_ARM_CORESIGHT_CTI_STARTING);
-		}
-		cti_cpu_drvdata[drvdata->ctidev.cpu] = NULL;
+	cti_cpu_drvdata[drvdata->ctidev.cpu] = drvdata;
+	if (--nr_cti_cpu = 0) {
+		cpu_pm_unregister_notifier(&cti_cpu_pm_nb);
+		cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_CTI_STARTING);
 	}
 }

regards,
dan carpenter

#10 lore raw
In reply to Dan Carpenter

Hi Dan,

Thanks for the comprehensive review of this.

Quoted 10 lines · Dan Carpenter

Hi Mike,

Here is the buggy line:

861 err_out:

 862    cti_pm_release(drvdata);
                       ^^^^^^^

Yes - caling cti_pm_release() if drvdata == NULL would result in bad things happening!

I would certainly be aiming to fix this, but as you have provided a patch on another mail - I have reviewed and compiled that, so will respond further there

Regards

Mike

Quoted 242 lines

To me it's a red flag any time there is a label called "out:". The label should say what is being released like "goto unregister_notifier;". The style of error handling here is called a "free everything function" and it is the most error prone style of error handling.

A better way to write error handling is to track the most recently allocated resource and free it with a well named goto.

        a = alloc();
        if (!a)
                return -ENOMEM;

        b = alloc();
        if (!b) {
                ret = -ENOMEM;
                goto free_a;
        }

        c = alloc();
        if (!c) {
                ret = -ENOMEM;
                goto free_b;

        ...
        return 0;

free_b:

        free(b);
free_a:
        free(a);

The advantage of this is that 1) You only have to track the most recent allocation. 2) You can easily verify that the most recent allocation is freed. 3) Now you can create a free function by copy and pasting and adding a free(c);

void my_free(struct whatever *p)

{
        free(c);
        free(b);
        free(a);
}

This style uses about the same number of lines of code because although we duplicate the free(b) and free(a), we can remove some if statements so it ends up being about the same.

The main problem with free everything function is that they free things which have not been allocated. I have added more explanation at the bottom of this bug report.

I am also sending a patch which hopefully is clear but I can't actually compile it. :(

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head:   b791d1bdf9212d944d749a5c7ff6febdba241771
commit: e9b880581d555c8f7b58c7d19cc3f8f9016a1b5f coresight: cti: Add CPU Hotplug handling to CTI driver
config: arm-randconfig-m031-20200612 (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>
Reported-by Dan Carpenter <[email protected]>

smatch warnings:
drivers/hwtracing/coresight/coresight-cti.c:862 cti_probe() error: we previously assumed 'drvdata' could be null (see line 759)

# https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e9b880581d555c8f7b58c7d19cc3f8f9016a1b5f
git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git remote update linus
git checkout e9b880581d555c8f7b58c7d19cc3f8f9016a1b5f
vim +/drvdata +862 drivers/hwtracing/coresight/coresight-cti.c

835d722ba10ac92 Mike Leach 2020-03-20  747  static int cti_probe(struct amba_device *adev, const struct amba_id *id)
835d722ba10ac92 Mike Leach 2020-03-20  748  {
835d722ba10ac92 Mike Leach 2020-03-20  749      int ret = 0;
835d722ba10ac92 Mike Leach 2020-03-20  750      void __iomem *base;
835d722ba10ac92 Mike Leach 2020-03-20  751      struct device *dev = &adev->dev;
835d722ba10ac92 Mike Leach 2020-03-20  752      struct cti_drvdata *drvdata = NULL;
835d722ba10ac92 Mike Leach 2020-03-20  753      struct coresight_desc cti_desc;
835d722ba10ac92 Mike Leach 2020-03-20  754      struct coresight_platform_data *pdata = NULL;
835d722ba10ac92 Mike Leach 2020-03-20  755      struct resource *res = &adev->res;
835d722ba10ac92 Mike Leach 2020-03-20  756
835d722ba10ac92 Mike Leach 2020-03-20  757      /* driver data*/
835d722ba10ac92 Mike Leach 2020-03-20  758      drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
835d722ba10ac92 Mike Leach 2020-03-20 @759      if (!drvdata) {
835d722ba10ac92 Mike Leach 2020-03-20  760              ret = -ENOMEM;
835d722ba10ac92 Mike Leach 2020-03-20  761              dev_info(dev, "%s, mem err\n", __func__);

No need to print an error message for kmalloc() failures. It already has a stack trace built in.

835d722ba10ac92 Mike Leach 2020-03-20  762              goto err_out;
                                                        ^^^^^^^^^^^^
835d722ba10ac92 Mike Leach 2020-03-20  763      }
835d722ba10ac92 Mike Leach 2020-03-20  764
835d722ba10ac92 Mike Leach 2020-03-20  765      /* Validity for the resource is already checked by the AMBA core */
835d722ba10ac92 Mike Leach 2020-03-20  766      base = devm_ioremap_resource(dev, res);
835d722ba10ac92 Mike Leach 2020-03-20  767      if (IS_ERR(base)) {
835d722ba10ac92 Mike Leach 2020-03-20  768              ret = PTR_ERR(base);
835d722ba10ac92 Mike Leach 2020-03-20  769              dev_err(dev, "%s, remap err\n", __func__);

At this point "drvdata->ctidev.cpu" is zero.

835d722ba10ac92 Mike Leach 2020-03-20  770              goto err_out;
835d722ba10ac92 Mike Leach 2020-03-20  771      }
835d722ba10ac92 Mike Leach 2020-03-20  772      drvdata->base = base;
835d722ba10ac92 Mike Leach 2020-03-20  773
835d722ba10ac92 Mike Leach 2020-03-20  774      dev_set_drvdata(dev, drvdata);
835d722ba10ac92 Mike Leach 2020-03-20  775
835d722ba10ac92 Mike Leach 2020-03-20  776      /* default CTI device info  */
835d722ba10ac92 Mike Leach 2020-03-20  777      drvdata->ctidev.cpu = -1;
                                                ^^^^^^^^^^^^^^^^^^^^^^^^
835d722ba10ac92 Mike Leach 2020-03-20  778      drvdata->ctidev.nr_trig_con = 0;
835d722ba10ac92 Mike Leach 2020-03-20  779      drvdata->ctidev.ctm_id = 0;
835d722ba10ac92 Mike Leach 2020-03-20  780      INIT_LIST_HEAD(&drvdata->ctidev.trig_cons);
835d722ba10ac92 Mike Leach 2020-03-20  781
835d722ba10ac92 Mike Leach 2020-03-20  782      spin_lock_init(&drvdata->spinlock);
835d722ba10ac92 Mike Leach 2020-03-20  783
835d722ba10ac92 Mike Leach 2020-03-20  784      /* initialise CTI driver config values */
835d722ba10ac92 Mike Leach 2020-03-20  785      cti_set_default_config(dev, drvdata);
835d722ba10ac92 Mike Leach 2020-03-20  786
835d722ba10ac92 Mike Leach 2020-03-20  787      pdata = coresight_cti_get_platform_data(dev);
                                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This function sets drvdata->ctidev.cpu on some success paths and also on certain failure paths.

835d722ba10ac92 Mike Leach 2020-03-20  788      if (IS_ERR(pdata)) {
835d722ba10ac92 Mike Leach 2020-03-20  789              dev_err(dev, "coresight_cti_get_platform_data err\n");
835d722ba10ac92 Mike Leach 2020-03-20  790              ret =  PTR_ERR(pdata);
835d722ba10ac92 Mike Leach 2020-03-20  791              goto err_out;
835d722ba10ac92 Mike Leach 2020-03-20  792      }
835d722ba10ac92 Mike Leach 2020-03-20  793
835d722ba10ac92 Mike Leach 2020-03-20  794      /* default to powered - could change on PM notifications */
835d722ba10ac92 Mike Leach 2020-03-20  795      drvdata->config.hw_powered = true;
835d722ba10ac92 Mike Leach 2020-03-20  796
835d722ba10ac92 Mike Leach 2020-03-20  797      /* set up device name - will depend if cpu bound or otherwise */
835d722ba10ac92 Mike Leach 2020-03-20  798      if (drvdata->ctidev.cpu >= 0)
835d722ba10ac92 Mike Leach 2020-03-20  799              cti_desc.name = devm_kasprintf(dev, GFP_KERNEL, "cti_cpu%d",
835d722ba10ac92 Mike Leach 2020-03-20  800                                             drvdata->ctidev.cpu);
835d722ba10ac92 Mike Leach 2020-03-20  801      else
835d722ba10ac92 Mike Leach 2020-03-20  802              cti_desc.name = coresight_alloc_device_name(&cti_sys_devs, dev);
835d722ba10ac92 Mike Leach 2020-03-20  803      if (!cti_desc.name) {
835d722ba10ac92 Mike Leach 2020-03-20  804              ret = -ENOMEM;
835d722ba10ac92 Mike Leach 2020-03-20  805              goto err_out;
835d722ba10ac92 Mike Leach 2020-03-20  806      }
835d722ba10ac92 Mike Leach 2020-03-20  807
e9b880581d555c8 Mike Leach 2020-05-18  808      /* setup CPU power management handling for CPU bound CTI devices. */
e9b880581d555c8 Mike Leach 2020-05-18  809      if (drvdata->ctidev.cpu >= 0) {
e9b880581d555c8 Mike Leach 2020-05-18  810              cti_cpu_drvdata[drvdata->ctidev.cpu] = drvdata;
e9b880581d555c8 Mike Leach 2020-05-18  811              if (!nr_cti_cpu++) {
                                                             ^^^^^^^^^^^^
e9b880581d555c8 Mike Leach 2020-05-18  812                      cpus_read_lock();
e9b880581d555c8 Mike Leach 2020-05-18  813                      ret = cpuhp_setup_state_nocalls_cpuslocked(
e9b880581d555c8 Mike Leach 2020-05-18  814                              CPUHP_AP_ARM_CORESIGHT_CTI_STARTING,
e9b880581d555c8 Mike Leach 2020-05-18  815                              "arm/coresight_cti:starting",
e9b880581d555c8 Mike Leach 2020-05-18  816                              cti_starting_cpu, cti_dying_cpu);
e9b880581d555c8 Mike Leach 2020-05-18  817
e9b880581d555c8 Mike Leach 2020-05-18  818                      cpus_read_unlock();
e9b880581d555c8 Mike Leach 2020-05-18  819                      if (ret)
e9b880581d555c8 Mike Leach 2020-05-18  820                              goto err_out;
e9b880581d555c8 Mike Leach 2020-05-18  821              }
e9b880581d555c8 Mike Leach 2020-05-18  822      }
e9b880581d555c8 Mike Leach 2020-05-18  823
3c5597e398124e6 Mike Leach 2020-03-20  824      /* create dynamic attributes for connections */
3c5597e398124e6 Mike Leach 2020-03-20  825      ret = cti_create_cons_sysfs(dev, drvdata);
3c5597e398124e6 Mike Leach 2020-03-20  826      if (ret) {
3c5597e398124e6 Mike Leach 2020-03-20  827              dev_err(dev, "%s: create dynamic sysfs entries failed\n",
3c5597e398124e6 Mike Leach 2020-03-20  828                      cti_desc.name);
3c5597e398124e6 Mike Leach 2020-03-20  829              goto err_out;
3c5597e398124e6 Mike Leach 2020-03-20  830      }
3c5597e398124e6 Mike Leach 2020-03-20  831
835d722ba10ac92 Mike Leach 2020-03-20  832      /* set up coresight component description */
835d722ba10ac92 Mike Leach 2020-03-20  833      cti_desc.pdata = pdata;
835d722ba10ac92 Mike Leach 2020-03-20  834      cti_desc.type = CORESIGHT_DEV_TYPE_ECT;
835d722ba10ac92 Mike Leach 2020-03-20  835      cti_desc.subtype.ect_subtype = CORESIGHT_DEV_SUBTYPE_ECT_CTI;
835d722ba10ac92 Mike Leach 2020-03-20  836      cti_desc.ops = &cti_ops;
3c5597e398124e6 Mike Leach 2020-03-20  837      cti_desc.groups = drvdata->ctidev.con_groups;
835d722ba10ac92 Mike Leach 2020-03-20  838      cti_desc.dev = dev;
835d722ba10ac92 Mike Leach 2020-03-20  839      drvdata->csdev = coresight_register(&cti_desc);
835d722ba10ac92 Mike Leach 2020-03-20  840      if (IS_ERR(drvdata->csdev)) {
835d722ba10ac92 Mike Leach 2020-03-20  841              ret = PTR_ERR(drvdata->csdev);
835d722ba10ac92 Mike Leach 2020-03-20  842              goto err_out;
835d722ba10ac92 Mike Leach 2020-03-20  843      }
835d722ba10ac92 Mike Leach 2020-03-20  844
835d722ba10ac92 Mike Leach 2020-03-20  845      /* add to list of CTI devices */
835d722ba10ac92 Mike Leach 2020-03-20  846      mutex_lock(&ect_mutex);
835d722ba10ac92 Mike Leach 2020-03-20  847      list_add(&drvdata->node, &ect_net);
177af8285b59a38 Mike Leach 2020-03-20  848      /* set any cross references */
177af8285b59a38 Mike Leach 2020-03-20  849      cti_update_conn_xrefs(drvdata);
835d722ba10ac92 Mike Leach 2020-03-20  850      mutex_unlock(&ect_mutex);
835d722ba10ac92 Mike Leach 2020-03-20  851
835d722ba10ac92 Mike Leach 2020-03-20  852      /* set up release chain */
835d722ba10ac92 Mike Leach 2020-03-20  853      drvdata->csdev_release = drvdata->csdev->dev.release;
835d722ba10ac92 Mike Leach 2020-03-20  854      drvdata->csdev->dev.release = cti_device_release;
835d722ba10ac92 Mike Leach 2020-03-20  855
835d722ba10ac92 Mike Leach 2020-03-20  856      /* all done - dec pm refcount */
835d722ba10ac92 Mike Leach 2020-03-20  857      pm_runtime_put(&adev->dev);
835d722ba10ac92 Mike Leach 2020-03-20  858      dev_info(&drvdata->csdev->dev, "CTI initialized\n");
835d722ba10ac92 Mike Leach 2020-03-20  859      return 0;
835d722ba10ac92 Mike Leach 2020-03-20  860
835d722ba10ac92 Mike Leach 2020-03-20  861  err_out:
e9b880581d555c8 Mike Leach 2020-05-18 @862      cti_pm_release(drvdata);
                                                               ^^^^^^^

835d722ba10ac92 Mike Leach 2020-03-20 863 return ret;
835d722ba10ac92 Mike Leach 2020-03-20 864 }

   750  /* release PM registrations */
   751  static void cti_pm_release(struct cti_drvdata *drvdata)
   752  {
   753          if (drvdata->ctidev.cpu >= 0) {
                    ^^^^^^^

We are dereferencing this when it wasn't allocated.

   754                  if (--nr_cti_cpu == 0) {
                            ^^^^^^^^^^^^

If devm_kasprintf() fails then we are decrementing this when it wasn't incremented so now it can be negative.

   755                          cpu_pm_unregister_notifier(&cti_cpu_pm_nb);

If the cpu_pm_register_notifier() fails then we are unregistering this when it wasn't registered. It turns out this is harmless but if we only free things which have been allocated then it becomes a lot easier to audit the code.

   756
   757                          cpuhp_remove_state_nocalls(
   758                                  CPUHP_AP_ARM_CORESIGHT_CTI_STARTING);

If cpuhp_setup_state_nocalls_cpuslocked() failed then this wasn't allocated. I believe this is harmless.

   759                  }
   760                  cti_cpu_drvdata[drvdata->ctidev.cpu] = NULL;
   761          }
   762  }
Quoted 6 lines

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
_______________________________________________
kbuild mailing list -- kbuild(a)lists.01.org
To unsubscribe send an email to kbuild-leave(a)lists.01.org

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

#7 lore raw

Re: [PATCH] coresight: cti: Fix error handling in probe

In reply to Dan Carpenter

Hi Dan,

Thanks for looking at this. I agree with the patch, other than the one change below.
I have compiled and run on my DB410 system, against 5.8-rc1.

Quoted 73 lines · Dan Carpenter

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 <[email protected]>

---
Please note!!! I cannot compile this patch. Mike can you review it?

 drivers/hwtracing/coresight/coresight-cti.c | 96 ++++++++++++---------
 1 file changed, 54 insertions(+), 42 deletions(-)
drivers/hwtracing/coresight/coresight-cti.c +34 -1
index 40387d58c8e7..d2da5bf9f552 100644
@@ -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) {

drivers/hwtracing/coresight/coresight-cti.c static int cti_dying_cpu(unsigned int cpu) · line 790

Quoted 10 lines
drivers/hwtracing/coresight/coresight-cti.c +3 -6
-               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] = drvdata;

This should remain as cti_cpu_drvdata[drvdata->ctidev.cpu] = NULL; here. We are reversing the assignment in cti_pm_setup().

Quoted 102 lines
drivers/hwtracing/coresight/coresight-cti.c +17 -36
+       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;
 }
--
Quoted 2 lines

2.27.0

Reviewed-by Mike Leach <[email protected]>

Signature · 3 lines
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
#5 lore raw

Re: [PATCH] coresight: cti: Fix error handling in probe

In reply to Dan Carpenter

Hi Dan,

Quoted 14 lines · Dan Carpenter

drivers/hwtracing/coresight/coresight-cti.c static int cti_dying_cpu(unsigned int cpu) · line 760

Quoted 11 lines · Dan Carpenter
drivers/hwtracing/coresight/coresight-cti.c +11 -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();

        ^^^^^^^^^^^^^^^^
Quoted 4 lines

One thing which I do wonder is why we have locking here but not in the cti_pm_release() function. That was how the original code was so the patch doesn't change anything, but I am curious.

Good point - the CTI PM code was modelled on the same code in the ETM drivers, which show the same pattern.
Perhaps something we need to revisit in both drivers.

Regards

Mike

Quoted 46 lines
Quoted 32 lines
drivers/hwtracing/coresight/coresight-cti.c +25 -3
+     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;

drivers/hwtracing/coresight/coresight-cti.c static int cti_dying_cpu(unsigned int cpu) · line 795

Quoted 10 lines
drivers/hwtracing/coresight/coresight-cti.c +4 -4
-                     cpuhp_remove_state_nocalls(
-                             CPUHP_AP_ARM_CORESIGHT_CTI_STARTING);
-             }
-             cti_cpu_drvdata[drvdata->ctidev.cpu] = NULL;
+     cti_cpu_drvdata[drvdata->ctidev.cpu] = drvdata;
+     if (--nr_cti_cpu = 0) {
+             cpu_pm_unregister_notifier(&cti_cpu_pm_nb);
+             cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_CTI_STARTING);
      }
 }

regards,
dan carpenter

Signature · 3 lines
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
#6 lore raw

Re: [PATCH] coresight: cti: Fix error handling in probe

HI Dan,

Looked into this some more...

Quoted 23 lines · Mike Leach

Hi Dan,

Quoted 14 lines · Dan Carpenter

drivers/hwtracing/coresight/coresight-cti.c static int cti_dying_cpu(unsigned int cpu) · line 760

Quoted 11 lines · Dan Carpenter
drivers/hwtracing/coresight/coresight-cti.c +11 -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();

        ^^^^^^^^^^^^^^^^
Quoted 4 lines

One thing which I do wonder is why we have locking here but not in the cti_pm_release() function. That was how the original code was so the patch doesn't change anything, but I am curious.

Quoted 4 lines

Good point - the CTI PM code was modelled on the same code in the ETM drivers, which show the same pattern.
Perhaps something we need to revisit in both drivers.

The ETMv4 code calls into the hotplug API twice - so takes the lock and makes both calls while holding the lock - using the "_cpuslocked" call variant to render the pair of calls atomic from the CPUHP context point of view.
CTI only calls once so does not really need to take the locks and could simply use the normal variant.

In both cases the cpuhp_remove_state uses the normal variant, which takes the locks inside the api call. For the CTI there is certainly a case for simplification, i..e drop the "_cpuslocked" variant and remove the explicit taking of the locks.

Something along the lines of....

...
if (nr_cti_cpu)

       goto done;

ret = cpu_pm_register_notifier(&cti_cpu_pm_nb);
if (ret)

     return ret;

ret = cpuhp_setup_state_nocalls(......);

if (ret) {
    cpu_pm_unregister_notifier(....);

return ret;
}

done:
....

Regards

Mike

Quoted 56 lines

Regards

Mike

Quoted 32 lines
Quoted 32 lines
drivers/hwtracing/coresight/coresight-cti.c +25 -3
+     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;
Quoted 14 lines

drivers/hwtracing/coresight/coresight-cti.c static int cti_dying_cpu(unsigned int cpu) · line 795

Quoted 10 lines
drivers/hwtracing/coresight/coresight-cti.c +4 -4
-                     cpuhp_remove_state_nocalls(
-                             CPUHP_AP_ARM_CORESIGHT_CTI_STARTING);
-             }
-             cti_cpu_drvdata[drvdata->ctidev.cpu] = NULL;
+     cti_cpu_drvdata[drvdata->ctidev.cpu] = drvdata;
+     if (--nr_cti_cpu = 0) {
+             cpu_pm_unregister_notifier(&cti_cpu_pm_nb);
+             cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_CTI_STARTING);
      }
 }

regards,
dan carpenter

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

Signature · 3 lines
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
author
#8 lore raw

PATCH v2 1/1 coresight: cti: Fix error handling in probe

In reply to Mike Leach

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 <[email protected]>

---
v2: I accidentally introduced a bug in cti_pm_release() in v1.

 drivers/hwtracing/coresight/coresight-cti.c | 96 ++++++++++++---------
 1 file changed, 54 insertions(+), 42 deletions(-)
drivers/hwtracing/coresight/coresight-cti.c +54 -42
index 40387d58c8e7..d2da5bf9f552 100644
@@ -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;
 }
Signature · 1 line
2.27.0
#9 lore raw

Re: [PATCH v2] coresight: cti: Fix error handling in probe

Quoted 15 lines · Dan Carpenter

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.

Quoted 7 lines

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 <[email protected]>

---
v2: I accidentally introduced a bug in cti_pm_release() in v1.

Thanks for the cleanup. I'll send this to Greg for a 5.8 fixup.

Regards,
Mathieu

Quoted 163 lines
 drivers/hwtracing/coresight/coresight-cti.c | 96 ++++++++++++---------
 1 file changed, 54 insertions(+), 42 deletions(-)
drivers/hwtracing/coresight/coresight-cti.c +54 -42
index 40387d58c8e7..d2da5bf9f552 100644
@@ -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;
 }
Signature · 1 line
2.27.0