* [PATCH v2] export: fix string handling of namespace in EXPORT_SYMBOL_NS @ 2022-04-27 9:04 Greg Kroah-Hartman 2022-04-27 14:29 ` Masahiro Yamada 0 siblings, 1 reply; 9+ messages in thread From: Greg Kroah-Hartman @ 2022-04-27 9:04 UTC (permalink / raw) To: linux-kernel Cc: Greg Kroah-Hartman, Miroslav Benes, Emil Velikov, Jessica Yu, Quentin Perret, Masahiro Yamada, Matthias Maennich Commit c3a6cf19e695 ("export: avoid code duplication in include/linux/export.h") broke the ability for a defined string to be used as a namespace value. Fix this up by using stringify to properly encode the namespace name. Fixes: c3a6cf19e695 ("export: avoid code duplication in include/linux/export.h") Cc: Miroslav Benes <mbenes@suse.cz> Cc: Emil Velikov <emil.l.velikov@gmail.com> Cc: Jessica Yu <jeyu@kernel.org> Cc: Quentin Perret <qperret@google.com> Cc: Masahiro Yamada <yamada.masahiro@socionext.com> Cc: Matthias Maennich <maennich@google.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- v2: use stringify() instead of 2 step redirection as pointed out by Masahiro include/linux/export.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/include/linux/export.h b/include/linux/export.h index 27d848712b90..5910ccb66ca2 100644 --- a/include/linux/export.h +++ b/include/linux/export.h @@ -2,6 +2,8 @@ #ifndef _LINUX_EXPORT_H #define _LINUX_EXPORT_H +#include <linux/stringify.h> + /* * Export symbols from the kernel to modules. Forked from module.h * to reduce the amount of pointless cruft we feed to gcc when only @@ -154,7 +156,6 @@ struct kernel_symbol { #endif /* CONFIG_MODULES */ #ifdef DEFAULT_SYMBOL_NAMESPACE -#include <linux/stringify.h> #define _EXPORT_SYMBOL(sym, sec) __EXPORT_SYMBOL(sym, sec, __stringify(DEFAULT_SYMBOL_NAMESPACE)) #else #define _EXPORT_SYMBOL(sym, sec) __EXPORT_SYMBOL(sym, sec, "") @@ -162,8 +163,8 @@ struct kernel_symbol { #define EXPORT_SYMBOL(sym) _EXPORT_SYMBOL(sym, "") #define EXPORT_SYMBOL_GPL(sym) _EXPORT_SYMBOL(sym, "_gpl") -#define EXPORT_SYMBOL_NS(sym, ns) __EXPORT_SYMBOL(sym, "", #ns) -#define EXPORT_SYMBOL_NS_GPL(sym, ns) __EXPORT_SYMBOL(sym, "_gpl", #ns) +#define EXPORT_SYMBOL_NS(sym, ns) __EXPORT_SYMBOL(sym, "", __stringify(ns)) +#define EXPORT_SYMBOL_NS_GPL(sym, ns) __EXPORT_SYMBOL(sym, "_gpl", __stringify(ns)) #endif /* !__ASSEMBLY__ */ -- 2.36.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] export: fix string handling of namespace in EXPORT_SYMBOL_NS 2022-04-27 9:04 [PATCH v2] export: fix string handling of namespace in EXPORT_SYMBOL_NS Greg Kroah-Hartman @ 2022-04-27 14:29 ` Masahiro Yamada 2022-04-27 14:50 ` Greg Kroah-Hartman 0 siblings, 1 reply; 9+ messages in thread From: Masahiro Yamada @ 2022-04-27 14:29 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Linux Kernel Mailing List, Miroslav Benes, Emil Velikov, Jessica Yu, Quentin Perret, Matthias Maennich On Wed, Apr 27, 2022 at 6:06 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > Commit c3a6cf19e695 ("export: avoid code duplication in > include/linux/export.h") broke the ability for a defined string to be > used as a namespace value. In hindsight, this was a bad idea. EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, "SPI_DW_CORE") is much much better than: EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, SPI_DW_CORE) ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=USB_STORAGE is also a bad idea. When you look at EXPORT_SYMBOL_GPL() in C files, you will not be aware of the presence of the namespace. Anyway, it is presumably too late to fix it. > Fix this up by using stringify to properly > encode the namespace name. > > Fixes: c3a6cf19e695 ("export: avoid code duplication in include/linux/export.h") > Cc: Miroslav Benes <mbenes@suse.cz> > Cc: Emil Velikov <emil.l.velikov@gmail.com> > Cc: Jessica Yu <jeyu@kernel.org> > Cc: Quentin Perret <qperret@google.com> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com> This email is no longer valid. Feel free to replace it with Reviewed-by: Masahiro Yamada <masahiroy@kernel.org> > Cc: Matthias Maennich <maennich@google.com> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > v2: use stringify() instead of 2 step redirection as pointed out by Masahiro > > include/linux/export.h | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/include/linux/export.h b/include/linux/export.h > index 27d848712b90..5910ccb66ca2 100644 > --- a/include/linux/export.h > +++ b/include/linux/export.h > @@ -2,6 +2,8 @@ > #ifndef _LINUX_EXPORT_H > #define _LINUX_EXPORT_H > > +#include <linux/stringify.h> > + > /* > * Export symbols from the kernel to modules. Forked from module.h > * to reduce the amount of pointless cruft we feed to gcc when only > @@ -154,7 +156,6 @@ struct kernel_symbol { > #endif /* CONFIG_MODULES */ > > #ifdef DEFAULT_SYMBOL_NAMESPACE > -#include <linux/stringify.h> > #define _EXPORT_SYMBOL(sym, sec) __EXPORT_SYMBOL(sym, sec, __stringify(DEFAULT_SYMBOL_NAMESPACE)) > #else > #define _EXPORT_SYMBOL(sym, sec) __EXPORT_SYMBOL(sym, sec, "") > @@ -162,8 +163,8 @@ struct kernel_symbol { > > #define EXPORT_SYMBOL(sym) _EXPORT_SYMBOL(sym, "") > #define EXPORT_SYMBOL_GPL(sym) _EXPORT_SYMBOL(sym, "_gpl") > -#define EXPORT_SYMBOL_NS(sym, ns) __EXPORT_SYMBOL(sym, "", #ns) > -#define EXPORT_SYMBOL_NS_GPL(sym, ns) __EXPORT_SYMBOL(sym, "_gpl", #ns) > +#define EXPORT_SYMBOL_NS(sym, ns) __EXPORT_SYMBOL(sym, "", __stringify(ns)) > +#define EXPORT_SYMBOL_NS_GPL(sym, ns) __EXPORT_SYMBOL(sym, "_gpl", __stringify(ns)) > > #endif /* !__ASSEMBLY__ */ > > -- > 2.36.0 > -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] export: fix string handling of namespace in EXPORT_SYMBOL_NS 2022-04-27 14:29 ` Masahiro Yamada @ 2022-04-27 14:50 ` Greg Kroah-Hartman 2022-04-27 15:11 ` Masahiro Yamada 2022-05-23 18:31 ` Masahiro Yamada 0 siblings, 2 replies; 9+ messages in thread From: Greg Kroah-Hartman @ 2022-04-27 14:50 UTC (permalink / raw) To: Masahiro Yamada Cc: Linux Kernel Mailing List, Miroslav Benes, Emil Velikov, Jessica Yu, Quentin Perret, Matthias Maennich On Wed, Apr 27, 2022 at 11:29:19PM +0900, Masahiro Yamada wrote: > On Wed, Apr 27, 2022 at 6:06 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > Commit c3a6cf19e695 ("export: avoid code duplication in > > include/linux/export.h") broke the ability for a defined string to be > > used as a namespace value. > > In hindsight, this was a bad idea. > > > EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, "SPI_DW_CORE") > > is much much better than: > > EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, SPI_DW_CORE) I agree, but it's really not that big of a deal. We could change it if you want. > ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=USB_STORAGE > > is also a bad idea. That's not such a bad idea as it lets you set a namespace for a directory and below easily. What would you want to use instead? > When you look at EXPORT_SYMBOL_GPL() in C files, you will not be > aware of the presence of the namespace. It's easy to tell when things do not link properly :) > Anyway, it is presumably too late to fix it. Not really, the number of in-kernel users are still small and can be changed if you like. External users can update when they hit the change as well, not a big deal. Other than using a string for the namespace definition, what would you like to see done differently? > > Fix this up by using stringify to properly > > encode the namespace name. > > > > Fixes: c3a6cf19e695 ("export: avoid code duplication in include/linux/export.h") > > Cc: Miroslav Benes <mbenes@suse.cz> > > Cc: Emil Velikov <emil.l.velikov@gmail.com> > > Cc: Jessica Yu <jeyu@kernel.org> > > Cc: Quentin Perret <qperret@google.com> > > Cc: Masahiro Yamada <yamada.masahiro@socionext.com> > > This email is no longer valid. Ah, sorry about that. > Feel free to replace it with > > Reviewed-by: Masahiro Yamada <masahiroy@kernel.org> Will do, thanks! greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] export: fix string handling of namespace in EXPORT_SYMBOL_NS 2022-04-27 14:50 ` Greg Kroah-Hartman @ 2022-04-27 15:11 ` Masahiro Yamada 2022-04-27 16:21 ` Greg Kroah-Hartman 2022-05-23 18:31 ` Masahiro Yamada 1 sibling, 1 reply; 9+ messages in thread From: Masahiro Yamada @ 2022-04-27 15:11 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Linux Kernel Mailing List, Miroslav Benes, Emil Velikov, Jessica Yu, Quentin Perret, Matthias Maennich On Wed, Apr 27, 2022 at 11:50 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Apr 27, 2022 at 11:29:19PM +0900, Masahiro Yamada wrote: > > On Wed, Apr 27, 2022 at 6:06 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > Commit c3a6cf19e695 ("export: avoid code duplication in > > > include/linux/export.h") broke the ability for a defined string to be > > > used as a namespace value. > > > > In hindsight, this was a bad idea. > > > > > > EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, "SPI_DW_CORE") > > > > is much much better than: > > > > EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, SPI_DW_CORE) > > I agree, but it's really not that big of a deal. We could change it if > you want. > > > ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=USB_STORAGE > > > > is also a bad idea. > > That's not such a bad idea as it lets you set a namespace for a > directory and below easily. What would you want to use instead? I like "explicit" better than "implicit". With EXPORT_SYMBOL_GPL(usb_stor_resume); you need to check Makefile to know whether it is global or is in a specific namespace. EXPORT_SYMBOL_NS_GPL(usb_stor_resume, "USB_STORAGE"); will clarify the namespace at a glance. > > When you look at EXPORT_SYMBOL_GPL() in C files, you will not be > > aware of the presence of the namespace. > > It's easy to tell when things do not link properly :) Right, modpost will tell you that once you compile it. > > > Anyway, it is presumably too late to fix it. > > Not really, the number of in-kernel users are still small and can be > changed if you like. External users can update when they hit the change > as well, not a big deal. > > Other than using a string for the namespace definition, what would you > like to see done differently? Nothing else. (but I am not a big fan of the module namespace itself.) > > > > Fix this up by using stringify to properly > > > encode the namespace name. > > > > > > Fixes: c3a6cf19e695 ("export: avoid code duplication in include/linux/export.h") > > > Cc: Miroslav Benes <mbenes@suse.cz> > > > Cc: Emil Velikov <emil.l.velikov@gmail.com> > > > Cc: Jessica Yu <jeyu@kernel.org> > > > Cc: Quentin Perret <qperret@google.com> > > > Cc: Masahiro Yamada <yamada.masahiro@socionext.com> > > > > This email is no longer valid. > > Ah, sorry about that. > > > Feel free to replace it with > > > > Reviewed-by: Masahiro Yamada <masahiroy@kernel.org> > > Will do, thanks! > > greg k-h -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] export: fix string handling of namespace in EXPORT_SYMBOL_NS 2022-04-27 15:11 ` Masahiro Yamada @ 2022-04-27 16:21 ` Greg Kroah-Hartman 0 siblings, 0 replies; 9+ messages in thread From: Greg Kroah-Hartman @ 2022-04-27 16:21 UTC (permalink / raw) To: Masahiro Yamada Cc: Linux Kernel Mailing List, Miroslav Benes, Emil Velikov, Jessica Yu, Quentin Perret, Matthias Maennich On Thu, Apr 28, 2022 at 12:11:13AM +0900, Masahiro Yamada wrote: > On Wed, Apr 27, 2022 at 11:50 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Wed, Apr 27, 2022 at 11:29:19PM +0900, Masahiro Yamada wrote: > > > On Wed, Apr 27, 2022 at 6:06 PM Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > Commit c3a6cf19e695 ("export: avoid code duplication in > > > > include/linux/export.h") broke the ability for a defined string to be > > > > used as a namespace value. > > > > > > In hindsight, this was a bad idea. > > > > > > > > > EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, "SPI_DW_CORE") > > > > > > is much much better than: > > > > > > EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, SPI_DW_CORE) > > > > I agree, but it's really not that big of a deal. We could change it if > > you want. > > > > > ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=USB_STORAGE > > > > > > is also a bad idea. > > > > That's not such a bad idea as it lets you set a namespace for a > > directory and below easily. What would you want to use instead? > > I like "explicit" better than "implicit". > > With EXPORT_SYMBOL_GPL(usb_stor_resume); > you need to check Makefile to know whether it is global > or is in a specific namespace. > > EXPORT_SYMBOL_NS_GPL(usb_stor_resume, "USB_STORAGE"); > will clarify the namespace at a glance. I agree, but currently I use the makefile change in a big out-of-tree project to do "all exported symbols in this directory and lower are now in this namespace". That's a huge help and makes for a 2 line change vs. a hundreds-of-lines change. > > > When you look at EXPORT_SYMBOL_GPL() in C files, you will not be > > > aware of the presence of the namespace. > > > > It's easy to tell when things do not link properly :) > > Right, modpost will tell you that once you compile it. And that's the best thing, 'make nsdeps' will then even generate a patch to fix things up. > > > Anyway, it is presumably too late to fix it. > > > > Not really, the number of in-kernel users are still small and can be > > changed if you like. External users can update when they hit the change > > as well, not a big deal. > > > > Other than using a string for the namespace definition, what would you > > like to see done differently? > > Nothing else. > > (but I am not a big fan of the module namespace itself.) I think more subsystems need to start using them as it instantly shows where some drivers are doing things that maybe they shouldn't be doing. It was insightful when the dma-buf code moved to a module namespace as it found places the maintainers were not even aware of. Anyway, it's on my long-term todo list... thanks, greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] export: fix string handling of namespace in EXPORT_SYMBOL_NS 2022-04-27 14:50 ` Greg Kroah-Hartman 2022-04-27 15:11 ` Masahiro Yamada @ 2022-05-23 18:31 ` Masahiro Yamada 2022-05-24 8:10 ` Greg Kroah-Hartman 1 sibling, 1 reply; 9+ messages in thread From: Masahiro Yamada @ 2022-05-23 18:31 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Linux Kernel Mailing List, Miroslav Benes, Emil Velikov, Jessica Yu, Quentin Perret, Matthias Maennich On Wed, Apr 27, 2022 at 11:50 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Apr 27, 2022 at 11:29:19PM +0900, Masahiro Yamada wrote: > > On Wed, Apr 27, 2022 at 6:06 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > Commit c3a6cf19e695 ("export: avoid code duplication in > > > include/linux/export.h") broke the ability for a defined string to be > > > used as a namespace value. > > > > In hindsight, this was a bad idea. > > > > > > EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, "SPI_DW_CORE") > > > > is much much better than: > > > > EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, SPI_DW_CORE) > > I agree, but it's really not that big of a deal. We could change it if > you want. > > > ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=USB_STORAGE > > > > is also a bad idea. > > That's not such a bad idea as it lets you set a namespace for a > directory and below easily. What would you want to use instead? > > > When you look at EXPORT_SYMBOL_GPL() in C files, you will not be > > aware of the presence of the namespace. > > It's easy to tell when things do not link properly :) > > > Anyway, it is presumably too late to fix it. > > Not really, the number of in-kernel users are still small and can be > changed if you like. External users can update when they hit the change > as well, not a big deal. If I send a patch for global conversion, will you be happy to pick it up? I think this should be applied at the very last moment of the MW. I can convert tree-wide by sed. Example of conversion: EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, SPI_DW_CORE) --> EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, "SPI_DW_CORE") > Other than using a string for the namespace definition, what would you > like to see done differently? > > > > Fix this up by using stringify to properly > > > encode the namespace name. > > > > > > Fixes: c3a6cf19e695 ("export: avoid code duplication in include/linux/export.h") > > > Cc: Miroslav Benes <mbenes@suse.cz> > > > Cc: Emil Velikov <emil.l.velikov@gmail.com> > > > Cc: Jessica Yu <jeyu@kernel.org> > > > Cc: Quentin Perret <qperret@google.com> > > > Cc: Masahiro Yamada <yamada.masahiro@socionext.com> > > > > This email is no longer valid. > > Ah, sorry about that. > > > Feel free to replace it with > > > > Reviewed-by: Masahiro Yamada <masahiroy@kernel.org> > > Will do, thanks! > > greg k-h -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] export: fix string handling of namespace in EXPORT_SYMBOL_NS 2022-05-23 18:31 ` Masahiro Yamada @ 2022-05-24 8:10 ` Greg Kroah-Hartman 2022-05-31 7:44 ` Masahiro Yamada 0 siblings, 1 reply; 9+ messages in thread From: Greg Kroah-Hartman @ 2022-05-24 8:10 UTC (permalink / raw) To: Masahiro Yamada Cc: Linux Kernel Mailing List, Miroslav Benes, Emil Velikov, Jessica Yu, Quentin Perret, Matthias Maennich On Tue, May 24, 2022 at 03:31:59AM +0900, Masahiro Yamada wrote: > On Wed, Apr 27, 2022 at 11:50 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Wed, Apr 27, 2022 at 11:29:19PM +0900, Masahiro Yamada wrote: > > > On Wed, Apr 27, 2022 at 6:06 PM Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > Commit c3a6cf19e695 ("export: avoid code duplication in > > > > include/linux/export.h") broke the ability for a defined string to be > > > > used as a namespace value. > > > > > > In hindsight, this was a bad idea. > > > > > > > > > EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, "SPI_DW_CORE") > > > > > > is much much better than: > > > > > > EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, SPI_DW_CORE) > > > > I agree, but it's really not that big of a deal. We could change it if > > you want. > > > > > ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=USB_STORAGE > > > > > > is also a bad idea. > > > > That's not such a bad idea as it lets you set a namespace for a > > directory and below easily. What would you want to use instead? > > > > > When you look at EXPORT_SYMBOL_GPL() in C files, you will not be > > > aware of the presence of the namespace. > > > > It's easy to tell when things do not link properly :) > > > > > Anyway, it is presumably too late to fix it. > > > > Not really, the number of in-kernel users are still small and can be > > changed if you like. External users can update when they hit the change > > as well, not a big deal. > > > If I send a patch for global conversion, > will you be happy to pick it up? > > > I think this should be applied at the very last moment > of the MW. Make up a patch right after -rc1 to get into -rc2 as that's usually the best time for tree-wide changes like this. > I can convert tree-wide by sed. > > > Example of conversion: > > EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, SPI_DW_CORE) > --> > EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, "SPI_DW_CORE") Ah, but then this breaks my use case where I did in the Android tree: EXPORT_SYMBOL_NS_GPL(symbol_name, ANDROID_GKI_VFS_EXPORT_ONLY); and then in the makefile I did: subdir-ccflags-y += -DANDROID_GKI_VFS_EXPORT_ONLY=VFS_internal_I_am_really_a_filesystem_and_am_NOT_a_driver That saved me typing a lot of "VFS_internal_I_am..." in the .c files. I know out-of-tree stuff does not matter, but a simple way to make this work would be nice. I could just do: #define ANDROID_GKI_VFS_EXPORT_ONLY "VFS_internal_I_am..." in a .c file which would handle this, right? Ok, so maybe that isn't an issue, nevermind... But, if we have a string here, what happens if someone puts a space in it: EXPORT_SYMBOL_NS_GPL(symbol_name, "MY DRIVER NAMESPACE"); that will not break the build but will cause the tools to really complain, right? Or do the wrong thing and put them in the "MY" namespace? thanks, greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] export: fix string handling of namespace in EXPORT_SYMBOL_NS 2022-05-24 8:10 ` Greg Kroah-Hartman @ 2022-05-31 7:44 ` Masahiro Yamada 2022-05-31 10:36 ` Masahiro Yamada 0 siblings, 1 reply; 9+ messages in thread From: Masahiro Yamada @ 2022-05-31 7:44 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Linux Kernel Mailing List, Miroslav Benes, Emil Velikov, Jessica Yu, Quentin Perret, Matthias Maennich On Tue, May 24, 2022 at 5:10 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Tue, May 24, 2022 at 03:31:59AM +0900, Masahiro Yamada wrote: > > On Wed, Apr 27, 2022 at 11:50 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Wed, Apr 27, 2022 at 11:29:19PM +0900, Masahiro Yamada wrote: > > > > On Wed, Apr 27, 2022 at 6:06 PM Greg Kroah-Hartman > > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > Commit c3a6cf19e695 ("export: avoid code duplication in > > > > > include/linux/export.h") broke the ability for a defined string to be > > > > > used as a namespace value. > > > > > > > > In hindsight, this was a bad idea. > > > > > > > > > > > > EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, "SPI_DW_CORE") > > > > > > > > is much much better than: > > > > > > > > EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, SPI_DW_CORE) > > > > > > I agree, but it's really not that big of a deal. We could change it if > > > you want. > > > > > > > ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=USB_STORAGE > > > > > > > > is also a bad idea. > > > > > > That's not such a bad idea as it lets you set a namespace for a > > > directory and below easily. What would you want to use instead? > > > > > > > When you look at EXPORT_SYMBOL_GPL() in C files, you will not be > > > > aware of the presence of the namespace. > > > > > > It's easy to tell when things do not link properly :) > > > > > > > Anyway, it is presumably too late to fix it. > > > > > > Not really, the number of in-kernel users are still small and can be > > > changed if you like. External users can update when they hit the change > > > as well, not a big deal. > > > > > > If I send a patch for global conversion, > > will you be happy to pick it up? > > > > > > I think this should be applied at the very last moment > > of the MW. > > Make up a patch right after -rc1 to get into -rc2 as that's usually the > best time for tree-wide changes like this. > > > I can convert tree-wide by sed. > > > > > > Example of conversion: > > > > EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, SPI_DW_CORE) > > --> > > EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, "SPI_DW_CORE") > > Ah, but then this breaks my use case where I did in the Android tree: > > EXPORT_SYMBOL_NS_GPL(symbol_name, ANDROID_GKI_VFS_EXPORT_ONLY); > > and then in the makefile I did: > subdir-ccflags-y += -DANDROID_GKI_VFS_EXPORT_ONLY=VFS_internal_I_am_really_a_filesystem_and_am_NOT_a_driver > > That saved me typing a lot of "VFS_internal_I_am..." in the .c files. Kbuild provides a macro "stringify" It shall be converted to: subdir-ccflags-y += -DANDROID_GKI_VFS_EXPORT_ONLY=$(call stringify,VFS_internal_I_am_really_a_filesystem_and_am_NOT_a_driver) > > I know out-of-tree stuff does not matter, but a simple way to make this > work would be nice. > > I could just do: > #define ANDROID_GKI_VFS_EXPORT_ONLY "VFS_internal_I_am..." > in a .c file which would handle this, right? Yes, that is an alternative. > Ok, so maybe that isn't an issue, nevermind... > > But, if we have a string here, what happens if someone puts a space in > it: > EXPORT_SYMBOL_NS_GPL(symbol_name, "MY DRIVER NAMESPACE"); > that will not break the build but will cause the tools to really > complain, right? Or do the wrong thing and put them in the "MY" > namespace? Right, the compiler does not complain about a space in the namespace. We can check it in modpost if we like. BTW, do we need EXPORT_SYMBOL_NS() in the first place? If you think DEFAULT_SYMBOL_NAMESPACE is useful, you have already noticed that the per-module namespace is more convenient than the per-symbol namespace. For example, if you look at drivers/hwmon/pmbus/pmbus_core.c, all symbols are exported by EXPORT_SYMBOL_NS_GPL(... , PMBUS). I do not see any module that exports multiple namespaces. So, I think per-module namespace control is enough. [Deprecate] EXPORT_SYMBOL_NS(<symbol>, <namespace>) EXPORT_SYMBOL_NS_GPL(<symbol>, <namespace>) [Add] MODULE_NAMESPACE(<namespace>) If MODULE_NAMESPACE(<namespace>) is added to a module, all symbols exported from that module belong to <namespace>. > thanks, > > greg k-h -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] export: fix string handling of namespace in EXPORT_SYMBOL_NS 2022-05-31 7:44 ` Masahiro Yamada @ 2022-05-31 10:36 ` Masahiro Yamada 0 siblings, 0 replies; 9+ messages in thread From: Masahiro Yamada @ 2022-05-31 10:36 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Linux Kernel Mailing List, Miroslav Benes, Emil Velikov, Jessica Yu, Quentin Perret, Matthias Maennich On Tue, May 31, 2022 at 4:44 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Tue, May 24, 2022 at 5:10 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Tue, May 24, 2022 at 03:31:59AM +0900, Masahiro Yamada wrote: > > > On Wed, Apr 27, 2022 at 11:50 PM Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > On Wed, Apr 27, 2022 at 11:29:19PM +0900, Masahiro Yamada wrote: > > > > > On Wed, Apr 27, 2022 at 6:06 PM Greg Kroah-Hartman > > > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > > > Commit c3a6cf19e695 ("export: avoid code duplication in > > > > > > include/linux/export.h") broke the ability for a defined string to be > > > > > > used as a namespace value. > > > > > > > > > > In hindsight, this was a bad idea. > > > > > > > > > > > > > > > EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, "SPI_DW_CORE") > > > > > > > > > > is much much better than: > > > > > > > > > > EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, SPI_DW_CORE) > > > > > > > > I agree, but it's really not that big of a deal. We could change it if > > > > you want. > > > > > > > > > ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=USB_STORAGE > > > > > > > > > > is also a bad idea. > > > > > > > > That's not such a bad idea as it lets you set a namespace for a > > > > directory and below easily. What would you want to use instead? > > > > > > > > > When you look at EXPORT_SYMBOL_GPL() in C files, you will not be > > > > > aware of the presence of the namespace. > > > > > > > > It's easy to tell when things do not link properly :) > > > > > > > > > Anyway, it is presumably too late to fix it. > > > > > > > > Not really, the number of in-kernel users are still small and can be > > > > changed if you like. External users can update when they hit the change > > > > as well, not a big deal. > > > > > > > > > If I send a patch for global conversion, > > > will you be happy to pick it up? > > > > > > > > > I think this should be applied at the very last moment > > > of the MW. > > > > Make up a patch right after -rc1 to get into -rc2 as that's usually the > > best time for tree-wide changes like this. > > > > > I can convert tree-wide by sed. > > > > > > > > > Example of conversion: > > > > > > EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, SPI_DW_CORE) > > > --> > > > EXPORT_SYMBOL_NS_GPL(dw_spi_resume_host, "SPI_DW_CORE") > > > > Ah, but then this breaks my use case where I did in the Android tree: > > > > EXPORT_SYMBOL_NS_GPL(symbol_name, ANDROID_GKI_VFS_EXPORT_ONLY); > > > > and then in the makefile I did: > > subdir-ccflags-y += -DANDROID_GKI_VFS_EXPORT_ONLY=VFS_internal_I_am_really_a_filesystem_and_am_NOT_a_driver > > > > That saved me typing a lot of "VFS_internal_I_am..." in the .c files. > > > Kbuild provides a macro "stringify" > > It shall be converted to: > > subdir-ccflags-y += -DANDROID_GKI_VFS_EXPORT_ONLY=$(call > stringify,VFS_internal_I_am_really_a_filesystem_and_am_NOT_a_driver) > > > > > > I know out-of-tree stuff does not matter, but a simple way to make this > > work would be nice. > > > > I could just do: > > #define ANDROID_GKI_VFS_EXPORT_ONLY "VFS_internal_I_am..." > > in a .c file which would handle this, right? > > Yes, that is an alternative. > > > > Ok, so maybe that isn't an issue, nevermind... > > > > But, if we have a string here, what happens if someone puts a space in > > it: > > EXPORT_SYMBOL_NS_GPL(symbol_name, "MY DRIVER NAMESPACE"); > > that will not break the build but will cause the tools to really > > complain, right? Or do the wrong thing and put them in the "MY" > > namespace? > > Right, the compiler does not complain about a space in the namespace. > We can check it in modpost if we like. > > > > > > > BTW, do we need EXPORT_SYMBOL_NS() in the first place? > > If you think DEFAULT_SYMBOL_NAMESPACE is useful, > you have already noticed that the per-module namespace is > more convenient than the per-symbol namespace. > > > > > For example, if you look at drivers/hwmon/pmbus/pmbus_core.c, > all symbols are exported by EXPORT_SYMBOL_NS_GPL(... , PMBUS). > I do not see any module that exports multiple namespaces. > > So, I think per-module namespace control is enough. > > > [Deprecate] > EXPORT_SYMBOL_NS(<symbol>, <namespace>) > EXPORT_SYMBOL_NS_GPL(<symbol>, <namespace>) > > [Add] > MODULE_NAMESPACE(<namespace>) > > > If MODULE_NAMESPACE(<namespace>) is added to a module, > all symbols exported from that module belong to <namespace>. Sorry, this does not work for built-in objects. I take back this comment. Sorry for the noise. -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-05-31 10:37 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-27 9:04 [PATCH v2] export: fix string handling of namespace in EXPORT_SYMBOL_NS Greg Kroah-Hartman 2022-04-27 14:29 ` Masahiro Yamada 2022-04-27 14:50 ` Greg Kroah-Hartman 2022-04-27 15:11 ` Masahiro Yamada 2022-04-27 16:21 ` Greg Kroah-Hartman 2022-05-23 18:31 ` Masahiro Yamada 2022-05-24 8:10 ` Greg Kroah-Hartman 2022-05-31 7:44 ` Masahiro Yamada 2022-05-31 10:36 ` Masahiro Yamada
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).