* [kbuild] drivers/hwtracing/coresight/coresight-cti.c:862 cti_probe() error: we previously assumed 'drvdata' could be null (see line 759)
@ 2020-06-12 12:10 Dan Carpenter
2020-06-12 12:11 ` [PATCH] coresight: cti: Fix error handling in probe Dan Carpenter
2020-06-17 9:20 ` [kbuild] drivers/hwtracing/coresight/coresight-cti.c:862 cti_probe() error: we previously assumed 'drvdata' could be null (see line 759) Mike Leach
0 siblings, 2 replies; 11+ messages in thread
From: Dan Carpenter @ 2020-06-12 12:10 UTC (permalink / raw)
To: kbuild, Mike Leach
Cc: lkp, kbuild-all, linux-kernel, Greg Kroah-Hartman, Mathieu Poirier
[-- Attachment #1: Type: text/plain, Size: 12693 bytes --]
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 <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
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@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31985 bytes --]
[-- Attachment #3: Type: text/plain, Size: 149 bytes --]
_______________________________________________
kbuild mailing list -- kbuild@lists.01.org
To unsubscribe send an email to kbuild-leave@lists.01.org
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] coresight: cti: Fix error handling in probe
2020-06-12 12:10 [kbuild] drivers/hwtracing/coresight/coresight-cti.c:862 cti_probe() error: we previously assumed 'drvdata' could be null (see line 759) Dan Carpenter
@ 2020-06-12 12:11 ` Dan Carpenter
2020-06-12 14:11 ` AW: " Walter Harms
` (2 more replies)
2020-06-17 9:20 ` [kbuild] drivers/hwtracing/coresight/coresight-cti.c:862 cti_probe() error: we previously assumed 'drvdata' could be null (see line 759) Mike Leach
1 sibling, 3 replies; 11+ messages in thread
From: Dan Carpenter @ 2020-06-12 12:11 UTC (permalink / raw)
To: Mike Leach
Cc: Mathieu Poirier, Suzuki K Poulose, Alexander Shishkin,
Greg Kroah-Hartman, linux-arm-kernel, linux-kernel,
kernel-janitors
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>
---
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(-)
diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c
index 40387d58c8e7..d2da5bf9f552 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] = 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* AW: [PATCH] coresight: cti: Fix error handling in probe
2020-06-12 12:11 ` [PATCH] coresight: cti: Fix error handling in probe Dan Carpenter
@ 2020-06-12 14:11 ` Walter Harms
2020-06-12 17:38 ` Dan Carpenter
2020-06-12 17:42 ` Dan Carpenter
2020-06-17 10:49 ` Mike Leach
2 siblings, 1 reply; 11+ messages in thread
From: Walter Harms @ 2020-06-12 14:11 UTC (permalink / raw)
To: Dan Carpenter, Mike Leach
Cc: Mathieu Poirier, Suzuki K Poulose, Alexander Shishkin,
Greg Kroah-Hartman, linux-arm-kernel, linux-kernel,
kernel-janitors
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: kernel-janitors-owner@vger.kernel.org <kernel-janitors-owner@vger.kernel.org> im Auftrag von Dan Carpenter <dan.carpenter@oracle.com>
Gesendet: Freitag, 12. Juni 2020 14:11:33
An: Mike Leach
Cc: Mathieu Poirier; Suzuki K Poulose; Alexander Shishkin; Greg Kroah-Hartman; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org
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 <dan.carpenter@oracle.com>
---
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(-)
diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c
index 40387d58c8e7..d2da5bf9f552 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] = 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] coresight: cti: Fix error handling in probe
2020-06-12 14:11 ` AW: " Walter Harms
@ 2020-06-12 17:38 ` Dan Carpenter
0 siblings, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2020-06-12 17:38 UTC (permalink / raw)
To: Walter Harms
Cc: Mike Leach, Mathieu Poirier, Suzuki K Poulose,
Alexander Shishkin, Greg Kroah-Hartman, linux-arm-kernel,
linux-kernel, kernel-janitors
On Fri, Jun 12, 2020 at 02:11:16PM +0000, Walter Harms wrote:
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] coresight: cti: Fix error handling in probe
2020-06-12 12:11 ` [PATCH] coresight: cti: Fix error handling in probe Dan Carpenter
2020-06-12 14:11 ` AW: " Walter Harms
@ 2020-06-12 17:42 ` Dan Carpenter
2020-06-17 10:53 ` Mike Leach
2020-06-17 10:49 ` Mike Leach
2 siblings, 1 reply; 11+ messages in thread
From: Dan Carpenter @ 2020-06-12 17:42 UTC (permalink / raw)
To: Mike Leach
Cc: Mathieu Poirier, Suzuki K Poulose, Alexander Shishkin,
Greg Kroah-Hartman, linux-arm-kernel, linux-kernel,
kernel-janitors
On Fri, Jun 12, 2020 at 03:11:33PM +0300, Dan Carpenter wrote:
> +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.
> + 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);
> }
> }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [kbuild] drivers/hwtracing/coresight/coresight-cti.c:862 cti_probe() error: we previously assumed 'drvdata' could be null (see line 759)
2020-06-12 12:10 [kbuild] drivers/hwtracing/coresight/coresight-cti.c:862 cti_probe() error: we previously assumed 'drvdata' could be null (see line 759) Dan Carpenter
2020-06-12 12:11 ` [PATCH] coresight: cti: Fix error handling in probe Dan Carpenter
@ 2020-06-17 9:20 ` Mike Leach
1 sibling, 0 replies; 11+ messages in thread
From: Mike Leach @ 2020-06-17 9:20 UTC (permalink / raw)
To: Dan Carpenter
Cc: kbuild, lkp, kbuild-all, Linux Kernel Mailing List,
Greg Kroah-Hartman, Mathieu Poirier
Hi Dan,
Thanks for the comprehensive review of this.
On Fri, 12 Jun 2020 at 14:47, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
>
> 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
> 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 <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> 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@lists.01.org
> _______________________________________________
> kbuild mailing list -- kbuild@lists.01.org
> To unsubscribe send an email to kbuild-leave@lists.01.org
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] coresight: cti: Fix error handling in probe
2020-06-12 12:11 ` [PATCH] coresight: cti: Fix error handling in probe Dan Carpenter
2020-06-12 14:11 ` AW: " Walter Harms
2020-06-12 17:42 ` Dan Carpenter
@ 2020-06-17 10:49 ` Mike Leach
2020-06-17 17:15 ` [PATCH v2] " Dan Carpenter
2 siblings, 1 reply; 11+ messages in thread
From: Mike Leach @ 2020-06-17 10:49 UTC (permalink / raw)
To: Dan Carpenter
Cc: Mathieu Poirier, Suzuki K Poulose, Alexander Shishkin,
Greg Kroah-Hartman, linux-arm-kernel, Linux Kernel Mailing List,
kernel-janitors
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.
On Fri, 12 Jun 2020 at 14:46, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> 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>
> ---
> 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(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c
> index 40387d58c8e7..d2da5bf9f552 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] = drvdata;
This should remain as cti_cpu_drvdata[drvdata->ctidev.cpu] = NULL;
here. We are reversing the assignment in cti_pm_setup().
> + 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
>
Reviewed-by Mike Leach <mike.leach@linaro.org>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] coresight: cti: Fix error handling in probe
2020-06-12 17:42 ` Dan Carpenter
@ 2020-06-17 10:53 ` Mike Leach
2020-06-17 14:24 ` Mike Leach
0 siblings, 1 reply; 11+ messages in thread
From: Mike Leach @ 2020-06-17 10:53 UTC (permalink / raw)
To: Dan Carpenter
Cc: Mathieu Poirier, Suzuki K Poulose, Alexander Shishkin,
Greg Kroah-Hartman, linux-arm-kernel, Linux Kernel Mailing List,
kernel-janitors
Hi Dan,
On Fri, 12 Jun 2020 at 18:43, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Fri, Jun 12, 2020 at 03:11:33PM +0300, Dan Carpenter wrote:
> > +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.
>
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
> > + 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);
> > }
> > }
>
> regards,
> dan carpenter
>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] coresight: cti: Fix error handling in probe
2020-06-17 10:53 ` Mike Leach
@ 2020-06-17 14:24 ` Mike Leach
0 siblings, 0 replies; 11+ messages in thread
From: Mike Leach @ 2020-06-17 14:24 UTC (permalink / raw)
To: Dan Carpenter
Cc: Mathieu Poirier, Suzuki K Poulose, Alexander Shishkin,
Greg Kroah-Hartman, linux-arm-kernel, Linux Kernel Mailing List,
kernel-janitors
HI Dan,
Looked into this some more...
On Wed, 17 Jun 2020 at 11:53, Mike Leach <mike.leach@linaro.org> wrote:
>
> Hi Dan,
>
> On Fri, 12 Jun 2020 at 18:43, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Fri, Jun 12, 2020 at 03:11:33PM +0300, Dan Carpenter wrote:
> > > +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.
> >
>
> 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
> Regards
>
> Mike
>
> > > + 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);
> > > }
> > > }
> >
> > regards,
> > dan carpenter
> >
>
>
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] coresight: cti: Fix error handling in probe
2020-06-17 10:49 ` Mike Leach
@ 2020-06-17 17:15 ` Dan Carpenter
2020-06-29 20:29 ` Mathieu Poirier
0 siblings, 1 reply; 11+ messages in thread
From: Dan Carpenter @ 2020-06-17 17:15 UTC (permalink / raw)
To: Mike Leach
Cc: Mathieu Poirier, Suzuki K Poulose, Alexander Shishkin,
Greg Kroah-Hartman, linux-arm-kernel, linux-kernel,
kernel-janitors
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>
---
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(-)
diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c
index 40387d58c8e7..d2da5bf9f552 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.27.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] coresight: cti: Fix error handling in probe
2020-06-17 17:15 ` [PATCH v2] " Dan Carpenter
@ 2020-06-29 20:29 ` Mathieu Poirier
0 siblings, 0 replies; 11+ messages in thread
From: Mathieu Poirier @ 2020-06-29 20:29 UTC (permalink / raw)
To: Dan Carpenter
Cc: Mike Leach, Suzuki K Poulose, Alexander Shishkin,
Greg Kroah-Hartman, linux-arm-kernel, linux-kernel,
kernel-janitors
On Wed, Jun 17, 2020 at 08:15:50PM +0300, Dan Carpenter wrote:
> 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>
> ---
> 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
>
> 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..d2da5bf9f552 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.27.0
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-06-29 20:29 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-12 12:10 [kbuild] drivers/hwtracing/coresight/coresight-cti.c:862 cti_probe() error: we previously assumed 'drvdata' could be null (see line 759) Dan Carpenter
2020-06-12 12:11 ` [PATCH] coresight: cti: Fix error handling in probe Dan Carpenter
2020-06-12 14:11 ` AW: " Walter Harms
2020-06-12 17:38 ` Dan Carpenter
2020-06-12 17:42 ` Dan Carpenter
2020-06-17 10:53 ` Mike Leach
2020-06-17 14:24 ` Mike Leach
2020-06-17 10:49 ` Mike Leach
2020-06-17 17:15 ` [PATCH v2] " Dan Carpenter
2020-06-29 20:29 ` Mathieu Poirier
2020-06-17 9:20 ` [kbuild] drivers/hwtracing/coresight/coresight-cti.c:862 cti_probe() error: we previously assumed 'drvdata' could be null (see line 759) Mike Leach
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).