Re: [PATCH 07/13] opp: Allow _generic_set_opp_clk_only() to work for non-freq devices

[Viresh Kumar]

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.

-- 
viresh

Re: [PATCH 07/13] opp: Allow _generic_set_opp_clk_only() to work for non-freq devices

[Dmitry Osipenko]

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 
>>> ---
>> ...
>>
>> 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.

Re: [PATCH 07/13] opp: Allow _generic_set_opp_clk_only() to work for non-freq devices

[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 
> > ---
> ...
> 
> 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.

-- 
viresh

Re: [PATCH 07/13] opp: Allow _generic_set_opp_clk_only() to work for non-freq devices

[Dmitry Osipenko]

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 
> ---
...

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 0016
...
(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)

[PATCH 07/13] opp: Allow _generic_set_opp_clk_only() to work for non-freq devices

[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 
---
 drivers/opp/core.c | 4 
 1 file changed, 4 insertions(+)

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__,
-- 
2.25.0.rc1.19.g042ed3e048af