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

From: Dmitry Osipenko <digetx@gmail.com>
To: Viresh Kumar <viresh.kumar@linaro.org>,
	Viresh Kumar <vireshk@kernel.org>, Nishanth Menon <nm@ti.com>,
	Stephen Boyd <sboyd@kernel.org>
Cc: linux-pm@vger.kernel.org,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Rafael Wysocki <rjw@rjwysocki.net>,
	Sibi Sankar <sibis@codeaurora.org>,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	"linux-tegra@vger.kernel.org" <linux-tegra@vger.kernel.org>
Subject: Re: [PATCH 07/13] opp: Allow _generic_set_opp_clk_only() to work for non-freq devices
Date: Thu, 21 Jan 2021 23:26:43 +0300	[thread overview]
Message-ID: <9b2638e6-b842-8737-e5a0-aeeb84927fce@gmail.com> (raw)
In-Reply-To: <1585f6c21ea8aee64fe4da0bf72b36ea4d74a779.1611227342.git.viresh.kumar@linaro.org>

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)