Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 3/7] thermal/drivers/core: Add init section table for
 self-encapsulation
To:     Zhang Rui <rui.zhang@intel.com>, edubezval@gmail.com
Cc:     linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Arnd Bergmann <arnd@arndb.de>,
        "open list:GENERIC INCLUDE/ASM HEADER FILES" 
        <linux-arch@vger.kernel.org>
References: <20190402161256.11044-1-daniel.lezcano@linaro.org>
 <20190402161256.11044-3-daniel.lezcano@linaro.org>
 <1555922585.26198.19.camel@intel.com>
From:   Daniel Lezcano <daniel.lezcano@linaro.org>
Message-ID: <fb45157c-38c4-7940-3252-af459d446323@linaro.org>
Date:   Mon, 22 Apr 2019 14:11:08 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <1555922585.26198.19.camel@intel.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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