| Message ID | 20221209150759.396255-1-konrad.dybcio@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cpufreq: qcom-hw: Fix reading "reg" with address/size-cells != 2 | expand |
On 09-12-22, 16:07, Konrad Dybcio wrote: > Commit 054a3ef683a1 ("cpufreq: qcom-hw: Allocate qcom_cpufreq_data during > probe") assumed that every reg variable is 4*u32 wide (as most new qcom corresponding ")" is missing. > SoCs set #address- and #size-cells to <2>. That is not the case for all of > them though. Check the cells values dynamically to ensure the proper > region of the DTB is being read. > > Fixes: 054a3ef683a1 ("cpufreq: qcom-hw: Allocate qcom_cpufreq_data during probe") > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > drivers/cpufreq/qcom-cpufreq-hw.c | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c > index 340fed35e45d..22f48f789557 100644 > --- a/drivers/cpufreq/qcom-cpufreq-hw.c > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c > @@ -649,9 +649,10 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev) > { > struct clk_hw_onecell_data *clk_data; > struct device *dev = &pdev->dev; > + struct device_node *soc_node; > struct device *cpu_dev; > struct clk *clk; > - int ret, i, num_domains; > + int ret, i, num_domains, reg_sz; > > clk = clk_get(dev, "xo"); > if (IS_ERR(clk)) > @@ -679,7 +680,22 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev) > return ret; > > /* Allocate qcom_cpufreq_data based on the available frequency domains in DT */ > - num_domains = of_property_count_elems_of_size(dev->of_node, "reg", sizeof(u32) * 4); > + soc_node = of_get_parent(dev->of_node); This must be dropped on failures later ? > + if (!soc_node) > + return -EINVAL; > + > + ret = of_property_read_u32(soc_node, "#address-cells", ®_sz); > + if (ret) > + return ret; > + > + /* Reuse 'i', as it's only used later in the loop */ This can be dropped in my opinion. > + ret = of_property_read_u32(soc_node, "#size-cells", &i); Should i be initialized ? > + if (ret) > + return ret; > + > + reg_sz += i; > + > + num_domains = of_property_count_elems_of_size(dev->of_node, "reg", sizeof(u32) * reg_sz); > if (num_domains <= 0) > return num_domains; > > -- > 2.38.1
On 12.12.2022 07:44, Viresh Kumar wrote: > On 09-12-22, 16:07, Konrad Dybcio wrote: >> Commit 054a3ef683a1 ("cpufreq: qcom-hw: Allocate qcom_cpufreq_data during >> probe") assumed that every reg variable is 4*u32 wide (as most new qcom > > corresponding ")" is missing. > >> SoCs set #address- and #size-cells to <2>. That is not the case for all of >> them though. Check the cells values dynamically to ensure the proper >> region of the DTB is being read. >> >> Fixes: 054a3ef683a1 ("cpufreq: qcom-hw: Allocate qcom_cpufreq_data during probe") >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> --- >> drivers/cpufreq/qcom-cpufreq-hw.c | 20 ++++++++++++++++++-- >> 1 file changed, 18 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c >> index 340fed35e45d..22f48f789557 100644 >> --- a/drivers/cpufreq/qcom-cpufreq-hw.c >> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c >> @@ -649,9 +649,10 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev) >> { >> struct clk_hw_onecell_data *clk_data; >> struct device *dev = &pdev->dev; >> + struct device_node *soc_node; >> struct device *cpu_dev; >> struct clk *clk; >> - int ret, i, num_domains; >> + int ret, i, num_domains, reg_sz; >> >> clk = clk_get(dev, "xo"); >> if (IS_ERR(clk)) >> @@ -679,7 +680,22 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev) >> return ret; >> >> /* Allocate qcom_cpufreq_data based on the available frequency domains in DT */ >> - num_domains = of_property_count_elems_of_size(dev->of_node, "reg", sizeof(u32) * 4); >> + soc_node = of_get_parent(dev->of_node); > > This must be dropped on failures later ? Right. > >> + if (!soc_node) >> + return -EINVAL; >> + >> + ret = of_property_read_u32(soc_node, "#address-cells", ®_sz); >> + if (ret) >> + return ret; >> + >> + /* Reuse 'i', as it's only used later in the loop */ > > This can be dropped in my opinion. Sure. > >> + ret = of_property_read_u32(soc_node, "#size-cells", &i); > > Should i be initialized ? Why? If this call succeeds, i is never read before being assigned a value, and if it fails, the probe function will exit with an error. Konrad > >> + if (ret) >> + return ret; >> + >> + reg_sz += i; >> + >> + num_domains = of_property_count_elems_of_size(dev->of_node, "reg", sizeof(u32) * reg_sz); >> if (num_domains <= 0) >> return num_domains; >> >> -- >> 2.38.1 >
diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c index 340fed35e45d..22f48f789557 100644 --- a/drivers/cpufreq/qcom-cpufreq-hw.c +++ b/drivers/cpufreq/qcom-cpufreq-hw.c @@ -649,9 +649,10 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev) { struct clk_hw_onecell_data *clk_data; struct device *dev = &pdev->dev; + struct device_node *soc_node; struct device *cpu_dev; struct clk *clk; - int ret, i, num_domains; + int ret, i, num_domains, reg_sz; clk = clk_get(dev, "xo"); if (IS_ERR(clk)) @@ -679,7 +680,22 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev) return ret; /* Allocate qcom_cpufreq_data based on the available frequency domains in DT */ - num_domains = of_property_count_elems_of_size(dev->of_node, "reg", sizeof(u32) * 4); + soc_node = of_get_parent(dev->of_node); + if (!soc_node) + return -EINVAL; + + ret = of_property_read_u32(soc_node, "#address-cells", ®_sz); + if (ret) + return ret; + + /* Reuse 'i', as it's only used later in the loop */ + ret = of_property_read_u32(soc_node, "#size-cells", &i); + if (ret) + return ret; + + reg_sz += i; + + num_domains = of_property_count_elems_of_size(dev->of_node, "reg", sizeof(u32) * reg_sz); if (num_domains <= 0) return num_domains;
Commit 054a3ef683a1 ("cpufreq: qcom-hw: Allocate qcom_cpufreq_data during probe") assumed that every reg variable is 4*u32 wide (as most new qcom SoCs set #address- and #size-cells to <2>. That is not the case for all of them though. Check the cells values dynamically to ensure the proper region of the DTB is being read. Fixes: 054a3ef683a1 ("cpufreq: qcom-hw: Allocate qcom_cpufreq_data during probe") Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- drivers/cpufreq/qcom-cpufreq-hw.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)