diff mbox series

PCI: Use subdir-ccflags-* to inherit debug flag

Message ID 1612438215-33105-1-git-send-email-yangyicong@hisilicon.com (mailing list archive)
State Accepted
Delegated to: Bjorn Helgaas
Headers show
Series PCI: Use subdir-ccflags-* to inherit debug flag | expand

Commit Message

Yicong Yang Feb. 4, 2021, 11:30 a.m. UTC
From: Junhao He <hejunhao2@hisilicon.com>

Use subdir-ccflags-* instead of ccflags-* to inherit the debug
settings from Kconfig when traversing subdirectories.

Signed-off-by: Junhao He <hejunhao2@hisilicon.com>
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/pci/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Krzysztof Wilczyński Feb. 4, 2021, 12:28 p.m. UTC | #1
Hi Yicong,

Thank you for taking care of this!

By the way, did you have some issues with things like pr_debug() and other
things printing debug information not working correctly before?

On 21-02-04 19:30:15, Yicong Yang wrote:
> From: Junhao He <hejunhao2@hisilicon.com>
> 
> Use subdir-ccflags-* instead of ccflags-* to inherit the debug
> settings from Kconfig when traversing subdirectories.
> 
> Signed-off-by: Junhao He <hejunhao2@hisilicon.com>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/pci/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 11cc794..d62c4ac 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -36,4 +36,4 @@ obj-$(CONFIG_PCI_ENDPOINT)	+= endpoint/
>  obj-y				+= controller/
>  obj-y				+= switch/
>  
> -ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
> +subdir-ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
> -- 
> 2.8.1

Reviewed-by: Krzysztof Wilczyński <kw@linux.com>

Krzysztof
Yicong Yang Feb. 4, 2021, 1:19 p.m. UTC | #2
On 2021/2/4 20:28, Krzysztof Wilczyński wrote:
> Hi Yicong,
> 
> Thank you for taking care of this!
> 
> By the way, did you have some issues with things like pr_debug() and other
> things printing debug information not working correctly before?

i cannot remember that i have met any, so maybe they work properly. :)

> 
> On 21-02-04 19:30:15, Yicong Yang wrote:
>> From: Junhao He <hejunhao2@hisilicon.com>
>>
>> Use subdir-ccflags-* instead of ccflags-* to inherit the debug
>> settings from Kconfig when traversing subdirectories.
>>
>> Signed-off-by: Junhao He <hejunhao2@hisilicon.com>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> ---
>>  drivers/pci/Makefile | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
>> index 11cc794..d62c4ac 100644
>> --- a/drivers/pci/Makefile
>> +++ b/drivers/pci/Makefile
>> @@ -36,4 +36,4 @@ obj-$(CONFIG_PCI_ENDPOINT)	+= endpoint/
>>  obj-y				+= controller/
>>  obj-y				+= switch/
>>  
>> -ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
>> +subdir-ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
>> -- 
>> 2.8.1
> 
> Reviewed-by: Krzysztof Wilczyński <kw@linux.com>
> 
> Krzysztof
> 
> .
>
Bjorn Helgaas Feb. 4, 2021, 4:10 p.m. UTC | #3
[+cc Masahiro, Michal, linux-kbuild, linux-kernel]

On Thu, Feb 04, 2021 at 07:30:15PM +0800, Yicong Yang wrote:
> From: Junhao He <hejunhao2@hisilicon.com>
> 
> Use subdir-ccflags-* instead of ccflags-* to inherit the debug
> settings from Kconfig when traversing subdirectories.

So I guess the current behavior is:

  If CONFIG_PCI_DEBUG=y, add -DDEBUG to CFLAGS in the current
  directory, but not in any subdirectories

and the behavior after this patch is:

  If CONFIG_PCI_DEBUG=y, add -DDEBUG to CFLAGS in the current
  directory and any subdirectories

Is that right?  That makes sense to me.  I wonder if any other places
have this issue?

'git grep "^ccflags.*-DDEBUG"' finds a few cases where subdirectories
use their own debug config options, e.g.,

  drivers/i2c/Makefile:ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG
  drivers/i2c/algos/Makefile:ccflags-$(CONFIG_I2C_DEBUG_ALGO) := -DDEBUG
  drivers/i2c/busses/Makefile:ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
  drivers/i2c/muxes/Makefile:ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG

But some have subdirectories that look like they probably should be
included by using subdir-ccflags, e.g.,

  drivers/base/Makefile:ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
  drivers/base/power/Makefile:ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
    # drivers/base/{firmware_loader,regmap,test}/ not included

  drivers/hwmon/Makefile:ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
    # drivers/hwmon/{occ,pmbus}/ not included

  drivers/pps/Makefile:ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
  drivers/pps/clients/Makefile:ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
    # drivers/pps/generators/ not included

There are many more places that add -DDEBUG to ccflags-y that *don't*
have subdirectories.

I wonder the default should be that we use subdir-ccflags all the
time, and use ccflags only when we actually want different
CONFIG_*_DEBUG options for subdirectories.

> Signed-off-by: Junhao He <hejunhao2@hisilicon.com>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/pci/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 11cc794..d62c4ac 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -36,4 +36,4 @@ obj-$(CONFIG_PCI_ENDPOINT)	+= endpoint/
>  obj-y				+= controller/
>  obj-y				+= switch/
>  
> -ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
> +subdir-ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
> -- 
> 2.8.1
>
Yicong Yang Feb. 5, 2021, 1:32 a.m. UTC | #4
On 2021/2/5 0:10, Bjorn Helgaas wrote:
> [+cc Masahiro, Michal, linux-kbuild, linux-kernel]
> 
> On Thu, Feb 04, 2021 at 07:30:15PM +0800, Yicong Yang wrote:
>> From: Junhao He <hejunhao2@hisilicon.com>
>>
>> Use subdir-ccflags-* instead of ccflags-* to inherit the debug
>> settings from Kconfig when traversing subdirectories.
> 
> So I guess the current behavior is:
> 
>   If CONFIG_PCI_DEBUG=y, add -DDEBUG to CFLAGS in the current
>   directory, but not in any subdirectories
> 
> and the behavior after this patch is:
> 
>   If CONFIG_PCI_DEBUG=y, add -DDEBUG to CFLAGS in the current
>   directory and any subdirectories
> 
> Is that right?  That makes sense to me.  I wonder if any other places
> have this issue?

that's right. we didn't check other places, but some have individual config
in their sub-directory as you mentioned below.

> 
> 'git grep "^ccflags.*-DDEBUG"' finds a few cases where subdirectories
> use their own debug config options, e.g.,
> 
>   drivers/i2c/Makefile:ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG
>   drivers/i2c/algos/Makefile:ccflags-$(CONFIG_I2C_DEBUG_ALGO) := -DDEBUG
>   drivers/i2c/busses/Makefile:ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
>   drivers/i2c/muxes/Makefile:ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
> 
> But some have subdirectories that look like they probably should be
> included by using subdir-ccflags, e.g.,
> 
>   drivers/base/Makefile:ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
>   drivers/base/power/Makefile:ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
>     # drivers/base/{firmware_loader,regmap,test}/ not included
> 
>   drivers/hwmon/Makefile:ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
>     # drivers/hwmon/{occ,pmbus}/ not included
> 
>   drivers/pps/Makefile:ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
>   drivers/pps/clients/Makefile:ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
>     # drivers/pps/generators/ not included
> 
> There are many more places that add -DDEBUG to ccflags-y that *don't*
> have subdirectories.
> 
> I wonder the default should be that we use subdir-ccflags all the
> time, and use ccflags only when we actually want different
> CONFIG_*_DEBUG options for subdirectories.

agree. if there is no debug config in the sub-directory, the
config should be inherited from its parent directory using subdir-ccflags.

we can post a separate serial to issue other places.

Thanks,
Yicong

> 
>> Signed-off-by: Junhao He <hejunhao2@hisilicon.com>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> ---
>>  drivers/pci/Makefile | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
>> index 11cc794..d62c4ac 100644
>> --- a/drivers/pci/Makefile
>> +++ b/drivers/pci/Makefile
>> @@ -36,4 +36,4 @@ obj-$(CONFIG_PCI_ENDPOINT)	+= endpoint/
>>  obj-y				+= controller/
>>  obj-y				+= switch/
>>  
>> -ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
>> +subdir-ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
>> -- 
>> 2.8.1
>>
> 
> .
>
Krzysztof Wilczyński Feb. 9, 2021, 1:27 p.m. UTC | #5
Hi Yicong,

[...]
> > By the way, did you have some issues with things like pr_debug() and other
> > things printing debug information not working correctly before?
> 
> i cannot remember that i have met any, so maybe they work properly. :)
[...]

That's good to know!  I suspect, some of the pr_debug() invocations
might not be working correctly at the moment, so this is a nice
improvement.

Having said that, if you have some time, can you update the patch
against the PCI tree with the commit message from this thread?

  https://lore.kernel.org/lkml/1612868899-9185-1-git-send-email-yangyicong@hisilicon.com/

The commit messages there for the individual patches are much nicer, so
if you don't mind, it would be nice have the same one in the PCI tree.

My review still applies.

Thank you again for sending the patch over!

Krzysztof
Bjorn Helgaas Feb. 9, 2021, 9:25 p.m. UTC | #6
[+cc Masahiro, Michal, linux-kbuild, linux-kernel]

On Thu, Feb 04, 2021 at 07:30:15PM +0800, Yicong Yang wrote:
> From: Junhao He <hejunhao2@hisilicon.com>
> 
> Use subdir-ccflags-* instead of ccflags-* to inherit the debug
> settings from Kconfig when traversing subdirectories.
> 
> Signed-off-by: Junhao He <hejunhao2@hisilicon.com>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

I applied this with Krzysztof's reviewed-by and the commit log below
to pci/misc for v5.12, thanks!

Feel free to copy or improve the commit log for use elsewhere.

> ---
>  drivers/pci/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 11cc794..d62c4ac 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -36,4 +36,4 @@ obj-$(CONFIG_PCI_ENDPOINT)	+= endpoint/
>  obj-y				+= controller/
>  obj-y				+= switch/
>  
> -ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
> +subdir-ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG

commit e8e9aababe60 ("PCI: Apply CONFIG_PCI_DEBUG to entire drivers/pci hierarchy")
Author: Junhao He <hejunhao2@hisilicon.com>
Date:   Thu Feb 4 19:30:15 2021 +0800

    PCI: Apply CONFIG_PCI_DEBUG to entire drivers/pci hierarchy
    
    CONFIG_PCI_DEBUG=y adds -DDEBUG to CFLAGS, which enables things like
    pr_debug() and dev_dbg() (and hence pci_dbg()).  Previously we added
    -DDEBUG for files in drivers/pci/, but not files in subdirectories of
    drivers/pci/.
    
    Add -DDEBUG to CFLAGS for all files below drivers/pci/ so CONFIG_PCI_DEBUG
    applies to the entire hierarchy.
    
    [bhelgaas: commit log]
    Link: https://lore.kernel.org/r/1612438215-33105-1-git-send-email-yangyicong@hisilicon.com
    Signed-off-by: Junhao He <hejunhao2@hisilicon.com>
    Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Reviewed-by: Krzysztof Wilczyński <kw@linux.com>

diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 11cc79411e2d..d62c4ac4ae1b 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -36,4 +36,4 @@ obj-$(CONFIG_PCI_ENDPOINT)	+= endpoint/
 obj-y				+= controller/
 obj-y				+= switch/
 
-ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
+subdir-ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
Krzysztof Wilczyński Feb. 9, 2021, 11:43 p.m. UTC | #7
Hi Bjorn,

Thank you!  This looks great!

[...]
> commit e8e9aababe60 ("PCI: Apply CONFIG_PCI_DEBUG to entire drivers/pci hierarchy")
> Author: Junhao He <hejunhao2@hisilicon.com>
> Date:   Thu Feb 4 19:30:15 2021 +0800
> 
>     PCI: Apply CONFIG_PCI_DEBUG to entire drivers/pci hierarchy
>     
>     CONFIG_PCI_DEBUG=y adds -DDEBUG to CFLAGS, which enables things like
>     pr_debug() and dev_dbg() (and hence pci_dbg()).  Previously we added
>     -DDEBUG for files in drivers/pci/, but not files in subdirectories of
>     drivers/pci/.
>     
>     Add -DDEBUG to CFLAGS for all files below drivers/pci/ so CONFIG_PCI_DEBUG
>     applies to the entire hierarchy.
>     
>     [bhelgaas: commit log]
>     Link: https://lore.kernel.org/r/1612438215-33105-1-git-send-email-yangyicong@hisilicon.com
>     Signed-off-by: Junhao He <hejunhao2@hisilicon.com>
>     Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>     Reviewed-by: Krzysztof Wilczyński <kw@linux.com>
> 
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 11cc79411e2d..d62c4ac4ae1b 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -36,4 +36,4 @@ obj-$(CONFIG_PCI_ENDPOINT)	+= endpoint/
>  obj-y				+= controller/
>  obj-y				+= switch/
>  
> -ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
> +subdir-ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG

And thank you again, Yicong, for fixing this.  Much appreciated.

Krzysztof
Yicong Yang Feb. 10, 2021, 9:29 a.m. UTC | #8
On 2021/2/9 21:27, Krzysztof Wilczyński wrote:
> Hi Yicong,
> 
> [...]
>>> By the way, did you have some issues with things like pr_debug() and other
>>> things printing debug information not working correctly before?
>>
>> i cannot remember that i have met any, so maybe they work properly. :)
> [...]
> 
> That's good to know!  I suspect, some of the pr_debug() invocations
> might not be working correctly at the moment, so this is a nice
> improvement.
> 
> Having said that, if you have some time, can you update the patch
> against the PCI tree with the commit message from this thread?
> 
>   https://lore.kernel.org/lkml/1612868899-9185-1-git-send-email-yangyicong@hisilicon.com/
> 
> The commit messages there for the individual patches are much nicer, so
> if you don't mind, it would be nice have the same one in the PCI tree.
> 
> My review still applies.
> 
> Thank you again for sending the patch over!

Bjorn has applied this with nicer commit. Thanks for reviewing this. :)

Regards,
Yicong

> 
> Krzysztof
> _______________________________________________
> Linuxarm mailing list -- linuxarm@openeuler.org
> To unsubscribe send an email to linuxarm-leave@openeuler.org
>
Yicong Yang Feb. 10, 2021, 9:33 a.m. UTC | #9
On 2021/2/10 5:25, Bjorn Helgaas wrote:
> [+cc Masahiro, Michal, linux-kbuild, linux-kernel]
> 
> On Thu, Feb 04, 2021 at 07:30:15PM +0800, Yicong Yang wrote:
>> From: Junhao He <hejunhao2@hisilicon.com>
>>
>> Use subdir-ccflags-* instead of ccflags-* to inherit the debug
>> settings from Kconfig when traversing subdirectories.
>>
>> Signed-off-by: Junhao He <hejunhao2@hisilicon.com>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> 
> I applied this with Krzysztof's reviewed-by and the commit log below
> to pci/misc for v5.12, thanks!
> 
> Feel free to copy or improve the commit log for use elsewhere.
> 

thanks for improving the commit. i admit that i didn't make the it
clear enough. it's much better now.

Thanks,
Yicong

>> ---
>>  drivers/pci/Makefile | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
>> index 11cc794..d62c4ac 100644
>> --- a/drivers/pci/Makefile
>> +++ b/drivers/pci/Makefile
>> @@ -36,4 +36,4 @@ obj-$(CONFIG_PCI_ENDPOINT)	+= endpoint/
>>  obj-y				+= controller/
>>  obj-y				+= switch/
>>  
>> -ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
>> +subdir-ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
> 
> commit e8e9aababe60 ("PCI: Apply CONFIG_PCI_DEBUG to entire drivers/pci hierarchy")
> Author: Junhao He <hejunhao2@hisilicon.com>
> Date:   Thu Feb 4 19:30:15 2021 +0800
> 
>     PCI: Apply CONFIG_PCI_DEBUG to entire drivers/pci hierarchy
>     
>     CONFIG_PCI_DEBUG=y adds -DDEBUG to CFLAGS, which enables things like
>     pr_debug() and dev_dbg() (and hence pci_dbg()).  Previously we added
>     -DDEBUG for files in drivers/pci/, but not files in subdirectories of
>     drivers/pci/.
>     
>     Add -DDEBUG to CFLAGS for all files below drivers/pci/ so CONFIG_PCI_DEBUG
>     applies to the entire hierarchy.
>     
>     [bhelgaas: commit log]
>     Link: https://lore.kernel.org/r/1612438215-33105-1-git-send-email-yangyicong@hisilicon.com
>     Signed-off-by: Junhao He <hejunhao2@hisilicon.com>
>     Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>     Reviewed-by: Krzysztof Wilczyński <kw@linux.com>
> 
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 11cc79411e2d..d62c4ac4ae1b 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -36,4 +36,4 @@ obj-$(CONFIG_PCI_ENDPOINT)	+= endpoint/
>  obj-y				+= controller/
>  obj-y				+= switch/
>  
> -ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
> +subdir-ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
> 
> .
>
diff mbox series

Patch

diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 11cc794..d62c4ac 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -36,4 +36,4 @@  obj-$(CONFIG_PCI_ENDPOINT)	+= endpoint/
 obj-y				+= controller/
 obj-y				+= switch/
 
-ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
+subdir-ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG