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

[PATCH 3/7] 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
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 <daniel.lezcano@linaro.org>
---
 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

[PATCH 3/7] 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
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 <daniel.lezcano@linaro.org>
---
 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

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

[Zhang Rui]

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 <daniel.lezcano@linaro.org>
> ---
>  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()
>  

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

[Zhang Rui]

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 <daniel.lezcano@linaro.org>
> ---
>  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()
>  

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

[Daniel Lezcano]

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 <daniel.lezcano@linaro.org>
>> ---
>>  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

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

[Daniel Lezcano]

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.

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

[Zhang Rui]

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 <daniel.lezcano@linaro.org>
> > > ---
> > >  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()
> > >  
> 

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

[Zhang Rui]

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 <daniel.lezcano@linaro.org>
> > > ---
> > >  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()
> > >  
> 

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

[Daniel Lezcano]

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 <daniel.lezcano@linaro.org>
>>>> ---
>>>>  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

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

[Daniel Lezcano]

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 <daniel.lezcano@linaro.org>
>>>> ---
>>>>  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

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

[Arnd Bergmann]

On Mon, Apr 29, 2019 at 1:23 AM Daniel Lezcano
<daniel.lezcano@linaro.org> 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 <arnd@arndb.de>

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

[Arnd Bergmann]

On Mon, Apr 29, 2019 at 1:23 AM Daniel Lezcano
<daniel.lezcano@linaro.org> 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 <arnd@arndb.de>

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

[Daniel Lezcano]

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 <daniel.lezcano@linaro.org>
>>>> ---
>>>>  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

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

[Daniel Lezcano]

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 <daniel.lezcano@linaro.org>
>>>> ---
>>>>  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

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

[Zhang Rui]

On 五, 2019-05-03 at 22:28 +0200, Daniel Lezcano 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?
> Hi Zhang,
> 
> given the Acked-by from Arnd, will you add the missing patches in the
> tree for 5.2?
> 
I got a lot of checkpatch warnings when applying this patch.
"total: 9 errors, 6 warnings, 45 lines checked"
please resend with those errors/warnings addressed.

I don't think I will include it in the pull request for this merge
window. But I will try to queue them for -rc2 as cleanups.

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 <daniel.lezcano@linaro.org>
> > > > > ---
> > > > >  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()
> > > > >  
> 

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

[Zhang Rui]

On 五, 2019-05-03 at 22:28 +0200, Daniel Lezcano 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?
> Hi Zhang,
> 
> given the Acked-by from Arnd, will you add the missing patches in the
> tree for 5.2?
> 
I got a lot of checkpatch warnings when applying this patch.
"total: 9 errors, 6 warnings, 45 lines checked"
please resend with those errors/warnings addressed.

I don't think I will include it in the pull request for this merge
window. But I will try to queue them for -rc2 as cleanups.

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 <daniel.lezcano@linaro.org>
> > > > > ---
> > > > >  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()
> > > > >  
>