[PATCH 1/2] thermal/drivers/core: Add init section table for self-encapsulation

[PATCH 1/2] thermal/drivers/core: Add init section table for self-encapsulation

[Daniel Lezcano]

Currently the governors are declared in their respective files but they
export their [un]register functions which in turn call the [un]register
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.

Reviewed-by: Amit Kucheria <amit.kucheria@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.h    | 15 +++++++++++++++
 include/asm-generic/vmlinux.lds.h | 11 +++++++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 0df190ed82a7..be901e84aa65 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -15,6 +15,21 @@
 /* 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..8312fdc2b2fa 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

[PATCH 2/2] thermal/drivers/core: Use governor table to initialize

[Daniel Lezcano]

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.

Reviewed-by: Amit Kucheria <amit.kucheria@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 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    | 52 +++++++++++++++++--------------
 drivers/thermal/thermal_core.h    | 40 ------------------------
 drivers/thermal/user_space.c      | 12 +------
 7 files changed, 34 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 3ac0e2b564e2..533530529607 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -243,36 +243,42 @@ 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;
 
-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();
+		for_each_governor_table(gov) {
+			if (gov == governor)
+				break;
+			thermal_unregister_governor(*gov);
+		}
+	}
+
+	return ret;
 }
 
 /*
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index be901e84aa65..cb423b97ef01 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -89,46 +89,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

Re: [PATCH 1/2] thermal/drivers/core: Add init section table for self-encapsulation

[Daniel Lezcano]

Any chance this patch gets merged for v5.4?

Thanks
  -- Daniel

On 12/06/2019 22:13, 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
> 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.
> 
> Reviewed-by: Amit Kucheria <amit.kucheria@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/thermal_core.h    | 15 +++++++++++++++
>  include/asm-generic/vmlinux.lds.h | 11 +++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index 0df190ed82a7..be901e84aa65 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -15,6 +15,21 @@
>  /* 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..8312fdc2b2fa 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

Re: [PATCH 1/2] thermal/drivers/core: Add init section table for self-encapsulation

[Zhang Rui]

On 一, 2019-06-24 at 09:32 +0200, Daniel Lezcano wrote:
> Any chance this patch gets merged for v5.4?
> 
> Thanks
>   -- Daniel
> 

have you run compile test for the patch?
I got the following errors when compiling.

In file included from drivers/thermal/fair_share.c:16:0:
drivers/thermal/thermal_core.h:23:3: error: expected identifier or ‘(’
before ‘static’
  (static typeof(name) *__thermal_table_entry_##name \
   ^
drivers/thermal/thermal_core.h:26:40: note: in expansion of macro
‘THERMAL_TABLE_ENTRY’
 #define THERMAL_GOVERNOR_DECLARE(name) THERMAL_TABLE_ENTRY(governor,
name)
                                        ^
drivers/thermal/fair_share.c:120:1: note: in expansion of macro
‘THERMAL_GOVERNOR_DECLARE’
 THERMAL_GOVERNOR_DECLARE(thermal_gov_fair_share);
 ^
drivers/thermal/fair_share.c:116:32: warning: ‘thermal_gov_fair_share’
defined but not used [-Wunused-variable]
 static struct thermal_governor thermal_gov_fair_share = {
                                ^
make[2]: *** [drivers/thermal/fair_share.o] Error 1
make[2]: *** Waiting for unfinished jobs....
In file included from drivers/thermal/gov_bang_bang.c:14:0:
drivers/thermal/thermal_core.h:23:3: error: expected identifier or ‘(’
before ‘static’
  (static typeof(name) *__thermal_table_entry_##name \
   ^
drivers/thermal/thermal_core.h:26:40: note: in expansion of macro
‘THERMAL_TABLE_ENTRY’
 #define THERMAL_GOVERNOR_DECLARE(name) THERMAL_TABLE_ENTRY(governor,
name)
                                        ^
drivers/thermal/gov_bang_bang.c:119:1: note: in expansion of macro
‘THERMAL_GOVERNOR_DECLARE’
 THERMAL_GOVERNOR_DECLARE(thermal_gov_bang_bang);
 ^
drivers/thermal/gov_bang_bang.c:115:32: warning:
‘thermal_gov_bang_bang’ defined but not used [-Wunused-variable]
 static struct thermal_governor thermal_gov_bang_bang = {
                                ^
make[2]: *** [drivers/thermal/gov_bang_bang.o] Error 1
make[1]: *** [drivers/thermal] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [drivers] Error 2

Fix the problem by removing the round brackets
of THERMAL_TABLE_ENTRY(), and applied.

thanks,
rui
> On 12/06/2019 22:13, 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
> > 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.
> > 
> > Reviewed-by: Amit Kucheria <amit.kucheria@linaro.org>
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > ---
> >  drivers/thermal/thermal_core.h    | 15 +++++++++++++++
> >  include/asm-generic/vmlinux.lds.h | 11 +++++++++++
> >  2 files changed, 26 insertions(+)
> > 
> > diff --git a/drivers/thermal/thermal_core.h
> > b/drivers/thermal/thermal_core.h
> > index 0df190ed82a7..be901e84aa65 100644
> > --- a/drivers/thermal/thermal_core.h
> > +++ b/drivers/thermal/thermal_core.h
> > @@ -15,6 +15,21 @@
> >  /* 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..8312fdc2b2fa 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()
> >  
> > 
> 

Re: [PATCH 1/2] thermal/drivers/core: Add init section table for self-encapsulation

[Daniel Lezcano]

On 27/06/2019 15:21, Zhang Rui wrote:
> On 一, 2019-06-24 at 09:32 +0200, Daniel Lezcano wrote:
>> Any chance this patch gets merged for v5.4?
>>
>> Thanks
>>   -- Daniel
>>
> 
> have you run compile test for the patch?
> I got the following errors when compiling.

Yes I did and also booted, changed the governor at runtime, etc ... I
already got this error and fixed it. I probably forgot to fold or commit
the fix ... :/


> In file included from drivers/thermal/fair_share.c:16:0:
> drivers/thermal/thermal_core.h:23:3: error: expected identifier or ‘(’
> before ‘static’
>   (static typeof(name) *__thermal_table_entry_##name \
>    ^
> drivers/thermal/thermal_core.h:26:40: note: in expansion of macro
> ‘THERMAL_TABLE_ENTRY’
>  #define THERMAL_GOVERNOR_DECLARE(name) THERMAL_TABLE_ENTRY(governor,
> name)
>                                         ^

[ ... ]

>                                 ^
> make[2]: *** [drivers/thermal/gov_bang_bang.o] Error 1
> make[1]: *** [drivers/thermal] Error 2
> make[1]: *** Waiting for unfinished jobs....
> make: *** [drivers] Error 2
> 
> Fix the problem by removing the round brackets
> of THERMAL_TABLE_ENTRY(), and applied.

Ok, thanks for fixing it!

  -- Daniel



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