| Message ID | 1585f6c21ea8aee64fe4da0bf72b36ea4d74a779.1611227342.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | opp: Implement dev_pm_opp_set_opp() | expand |
21.01.2021 14:17, Viresh Kumar пишет: > In order to avoid conditional statements at the caller site, this patch > updates _generic_set_opp_clk_only() to work for devices that don't > change frequency (like power domains, etc.). Return 0 if the clk pointer > passed to this routine is not valid. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- ... Hello Viresh, Thank you very much for yours effort! I gave a quick test to this series and instantly found one small issue in this patch. > + /* We may reach here for devices which don't change frequency */ > + if (unlikely(!clk)) I replaced dev_pm_opp_set_voltage() with dev_pm_opp_set_opp() in the Tegra PD driver and got a crash, which happens because the above line should be: if (IS_ERR(clk)) The opp_table->clk is initialized to ERR_PTR(-ENOENT) if device doesn't have a clock, like a power domain device in my case. Everything works good after fixing this patch. I'll keep testing and will be taking a closer look at the rest of the patches over this weekend. For the record, here is a backtrace of the crash: Unable to handle kernel NULL pointer dereference at virtual address 00000016 ... (clk_set_rate) from (_set_opp) (_set_opp) from (dev_pm_opp_set_opp) (dev_pm_opp_set_opp) from (tegra_genpd_set_performance_state) (tegra_genpd_set_performance_state) from (_genpd_set_performance_state) (_genpd_set_performance_state) from (dev_pm_genpd_set_performance_state) (dev_pm_genpd_set_performance_state) from (_set_required_opp) (_set_required_opp) from (_set_opp) (_set_opp) from (dev_pm_opp_set_rate) (dev_pm_opp_set_rate) from (host1x_runtime_resume) (host1x_runtime_resume) from (genpd_runtime_resume) (genpd_runtime_resume) from (__rpm_callback) (__rpm_callback) from (rpm_callback) (rpm_callback) from (rpm_resume) (rpm_resume) from (__pm_runtime_resume) (__pm_runtime_resume) from (host1x_probe) (host1x_probe) from (platform_probe) (platform_probe) from (really_probe) (really_probe) from (driver_probe_device) (driver_probe_device) from (device_driver_attach) (device_driver_attach) from (__driver_attach) (__driver_attach) from (bus_for_each_dev) (bus_for_each_dev) from (bus_add_driver) (bus_add_driver) from (driver_register) (driver_register) from (__platform_register_drivers) (__platform_register_drivers) from (host1x_module_init) (host1x_module_init) from (do_one_initcall) (do_one_initcall) from (kernel_init_freeable) (kernel_init_freeable) from (kernel_init)
On 21-01-21, 23:26, Dmitry Osipenko wrote: > 21.01.2021 14:17, Viresh Kumar пишет: > > In order to avoid conditional statements at the caller site, this patch > > updates _generic_set_opp_clk_only() to work for devices that don't > > change frequency (like power domains, etc.). Return 0 if the clk pointer > > passed to this routine is not valid. > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > ... > > Hello Viresh, > > Thank you very much for yours effort! I gave a quick test to this series > and instantly found one small issue in this patch. > > > + /* We may reach here for devices which don't change frequency */ > > + if (unlikely(!clk)) > > I replaced dev_pm_opp_set_voltage() with dev_pm_opp_set_opp() in the > Tegra PD driver and got a crash, which happens because the above line > should be: > > if (IS_ERR(clk)) Fixed, thanks.
22.01.2021 07:35, Viresh Kumar пишет: > On 21-01-21, 23:26, Dmitry Osipenko wrote: >> 21.01.2021 14:17, Viresh Kumar пишет: >>> In order to avoid conditional statements at the caller site, this patch >>> updates _generic_set_opp_clk_only() to work for devices that don't >>> change frequency (like power domains, etc.). Return 0 if the clk pointer >>> passed to this routine is not valid. >>> >>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> >>> --- >> ... >> >> Hello Viresh, >> >> Thank you very much for yours effort! I gave a quick test to this series >> and instantly found one small issue in this patch. >> >>> + /* We may reach here for devices which don't change frequency */ >>> + if (unlikely(!clk)) >> >> I replaced dev_pm_opp_set_voltage() with dev_pm_opp_set_opp() in the >> Tegra PD driver and got a crash, which happens because the above line >> should be: >> >> if (IS_ERR(clk)) > > Fixed, thanks. > Please remove unlikely() around IS_ERR(), it already has the unlikely(). https://elixir.bootlin.com/linux/v5.11-rc4/source/include/linux/err.h#L22 I'd also recommend to remove all the unlikely() from OPP code since it doesn't bring any value if not used in a very performance-critical code path. OPP core doesn't have such code paths. The [un]likely() only make code less readable and may result in a worse assembly.
On 26-01-21, 00:09, Dmitry Osipenko wrote: > Please remove unlikely() around IS_ERR(), it already has the unlikely(). Right. > https://elixir.bootlin.com/linux/v5.11-rc4/source/include/linux/err.h#L22 > > I'd also recommend to remove all the unlikely() from OPP code since it > doesn't bring any value if not used in a very performance-critical code > path. OPP core doesn't have such code paths. The [un]likely() only make > code less readable and may result in a worse assembly. The likely/unlikely() stuff is to optimize code, not necessarily the stuff in the hot path alone, therwise stuff like IS_ERR() would never have it. It surely does bring value by optimizing the code, surely the result isn't significant enough but that is fine, every effort counts. AFAIK, if we are sure of path the code will almost always take, then we should rather use these and so I am inclined towards keeping them. Though I understand that using them may result in bad behavior (performance) if they fail.
diff --git a/drivers/opp/core.c b/drivers/opp/core.c index a96ffd9051b1..6b09d468d37a 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -726,6 +726,10 @@ static inline int _generic_set_opp_clk_only(struct device *dev, struct clk *clk, { int ret; + /* We may reach here for devices which don't change frequency */ + if (unlikely(!clk)) + return 0; + ret = clk_set_rate(clk, freq); if (ret) { dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
In order to avoid conditional statements at the caller site, this patch updates _generic_set_opp_clk_only() to work for devices that don't change frequency (like power domains, etc.). Return 0 if the clk pointer passed to this routine is not valid. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/opp/core.c | 4 ++++ 1 file changed, 4 insertions(+)