diff mbox series

[02/31] OPP: Add dev_pm_opp_set_config() and friends

Message ID 9c4b2bfe628bf7a583a96cee7cc3539e2e66245e.1653564321.git.viresh.kumar@linaro.org
State New
Headers show
Series OPP: Add new configuration interface: dev_pm_opp_set_config() | expand

Commit Message

Viresh Kumar May 26, 2022, 11:42 a.m. UTC
The OPP core already have few configuration specific APIs and it is
getting complex or messy for both the OPP core and its users.

Lets introduce a new set of API which will be used for all kind of
different configurations, and shall eventually replace all the existing
ones.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c     | 145 +++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h |  42 ++++++++++++
 2 files changed, 187 insertions(+)

Comments

Dmitry Osipenko June 21, 2022, 3:09 p.m. UTC | #1
Hi,

On 5/26/22 14:42, Viresh Kumar wrote:
> +/**
> + * dev_pm_opp_clear_config() - Releases resources blocked for OPP configuration.
> + * @opp_table: OPP table returned from dev_pm_opp_set_config().
> + *
> + * This allows all device OPP configurations to be cleared at once. This must be
> + * called once for each call made to dev_pm_opp_set_config(), in order to free
> + * the OPPs properly.
> + *
> + * Currently the first call itself ends up freeing all the OPP configurations,
> + * while the later ones only drop the OPP table reference. This works well for
> + * now as we would never want to use an half initialized OPP table and want to
> + * remove the configurations together.
> + */
> +void dev_pm_opp_clear_config(struct opp_table *opp_table)
> +{
> +	if (opp_table->genpd_virt_devs)
> +		dev_pm_opp_detach_genpd(opp_table);
> +
> +	if (opp_table->regulators)
> +		dev_pm_opp_put_regulators(opp_table);
> +
> +	if (opp_table->supported_hw)
> +		dev_pm_opp_put_supported_hw(opp_table);
> +
> +	if (opp_table->set_opp)
> +		dev_pm_opp_unregister_set_opp_helper(opp_table);
> +
> +	if (opp_table->prop_name)
> +		dev_pm_opp_put_prop_name(opp_table);
> +
> +	if (opp_table->clk_configured)
> +		dev_pm_opp_put_clkname(opp_table);
> +
> +	dev_pm_opp_put_opp_table(opp_table);
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_opp_clear_config);

1. I started to look at the Tegra regressions caused by these OPP
patches and this one looks wrong to me because dev_pm_opp_set_config()
could be invoked multiple times by different drivers for the same device
and then you're putting table not in accordance to the config that was
used by a particular driver.

For example, if parent tegra-cpufreq driver sets supported_hw for
cpu_dev and then cpufreq-dt also does dev_pm_opp_set_config(cpu_dev),
then dev_pm_opp_clear_config(cpu_dev) of cpufreq-dt will put
supported_hw(cpu_dev) of tegra-cpufreq. Hence this
dev_pm_opp_set/clear_config() approach isn't viable, unless I'm missing
something.

2. Patches aren't bisectable, please make sure that all patches compile
individually and without warnings.

3. There is a new NULL dereference in the recent linux-next on Tegra in
_set_opp() of the gpu/host1x driver. I'll take a closer look at this
crash a bit later.

Unable to handle kernel NULL pointer dereference at virtual address 00000000
[00000000] *pgd=00000000
Internal error: Oops: 80000005 [#1] SMP ARM
Modules linked in:
CPU: 3 PID: 38 Comm: kworker/u8:1 Not tainted
5.19.0-rc3-next-20220621-00013-g0f8bc1c418c4-dirty #18
Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
Workqueue: events_unbound deferred_probe_work_func
PC is at 0x0
LR is at _set_opp+0x15c/0x414
pc : [<00000000>]    lr : [<c0afa928>]    psr: 20000013
sp : df989b60  ip : df989b60  fp : df989ba4
r10: 00000000  r9 : c21e4b40  r8 : c2861e34
r7 : c21b3010  r6 : c28a5080  r5 : c2861e00  r4 : 00000000
r3 : 00000000  r2 : c28a5080  r1 : c2861e00  r0 : c21b3010
Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: 8000404a  DAC: 00000051
Register r0 information: slab kmalloc-1k start c21b3000 pointer offset
16 size 1024
Register r1 information: slab kmalloc-512 start c2861e00 pointer offset
0 size 512
Register r2 information: slab kmalloc-128 start c28a5080 pointer offset
0 size 128
Register r3 information: NULL pointer
Register r4 information: NULL pointer
Register r5 information: slab kmalloc-512 start c2861e00 pointer offset
0 size 512
Register r6 information: slab kmalloc-128 start c28a5080 pointer offset
0 size 128
Register r7 information: slab kmalloc-1k start c21b3000 pointer offset
16 size 1024
Register r8 information: slab kmalloc-512 start c2861e00 pointer offset
52 size 512
Register r9 information: slab task_struct start c21e4b40 pointer offset 0
Register r10 information: NULL pointer
Register r11 information: 2-page vmalloc region starting at 0xdf988000
allocated at kernel_clone+0x64/0x43c
Register r12 information: 2-page vmalloc region starting at 0xdf988000
allocated at kernel_clone+0x64/0x43c
Process kworker/u8:1 (pid: 38, stack limit = 0x(ptrval))
Stack: (0xdf989b60 to 0xdf98a000)
...
Backtrace:
_set_opp from dev_pm_opp_set_opp+0x70/0xd8
r10:00000001 r9:000f4240 r8:c2848440 r7:c2848440 r6:c28a5080 r5:c21b3010
r4:c2861e00
dev_pm_opp_set_opp from tegra_pmc_core_pd_set_performance_state+0x50/0xb8
r6:c2848440 r5:c1807654 r4:c28a5080
tegra_pmc_core_pd_set_performance_state from
_genpd_set_performance_state+0x1fc/0x288
r6:c2848690 r5:c2848680 r4:000f4240
_genpd_set_performance_state from _genpd_set_performance_state+0xb8/0x288
r10:00000001 r9:000f4240 r8:c284a000 r7:c2848440 r6:c284a250 r5:c28a7180
r4:000f4240
_genpd_set_performance_state from genpd_set_performance_state+0xb8/0xd4
r10:c0185b00 r9:c28a7580 r8:00000000 r7:00000000 r6:c284a000 r5:00000000
r4:c28a7580
genpd_set_performance_state from genpd_runtime_resume+0x228/0x29c
r5:c21b3810 r4:c284a1d0
genpd_runtime_resume from __rpm_callback+0x68/0x1a0
r10:c0185b00 r9:00000004 r8:00000000 r7:c08dd55c r6:c2173800 r5:c08dd55c
r4:c21b3810
__rpm_callback from rpm_callback+0x60/0x64
r9:00000004 r8:c21b3894 r7:c08dd55c r6:c2173800 r5:c21e4b40 r4:c21b3810
rpm_callback from rpm_resume+0x608/0x83c
r7:c08dd55c r6:c2173800 r5:c21e4b40 r4:c21b3810
rpm_resume from __pm_runtime_resume+0x58/0xb4
r10:c21e4b40 r9:c21b3810 r8:c21b3800 r7:00000000 r6:c21b3894 r5:60000013
r4:c21b3810
__pm_runtime_resume from host1x_probe+0x48c/0x700
r7:00000000 r6:c2862504 r5:00000000 r4:c2862440
host1x_probe from platform_probe+0x6c/0xcc
r10:c202c00d r9:c21e4b40 r8:00000001 r7:00000000 r6:c1812420 r5:c21b3810
r4:00000000
platform_probe from really_probe+0xd8/0x300
r7:00000000 r6:c1812420 r5:00000000 r4:c21b3810
really_probe from __driver_probe_device+0x94/0xf4
r7:0000000b r6:c21b3810 r5:c1812420 r4:c21b3810
__driver_probe_device from driver_probe_device+0x40/0x114
r5:df989e84 r4:c1901580
driver_probe_device from __device_attach_driver+0xc4/0x108
r9:c21e4b40 r8:00000001 r7:c08c0fb4 r6:c21b3810 r5:df989e84 r4:c1812420
__device_attach_driver from bus_for_each_drv+0x8c/0xd0
r7:c08c0fb4 r6:c21e4b40 r5:df989e84 r4:00000000
bus_for_each_drv from __device_attach+0xb8/0x1e8
r7:c21b3854 r6:c21e4b40 r5:c21b3810 r4:c21b3810
__device_attach from device_initial_probe+0x1c/0x20
r8:c1882620 r7:00000000 r6:c1814e90 r5:c21b3810 r4:c21b3810
device_initial_probe from bus_probe_device+0x94/0x9c
bus_probe_device from deferred_probe_work_func+0x88/0xb4
r7:00000000 r6:c1814c00 r5:c1814bec r4:c21b3810
deferred_probe_work_func from process_one_work+0x21c/0x548
r7:c202c000 r6:c2006a00 r5:c23e8380 r4:c1814c14
process_one_work from worker_thread+0x27c/0x5ac
r10:00000088 r9:c2006a00 r8:c1703d40 r7:c2006a1c r6:c23e8398 r5:c2006a00
r4:c23e8380
worker_thread from kthread+0x100/0x120
r10:00000000 r9:df831e7c r8:c23ed0c0 r7:c23e8380 r6:c014bcfc r5:c23ed000
r4:c21e4b40
kthread from ret_from_fork+0x14/0x2c
Exception stack(0xdf989fb0 to 0xdf989ff8)
9fa0:                                     00000000 00000000 00000000
00000000
9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000
9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c01539f4 r4:c23ed000
Code: bad PC value
---[ end trace 0000000000000000 ]---
Viresh Kumar June 21, 2022, 3:34 p.m. UTC | #2
Hi Dmitry,

On 21-06-22, 18:09, Dmitry Osipenko wrote:
> 1. I started to look at the Tegra regressions caused by these OPP
> patches and this one looks wrong to me because dev_pm_opp_set_config()
> could be invoked multiple times by different drivers for the same device
> and then you're putting table not in accordance to the config that was
> used by a particular driver.
> 
> For example, if parent tegra-cpufreq driver sets supported_hw for
> cpu_dev and then cpufreq-dt also does dev_pm_opp_set_config(cpu_dev),
> then dev_pm_opp_clear_config(cpu_dev) of cpufreq-dt will put
> supported_hw(cpu_dev) of tegra-cpufreq. Hence this
> dev_pm_opp_set/clear_config() approach isn't viable, unless I'm missing
> something.

Yeah, I know that and I didn't put a lot of effort into it because of multiple
reasons:

- That is partially the existing behavior. For example, we can call the
  set-supported-hw interface right now for each CPU of a policy, which many
  drivers do right now btw, and then while putting them back we drop the
  resource on the first call itself and not on the last CPU.

- Yes, with the new patchset we will drop the resources even for an unrelated
  resource call, I will think again about it though and maybe add a flag field
  to notify which all resources to clean, but even in the current case it should
  be fine as we won't be able to use a half initialized OPP table anyway (which
  may actually be harmful). What I mean is, if you set regulators and
  supported-hw in the beginning, then on un-init, we won't want to work with
  only one of them in place. We always want all of them.

> 2. Patches aren't bisectable, please make sure that all patches compile
> individually and without warnings.

That is strange. I will try build over each and every patch (again). Also I
think the kernel bots (from LKP) test individual patches and I haven't got any
failure messages yet. Which patch broke the build for you ?

> 3. There is a new NULL dereference in the recent linux-next on Tegra in
> _set_opp() of the gpu/host1x driver. I'll take a closer look at this
> crash a bit later.

I just fixed a bug for devices which don't have the clock property, but just
level or bandwidth. Not sure if that is the one that caused trouble for you. It
is pushed to opp/linux-next.
Viresh Kumar June 22, 2022, 6:13 a.m. UTC | #3
On 21-06-22, 21:04, Viresh Kumar wrote:
> > 2. Patches aren't bisectable, please make sure that all patches compile
> > individually and without warnings.
> 
> That is strange. I will try build over each and every patch (again). Also I
> think the kernel bots (from LKP) test individual patches and I haven't got any
> failure messages yet. Which patch broke the build for you ?

Hmm, surprisingly (as I do test this for my patches) one patch was emitting
non-harmful warnings and one broke the build. Fixed them both, the final diff
remains the same though for opp/linux-next.

Thanks for reporting this.
diff mbox series

Patch

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 254782b3a6a0..30dbef0f4d17 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2618,6 +2618,151 @@  int devm_pm_opp_attach_genpd(struct device *dev, const char * const *names,
 }
 EXPORT_SYMBOL_GPL(devm_pm_opp_attach_genpd);
 
+/**
+ * dev_pm_opp_set_config() - Set OPP configuration for the device.
+ * @dev: Device for which configuration is being set.
+ * @config: OPP configuration.
+ *
+ * This allows all device OPP configurations to be performed at once.
+ *
+ * This must be called before any OPPs are initialized for the device. This may
+ * be called multiple times for the same OPP table, for example once for each
+ * CPU that share the same table. This must be balanced by the same number of
+ * calls to dev_pm_opp_clear_config() in order to free the OPP table properly.
+ */
+struct opp_table *dev_pm_opp_set_config(struct device *dev,
+					struct dev_pm_opp_config *config)
+{
+	struct opp_table *opp_table, *ret;
+
+	opp_table = _add_opp_table(dev, false);
+	if (IS_ERR(opp_table))
+		return opp_table;
+
+	/* This should be called before OPPs are initialized */
+	if (WARN_ON(!list_empty(&opp_table->opp_list))) {
+		ret = ERR_PTR(-EBUSY);
+		goto err;
+	}
+
+	// Configure clock
+	if (config->clk_name) {
+		ret = dev_pm_opp_set_clkname(dev, config->clk_name);
+		if (IS_ERR(ret))
+			goto err;
+	}
+
+	// Configure property names
+	if (config->prop_name) {
+		ret = dev_pm_opp_set_prop_name(dev, config->prop_name);
+		if (IS_ERR(ret))
+			goto err;
+	}
+
+	// Configure opp helper
+	if (config->set_opp) {
+		ret = dev_pm_opp_register_set_opp_helper(dev, config->set_opp);
+		if (IS_ERR(ret))
+			goto err;
+	}
+
+	// Configure supported hardware
+	if (config->supported_hw) {
+		ret = dev_pm_opp_set_supported_hw(dev, config->supported_hw,
+						  config->supported_hw_count);
+		if (IS_ERR(ret))
+			goto err;
+	}
+
+	// Configure supplies
+	if (config->regulator_names) {
+		ret = dev_pm_opp_set_regulators(dev, config->regulator_names,
+						config->regulator_count);
+		if (IS_ERR(ret))
+			goto err;
+	}
+
+	// Attach genpds
+	if (config->genpd_names) {
+		ret = dev_pm_opp_attach_genpd(dev, config->genpd_names,
+					      config->virt_devs);
+		if (IS_ERR(ret))
+			goto err;
+	}
+
+	return opp_table;
+
+err:
+	dev_pm_opp_clear_config(opp_table);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_config);
+
+/**
+ * dev_pm_opp_clear_config() - Releases resources blocked for OPP configuration.
+ * @opp_table: OPP table returned from dev_pm_opp_set_config().
+ *
+ * This allows all device OPP configurations to be cleared at once. This must be
+ * called once for each call made to dev_pm_opp_set_config(), in order to free
+ * the OPPs properly.
+ *
+ * Currently the first call itself ends up freeing all the OPP configurations,
+ * while the later ones only drop the OPP table reference. This works well for
+ * now as we would never want to use an half initialized OPP table and want to
+ * remove the configurations together.
+ */
+void dev_pm_opp_clear_config(struct opp_table *opp_table)
+{
+	if (opp_table->genpd_virt_devs)
+		dev_pm_opp_detach_genpd(opp_table);
+
+	if (opp_table->regulators)
+		dev_pm_opp_put_regulators(opp_table);
+
+	if (opp_table->supported_hw)
+		dev_pm_opp_put_supported_hw(opp_table);
+
+	if (opp_table->set_opp)
+		dev_pm_opp_unregister_set_opp_helper(opp_table);
+
+	if (opp_table->prop_name)
+		dev_pm_opp_put_prop_name(opp_table);
+
+	if (opp_table->clk_configured)
+		dev_pm_opp_put_clkname(opp_table);
+
+	dev_pm_opp_put_opp_table(opp_table);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_clear_config);
+
+static void devm_pm_opp_config_release(void *data)
+{
+	dev_pm_opp_clear_config(data);
+}
+
+/**
+ * devm_pm_opp_set_config() - Set OPP configuration for the device.
+ * @dev: Device for which configuration is being set.
+ * @config: OPP configuration.
+ *
+ * This allows all device OPP configurations to be performed at once.
+ * This is a resource-managed variant of dev_pm_opp_set_config().
+ *
+ * Return: 0 on success and errorno otherwise.
+ */
+int devm_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config)
+{
+	struct opp_table *opp_table;
+
+	opp_table = dev_pm_opp_set_config(dev, config);
+	if (IS_ERR(opp_table))
+		return PTR_ERR(opp_table);
+
+	return devm_add_action_or_reset(dev, devm_pm_opp_config_release,
+					opp_table);
+}
+EXPORT_SYMBOL_GPL(devm_pm_opp_set_config);
+
 /**
  * dev_pm_opp_xlate_required_opp() - Find required OPP for @src_table OPP.
  * @src_table: OPP table which has @dst_table as one of its required OPP table.
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 6708b4ec244d..0d5d07dd164a 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -90,6 +90,32 @@  struct dev_pm_set_opp_data {
 	struct device *dev;
 };
 
+/**
+ * struct dev_pm_opp_config - Device OPP configuration values
+ * @clk_name: Clk name.
+ * @prop_name: Name to postfix to properties.
+ * @set_opp: Custom set OPP helper.
+ * @supported_hw: Array of hierarchy of versions to match.
+ * @supported_hw_count: Number of elements in the array.
+ * @regulator_names: Array of pointers to the names of the regulator.
+ * @regulator_count: Number of regulators.
+ * @genpd_names: Null terminated array of pointers containing names of genpd to attach.
+ * @virt_devs: Pointer to return the array of virtual devices.
+ *
+ * This structure contains platform specific OPP configurations for the device.
+ */
+struct dev_pm_opp_config {
+	const char *clk_name;
+	const char *prop_name;
+	int (*set_opp)(struct dev_pm_set_opp_data *data);
+	unsigned int *supported_hw;
+	unsigned int supported_hw_count;
+	const char * const *regulator_names;
+	unsigned int regulator_count;
+	const char * const *genpd_names;
+	struct device ***virt_devs;
+};
+
 #if defined(CONFIG_PM_OPP)
 
 struct opp_table *dev_pm_opp_get_opp_table(struct device *dev);
@@ -154,6 +180,10 @@  int dev_pm_opp_disable(struct device *dev, unsigned long freq);
 int dev_pm_opp_register_notifier(struct device *dev, struct notifier_block *nb);
 int dev_pm_opp_unregister_notifier(struct device *dev, struct notifier_block *nb);
 
+struct opp_table *dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config);
+int devm_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config);
+void dev_pm_opp_clear_config(struct opp_table *opp_table);
+
 struct opp_table *dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions, unsigned int count);
 void dev_pm_opp_put_supported_hw(struct opp_table *opp_table);
 int devm_pm_opp_set_supported_hw(struct device *dev, const u32 *versions, unsigned int count);
@@ -419,6 +449,18 @@  static inline int devm_pm_opp_attach_genpd(struct device *dev,
 	return -EOPNOTSUPP;
 }
 
+static inline struct opp_table *dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline int devm_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline void dev_pm_opp_clear_config(struct opp_table *opp_table) {}
+
 static inline struct dev_pm_opp *dev_pm_opp_xlate_required_opp(struct opp_table *src_table,
 				struct opp_table *dst_table, struct dev_pm_opp *src_opp)
 {