diff mbox series

perf: qcom_l2_pmu: Make l2_cache_pmu_probe_cluster() more robust

Message ID 6a0f5bdb6b7b2ed4ef194fc49693e902ad5b95ea.1684397879.git.christophe.jaillet@wanadoo.fr
State New
Headers
Series perf: qcom_l2_pmu: Make l2_cache_pmu_probe_cluster() more robust |

Commit Message

Christophe JAILLET May 18, 2023, 8:18 a.m. UTC 
  If an error occurs after calling list_add(), the &l2cache_pmu->clusters
list will reference some memory that will be freed when the managed
resources will be released.

Move the list_add() at the end of the function when everything is in fine.

This is harmless because if l2_cache_pmu_probe_cluster() fails, then
l2_cache_pmu_probe() will fail as well and 'l2cache_pmu' will be released
as well.
But it looks cleaner and could silence static checker warning.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This is not a fix, because there is no issue.
But in case of interest:
Fixes: 21bdbb7102ed ("perf: add qcom l2 cache perf events driver")
---
 drivers/perf/qcom_l2_pmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Dan Carpenter May 19, 2023, 4:27 a.m. UTC | #1 
On Thu, May 18, 2023 at 10:18:08AM +0200, Christophe JAILLET wrote:
> But it looks cleaner and could silence static checker warning.

Thanks Christophe,

Of course, you found found this with your Coccinelle script so
technically it already silences a static checker warning...  But I
ran into two list related use after frees yesterday so I'm definitely
going to create a Smatch warning which will trigger here as well.

I'm NOT going to add a if (if_list_is_devm()) exception because I feel
like that encourages subtle code and because devm_ lifetimes are
complicated.

regards,
dan carpenter
Will Deacon June 5, 2023, 4:35 p.m. UTC | #2 
On Thu, 18 May 2023 10:18:08 +0200, Christophe JAILLET wrote:
> If an error occurs after calling list_add(), the &l2cache_pmu->clusters
> list will reference some memory that will be freed when the managed
> resources will be released.
> 
> Move the list_add() at the end of the function when everything is in fine.
> 
> This is harmless because if l2_cache_pmu_probe_cluster() fails, then
> l2_cache_pmu_probe() will fail as well and 'l2cache_pmu' will be released
> as well.
> But it looks cleaner and could silence static checker warning.
> 
> [...]

Applied to will (for-next/perf), thanks!

[1/1] perf: qcom_l2_pmu: Make l2_cache_pmu_probe_cluster() more robust
      https://git.kernel.org/will/c/7bd42f122c7c

Cheers,
diff mbox series

Patch

diff --git a/drivers/perf/qcom_l2_pmu.c b/drivers/perf/qcom_l2_pmu.c
index aaca6db7d8f6..3f9a98c17a89 100644
--- a/drivers/perf/qcom_l2_pmu.c
+++ b/drivers/perf/qcom_l2_pmu.c
@@ -857,7 +857,6 @@  static int l2_cache_pmu_probe_cluster(struct device *dev, void *data)
 		return -ENOMEM;
 
 	INIT_LIST_HEAD(&cluster->next);
-	list_add(&cluster->next, &l2cache_pmu->clusters);
 	cluster->cluster_id = fw_cluster_id;
 
 	irq = platform_get_irq(sdev, 0);
@@ -883,6 +882,7 @@  static int l2_cache_pmu_probe_cluster(struct device *dev, void *data)
 
 	spin_lock_init(&cluster->pmu_lock);
 
+	list_add(&cluster->next, &l2cache_pmu->clusters);
 	l2cache_pmu->num_pmus++;
 
 	return 0;