| Message ID | 20240710110813.GA15351@bnew-VirtualBox (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drivers/clocksource/qcom: Add missing iounmap() on error when reading clock frequency. | expand |
On 10.07.2024 1:08 PM, Ankit Agrawal wrote: > Add the missing iounmap() when clock frequency fails to get read by the > of_property_read_u32() call. > > Signed-off-by: Ankit Agrawal <agrawal.ag.ankit@gmail.com> > --- Or even better, you can extract: drivers/platform/x86/intel/pmc/core_ssram.c 32:DEFINE_FREE(pmc_core_iounmap, void __iomem *, iounmap(_T)); into a common header, call it something less intel-specific and use it with __free() here Konrad
On Wed, Jul 10, 2024 at 01:54:01PM +0200, Konrad Dybcio wrote: > On 10.07.2024 1:08 PM, Ankit Agrawal wrote: > > Add the missing iounmap() when clock frequency fails to get read by the > > of_property_read_u32() call. > > > > Signed-off-by: Ankit Agrawal <agrawal.ag.ankit@gmail.com> > > --- > > Or even better, you can extract: > > drivers/platform/x86/intel/pmc/core_ssram.c > 32:DEFINE_FREE(pmc_core_iounmap, void __iomem *, iounmap(_T)); > > into a common header, call it something less intel-specific and use > it with __free() here Can you please give a place where adding it would be appropriate? I am new to contributing here, so any guidance on where to add the DEFINE_FREE would be really helpful! Also, just trying to think out loud. Will the cpu0_base pointer (and also the source_base pointer) be required once this function exits? If so, I think I will also need to use no_free_ptr() to ensure that the memory doesn't get iounmap-ed once the function exits. Thanks! Ankit
On 11.07.2024 7:49 AM, Ankit Agrawal wrote: > On Wed, Jul 10, 2024 at 01:54:01PM +0200, Konrad Dybcio wrote: >> On 10.07.2024 1:08 PM, Ankit Agrawal wrote: >>> Add the missing iounmap() when clock frequency fails to get read by the >>> of_property_read_u32() call. >>> >>> Signed-off-by: Ankit Agrawal <agrawal.ag.ankit@gmail.com> >>> --- >> >> Or even better, you can extract: >> >> drivers/platform/x86/intel/pmc/core_ssram.c >> 32:DEFINE_FREE(pmc_core_iounmap, void __iomem *, iounmap(_T)); >> >> into a common header, call it something less intel-specific and use >> it with __free() here > > Can you please give a place where adding it would be appropriate? I am > new to contributing here, so any guidance on where to add the > DEFINE_FREE would be really helpful! I'd say include/linux/io.h sounds alright, but worst case scenario the maintainer of that file will ask you to move it somewhere else > Also, just trying to think out loud. Will the cpu0_base pointer (and > also the source_base pointer) be required once this function exits? If > so, I think I will also need to use no_free_ptr() to ensure that the > memory doesn't get iounmap-ed once the function exits. Ohh right source_base is a global variable.. perhaps the original patch here makes more sense given we definitely don't wanna unmap that.. So I'd say let's go with this one after all Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad
On 11.07.2024 10:11 AM, Konrad Dybcio wrote: > On 11.07.2024 7:49 AM, Ankit Agrawal wrote: >> On Wed, Jul 10, 2024 at 01:54:01PM +0200, Konrad Dybcio wrote: >>> On 10.07.2024 1:08 PM, Ankit Agrawal wrote: [...] > > Ohh right source_base is a global variable.. perhaps the original patch > here makes more sense given we definitely don't wanna unmap that.. > > So I'd say let's go with this one after all > > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> We should probably also check if msm_timer_init() succeeds and unmap if that fails.. we can just check the return value of that function and if it's non-zero, call iounmap Konrad
On Thu, Jul 11, 2024 at 10:13:38AM +0200, Konrad Dybcio wrote: > On 11.07.2024 10:11 AM, Konrad Dybcio wrote: > > On 11.07.2024 7:49 AM, Ankit Agrawal wrote: > >> On Wed, Jul 10, 2024 at 01:54:01PM +0200, Konrad Dybcio wrote: > >>> On 10.07.2024 1:08 PM, Ankit Agrawal wrote: [...] > > We should probably also check if msm_timer_init() succeeds and unmap > if that fails.. we can just check the return value of that function > and if it's non-zero, call iounmap Sure. I have updated the patch accordingly. Please find version 2 of the patch here: https://lore.kernel.org/linux-arm-msm/20240712082747.GA182658@bnew-VirtualBox/ Thanks! Ankit
diff --git a/drivers/clocksource/timer-qcom.c b/drivers/clocksource/timer-qcom.c index b4afe3a67..a66fa7f8e 100644 --- a/drivers/clocksource/timer-qcom.c +++ b/drivers/clocksource/timer-qcom.c @@ -233,6 +233,7 @@ static int __init msm_dt_timer_init(struct device_node *np) } if (of_property_read_u32(np, "clock-frequency", &freq)) { + iounmap(cpu0_base); pr_err("Unknown frequency\n"); return -EINVAL; }
Add the missing iounmap() when clock frequency fails to get read by the of_property_read_u32() call. Signed-off-by: Ankit Agrawal <agrawal.ag.ankit@gmail.com> --- drivers/clocksource/timer-qcom.c | 1 + 1 file changed, 1 insertion(+)