| Message ID | 1575974784-55046-1-git-send-email-guohanjun@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf/smmuv3: Remove the leftover put_cpu() in error path | expand |
On 10/12/2019 10:46 am, Hanjun Guo wrote: > In smmu_pmu_probe(), there is put_cpu() in the error path, > which is wrong because we use raw_smp_processor_id() to > get the cpu ID, not get_cpu(), remove it. Bah, somehow that slipped through the last round of review :) Acked-by: Robin Murphy <robin.murphy@arm.com> > Signed-off-by: Hanjun Guo <guohanjun@huawei.com> > --- > drivers/perf/arm_smmuv3_pmu.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c > index 773128f..fd1d46a 100644 > --- a/drivers/perf/arm_smmuv3_pmu.c > +++ b/drivers/perf/arm_smmuv3_pmu.c > @@ -834,7 +834,6 @@ static int smmu_pmu_probe(struct platform_device *pdev) > out_unregister: > cpuhp_state_remove_instance_nocalls(cpuhp_state_num, &smmu_pmu->node); > out_cpuhp_err: > - put_cpu(); > return err; > } > >
On Tue, Dec 10, 2019 at 06:46:24PM +0800, Hanjun Guo wrote: > In smmu_pmu_probe(), there is put_cpu() in the error path, > which is wrong because we use raw_smp_processor_id() to > get the cpu ID, not get_cpu(), remove it. > > Signed-off-by: Hanjun Guo <guohanjun@huawei.com> > --- > drivers/perf/arm_smmuv3_pmu.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c > index 773128f..fd1d46a 100644 > --- a/drivers/perf/arm_smmuv3_pmu.c > +++ b/drivers/perf/arm_smmuv3_pmu.c > @@ -834,7 +834,6 @@ static int smmu_pmu_probe(struct platform_device *pdev) > out_unregister: > cpuhp_state_remove_instance_nocalls(cpuhp_state_num, &smmu_pmu->node); > out_cpuhp_err: > - put_cpu(); > return err; Can we kill 'out_cpuhp_err' altogether then and just return err if we fail to add the hotplug instance? Will
On 2019/12/10 21:24, Will Deacon wrote: > On Tue, Dec 10, 2019 at 06:46:24PM +0800, Hanjun Guo wrote: >> In smmu_pmu_probe(), there is put_cpu() in the error path, >> which is wrong because we use raw_smp_processor_id() to >> get the cpu ID, not get_cpu(), remove it. >> >> Signed-off-by: Hanjun Guo <guohanjun@huawei.com> >> --- >> drivers/perf/arm_smmuv3_pmu.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c >> index 773128f..fd1d46a 100644 >> --- a/drivers/perf/arm_smmuv3_pmu.c >> +++ b/drivers/perf/arm_smmuv3_pmu.c >> @@ -834,7 +834,6 @@ static int smmu_pmu_probe(struct platform_device *pdev) >> out_unregister: >> cpuhp_state_remove_instance_nocalls(cpuhp_state_num, &smmu_pmu->node); >> out_cpuhp_err: >> - put_cpu(); >> return err; > > Can we kill 'out_cpuhp_err' altogether then and just return err if we fail > to add the hotplug instance? Makes sense, but I think we can go further to kill both 'out_cpuhp_err' and 'out_register' as below [1], what do you think? Thanks Hanjun [1]: diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c index fd1d46a..a5adaba 100644 --- a/drivers/perf/arm_smmuv3_pmu.c +++ b/drivers/perf/arm_smmuv3_pmu.c @@ -814,14 +814,15 @@ static int smmu_pmu_probe(struct platform_device *pdev) if (err) { dev_err(dev, "Error %d registering hotplug, PMU @%pa\n", err, &res_0->start); - goto out_cpuhp_err; + return err; } err = perf_pmu_register(&smmu_pmu->pmu, name, -1); if (err) { + cpuhp_state_remove_instance_nocalls(cpuhp_state_num, &smmu_pmu->node); dev_err(dev, "Error %d registering PMU @%pa\n", err, &res_0->start); - goto out_unregister; + return err; } dev_info(dev, "Registered PMU @ %pa using %d counters with %s filter settings\n", @@ -830,11 +831,6 @@ static int smmu_pmu_probe(struct platform_device *pdev) "Individual"); return 0; - -out_unregister: - cpuhp_state_remove_instance_nocalls(cpuhp_state_num, &smmu_pmu->node); -out_cpuhp_err: - return err; } static int smmu_pmu_remove(struct platform_device *pdev)
On Tue, Dec 10, 2019 at 09:55:28PM +0800, Hanjun Guo wrote: > On 2019/12/10 21:24, Will Deacon wrote: > > On Tue, Dec 10, 2019 at 06:46:24PM +0800, Hanjun Guo wrote: > >> In smmu_pmu_probe(), there is put_cpu() in the error path, > >> which is wrong because we use raw_smp_processor_id() to > >> get the cpu ID, not get_cpu(), remove it. > >> > >> Signed-off-by: Hanjun Guo <guohanjun@huawei.com> > >> --- > >> drivers/perf/arm_smmuv3_pmu.c | 1 - > >> 1 file changed, 1 deletion(-) > >> > >> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c > >> index 773128f..fd1d46a 100644 > >> --- a/drivers/perf/arm_smmuv3_pmu.c > >> +++ b/drivers/perf/arm_smmuv3_pmu.c > >> @@ -834,7 +834,6 @@ static int smmu_pmu_probe(struct platform_device *pdev) > >> out_unregister: > >> cpuhp_state_remove_instance_nocalls(cpuhp_state_num, &smmu_pmu->node); > >> out_cpuhp_err: > >> - put_cpu(); > >> return err; > > > > Can we kill 'out_cpuhp_err' altogether then and just return err if we fail > > to add the hotplug instance? > > Makes sense, but I think we can go further to kill both 'out_cpuhp_err' and > 'out_register' as below [1], what do you think? Although that's functionally correct, I'd prefer to keep out_unregister(), since it acts as good reminder to anybody extending this function in future that they need to unregister the hotplug instance on failure. Will
On 2019/12/10 22:10, Will Deacon wrote: > On Tue, Dec 10, 2019 at 09:55:28PM +0800, Hanjun Guo wrote: >> On 2019/12/10 21:24, Will Deacon wrote: >>> On Tue, Dec 10, 2019 at 06:46:24PM +0800, Hanjun Guo wrote: >>>> In smmu_pmu_probe(), there is put_cpu() in the error path, >>>> which is wrong because we use raw_smp_processor_id() to >>>> get the cpu ID, not get_cpu(), remove it. >>>> >>>> Signed-off-by: Hanjun Guo <guohanjun@huawei.com> >>>> --- >>>> drivers/perf/arm_smmuv3_pmu.c | 1 - >>>> 1 file changed, 1 deletion(-) >>>> >>>> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c >>>> index 773128f..fd1d46a 100644 >>>> --- a/drivers/perf/arm_smmuv3_pmu.c >>>> +++ b/drivers/perf/arm_smmuv3_pmu.c >>>> @@ -834,7 +834,6 @@ static int smmu_pmu_probe(struct platform_device *pdev) >>>> out_unregister: >>>> cpuhp_state_remove_instance_nocalls(cpuhp_state_num, &smmu_pmu->node); >>>> out_cpuhp_err: >>>> - put_cpu(); >>>> return err; >>> >>> Can we kill 'out_cpuhp_err' altogether then and just return err if we fail >>> to add the hotplug instance? >> >> Makes sense, but I think we can go further to kill both 'out_cpuhp_err' and >> 'out_register' as below [1], what do you think? > > Although that's functionally correct, I'd prefer to keep out_unregister(), > since it acts as good reminder to anybody extending this function in future > that they need to unregister the hotplug instance on failure. OK, I will add Robin's ACK and resend. Thanks Hanjun
diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c index 773128f..fd1d46a 100644 --- a/drivers/perf/arm_smmuv3_pmu.c +++ b/drivers/perf/arm_smmuv3_pmu.c @@ -834,7 +834,6 @@ static int smmu_pmu_probe(struct platform_device *pdev) out_unregister: cpuhp_state_remove_instance_nocalls(cpuhp_state_num, &smmu_pmu->node); out_cpuhp_err: - put_cpu(); return err; }
In smmu_pmu_probe(), there is put_cpu() in the error path, which is wrong because we use raw_smp_processor_id() to get the cpu ID, not get_cpu(), remove it. Signed-off-by: Hanjun Guo <guohanjun@huawei.com> --- drivers/perf/arm_smmuv3_pmu.c | 1 - 1 file changed, 1 deletion(-)