| Message ID | 6a0f5bdb6b7b2ed4ef194fc49693e902ad5b95ea.1684397879.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf: qcom_l2_pmu: Make l2_cache_pmu_probe_cluster() more robust | expand |
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
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 --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;
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(-)