| 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 |
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
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 > > . >
[+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 >
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 >> > > . >
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
[+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
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
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 >
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 --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