| Message ID | 9c4b2bfe628bf7a583a96cee7cc3539e2e66245e.1653564321.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | OPP: Add new configuration interface: dev_pm_opp_set_config() | expand |
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 ]---
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.
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 --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) {
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(+)