LinuxLists.cc - [PATCH 1/7] thermal/drivers/core: Remove the module Kconfig's option

[permalink] [raw]

Subject: [PATCH 4/7] thermal/drivers/core: Use governor table to initialize

Now that the governor table is in place and the macro allows to browse the
table, declare the governor so the entry is added in the governor table
in the init section.

The [un]register_thermal_governors function does no longer need to use the
exported [un]register thermal governor's specific function which in turn
call the [un]register_thermal_governor. The governors are fully
self-encapsulated.

The cyclic dependency is no longer needed, remove it.

Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/thermal/fair_share.c | 12 +-------
drivers/thermal/gov_bang_bang.c | 11 +------
drivers/thermal/power_allocator.c | 11 +------
drivers/thermal/step_wise.c | 11 +------
drivers/thermal/thermal_core.c | 51 +++++++++++++++++--------------
drivers/thermal/thermal_core.h | 40 ------------------------
drivers/thermal/user_space.c | 12 +-------
7 files changed, 33 insertions(+), 115 deletions(-)

diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
index d3469fbc5207..bda2afc63471 100644
--- a/drivers/thermal/fair_share.c
+++ b/drivers/thermal/fair_share.c
@@ -129,14 +129,4 @@ static struct thermal_governor thermal_gov_fair_share = {
.name = "fair_share",
.throttle = fair_share_throttle,
};
-
-int thermal_gov_fair_share_register(void)
-{
- return thermal_register_governor(&thermal_gov_fair_share);
-}
-
-void thermal_gov_fair_share_unregister(void)
-{
- thermal_unregister_governor(&thermal_gov_fair_share);
-}
-
+THERMAL_GOVERNOR_DECLARE(thermal_gov_fair_share);
diff --git a/drivers/thermal/gov_bang_bang.c b/drivers/thermal/gov_bang_bang.c
index fc5e5057f0de..c5e19c7d63da 100644
--- a/drivers/thermal/gov_bang_bang.c
+++ b/drivers/thermal/gov_bang_bang.c
@@ -126,13 +126,4 @@ static struct thermal_governor thermal_gov_bang_bang = {
.name = "bang_bang",
.throttle = bang_bang_control,
};
-
-int thermal_gov_bang_bang_register(void)
-{
- return thermal_register_governor(&thermal_gov_bang_bang);
-}
-
-void thermal_gov_bang_bang_unregister(void)
-{
- thermal_unregister_governor(&thermal_gov_bang_bang);
-}
+THERMAL_GOVERNOR_DECLARE(thermal_gov_bang_bang);
diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
index 3055f9a12a17..44636475b2a3 100644
--- a/drivers/thermal/power_allocator.c
+++ b/drivers/thermal/power_allocator.c
@@ -651,13 +651,4 @@ static struct thermal_governor thermal_gov_power_allocator = {
.unbind_from_tz = power_allocator_unbind,
.throttle = power_allocator_throttle,
};
-
-int thermal_gov_power_allocator_register(void)
-{
- return thermal_register_governor(&thermal_gov_power_allocator);
-}
-
-void thermal_gov_power_allocator_unregister(void)
-{
- thermal_unregister_governor(&thermal_gov_power_allocator);
-}
+THERMAL_GOVERNOR_DECLARE(thermal_gov_power_allocator);
diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
index ee047ca43084..6cd251ab56fc 100644
--- a/drivers/thermal/step_wise.c
+++ b/drivers/thermal/step_wise.c
@@ -218,13 +218,4 @@ static struct thermal_governor thermal_gov_step_wise = {
.name = "step_wise",
.throttle = step_wise_throttle,
};
-
-int thermal_gov_step_wise_register(void)
-{
- return thermal_register_governor(&thermal_gov_step_wise);
-}
-
-void thermal_gov_step_wise_unregister(void)
-{
- thermal_unregister_governor(&thermal_gov_step_wise);
-}
+THERMAL_GOVERNOR_DECLARE(thermal_gov_step_wise);
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 28f7ece0e8fe..50c88682a848 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -243,36 +243,41 @@ int thermal_build_list_of_policies(char *buf)
return count;
}

-static int __init thermal_register_governors(void)
+static void __init thermal_unregister_governors(void)
{
- int result;
+ struct thermal_governor **governor;

- result = thermal_gov_step_wise_register();
- if (result)
- return result;
+ for_each_governor_table(governor)
+ thermal_unregister_governor(*governor);
+}

- result = thermal_gov_fair_share_register();
- if (result)
- return result;
+static int __init thermal_register_governors(void)
+{
+ int ret = 0;
+ struct thermal_governor **governor;

- result = thermal_gov_bang_bang_register();
- if (result)
- return result;
+ for_each_governor_table(governor) {
+ ret = thermal_register_governor(*governor);
+ if (ret) {
+ pr_err("Failed to register governor: '%s'",
+ (*governor)->name);
+ break;
+ }

- result = thermal_gov_user_space_register();
- if (result)
- return result;
+ pr_info("Registered thermal governor '%s'",
+ (*governor)->name);
+ }

- return thermal_gov_power_allocator_register();
-}
+ if (ret) {
+ struct thermal_governor **gov;
+ for_each_governor_table(gov) {
+ if (gov == governor)
+ break;
+ thermal_unregister_governor(*gov);
+ }
+ }

-static void __init thermal_unregister_governors(void)
-{
- thermal_gov_step_wise_unregister();
- thermal_gov_fair_share_unregister();
- thermal_gov_bang_bang_unregister();
- thermal_gov_user_space_unregister();
- thermal_gov_power_allocator_unregister();
+ return ret;
}

/*
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 28d18083e969..a3c555b48eb3 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -90,46 +90,6 @@ thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
unsigned long new_state) {}
#endif /* CONFIG_THERMAL_STATISTICS */

-#ifdef CONFIG_THERMAL_GOV_STEP_WISE
-int thermal_gov_step_wise_register(void);
-void thermal_gov_step_wise_unregister(void);
-#else
-static inline int thermal_gov_step_wise_register(void) { return 0; }
-static inline void thermal_gov_step_wise_unregister(void) {}
-#endif /* CONFIG_THERMAL_GOV_STEP_WISE */
-
-#ifdef CONFIG_THERMAL_GOV_FAIR_SHARE
-int thermal_gov_fair_share_register(void);
-void thermal_gov_fair_share_unregister(void);
-#else
-static inline int thermal_gov_fair_share_register(void) { return 0; }
-static inline void thermal_gov_fair_share_unregister(void) {}
-#endif /* CONFIG_THERMAL_GOV_FAIR_SHARE */
-
-#ifdef CONFIG_THERMAL_GOV_BANG_BANG
-int thermal_gov_bang_bang_register(void);
-void thermal_gov_bang_bang_unregister(void);
-#else
-static inline int thermal_gov_bang_bang_register(void) { return 0; }
-static inline void thermal_gov_bang_bang_unregister(void) {}
-#endif /* CONFIG_THERMAL_GOV_BANG_BANG */
-
-#ifdef CONFIG_THERMAL_GOV_USER_SPACE
-int thermal_gov_user_space_register(void);
-void thermal_gov_user_space_unregister(void);
-#else
-static inline int thermal_gov_user_space_register(void) { return 0; }
-static inline void thermal_gov_user_space_unregister(void) {}
-#endif /* CONFIG_THERMAL_GOV_USER_SPACE */
-
-#ifdef CONFIG_THERMAL_GOV_POWER_ALLOCATOR
-int thermal_gov_power_allocator_register(void);
-void thermal_gov_power_allocator_unregister(void);
-#else
-static inline int thermal_gov_power_allocator_register(void) { return 0; }
-static inline void thermal_gov_power_allocator_unregister(void) {}
-#endif /* CONFIG_THERMAL_GOV_POWER_ALLOCATOR */
-
/* device tree support */
#ifdef CONFIG_THERMAL_OF
int of_parse_thermal_zones(void);
diff --git a/drivers/thermal/user_space.c b/drivers/thermal/user_space.c
index 8e92a06ef48a..5fac99e5221d 100644
--- a/drivers/thermal/user_space.c
+++ b/drivers/thermal/user_space.c
@@ -56,14 +56,4 @@ static struct thermal_governor thermal_gov_user_space = {
.name = "user_space",
.throttle = notify_user_space,
};
-
-int thermal_gov_user_space_register(void)
-{
- return thermal_register_governor(&thermal_gov_user_space);
-}
-
-void thermal_gov_user_space_unregister(void)
-{
- thermal_unregister_governor(&thermal_gov_user_space);
-}
-
+THERMAL_GOVERNOR_DECLARE(thermal_gov_user_space);
--
2.17.1

2019-04-02 16:15:11

[permalink] [raw]

Subject: [PATCH 3/7] thermal/drivers/core: Add init section table for self-encapsulation

Currently the governors are declared in their respective files but they
export their [un]register functions which in turn call the [un]register
the governors core's functions. That implies a cyclic dependency which is
not desirable. There is a way to self-encapsulate the governors by letting
them to declare themselves in a __init section table.

Define the table in the asm generic linker description like the other
tables and provide the specific macros to deal with.

Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/thermal/thermal_core.h | 16 ++++++++++++++++
include/asm-generic/vmlinux.lds.h | 11 +++++++++++
2 files changed, 27 insertions(+)

diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 0df190ed82a7..28d18083e969 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -15,6 +15,22 @@
/* Initial state of a cooling device during binding */
#define THERMAL_NO_TARGET -1UL

+/* Init section thermal table */
+extern struct thermal_governor * __governor_thermal_table[];
+extern struct thermal_governor * __governor_thermal_table_end[];
+
+#define THERMAL_TABLE_ENTRY(table, name) \
+ static typeof(name) * __thermal_table_entry_##name \
+ __used __section(__##table##_thermal_table) \
+ = &name;
+
+#define THERMAL_GOVERNOR_DECLARE(name) THERMAL_TABLE_ENTRY(governor, name)
+
+#define for_each_governor_table(__governor) \
+ for (__governor = __governor_thermal_table; \
+ __governor < __governor_thermal_table_end; \
+ __governor++)
+
/*
* This structure is used to describe the behavior of
* a certain cooling device on a certain trip point
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index f8f6f04c4453..9893a3ed242a 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -239,6 +239,16 @@
#define ACPI_PROBE_TABLE(name)
#endif

+#ifdef CONFIG_THERMAL
+#define THERMAL_TABLE(name) \
+ . = ALIGN(8); \
+ __##name##_thermal_table = .; \
+ KEEP(*(__##name##_thermal_table)) \
+ __##name##_thermal_table_end = .;
+#else
+#define THERMAL_TABLE(name)
+#endif
+
#define KERNEL_DTB() \
STRUCT_ALIGN(); \
__dtb_start = .; \
@@ -609,6 +619,7 @@
IRQCHIP_OF_MATCH_TABLE() \
ACPI_PROBE_TABLE(irqchip) \
ACPI_PROBE_TABLE(timer) \
+ THERMAL_TABLE(governor) \
EARLYCON_TABLE() \
LSM_TABLE()

--
2.17.1

2019-04-02 16:15:20

[permalink] [raw]

Subject: [PATCH 5/7] thermal/drivers/core: Remove depends on THERMAL in Kconfig

The dependency on the THERMAL option to be set is already there implicitly
by the "if THERMAL" conditionnal option. The sub Kconfigs do not have to
check against the THERMAL option as they are called from a Kconfig block
which is enabled by the conditionnal option.

Remove the useless "depends on THERMAL" in the Kconfigs.

Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/thermal/Kconfig | 2 --
drivers/thermal/intel/Kconfig | 1 -
drivers/thermal/qcom/Kconfig | 1 -
3 files changed, 4 deletions(-)

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index ccf5b9408d7a..d8aa7fac8c56 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -24,7 +24,6 @@ config THERMAL_STATISTICS

config THERMAL_EMERGENCY_POWEROFF_DELAY_MS
int "Emergency poweroff delay in milli-seconds"
- depends on THERMAL
default 0
help
Thermal subsystem will issue a graceful shutdown when
@@ -152,7 +151,6 @@ config CPU_THERMAL
bool "generic cpu cooling support"
depends on CPU_FREQ
depends on THERMAL_OF
- depends on THERMAL=y
help
This implements the generic cpu cooling mechanism through frequency
reduction. An ACPI version of this already exists
diff --git a/drivers/thermal/intel/Kconfig b/drivers/thermal/intel/Kconfig
index 2e013eeb4a1d..2c727a820759 100644
--- a/drivers/thermal/intel/Kconfig
+++ b/drivers/thermal/intel/Kconfig
@@ -1,6 +1,5 @@
config INTEL_POWERCLAMP
tristate "Intel PowerClamp idle injection driver"
- depends on THERMAL
depends on X86
depends on CPU_SUP_INTEL
help
diff --git a/drivers/thermal/qcom/Kconfig b/drivers/thermal/qcom/Kconfig
index cdb455ffd575..3ce20fec86a2 100644
--- a/drivers/thermal/qcom/Kconfig
+++ b/drivers/thermal/qcom/Kconfig
@@ -1,6 +1,5 @@
config QCOM_TSENS
tristate "Qualcomm TSENS Temperature Alarm"
- depends on THERMAL
depends on QCOM_QFPROM
depends on ARCH_QCOM || COMPILE_TEST
help
--
2.17.1

2019-04-02 16:15:46

[permalink] [raw]

Subject: [PATCH 2/7] thermal/drivers/core: Remove module unload code

Now the thermal core is no longer compiled as a module. Remove the unloading
module code and move the unregister function to the __init section.

Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/thermal/thermal_core.c | 17 +----------------
1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 6590bb5cb688..28f7ece0e8fe 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -266,7 +266,7 @@ static int __init thermal_register_governors(void)
return thermal_gov_power_allocator_register();
}

-static void thermal_unregister_governors(void)
+static void __init thermal_unregister_governors(void)
{
thermal_gov_step_wise_unregister();
thermal_gov_fair_share_unregister();
@@ -1563,19 +1563,4 @@ static int __init thermal_init(void)
mutex_destroy(&poweroff_lock);
return result;
}
-
-static void __exit thermal_exit(void)
-{
- unregister_pm_notifier(&thermal_pm_nb);
- of_thermal_destroy_zones();
- genetlink_exit();
- class_unregister(&thermal_class);
- thermal_unregister_governors();
- ida_destroy(&thermal_tz_ida);
- ida_destroy(&thermal_cdev_ida);
- mutex_destroy(&thermal_list_lock);
- mutex_destroy(&thermal_governor_lock);
-}
-
fs_initcall(thermal_init);
-module_exit(thermal_exit);
--
2.17.1

2019-04-02 16:16:09

[permalink] [raw]

Subject: [PATCH 7/7] hwmon/drivers/core: Simplify complex dependency

As the thermal framework does not longer compile as a module, we can
simplify this condition below:

if IS_REACHABLE(CONFIG_THERMAL) && defined(CONFIG_THERMAL_OF) && \
(!defined(CONFIG_THERMAL_HWMON) || \
!(defined(MODULE) && IS_MODULE(CONFIG_THERMAL)))

if IS_REACHABLE(CONFIG_THERMAL) && defined(CONFIG_THERMAL_OF) && \
(!defined(CONFIG_THERMAL_HWMON) || \
!(defined(MODULE) && 0))

=> (whatever && 0) = 0

if IS_REACHABLE(CONFIG_THERMAL) && defined(CONFIG_THERMAL_OF) && \
(!defined(CONFIG_THERMAL_HWMON) || !(0))

if IS_REACHABLE(CONFIG_THERMAL) && defined(CONFIG_THERMAL_OF) && \
(!defined(CONFIG_THERMAL_HWMON) || 1)

=> (whatever || 1) = 1

if IS_REACHABLE(CONFIG_THERMAL) && defined(CONFIG_THERMAL_OF) && \
(1)

=> (whatever && 1) = whatever

if IS_REACHABLE(CONFIG_THERMAL) && defined(CONFIG_THERMAL_OF)

CONFIG_THERMAL can not be a module anymore, then:

if defined(CONFIG_THERMAL) && defined(CONFIG_THERMAL_OF)

And CONFIG_THERMAL_OF already depends on CONFIG_THERMAL, so:

if defined(CONFIG_THERMAL_OF)

Thus,

ifdef CONFIG_THERMAL_OF

Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/hwmon/hwmon.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index c22dc1e07911..6f76f2484db0 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -118,9 +118,7 @@ static DEFINE_IDA(hwmon_ida);
* The complex conditional is necessary to avoid a cyclic dependency
* between hwmon and thermal_sys modules.
*/
-#if IS_REACHABLE(CONFIG_THERMAL) && defined(CONFIG_THERMAL_OF) && \
- (!defined(CONFIG_THERMAL_HWMON) || \
- !(defined(MODULE) && IS_MODULE(CONFIG_THERMAL)))
+#ifdef CONFIG_THERMAL_OF
static int hwmon_thermal_get_temp(void *data, int *temp)
{
struct hwmon_thermal_data *tdata = data;
--
2.17.1

2019-04-02 16:16:57

[permalink] [raw]

Subject: [PATCH 6/7] thermal/drivers/core: Fix typo in the option name

Unlike the other options, the cpu cooling option is beginning
with a lowercase letter, changing to a uppercase.

Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/thermal/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index d8aa7fac8c56..83e95a4fb61b 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -148,7 +148,7 @@ config THERMAL_GOV_POWER_ALLOCATOR
allocating and limiting power to devices.

config CPU_THERMAL
- bool "generic cpu cooling support"
+ bool "Generic cpu cooling support"
depends on CPU_FREQ
depends on THERMAL_OF
help
--
2.17.1

2019-04-02 18:32:49

by Guenter Roeck

[permalink] [raw]

Subject: Re: [PATCH 7/7] hwmon/drivers/core: Simplify complex dependency

On Tue, Apr 02, 2019 at 06:12:50PM +0200, Daniel Lezcano wrote:
> As the thermal framework does not longer compile as a module, we can
> simplify this condition below:
>
> if IS_REACHABLE(CONFIG_THERMAL) && defined(CONFIG_THERMAL_OF) && \
> (!defined(CONFIG_THERMAL_HWMON) || \
> !(defined(MODULE) && IS_MODULE(CONFIG_THERMAL)))
>
> if IS_REACHABLE(CONFIG_THERMAL) && defined(CONFIG_THERMAL_OF) && \
> (!defined(CONFIG_THERMAL_HWMON) || \
> !(defined(MODULE) && 0))
>
> => (whatever && 0) = 0
>
> if IS_REACHABLE(CONFIG_THERMAL) && defined(CONFIG_THERMAL_OF) && \
> (!defined(CONFIG_THERMAL_HWMON) || !(0))
>
> if IS_REACHABLE(CONFIG_THERMAL) && defined(CONFIG_THERMAL_OF) && \
> (!defined(CONFIG_THERMAL_HWMON) || 1)
>
> => (whatever || 1) = 1
>
> if IS_REACHABLE(CONFIG_THERMAL) && defined(CONFIG_THERMAL_OF) && \
> (1)
>
> => (whatever && 1) = whatever
>
> if IS_REACHABLE(CONFIG_THERMAL) && defined(CONFIG_THERMAL_OF)
>
> CONFIG_THERMAL can not be a module anymore, then:
>
> if defined(CONFIG_THERMAL) && defined(CONFIG_THERMAL_OF)
>
> And CONFIG_THERMAL_OF already depends on CONFIG_THERMAL, so:
>
> if defined(CONFIG_THERMAL_OF)
>
> Thus,
>
> ifdef CONFIG_THERMAL_OF
>
> Signed-off-by: Daniel Lezcano <[email protected]>

Acked-by: Guenter Roeck <[email protected]>

... assuming this will be pushed with the series. Otherwise please
let me know.

Thanks,
Guenter

> ---
> drivers/hwmon/hwmon.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index c22dc1e07911..6f76f2484db0 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -118,9 +118,7 @@ static DEFINE_IDA(hwmon_ida);
> * The complex conditional is necessary to avoid a cyclic dependency
> * between hwmon and thermal_sys modules.
> */
> -#if IS_REACHABLE(CONFIG_THERMAL) && defined(CONFIG_THERMAL_OF) && \
> - (!defined(CONFIG_THERMAL_HWMON) || \
> - !(defined(MODULE) && IS_MODULE(CONFIG_THERMAL)))
> +#ifdef CONFIG_THERMAL_OF
> static int hwmon_thermal_get_temp(void *data, int *temp)
> {
> struct hwmon_thermal_data *tdata = data;
> --
> 2.17.1
>

2019-04-02 19:38:42

[permalink] [raw]

Subject: Re: [PATCH 5/7] thermal/drivers/core: Remove depends on THERMAL in Kconfig

On Tue, Apr 2, 2019 at 11:14 PM Daniel Lezcano
<[email protected]> wrote:
>
> The dependency on the THERMAL option to be set is already there implicitly
> by the "if THERMAL" conditionnal option. The sub Kconfigs do not have to
> check against the THERMAL option as they are called from a Kconfig block
> which is enabled by the conditionnal option.
>
> Remove the useless "depends on THERMAL" in the Kconfigs.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
> ---
> drivers/thermal/Kconfig | 2 --
> drivers/thermal/intel/Kconfig | 1 -
> drivers/thermal/qcom/Kconfig | 1 -
> 3 files changed, 4 deletions(-)
>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index ccf5b9408d7a..d8aa7fac8c56 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -24,7 +24,6 @@ config THERMAL_STATISTICS
>
> config THERMAL_EMERGENCY_POWEROFF_DELAY_MS
> int "Emergency poweroff delay in milli-seconds"
> - depends on THERMAL
> default 0
> help
> Thermal subsystem will issue a graceful shutdown when
> @@ -152,7 +151,6 @@ config CPU_THERMAL
> bool "generic cpu cooling support"
> depends on CPU_FREQ
> depends on THERMAL_OF
> - depends on THERMAL=y

This will break the intention of afa1f2ab43d48d0e1. At the moment, we
need THERMAL=y if we want cpu_cooling.

> help
> This implements the generic cpu cooling mechanism through frequency
> reduction. An ACPI version of this already exists
> diff --git a/drivers/thermal/intel/Kconfig b/drivers/thermal/intel/Kconfig
> index 2e013eeb4a1d..2c727a820759 100644
> --- a/drivers/thermal/intel/Kconfig
> +++ b/drivers/thermal/intel/Kconfig
> @@ -1,6 +1,5 @@
> config INTEL_POWERCLAMP
> tristate "Intel PowerClamp idle injection driver"
> - depends on THERMAL
> depends on X86
> depends on CPU_SUP_INTEL
> help
> diff --git a/drivers/thermal/qcom/Kconfig b/drivers/thermal/qcom/Kconfig
> index cdb455ffd575..3ce20fec86a2 100644
> --- a/drivers/thermal/qcom/Kconfig
> +++ b/drivers/thermal/qcom/Kconfig
> @@ -1,6 +1,5 @@
> config QCOM_TSENS
> tristate "Qualcomm TSENS Temperature Alarm"
> - depends on THERMAL
> depends on QCOM_QFPROM
> depends on ARCH_QCOM || COMPILE_TEST
> help
> --
> 2.17.1
>

2019-04-03 03:34:04

[permalink] [raw]

Subject: Re: [PATCH 5/7] thermal/drivers/core: Remove depends on THERMAL in Kconfig

Hi Amit,

On 02/04/2019 20:51, Amit Kucheria wrote:
> On Tue, Apr 2, 2019 at 11:14 PM Daniel Lezcano
> <[email protected]> wrote:
>>
>> The dependency on the THERMAL option to be set is already there implicitly
>> by the "if THERMAL" conditionnal option. The sub Kconfigs do not have to
>> check against the THERMAL option as they are called from a Kconfig block
>> which is enabled by the conditionnal option.
>>
>> Remove the useless "depends on THERMAL" in the Kconfigs.
>>
>> Signed-off-by: Daniel Lezcano <[email protected]>
>> ---
>> drivers/thermal/Kconfig | 2 --
>> drivers/thermal/intel/Kconfig | 1 -
>> drivers/thermal/qcom/Kconfig | 1 -
>> 3 files changed, 4 deletions(-)
>>
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index ccf5b9408d7a..d8aa7fac8c56 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -24,7 +24,6 @@ config THERMAL_STATISTICS
>>
>> config THERMAL_EMERGENCY_POWEROFF_DELAY_MS
>> int "Emergency poweroff delay in milli-seconds"
>> - depends on THERMAL
>> default 0
>> help
>> Thermal subsystem will issue a graceful shutdown when
>> @@ -152,7 +151,6 @@ config CPU_THERMAL
>> bool "generic cpu cooling support"
>> depends on CPU_FREQ
>> depends on THERMAL_OF
>> - depends on THERMAL=y
>
> This will break the intention of afa1f2ab43d48d0e1. At the moment, we
> need THERMAL=y if we want cpu_cooling.

The patch 1/7 changes the thermal option from tristate to bool, so the
thermal is compiled-in always. The dependency is no longer needed in
this case. Did I miss something?

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

2019-04-03 22:56:35

by Paul Burton

[permalink] [raw]

Subject: Re: [PATCH 1/7] thermal/drivers/core: Remove the module Kconfig's option

Hi Daniel,

On Tue, Apr 02, 2019 at 06:12:44PM +0200, Daniel Lezcano wrote:
> The module support for the thermal subsystem makes little sense:
> - some subsystems relying on it are not modules, thus forcing the
> framework to be compiled in
> - it is compiled in for almost every configs, the remaining ones
> are a few platforms where I don't see why we can not switch the thermal
> to 'y'. The drivers can stay in tristate.
> - platforms need the thermal to be ready as soon as possible at boot time
> in order to mitigate

Nit: mitigate what? High temperatures? It feels like this sentence was
cut short.

> Usually the subsystems framework are compiled-in and the plugs are as module.
>
> Remove the module option. The removal of the module related dead code will
> come after this patch gets in or is acked.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
> Acked-by: Guenter Roeck <[email protected]>
> For mini2440:
> Acked-by: Krzysztof Kozlowski <[email protected]>

Acked-by: Paul Burton <[email protected]> # MIPS part

Thanks,
Paul

2019-04-04 21:24:27

by Robert Jarzmik

[permalink] [raw]

Subject: Re: [PATCH 1/7] thermal/drivers/core: Remove the module Kconfig's option

Daniel Lezcano <[email protected]> writes:

> The module support for the thermal subsystem makes little sense:
> - some subsystems relying on it are not modules, thus forcing the
> framework to be compiled in
> - it is compiled in for almost every configs, the remaining ones
> are a few platforms where I don't see why we can not switch the thermal
> to 'y'. The drivers can stay in tristate.
> - platforms need the thermal to be ready as soon as possible at boot time
> in order to mitigate
>
> Usually the subsystems framework are compiled-in and the plugs are as module.
>
> Remove the module option. The removal of the module related dead code will
> come after this patch gets in or is acked.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
> Acked-by: Guenter Roeck <[email protected]>
> For mini2440:
> Acked-by: Krzysztof Kozlowski <[email protected]>
For pxa:
Acked-by: Robert Jarzmik <[email protected]>

Cheers.

--
Robert

2019-04-22 09:00:07

[permalink] [raw]

Subject: Re: [PATCH 3/7] thermal/drivers/core: Add init section table for self-encapsulation

Hi, Daniel,

Thanks for the patches, it looks good to me except this one and patch
4/7.

First, I don't think this is a cyclic dependency issue as they are in
the same module.
Second, I have not read include/asm-generic/vmlinux.lds.h before, it
seems that it is used for architecture specific stuff. Fix a thermal
issue in this way seems overkill to me.

IMO, to make the code clean, we can build the governors as separate
modules just like we do for cpu governors.
This brings to the old commit 80a26a5c22b9("Thermal: build thermal
governors into thermal_sys module"), and that was introduced to fix a
problem when CONFIG_THERMAL is set to 'm'. So I think we can switch
back to the old way as the problem is gone now.

what do you think?

Patch 1,2,5,6,7 applied first.

thanks,
rui

On 二, 2019-04-02 at 18:12 +0200, Daniel Lezcano wrote:
> Currently the governors are declared in their respective files but
> they
> export their [un]register functions which in turn call the
> [un]register
> the governors core's functions. That implies a cyclic dependency
> which is
> not desirable. There is a way to self-encapsulate the governors by
> letting
> them to declare themselves in a __init section table.
>
> Define the table in the asm generic linker description like the other
> tables and provide the specific macros to deal with.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
> ---
> drivers/thermal/thermal_core.h    | 16 ++++++++++++++++
> include/asm-generic/vmlinux.lds.h | 11 +++++++++++
> 2 files changed, 27 insertions(+)
>
> diff --git a/drivers/thermal/thermal_core.h
> b/drivers/thermal/thermal_core.h
> index 0df190ed82a7..28d18083e969 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -15,6 +15,22 @@
> /* Initial state of a cooling device during binding */
> #define THERMAL_NO_TARGET -1UL
>
> +/* Init section thermal table */
> +extern struct thermal_governor * __governor_thermal_table[];
> +extern struct thermal_governor * __governor_thermal_table_end[];
> +
> +#define THERMAL_TABLE_ENTRY(table, name) \
> +        static typeof(name) * __thermal_table_entry_##name \
> + __used __section(__##table##_thermal_table) \
> + = &name;
> +
> +#define THERMAL_GOVERNOR_DECLARE(name) THERMAL_TABLE_ENTRY(go
> vernor, name)
> +
> +#define for_each_governor_table(__governor) \
> + for (__governor = __governor_thermal_table; \
> +      __governor < __governor_thermal_table_end; \
> +      __governor++)
> +
> /*
> * This structure is used to describe the behavior of
> * a certain cooling device on a certain trip point
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-
> generic/vmlinux.lds.h
> index f8f6f04c4453..9893a3ed242a 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -239,6 +239,16 @@
> #define ACPI_PROBE_TABLE(name)
> #endif
>
> +#ifdef CONFIG_THERMAL
> +#define THERMAL_TABLE(name)
> \
> +        . = ALIGN(8);
> \
> +        __##name##_thermal_table = .;
> \
> +        KEEP(*(__##name##_thermal_table))
> \
> +        __##name##_thermal_table_end = .;
> +#else
> +#define THERMAL_TABLE(name)
> +#endif
> +
> #define KERNEL_DTB()
> \
> STRUCT_ALIGN();
> \
> __dtb_start = .;
> \
> @@ -609,6 +619,7 @@
> IRQCHIP_OF_MATCH_TABLE()
> \
> ACPI_PROBE_TABLE(irqchip)
> \
> ACPI_PROBE_TABLE(timer)
> \
> + THERMAL_TABLE(governor)
> \
> EARLYCON_TABLE()
> \
> LSM_TABLE()
>

2019-04-22 12:13:28

[permalink] [raw]

Subject: Re: [PATCH 3/7] thermal/drivers/core: Add init section table for self-encapsulation

Hi Zhang,

On 22/04/2019 10:43, Zhang Rui wrote:
> Hi, Daniel,
>
> Thanks for the patches, it looks good to me except this one and patch
> 4/7.
>
> First, I don't think this is a cyclic dependency issue as they are in
> the same module.

The governors have to export their [un]register functions in order to
have the core to use them.

The core has to export the [un]register function in order to have the
governors to use them.

From my point of view it is a cyclic dependency. In any other
subsystems, the plugins/governor/drivers/whatever don't have to export
their functions to the core, they use the core's exported functions.

> Second, I have not read include/asm-generic/vmlinux.lds.h before, it
> seems that it is used for architecture specific stuff. Fix a thermal
> issue in this way seems overkill to me.

It is not architecture specific, it belongs to asm-generic. All init
calls are defined in it and more. It is a common way to define static
tables from different files without adding dependency and unload it
after init.

All clk, timers, acpi tables, irq chip, cpuidle and cpu methods are
defined this way.

When the thermal_core.c uses at the end fs_initcall it uses the same
mechanism.

> IMO, to make the code clean, we can build the governors as separate
> modules just like we do for cpu governors.
> This brings to the old commit 80a26a5c22b9("Thermal: build thermal
> governors into thermal_sys module"), and that was introduced to fix a
> problem when CONFIG_THERMAL is set to 'm'. So I think we can switch
> back to the old way as the problem is gone now.
>
> what do you think?

IMO, having the governors built as module is not a good thing because
the SoC needs the governor to be ready as soon as possible at boot time.
I've been told some boards reboot at boot time because the governor
comes too late with the userspace governor for example.

If you don't like the vmlinuz.lds.h approch (but again it is a common
way to initialize table and I wrote it to extend to more thermal table
in the future) we can change the thermal core to replace fs_initcall()
by core_initcall() and use postcore_initcall() in the governor.

> Patch 1,2,5,6,7 applied first.
>
> thanks,
> rui
>
> On 二, 2019-04-02 at 18:12 +0200, Daniel Lezcano wrote:
>> Currently the governors are declared in their respective files but
>> they
>> export their [un]register functions which in turn call the
>> [un]register
>> the governors core's functions. That implies a cyclic dependency
>> which is
>> not desirable. There is a way to self-encapsulate the governors by
>> letting
>> them to declare themselves in a __init section table.
>>
>> Define the table in the asm generic linker description like the other
>> tables and provide the specific macros to deal with.
>>
>> Signed-off-by: Daniel Lezcano <[email protected]>
>> ---
>> drivers/thermal/thermal_core.h    | 16 ++++++++++++++++
>> include/asm-generic/vmlinux.lds.h | 11 +++++++++++
>> 2 files changed, 27 insertions(+)
>>
>> diff --git a/drivers/thermal/thermal_core.h
>> b/drivers/thermal/thermal_core.h
>> index 0df190ed82a7..28d18083e969 100644
>> --- a/drivers/thermal/thermal_core.h
>> +++ b/drivers/thermal/thermal_core.h
>> @@ -15,6 +15,22 @@
>> /* Initial state of a cooling device during binding */
>> #define THERMAL_NO_TARGET -1UL
>>
>> +/* Init section thermal table */
>> +extern struct thermal_governor * __governor_thermal_table[];
>> +extern struct thermal_governor * __governor_thermal_table_end[];
>> +
>> +#define THERMAL_TABLE_ENTRY(table, name) \
>> +        static typeof(name) * __thermal_table_entry_##name \
>> + __used __section(__##table##_thermal_table) \
>> + = &name;
>> +
>> +#define THERMAL_GOVERNOR_DECLARE(name) THERMAL_TABLE_ENTRY(go
>> vernor, name)
>> +
>> +#define for_each_governor_table(__governor) \
>> + for (__governor = __governor_thermal_table; \
>> +      __governor < __governor_thermal_table_end; \
>> +      __governor++)
>> +
>> /*
>> * This structure is used to describe the behavior of
>> * a certain cooling device on a certain trip point
>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-
>> generic/vmlinux.lds.h
>> index f8f6f04c4453..9893a3ed242a 100644
>> --- a/include/asm-generic/vmlinux.lds.h
>> +++ b/include/asm-generic/vmlinux.lds.h
>> @@ -239,6 +239,16 @@
>> #define ACPI_PROBE_TABLE(name)
>> #endif
>>
>> +#ifdef CONFIG_THERMAL
>> +#define THERMAL_TABLE(name)
>> \
>> +        . = ALIGN(8);
>> \
>> +        __##name##_thermal_table = .;
>> \
>> +        KEEP(*(__##name##_thermal_table))
>> \
>> +        __##name##_thermal_table_end = .;
>> +#else
>> +#define THERMAL_TABLE(name)
>> +#endif
>> +
>> #define KERNEL_DTB()
>> \
>> STRUCT_ALIGN();
>> \
>> __dtb_start = .;
>> \
>> @@ -609,6 +619,7 @@
>> IRQCHIP_OF_MATCH_TABLE()
>> \
>> ACPI_PROBE_TABLE(irqchip)
>> \
>> ACPI_PROBE_TABLE(timer)
>> \
>> + THERMAL_TABLE(governor)
>> \
>> EARLYCON_TABLE()
>> \
>> LSM_TABLE()
>>

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

2019-04-23 06:00:36

[permalink] [raw]

Subject: Re: [PATCH 3/7] thermal/drivers/core: Add init section table for self-encapsulation

Hi, Daniel,

thanks for clarifying.
It is true that we need to make thermal framework ready as early as
possible. And a static table works for me as long as vmlinux.lds.h is
the proper place.

Arnd,
are you okay with this patch? if yes, I suppose I can take it through
my tree, right?

thanks,
rui

On 一, 2019-04-22 at 14:11 +0200, Daniel Lezcano wrote:
> Hi Zhang,
>
>
> On 22/04/2019 10:43, Zhang Rui wrote:
> >
> > Hi, Daniel,
> >
> > Thanks for the patches, it looks good to me except this one and
> > patch
> > 4/7.
> >
> > First, I don't think this is a cyclic dependency issue as they are
> > in
> > the same module.
> The governors have to export their [un]register functions in order to
> have the core to use them.
>
> The core has to export the [un]register function in order to have the
> governors to use them.
>
> From my point of view it is a cyclic dependency. In any other
> subsystems, the plugins/governor/drivers/whatever don't have to
> export
> their functions to the core, they use the core's exported functions.
>
> >
> > Second, I have not read include/asm-generic/vmlinux.lds.h before,
> > it
> > seems that it is used for architecture specific stuff. Fix a
> > thermal
> > issue in this way seems overkill to me.
> It is not architecture specific, it belongs to asm-generic. All init
> calls are defined in it and more. It is a common way to define static
> tables from different files without adding dependency and unload it
> after init.
>
> All clk, timers, acpi tables, irq chip, cpuidle and cpu methods are
> defined this way.
>
> When the thermal_core.c uses at the end fs_initcall it uses the same
> mechanism.
>
>
> >
> > IMO, to make the code clean, we can build the governors as separate
> > modules just like we do for cpu governors.
> > This brings to the old commit 80a26a5c22b9("Thermal: build thermal
> > governors into thermal_sys module"), and that was introduced to fix
> > a
> > problem when CONFIG_THERMAL is set to 'm'. So I think we can switch
> > back to the old way as the problem is gone now.
> >
> > what do you think?
> IMO, having the governors built as module is not a good thing because
> the SoC needs the governor to be ready as soon as possible at boot
> time.
> I've been told some boards reboot at boot time because the governor
> comes too late with the userspace governor for example.
>
> If you don't like the vmlinuz.lds.h approch (but again it is a common
> way to initialize table and I wrote it to extend to more thermal
> table
> in the future) we can change the thermal core to replace
> fs_initcall()
> by core_initcall() and use postcore_initcall() in the governor.
>
>
>
> >
> > Patch 1,2,5,6,7 applied first.
> >
> > thanks,
> > rui
> >
> > On 二, 2019-04-02 at 18:12 +0200, Daniel Lezcano wrote:
> > >
> > > Currently the governors are declared in their respective files
> > > but
> > > they
> > > export their [un]register functions which in turn call the
> > > [un]register
> > > the governors core's functions. That implies a cyclic dependency
> > > which is
> > > not desirable. There is a way to self-encapsulate the governors
> > > by
> > > letting
> > > them to declare themselves in a __init section table.
> > >
> > > Define the table in the asm generic linker description like the
> > > other
> > > tables and provide the specific macros to deal with.
> > >
> > > Signed-off-by: Daniel Lezcano <[email protected]>
> > > ---
> > > drivers/thermal/thermal_core.h    | 16 ++++++++++++++++
> > > include/asm-generic/vmlinux.lds.h | 11 +++++++++++
> > > 2 files changed, 27 insertions(+)
> > >
> > > diff --git a/drivers/thermal/thermal_core.h
> > > b/drivers/thermal/thermal_core.h
> > > index 0df190ed82a7..28d18083e969 100644
> > > --- a/drivers/thermal/thermal_core.h
> > > +++ b/drivers/thermal/thermal_core.h
> > > @@ -15,6 +15,22 @@
> > > /* Initial state of a cooling device during binding */
> > > #define THERMAL_NO_TARGET -1UL
> > >
> > > +/* Init section thermal table */
> > > +extern struct thermal_governor * __governor_thermal_table[];
> > > +extern struct thermal_governor * __governor_thermal_table_end[];
> > > +
> > > +#define THERMAL_TABLE_ENTRY(table, name)
> > > \
> > > +        static typeof(name) * __thermal_table_entry_##name
> > > \
> > > + __used __section(__##table##_thermal_table)
> > > \
> > > + = &name;
> > > +
> > > +#define THERMAL_GOVERNOR_DECLARE(name) THERMAL_TABLE_ENTR
> > > Y(go
> > > vernor, name)
> > > +
> > > +#define for_each_governor_table(__governor) \
> > > + for (__governor = __governor_thermal_table; \
> > > +      __governor < __governor_thermal_table_end; \
> > > +      __governor++)
> > > +
> > > /*
> > > * This structure is used to describe the behavior of
> > > * a certain cooling device on a certain trip point
> > > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-
> > > generic/vmlinux.lds.h
> > > index f8f6f04c4453..9893a3ed242a 100644
> > > --- a/include/asm-generic/vmlinux.lds.h
> > > +++ b/include/asm-generic/vmlinux.lds.h
> > > @@ -239,6 +239,16 @@
> > > #define ACPI_PROBE_TABLE(name)
> > > #endif
> > >
> > > +#ifdef CONFIG_THERMAL
> > > +#define THERMAL_TABLE(name)
> > >
> > > \
> > > +        . = ALIGN(8);
> > > \
> > > +        __##name##_thermal_table = .;
> > > \
> > > +        KEEP(*(__##name##_thermal_table))
> > >
> > > \
> > > +        __##name##_thermal_table_end = .;
> > > +#else
> > > +#define THERMAL_TABLE(name)
> > > +#endif
> > > +
> > > #define KERNEL_DTB()
> > >
> > > \
> > > STRUCT_ALIGN();
> > > \
> > > __dtb_start = .;
> > > \
> > > @@ -609,6 +619,7 @@
> > > IRQCHIP_OF_MATCH_TABLE()
> > > \
> > > ACPI_PROBE_TABLE(irqchip)
> > >
> > > \
> > > ACPI_PROBE_TABLE(timer)
> > > \
> > > + THERMAL_TABLE(governor)
> > > \
> > > EARLYCON_TABLE()
> > > \
> > > LSM_TABLE()
> > >
>

2019-04-23 10:36:16

[permalink] [raw]

Subject: Re: [PATCH 4/7] thermal/drivers/core: Use governor table to initialize

On Tue, Apr 2, 2019 at 9:43 PM Daniel Lezcano <[email protected]> wrote:
>
> Now that the governor table is in place and the macro allows to browse the
> table, declare the governor so the entry is added in the governor table
> in the init section.
>
> The [un]register_thermal_governors function does no longer need to use the
> exported [un]register thermal governor's specific function which in turn
> call the [un]register_thermal_governor. The governors are fully
> self-encapsulated.
>
> The cyclic dependency is no longer needed, remove it.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
> ---
> drivers/thermal/fair_share.c | 12 +-------
> drivers/thermal/gov_bang_bang.c | 11 +------
> drivers/thermal/power_allocator.c | 11 +------
> drivers/thermal/step_wise.c | 11 +------
> drivers/thermal/thermal_core.c | 51 +++++++++++++++++--------------
> drivers/thermal/thermal_core.h | 40 ------------------------
> drivers/thermal/user_space.c | 12 +-------
> 7 files changed, 33 insertions(+), 115 deletions(-)
>
> diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
> index d3469fbc5207..bda2afc63471 100644
> --- a/drivers/thermal/fair_share.c
> +++ b/drivers/thermal/fair_share.c
> @@ -129,14 +129,4 @@ static struct thermal_governor thermal_gov_fair_share = {
> .name = "fair_share",
> .throttle = fair_share_throttle,
> };
> -
> -int thermal_gov_fair_share_register(void)
> -{
> - return thermal_register_governor(&thermal_gov_fair_share);
> -}
> -
> -void thermal_gov_fair_share_unregister(void)
> -{
> - thermal_unregister_governor(&thermal_gov_fair_share);
> -}
> -
> +THERMAL_GOVERNOR_DECLARE(thermal_gov_fair_share);
> diff --git a/drivers/thermal/gov_bang_bang.c b/drivers/thermal/gov_bang_bang.c
> index fc5e5057f0de..c5e19c7d63da 100644
> --- a/drivers/thermal/gov_bang_bang.c
> +++ b/drivers/thermal/gov_bang_bang.c
> @@ -126,13 +126,4 @@ static struct thermal_governor thermal_gov_bang_bang = {
> .name = "bang_bang",
> .throttle = bang_bang_control,
> };
> -
> -int thermal_gov_bang_bang_register(void)
> -{
> - return thermal_register_governor(&thermal_gov_bang_bang);
> -}
> -
> -void thermal_gov_bang_bang_unregister(void)
> -{
> - thermal_unregister_governor(&thermal_gov_bang_bang);
> -}
> +THERMAL_GOVERNOR_DECLARE(thermal_gov_bang_bang);
> diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
> index 3055f9a12a17..44636475b2a3 100644
> --- a/drivers/thermal/power_allocator.c
> +++ b/drivers/thermal/power_allocator.c
> @@ -651,13 +651,4 @@ static struct thermal_governor thermal_gov_power_allocator = {
> .unbind_from_tz = power_allocator_unbind,
> .throttle = power_allocator_throttle,
> };
> -
> -int thermal_gov_power_allocator_register(void)
> -{
> - return thermal_register_governor(&thermal_gov_power_allocator);
> -}
> -
> -void thermal_gov_power_allocator_unregister(void)
> -{
> - thermal_unregister_governor(&thermal_gov_power_allocator);
> -}
> +THERMAL_GOVERNOR_DECLARE(thermal_gov_power_allocator);
> diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
> index ee047ca43084..6cd251ab56fc 100644
> --- a/drivers/thermal/step_wise.c
> +++ b/drivers/thermal/step_wise.c
> @@ -218,13 +218,4 @@ static struct thermal_governor thermal_gov_step_wise = {
> .name = "step_wise",
> .throttle = step_wise_throttle,
> };
> -
> -int thermal_gov_step_wise_register(void)
> -{
> - return thermal_register_governor(&thermal_gov_step_wise);
> -}
> -
> -void thermal_gov_step_wise_unregister(void)
> -{
> - thermal_unregister_governor(&thermal_gov_step_wise);
> -}
> +THERMAL_GOVERNOR_DECLARE(thermal_gov_step_wise);
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 28f7ece0e8fe..50c88682a848 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -243,36 +243,41 @@ int thermal_build_list_of_policies(char *buf)
> return count;
> }
>
> -static int __init thermal_register_governors(void)
> +static void __init thermal_unregister_governors(void)
> {
> - int result;
> + struct thermal_governor **governor;
>
> - result = thermal_gov_step_wise_register();
> - if (result)
> - return result;
> + for_each_governor_table(governor)
> + thermal_unregister_governor(*governor);

As a result of this function moving to __init, can't
thermal_unregister_governor also become __init now?

Nice cleanup.

Reviewed-by: Amit Kucheria <[email protected]>

> +}
>
> - result = thermal_gov_fair_share_register();
> - if (result)
> - return result;
> +static int __init thermal_register_governors(void)
> +{
> + int ret = 0;
> + struct thermal_governor **governor;
>
> - result = thermal_gov_bang_bang_register();
> - if (result)
> - return result;
> + for_each_governor_table(governor) {
> + ret = thermal_register_governor(*governor);
> + if (ret) {
> + pr_err("Failed to register governor: '%s'",
> + (*governor)->name);
> + break;
> + }
>
> - result = thermal_gov_user_space_register();
> - if (result)
> - return result;
> + pr_info("Registered thermal governor '%s'",
> + (*governor)->name);
> + }
>
> - return thermal_gov_power_allocator_register();
> -}
> + if (ret) {
> + struct thermal_governor **gov;
> + for_each_governor_table(gov) {
> + if (gov == governor)
> + break;
> + thermal_unregister_governor(*gov);
> + }
> + }
>
> -static void __init thermal_unregister_governors(void)
> -{
> - thermal_gov_step_wise_unregister();
> - thermal_gov_fair_share_unregister();
> - thermal_gov_bang_bang_unregister();
> - thermal_gov_user_space_unregister();
> - thermal_gov_power_allocator_unregister();
> + return ret;
> }
>
> /*
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index 28d18083e969..a3c555b48eb3 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -90,46 +90,6 @@ thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
> unsigned long new_state) {}
> #endif /* CONFIG_THERMAL_STATISTICS */
>
> -#ifdef CONFIG_THERMAL_GOV_STEP_WISE
> -int thermal_gov_step_wise_register(void);
> -void thermal_gov_step_wise_unregister(void);
> -#else
> -static inline int thermal_gov_step_wise_register(void) { return 0; }
> -static inline void thermal_gov_step_wise_unregister(void) {}
> -#endif /* CONFIG_THERMAL_GOV_STEP_WISE */
> -
> -#ifdef CONFIG_THERMAL_GOV_FAIR_SHARE
> -int thermal_gov_fair_share_register(void);
> -void thermal_gov_fair_share_unregister(void);
> -#else
> -static inline int thermal_gov_fair_share_register(void) { return 0; }
> -static inline void thermal_gov_fair_share_unregister(void) {}
> -#endif /* CONFIG_THERMAL_GOV_FAIR_SHARE */
> -
> -#ifdef CONFIG_THERMAL_GOV_BANG_BANG
> -int thermal_gov_bang_bang_register(void);
> -void thermal_gov_bang_bang_unregister(void);
> -#else
> -static inline int thermal_gov_bang_bang_register(void) { return 0; }
> -static inline void thermal_gov_bang_bang_unregister(void) {}
> -#endif /* CONFIG_THERMAL_GOV_BANG_BANG */
> -
> -#ifdef CONFIG_THERMAL_GOV_USER_SPACE
> -int thermal_gov_user_space_register(void);
> -void thermal_gov_user_space_unregister(void);
> -#else
> -static inline int thermal_gov_user_space_register(void) { return 0; }
> -static inline void thermal_gov_user_space_unregister(void) {}
> -#endif /* CONFIG_THERMAL_GOV_USER_SPACE */
> -
> -#ifdef CONFIG_THERMAL_GOV_POWER_ALLOCATOR
> -int thermal_gov_power_allocator_register(void);
> -void thermal_gov_power_allocator_unregister(void);
> -#else
> -static inline int thermal_gov_power_allocator_register(void) { return 0; }
> -static inline void thermal_gov_power_allocator_unregister(void) {}
> -#endif /* CONFIG_THERMAL_GOV_POWER_ALLOCATOR */
> -
> /* device tree support */
> #ifdef CONFIG_THERMAL_OF
> int of_parse_thermal_zones(void);
> diff --git a/drivers/thermal/user_space.c b/drivers/thermal/user_space.c
> index 8e92a06ef48a..5fac99e5221d 100644
> --- a/drivers/thermal/user_space.c
> +++ b/drivers/thermal/user_space.c
> @@ -56,14 +56,4 @@ static struct thermal_governor thermal_gov_user_space = {
> .name = "user_space",
> .throttle = notify_user_space,
> };
> -
> -int thermal_gov_user_space_register(void)
> -{
> - return thermal_register_governor(&thermal_gov_user_space);
> -}
> -
> -void thermal_gov_user_space_unregister(void)
> -{
> - thermal_unregister_governor(&thermal_gov_user_space);
> -}
> -
> +THERMAL_GOVERNOR_DECLARE(thermal_gov_user_space);
> --
> 2.17.1
>

2019-04-23 10:36:42

[permalink] [raw]

Subject: Re: [PATCH 5/7] thermal/drivers/core: Remove depends on THERMAL in Kconfig

On Wed, Apr 3, 2019 at 9:03 AM Daniel Lezcano <[email protected]> wrote:
>
> Hi Amit,
>
> On 02/04/2019 20:51, Amit Kucheria wrote:
> > On Tue, Apr 2, 2019 at 11:14 PM Daniel Lezcano
> > <[email protected]> wrote:
> >>
> >> The dependency on the THERMAL option to be set is already there implicitly
> >> by the "if THERMAL" conditionnal option. The sub Kconfigs do not have to
> >> check against the THERMAL option as they are called from a Kconfig block
> >> which is enabled by the conditionnal option.
> >>
> >> Remove the useless "depends on THERMAL" in the Kconfigs.
> >>
> >> Signed-off-by: Daniel Lezcano <[email protected]>
> >> ---
> >> drivers/thermal/Kconfig | 2 --
> >> drivers/thermal/intel/Kconfig | 1 -
> >> drivers/thermal/qcom/Kconfig | 1 -
> >> 3 files changed, 4 deletions(-)
> >>
> >> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> >> index ccf5b9408d7a..d8aa7fac8c56 100644
> >> --- a/drivers/thermal/Kconfig
> >> +++ b/drivers/thermal/Kconfig
> >> @@ -24,7 +24,6 @@ config THERMAL_STATISTICS
> >>
> >> config THERMAL_EMERGENCY_POWEROFF_DELAY_MS
> >> int "Emergency poweroff delay in milli-seconds"
> >> - depends on THERMAL
> >> default 0
> >> help
> >> Thermal subsystem will issue a graceful shutdown when
> >> @@ -152,7 +151,6 @@ config CPU_THERMAL
> >> bool "generic cpu cooling support"
> >> depends on CPU_FREQ
> >> depends on THERMAL_OF
> >> - depends on THERMAL=y
> >
> > This will break the intention of afa1f2ab43d48d0e1. At the moment, we
> > need THERMAL=y if we want cpu_cooling.
>
> The patch 1/7 changes the thermal option from tristate to bool, so the
> thermal is compiled-in always. The dependency is no longer needed in
> this case. Did I miss something?

Sorry, the threading of this series was a bit strange and there was no
cover letter. I wasn't cc'ed on the other patches, so missed the
earlier changes. This series looks good.

FWIW,

Reviewed-by: Amit Kucheria <[email protected]>

2019-04-23 15:53:27

by Eduardo Valentin

[permalink] [raw]

Subject: Re: [PATCH 1/7] thermal/drivers/core: Remove the module Kconfig's option

Hello,

On Tue, Apr 02, 2019 at 06:12:44PM +0200, Daniel Lezcano wrote:
> The module support for the thermal subsystem makes little sense:
> - some subsystems relying on it are not modules, thus forcing the
> framework to be compiled in
> - it is compiled in for almost every configs, the remaining ones
> are a few platforms where I don't see why we can not switch the thermal
> to 'y'. The drivers can stay in tristate.
> - platforms need the thermal to be ready as soon as possible at boot time
> in order to mitigate
>
> Usually the subsystems framework are compiled-in and the plugs are as module.
>
> Remove the module option. The removal of the module related dead code will
> come after this patch gets in or is acked.

I remember some buzilla entry around this some time back.

Rui, do you remember why you made this to be module?

I dont have strong opinion here, but I would like to see
a better description why we are going this direction rather
than "most people dont use it as module". Was there any particular
specific technical motivation?

>
> Signed-off-by: Daniel Lezcano <[email protected]>
> Acked-by: Guenter Roeck <[email protected]>
> For mini2440:
> Acked-by: Krzysztof Kozlowski <[email protected]>
> ---
> arch/arm/configs/mini2440_defconfig | 2 +-
> arch/arm/configs/pxa_defconfig | 2 +-
> arch/mips/configs/ip22_defconfig | 2 +-
> arch/mips/configs/ip27_defconfig | 2 +-
> arch/unicore32/configs/unicore32_defconfig | 2 +-
> drivers/thermal/Kconfig | 4 ++--
> 6 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/configs/mini2440_defconfig b/arch/arm/configs/mini2440_defconfig
> index d95a8059d30b..0cf1c120c4bb 100644
> --- a/arch/arm/configs/mini2440_defconfig
> +++ b/arch/arm/configs/mini2440_defconfig
> @@ -152,7 +152,7 @@ CONFIG_SPI_S3C24XX=y
> CONFIG_SPI_SPIDEV=y
> CONFIG_GPIO_SYSFS=y
> CONFIG_SENSORS_LM75=y
> -CONFIG_THERMAL=m
> +CONFIG_THERMAL=y
> CONFIG_WATCHDOG=y
> CONFIG_S3C2410_WATCHDOG=y
> CONFIG_FB=y
> diff --git a/arch/arm/configs/pxa_defconfig b/arch/arm/configs/pxa_defconfig
> index d4654755b09c..d4f9dda3a52f 100644
> --- a/arch/arm/configs/pxa_defconfig
> +++ b/arch/arm/configs/pxa_defconfig
> @@ -387,7 +387,7 @@ CONFIG_SENSORS_LM75=m
> CONFIG_SENSORS_LM90=m
> CONFIG_SENSORS_LM95245=m
> CONFIG_SENSORS_NTC_THERMISTOR=m
> -CONFIG_THERMAL=m
> +CONFIG_THERMAL=y
> CONFIG_WATCHDOG=y
> CONFIG_XILINX_WATCHDOG=m
> CONFIG_SA1100_WATCHDOG=m
> diff --git a/arch/mips/configs/ip22_defconfig b/arch/mips/configs/ip22_defconfig
> index ff40fbc2f439..21a1168ae301 100644
> --- a/arch/mips/configs/ip22_defconfig
> +++ b/arch/mips/configs/ip22_defconfig
> @@ -228,7 +228,7 @@ CONFIG_SERIAL_IP22_ZILOG=m
> # CONFIG_HW_RANDOM is not set
> CONFIG_RAW_DRIVER=m
> # CONFIG_HWMON is not set
> -CONFIG_THERMAL=m
> +CONFIG_THERMAL=y
> CONFIG_WATCHDOG=y
> CONFIG_INDYDOG=m
> # CONFIG_VGA_CONSOLE is not set
> diff --git a/arch/mips/configs/ip27_defconfig b/arch/mips/configs/ip27_defconfig
> index 81c47e18131b..54db5dedf776 100644
> --- a/arch/mips/configs/ip27_defconfig
> +++ b/arch/mips/configs/ip27_defconfig
> @@ -271,7 +271,7 @@ CONFIG_I2C_PARPORT_LIGHT=m
> CONFIG_I2C_TAOS_EVM=m
> CONFIG_I2C_STUB=m
> # CONFIG_HWMON is not set
> -CONFIG_THERMAL=m
> +CONFIG_THERMAL=y
> CONFIG_MFD_PCF50633=m
> CONFIG_PCF50633_ADC=m
> CONFIG_PCF50633_GPIO=m
> diff --git a/arch/unicore32/configs/unicore32_defconfig b/arch/unicore32/configs/unicore32_defconfig
> index aebd01fc28e5..360cc9abcdb0 100644
> --- a/arch/unicore32/configs/unicore32_defconfig
> +++ b/arch/unicore32/configs/unicore32_defconfig
> @@ -119,7 +119,7 @@ CONFIG_I2C_PUV3=y
> # Hardware Monitoring support
> #CONFIG_SENSORS_LM75=m
> # Generic Thermal sysfs driver
> -#CONFIG_THERMAL=m
> +#CONFIG_THERMAL=y
> #CONFIG_THERMAL_HWMON=y
>
> # Multimedia support
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 653aa27a25a4..ccf5b9408d7a 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -3,7 +3,7 @@
> #
>
> menuconfig THERMAL
> - tristate "Generic Thermal sysfs driver"
> + bool "Generic Thermal sysfs driver"
> help
> Generic Thermal Sysfs driver offers a generic mechanism for
> thermal management. Usually it's made up of one or more thermal
> @@ -11,7 +11,7 @@ menuconfig THERMAL
> Each thermal zone contains its own temperature, trip points,
> cooling devices.
> All platforms with ACPI thermal support can use this driver.
> - If you want this support, you should say Y or M here.
> + If you want this support, you should say Y here.
>
> if THERMAL
>
> --
> 2.17.1
>

2019-04-24 05:49:42

[permalink] [raw]

Subject: Re: [PATCH 1/7] thermal/drivers/core: Remove the module Kconfig's option

On Tue, Apr 23, 2019 at 9:22 PM Eduardo Valentin <[email protected]> wrote:
>
> Hello,
>
> On Tue, Apr 02, 2019 at 06:12:44PM +0200, Daniel Lezcano wrote:
> > The module support for the thermal subsystem makes little sense:
> > - some subsystems relying on it are not modules, thus forcing the
> > framework to be compiled in
> > - it is compiled in for almost every configs, the remaining ones
> > are a few platforms where I don't see why we can not switch the thermal
> > to 'y'. The drivers can stay in tristate.
> > - platforms need the thermal to be ready as soon as possible at boot time
> > in order to mitigate
> >
> > Usually the subsystems framework are compiled-in and the plugs are as module.
> >
> > Remove the module option. The removal of the module related dead code will
> > come after this patch gets in or is acked.
>
>
> I remember some buzilla entry around this some time back.
>
> Rui, do you remember why you made this to be module?
>
> I dont have strong opinion here, but I would like to see
> a better description why we are going this direction rather
> than "most people dont use it as module". Was there any particular
> specific technical motivation?

Speaking for Qualcomm platforms, we want the thermal subsystem
available as soon as possible for boot time thermal mitigation since
faster boot times equals hotter cpus. Also the dependency on cpufreq
subsystem due to the cpufreq cooling device would be simplified with
this.

In fact, I now have a follow on patch to move thermal init earlier
than fs_initcall since we'd now not wait on modules to be available.

/Amit

2019-04-24 19:42:11

[permalink] [raw]

Subject: Re: [PATCH 1/7] thermal/drivers/core: Remove the module Kconfig's option

On 二, 2019-04-23 at 08:52 -0700, Eduardo Valentin wrote:
> Hello,
>
> On Tue, Apr 02, 2019 at 06:12:44PM +0200, Daniel Lezcano wrote:
> >
> > The module support for the thermal subsystem makes little sense:
> > - some subsystems relying on it are not modules, thus forcing the
> >    framework to be compiled in
> > - it is compiled in for almost every configs, the remaining ones
> >    are a few platforms where I don't see why we can not switch the
> > thermal
> >    to 'y'. The drivers can stay in tristate.
> > - platforms need the thermal to be ready as soon as possible at
> > boot time
> >    in order to mitigate
> >
> > Usually the subsystems framework are compiled-in and the plugs are
> > as module.
> >
> > Remove the module option. The removal of the module related dead
> > code will
> > come after this patch gets in or is acked.
>
> I remember some buzilla entry around this some time back.
>
> Rui, do you remember why you made this to be module?
>
> I dont have strong opinion here, but I would like to see
> a better description why we are going this direction rather
> than "most people dont use it as module". Was there any particular
> specific technical motivation?
>
I checked the change log, it seems that we make CONFIG_THERMAL from
bool to tristate long time ago.

commit 63c4ec905d63834a97ec7dbbf0a2ec89ef5872be
Author:     Zhang Rui <[email protected]>
AuthorDate: Mon Apr 21 16:07:13 2008 +0800
Commit:     Len Brown <[email protected]>
CommitDate: Tue Apr 29 02:44:00 2008 -0400

    thermal: add the support for building the generic thermal as a
module

    Build the generic thermal driver as module "thermal_sys".

    Make ACPI thermal, video, processor and fan SELECT the generic
    thermal driver, as these drivers rely on it to build the sysfs I/F.

    Signed-off-by: Zhang Rui <[email protected]>
    Acked-by: Jean Delvare <[email protected]>
    Signed-off-by: Len Brown <[email protected]>

But the things described in the changelog does not seem to be a solid
reason why we need thermal to be a module.

Anyway, let's try bool instead of tristate and see if everything still
works well.

thanks,
rui
>
> >
> >
> > Signed-off-by: Daniel Lezcano <[email protected]>
> > Acked-by: Guenter Roeck <[email protected]>
> > For mini2440:
> > Acked-by: Krzysztof Kozlowski <[email protected]>
> > ---
> > arch/arm/configs/mini2440_defconfig        | 2 +-
> > arch/arm/configs/pxa_defconfig             | 2 +-
> > arch/mips/configs/ip22_defconfig           | 2 +-
> > arch/mips/configs/ip27_defconfig           | 2 +-
> > arch/unicore32/configs/unicore32_defconfig | 2 +-
> > drivers/thermal/Kconfig                    | 4 ++--
> > 6 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm/configs/mini2440_defconfig
> > b/arch/arm/configs/mini2440_defconfig
> > index d95a8059d30b..0cf1c120c4bb 100644
> > --- a/arch/arm/configs/mini2440_defconfig
> > +++ b/arch/arm/configs/mini2440_defconfig
> > @@ -152,7 +152,7 @@ CONFIG_SPI_S3C24XX=y
> > CONFIG_SPI_SPIDEV=y
> > CONFIG_GPIO_SYSFS=y
> > CONFIG_SENSORS_LM75=y
> > -CONFIG_THERMAL=m
> > +CONFIG_THERMAL=y
> > CONFIG_WATCHDOG=y
> > CONFIG_S3C2410_WATCHDOG=y
> > CONFIG_FB=y
> > diff --git a/arch/arm/configs/pxa_defconfig
> > b/arch/arm/configs/pxa_defconfig
> > index d4654755b09c..d4f9dda3a52f 100644
> > --- a/arch/arm/configs/pxa_defconfig
> > +++ b/arch/arm/configs/pxa_defconfig
> > @@ -387,7 +387,7 @@ CONFIG_SENSORS_LM75=m
> > CONFIG_SENSORS_LM90=m
> > CONFIG_SENSORS_LM95245=m
> > CONFIG_SENSORS_NTC_THERMISTOR=m
> > -CONFIG_THERMAL=m
> > +CONFIG_THERMAL=y
> > CONFIG_WATCHDOG=y
> > CONFIG_XILINX_WATCHDOG=m
> > CONFIG_SA1100_WATCHDOG=m
> > diff --git a/arch/mips/configs/ip22_defconfig
> > b/arch/mips/configs/ip22_defconfig
> > index ff40fbc2f439..21a1168ae301 100644
> > --- a/arch/mips/configs/ip22_defconfig
> > +++ b/arch/mips/configs/ip22_defconfig
> > @@ -228,7 +228,7 @@ CONFIG_SERIAL_IP22_ZILOG=m
> > # CONFIG_HW_RANDOM is not set
> > CONFIG_RAW_DRIVER=m
> > # CONFIG_HWMON is not set
> > -CONFIG_THERMAL=m
> > +CONFIG_THERMAL=y
> > CONFIG_WATCHDOG=y
> > CONFIG_INDYDOG=m
> > # CONFIG_VGA_CONSOLE is not set
> > diff --git a/arch/mips/configs/ip27_defconfig
> > b/arch/mips/configs/ip27_defconfig
> > index 81c47e18131b..54db5dedf776 100644
> > --- a/arch/mips/configs/ip27_defconfig
> > +++ b/arch/mips/configs/ip27_defconfig
> > @@ -271,7 +271,7 @@ CONFIG_I2C_PARPORT_LIGHT=m
> > CONFIG_I2C_TAOS_EVM=m
> > CONFIG_I2C_STUB=m
> > # CONFIG_HWMON is not set
> > -CONFIG_THERMAL=m
> > +CONFIG_THERMAL=y
> > CONFIG_MFD_PCF50633=m
> > CONFIG_PCF50633_ADC=m
> > CONFIG_PCF50633_GPIO=m
> > diff --git a/arch/unicore32/configs/unicore32_defconfig
> > b/arch/unicore32/configs/unicore32_defconfig
> > index aebd01fc28e5..360cc9abcdb0 100644
> > --- a/arch/unicore32/configs/unicore32_defconfig
> > +++ b/arch/unicore32/configs/unicore32_defconfig
> > @@ -119,7 +119,7 @@ CONFIG_I2C_PUV3=y
> > # Hardware Monitoring support
> > #CONFIG_SENSORS_LM75=m
> > # Generic Thermal sysfs driver
> > -#CONFIG_THERMAL=m
> > +#CONFIG_THERMAL=y
> > #CONFIG_THERMAL_HWMON=y
> >
> > # Multimedia support
> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> > index 653aa27a25a4..ccf5b9408d7a 100644
> > --- a/drivers/thermal/Kconfig
> > +++ b/drivers/thermal/Kconfig
> > @@ -3,7 +3,7 @@
> > #
> >
> > menuconfig THERMAL
> > - tristate "Generic Thermal sysfs driver"
> > + bool "Generic Thermal sysfs driver"
> > help
> >   Generic Thermal Sysfs driver offers a generic mechanism
> > for
> >   thermal management. Usually it's made up of one or more
> > thermal
> > @@ -11,7 +11,7 @@ menuconfig THERMAL
> >   Each thermal zone contains its own temperature, trip
> > points,
> >   cooling devices.
> >   All platforms with ACPI thermal support can use this
> > driver.
> > -   If you want this support, you should say Y or M here.
> > +   If you want this support, you should say Y here.
> >
> > if THERMAL
> >

2019-04-28 23:24:31

[permalink] [raw]

Subject: Re: [PATCH 3/7] thermal/drivers/core: Add init section table for self-encapsulation

On 23/04/2019 07:59, Zhang Rui wrote:
> Hi, Daniel,
>
> thanks for clarifying.
> It is true that we need to make thermal framework ready as early as
> possible. And a static table works for me as long as vmlinux.lds.h is
> the proper place.
>
> Arnd,
> are you okay with this patch? if yes, I suppose I can take it through
> my tree, right?

Arnd?

> thanks,
> rui
>
> On 一, 2019-04-22 at 14:11 +0200, Daniel Lezcano wrote:
>> Hi Zhang,
>>
>>
>> On 22/04/2019 10:43, Zhang Rui wrote:
>>>
>>> Hi, Daniel,
>>>
>>> Thanks for the patches, it looks good to me except this one and
>>> patch
>>> 4/7.
>>>
>>> First, I don't think this is a cyclic dependency issue as they are
>>> in
>>> the same module.
>> The governors have to export their [un]register functions in order to
>> have the core to use them.
>>
>> The core has to export the [un]register function in order to have the
>> governors to use them.
>>
>> From my point of view it is a cyclic dependency. In any other
>> subsystems, the plugins/governor/drivers/whatever don't have to
>> export
>> their functions to the core, they use the core's exported functions.
>>
>>>
>>> Second, I have not read include/asm-generic/vmlinux.lds.h before,
>>> it
>>> seems that it is used for architecture specific stuff. Fix a
>>> thermal
>>> issue in this way seems overkill to me.
>> It is not architecture specific, it belongs to asm-generic. All init
>> calls are defined in it and more. It is a common way to define static
>> tables from different files without adding dependency and unload it
>> after init.
>>
>> All clk, timers, acpi tables, irq chip, cpuidle and cpu methods are
>> defined this way.
>>
>> When the thermal_core.c uses at the end fs_initcall it uses the same
>> mechanism.
>>
>>
>>>
>>> IMO, to make the code clean, we can build the governors as separate
>>> modules just like we do for cpu governors.
>>> This brings to the old commit 80a26a5c22b9("Thermal: build thermal
>>> governors into thermal_sys module"), and that was introduced to fix
>>> a
>>> problem when CONFIG_THERMAL is set to 'm'. So I think we can switch
>>> back to the old way as the problem is gone now.
>>>
>>> what do you think?
>> IMO, having the governors built as module is not a good thing because
>> the SoC needs the governor to be ready as soon as possible at boot
>> time.
>> I've been told some boards reboot at boot time because the governor
>> comes too late with the userspace governor for example.
>>
>> If you don't like the vmlinuz.lds.h approch (but again it is a common
>> way to initialize table and I wrote it to extend to more thermal
>> table
>> in the future) we can change the thermal core to replace
>> fs_initcall()
>> by core_initcall() and use postcore_initcall() in the governor.
>>
>>
>>
>>>
>>> Patch 1,2,5,6,7 applied first.
>>>
>>> thanks,
>>> rui
>>>
>>> On 二, 2019-04-02 at 18:12 +0200, Daniel Lezcano wrote:
>>>>
>>>> Currently the governors are declared in their respective files
>>>> but
>>>> they
>>>> export their [un]register functions which in turn call the
>>>> [un]register
>>>> the governors core's functions. That implies a cyclic dependency
>>>> which is
>>>> not desirable. There is a way to self-encapsulate the governors
>>>> by
>>>> letting
>>>> them to declare themselves in a __init section table.
>>>>
>>>> Define the table in the asm generic linker description like the
>>>> other
>>>> tables and provide the specific macros to deal with.
>>>>
>>>> Signed-off-by: Daniel Lezcano <[email protected]>
>>>> ---
>>>> drivers/thermal/thermal_core.h    | 16 ++++++++++++++++
>>>> include/asm-generic/vmlinux.lds.h | 11 +++++++++++
>>>> 2 files changed, 27 insertions(+)
>>>>
>>>> diff --git a/drivers/thermal/thermal_core.h
>>>> b/drivers/thermal/thermal_core.h
>>>> index 0df190ed82a7..28d18083e969 100644
>>>> --- a/drivers/thermal/thermal_core.h
>>>> +++ b/drivers/thermal/thermal_core.h
>>>> @@ -15,6 +15,22 @@
>>>> /* Initial state of a cooling device during binding */
>>>> #define THERMAL_NO_TARGET -1UL
>>>>
>>>> +/* Init section thermal table */
>>>> +extern struct thermal_governor * __governor_thermal_table[];
>>>> +extern struct thermal_governor * __governor_thermal_table_end[];
>>>> +
>>>> +#define THERMAL_TABLE_ENTRY(table, name)
>>>> \
>>>> +        static typeof(name) * __thermal_table_entry_##name
>>>> \
>>>> + __used __section(__##table##_thermal_table)
>>>> \
>>>> + = &name;
>>>> +
>>>> +#define THERMAL_GOVERNOR_DECLARE(name) THERMAL_TABLE_ENTR
>>>> Y(go
>>>> vernor, name)
>>>> +
>>>> +#define for_each_governor_table(__governor) \
>>>> + for (__governor = __governor_thermal_table; \
>>>> +      __governor < __governor_thermal_table_end; \
>>>> +      __governor++)
>>>> +
>>>> /*
>>>> * This structure is used to describe the behavior of
>>>> * a certain cooling device on a certain trip point
>>>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-
>>>> generic/vmlinux.lds.h
>>>> index f8f6f04c4453..9893a3ed242a 100644
>>>> --- a/include/asm-generic/vmlinux.lds.h
>>>> +++ b/include/asm-generic/vmlinux.lds.h
>>>> @@ -239,6 +239,16 @@
>>>> #define ACPI_PROBE_TABLE(name)
>>>> #endif
>>>>
>>>> +#ifdef CONFIG_THERMAL
>>>> +#define THERMAL_TABLE(name)
>>>>
>>>> \
>>>> +        . = ALIGN(8);
>>>> \
>>>> +        __##name##_thermal_table = .;
>>>> \
>>>> +        KEEP(*(__##name##_thermal_table))
>>>>
>>>> \
>>>> +        __##name##_thermal_table_end = .;
>>>> +#else
>>>> +#define THERMAL_TABLE(name)
>>>> +#endif
>>>> +
>>>> #define KERNEL_DTB()
>>>>
>>>> \
>>>> STRUCT_ALIGN();
>>>> \
>>>> __dtb_start = .;
>>>> \
>>>> @@ -609,6 +619,7 @@
>>>> IRQCHIP_OF_MATCH_TABLE()
>>>> \
>>>> ACPI_PROBE_TABLE(irqchip)
>>>>
>>>> \
>>>> ACPI_PROBE_TABLE(timer)
>>>> \
>>>> + THERMAL_TABLE(governor)
>>>> \
>>>> EARLYCON_TABLE()
>>>> \
>>>> LSM_TABLE()
>>>>
>>

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

2019-04-30 13:27:01

by Arnd Bergmann

[permalink] [raw]

Subject: Re: [PATCH 3/7] thermal/drivers/core: Add init section table for self-encapsulation

On Mon, Apr 29, 2019 at 1:23 AM Daniel Lezcano
<[email protected]> wrote:
>
> On 23/04/2019 07:59, Zhang Rui wrote:
> > Hi, Daniel,
> >
> > thanks for clarifying.
> > It is true that we need to make thermal framework ready as early as
> > possible. And a static table works for me as long as vmlinux.lds.h is
> > the proper place.
> >
> > Arnd,
> > are you okay with this patch? if yes, I suppose I can take it through
> > my tree, right?
>
> Arnd?

Sorry for missing this earlier. I have no objections to this, or to merging
it through the thermal tree.

Acked-by: Arnd Bergmann <[email protected]>

2019-05-03 21:16:51

[permalink] [raw]

Subject: Re: [PATCH 3/7] thermal/drivers/core: Add init section table for self-encapsulation

On 23/04/2019 07:59, Zhang Rui wrote:
> Hi, Daniel,
>
> thanks for clarifying.
> It is true that we need to make thermal framework ready as early as
> possible. And a static table works for me as long as vmlinux.lds.h is
> the proper place.
>
> Arnd,
> are you okay with this patch? if yes, I suppose I can take it through
> my tree, right?

Hi Zhang,

given the Acked-by from Arnd, will you add the missing patches in the
tree for 5.2?

> On 一, 2019-04-22 at 14:11 +0200, Daniel Lezcano wrote:
>> Hi Zhang,
>>
>>
>> On 22/04/2019 10:43, Zhang Rui wrote:
>>>
>>> Hi, Daniel,
>>>
>>> Thanks for the patches, it looks good to me except this one and
>>> patch
>>> 4/7.
>>>
>>> First, I don't think this is a cyclic dependency issue as they are
>>> in
>>> the same module.
>> The governors have to export their [un]register functions in order to
>> have the core to use them.
>>
>> The core has to export the [un]register function in order to have the
>> governors to use them.
>>
>> From my point of view it is a cyclic dependency. In any other
>> subsystems, the plugins/governor/drivers/whatever don't have to
>> export
>> their functions to the core, they use the core's exported functions.
>>
>>>
>>> Second, I have not read include/asm-generic/vmlinux.lds.h before,
>>> it
>>> seems that it is used for architecture specific stuff. Fix a
>>> thermal
>>> issue in this way seems overkill to me.
>> It is not architecture specific, it belongs to asm-generic. All init
>> calls are defined in it and more. It is a common way to define static
>> tables from different files without adding dependency and unload it
>> after init.
>>
>> All clk, timers, acpi tables, irq chip, cpuidle and cpu methods are
>> defined this way.
>>
>> When the thermal_core.c uses at the end fs_initcall it uses the same
>> mechanism.
>>
>>
>>>
>>> IMO, to make the code clean, we can build the governors as separate
>>> modules just like we do for cpu governors.
>>> This brings to the old commit 80a26a5c22b9("Thermal: build thermal
>>> governors into thermal_sys module"), and that was introduced to fix
>>> a
>>> problem when CONFIG_THERMAL is set to 'm'. So I think we can switch
>>> back to the old way as the problem is gone now.
>>>
>>> what do you think?
>> IMO, having the governors built as module is not a good thing because
>> the SoC needs the governor to be ready as soon as possible at boot
>> time.
>> I've been told some boards reboot at boot time because the governor
>> comes too late with the userspace governor for example.
>>
>> If you don't like the vmlinuz.lds.h approch (but again it is a common
>> way to initialize table and I wrote it to extend to more thermal
>> table
>> in the future) we can change the thermal core to replace
>> fs_initcall()
>> by core_initcall() and use postcore_initcall() in the governor.
>>
>>
>>
>>>
>>> Patch 1,2,5,6,7 applied first.
>>>
>>> thanks,
>>> rui
>>>
>>> On 二, 2019-04-02 at 18:12 +0200, Daniel Lezcano wrote:
>>>>
>>>> Currently the governors are declared in their respective files
>>>> but
>>>> they
>>>> export their [un]register functions which in turn call the
>>>> [un]register
>>>> the governors core's functions. That implies a cyclic dependency
>>>> which is
>>>> not desirable. There is a way to self-encapsulate the governors
>>>> by
>>>> letting
>>>> them to declare themselves in a __init section table.
>>>>
>>>> Define the table in the asm generic linker description like the
>>>> other
>>>> tables and provide the specific macros to deal with.
>>>>
>>>> Signed-off-by: Daniel Lezcano <[email protected]>
>>>> ---
>>>> drivers/thermal/thermal_core.h    | 16 ++++++++++++++++
>>>> include/asm-generic/vmlinux.lds.h | 11 +++++++++++
>>>> 2 files changed, 27 insertions(+)
>>>>
>>>> diff --git a/drivers/thermal/thermal_core.h
>>>> b/drivers/thermal/thermal_core.h
>>>> index 0df190ed82a7..28d18083e969 100644
>>>> --- a/drivers/thermal/thermal_core.h
>>>> +++ b/drivers/thermal/thermal_core.h
>>>> @@ -15,6 +15,22 @@
>>>> /* Initial state of a cooling device during binding */
>>>> #define THERMAL_NO_TARGET -1UL
>>>>
>>>> +/* Init section thermal table */
>>>> +extern struct thermal_governor * __governor_thermal_table[];
>>>> +extern struct thermal_governor * __governor_thermal_table_end[];
>>>> +
>>>> +#define THERMAL_TABLE_ENTRY(table, name)
>>>> \
>>>> +        static typeof(name) * __thermal_table_entry_##name
>>>> \
>>>> + __used __section(__##table##_thermal_table)
>>>> \
>>>> + = &name;
>>>> +
>>>> +#define THERMAL_GOVERNOR_DECLARE(name) THERMAL_TABLE_ENTR
>>>> Y(go
>>>> vernor, name)
>>>> +
>>>> +#define for_each_governor_table(__governor) \
>>>> + for (__governor = __governor_thermal_table; \
>>>> +      __governor < __governor_thermal_table_end; \
>>>> +      __governor++)
>>>> +
>>>> /*
>>>> * This structure is used to describe the behavior of
>>>> * a certain cooling device on a certain trip point
>>>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-
>>>> generic/vmlinux.lds.h
>>>> index f8f6f04c4453..9893a3ed242a 100644
>>>> --- a/include/asm-generic/vmlinux.lds.h
>>>> +++ b/include/asm-generic/vmlinux.lds.h
>>>> @@ -239,6 +239,16 @@
>>>> #define ACPI_PROBE_TABLE(name)
>>>> #endif
>>>>
>>>> +#ifdef CONFIG_THERMAL
>>>> +#define THERMAL_TABLE(name)
>>>>
>>>> \
>>>> +        . = ALIGN(8);
>>>> \
>>>> +        __##name##_thermal_table = .;
>>>> \
>>>> +        KEEP(*(__##name##_thermal_table))
>>>>
>>>> \
>>>> +        __##name##_thermal_table_end = .;
>>>> +#else
>>>> +#define THERMAL_TABLE(name)
>>>> +#endif
>>>> +
>>>> #define KERNEL_DTB()
>>>>
>>>> \
>>>> STRUCT_ALIGN();
>>>> \
>>>> __dtb_start = .;
>>>> \
>>>> @@ -609,6 +619,7 @@
>>>> IRQCHIP_OF_MATCH_TABLE()
>>>> \
>>>> ACPI_PROBE_TABLE(irqchip)
>>>>
>>>> \
>>>> ACPI_PROBE_TABLE(timer)
>>>> \
>>>> + THERMAL_TABLE(governor)
>>>> \
>>>> EARLYCON_TABLE()
>>>> \
>>>> LSM_TABLE()
>>>>
>>

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

2019-05-06 12:48:22