Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [RFC PATCH V5] GCOV: Add config to check the preqequisites
 situation
To:     gengcixi@gmail.com, gregkh@linuxfoundation.org, jslaby@suse.com,
        linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org
Cc:     orsonzhai@gmail.com, zhang.lyra@gmail.com,
        Cixi Geng <cixi.geng1@unisoc.com>
References: <20200610021150.19233-1-gengcixi@gmail.com>
From:   Peter Oberparleiter <oberpar@linux.ibm.com>
Message-ID: <77484d44-966a-c3a8-cb81-a2c5776dcc23@linux.ibm.com>
Date:   Mon, 15 Jun 2020 11:45:26 +0200
MIME-Version: 1.0
In-Reply-To: <20200610021150.19233-1-gengcixi@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 10.06.2020 04:11, gengcixi@gmail.com wrote:
> From: Cixi Geng <cixi.geng1@unisoc.com>
> 
> Introduce new configuration option GCOV_PROFILE_PREREQS that can be
> used to check whether the prerequisites for enabling gcov profiling
> for specific files and directories are met.
> 
> Only add SERIAL_GCOV for an example.
> 
> Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
> ---
>  drivers/tty/serial/Kconfig  |  8 ++++++++
>  drivers/tty/serial/Makefile |  1 +
>  kernel/gcov/Kconfig         | 15 +++++++++++++++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index adf9e80e7dc9..3d7e811d90dc 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1566,3 +1566,11 @@ endmenu
>  
>  config SERIAL_MCTRL_GPIO
>  	tristate
> +
> +config SERIAL_GCOV
> +	bool "Enable profile gcov for serial directory"
> +	depends on GCOV_PROFILE_PREREQS
> +	default y if GCOV_PROFILE_PREREQS

I think the choice to enable each specific profiling symbol should not
be automated based on the PREREQS symbol. This should be a purely manual
setting with a depends relation on the PREREQS.

The logic should be:
- if the requirements are met
- then provide a way for users to manually enable each specific gcov
  profiling site

Otherwise you would be duplicating the meaning of CONFIG_GCOV_PROFILE_ALL.

> +	help
> +	  The SERIAL_GCOV will add Gcov profiling flags when kernel compiles.
> +	  Say 'Y' here if you want the gcov data for the serial directory,
> diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
> index d056ee6cca33..17272733db95 100644
> --- a/drivers/tty/serial/Makefile
> +++ b/drivers/tty/serial/Makefile
> @@ -3,6 +3,7 @@
>  # Makefile for the kernel serial device drivers.
>  #
>  
> +GCOV_PROFILE := $(CONFIG_SERIAL_GCOV)
>  obj-$(CONFIG_SERIAL_CORE) += serial_core.o
>  
>  obj-$(CONFIG_SERIAL_EARLYCON) += earlycon.o
> diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
> index 3941a9c48f83..35b839879553 100644
> --- a/kernel/gcov/Kconfig
> +++ b/kernel/gcov/Kconfig
> @@ -51,6 +51,21 @@ config GCOV_PROFILE_ALL
>  	larger and run slower. Also be sure to exclude files from profiling
>  	which are not linked to the kernel image to prevent linker errors.
>  
> +config GCOV_PROFILE_PREREQS
> +	bool "Profile Kernel for prereqs"
> +	depends on !COMPILE_TEST
> +	depends on GCOV_KERNEL
> +	depends on !COMPILE_PROFILE_ALL
> +	default y if GCOV_KERNEL && !COMPILE_TEST

This mix of depends and "default if" is confusing. As I mentioned in my
previous e-mail, the "default if" should be sufficient for an automatic
symbol, so the "depends" statements can be removed.

> +	help
> +	  This options activates profiling for the specified kernel modules.
> +
> +	  When some modules need Gcov data, enable this config, then configure
> +	  with gcov on the corresponding modules,The directories or files of
> +	  these modules will be added profiling flags after kernel compile.
> +
> +	  If unsure, say N.

What reason is there for a user to manually set this to N? In my opinion
the only use case where this symbol makes sense is as an automatic
config symbol that can be used by other symbols that enable specific
GCOV profiling sites via a "depends" relation. The PREREQ symbol
indicates that is is ok to provide the manual choice of enabling such
profiling.

> +
>  choice
>  	prompt "Specify GCOV format"
>  	depends on GCOV_KERNEL
> 


-- 
Peter Oberparleiter
Linux on Z Development - IBM Germany