[PATCH] module: Allow DEFAULT_SYMBOL_NAMESPACE be set after export.h included

[PATCH] module: Allow DEFAULT_SYMBOL_NAMESPACE be set after export.h included

[David Laight]

Commit ceb8bf2ceaa77 ("module: Convert default symbol namespace to string
literal") changed DEFAULT_SYMBOL_NAMESPACE to be a string literal.
However the conditional definition of _EXPORT_SYMBOL() was left in.

Instead just default DEFAULT_SYMBOL_NAMESPACE to "" and remove the
extra _EXPORT_SYMBOL() wrapper.

This lets DEFAULT_SYMBOL_NAMESPACE be defined after export.h is included.

Fixes fd57a3325a779 ("i2c: designware: Move exports to I2C_DW namespaces")

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---
 drivers/i2c/busses/i2c-designware-common.c |  1 +
 drivers/i2c/busses/i2c-designware-master.c |  1 +
 drivers/i2c/busses/i2c-designware-slave.c  |  1 +
 include/linux/export.h                     | 10 ++++------
 4 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 183a35038eef..be5850330c75 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -29,6 +29,7 @@
 #include <linux/types.h>
 #include <linux/units.h>
 
+#undef DEFAULT_SYMBOL_NAMESPACE
 #define DEFAULT_SYMBOL_NAMESPACE	"I2C_DW_COMMON"
 
 #include "i2c-designware-core.h"
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index c8cbe5b1aeb1..083c5961d189 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -22,6 +22,7 @@
 #include <linux/regmap.h>
 #include <linux/reset.h>
 
+#undef DEFAULT_SYMBOL_NAMESPACE
 #define DEFAULT_SYMBOL_NAMESPACE	"I2C_DW"
 
 #include "i2c-designware-core.h"
diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c
index dc2b788eac5b..72b973afb0ec 100644
--- a/drivers/i2c/busses/i2c-designware-slave.c
+++ b/drivers/i2c/busses/i2c-designware-slave.c
@@ -16,6 +16,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 
+#undef DEFAULT_SYMBOL_NAMESPACE
 #define DEFAULT_SYMBOL_NAMESPACE	"I2C_DW"
 
 #include "i2c-designware-core.h"
diff --git a/include/linux/export.h b/include/linux/export.h
index 2633df4d31e6..6cea1c3982cd 100644
--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -59,14 +59,12 @@
 
 #endif
 
-#ifdef DEFAULT_SYMBOL_NAMESPACE
-#define _EXPORT_SYMBOL(sym, license)	__EXPORT_SYMBOL(sym, license, DEFAULT_SYMBOL_NAMESPACE)
-#else
-#define _EXPORT_SYMBOL(sym, license)	__EXPORT_SYMBOL(sym, license, "")
+#ifndef DEFAULT_SYMBOL_NAMESPACE
+#define DEFAULT_SYMBOL_NAMESPACE ""
 #endif
 
-#define EXPORT_SYMBOL(sym)		_EXPORT_SYMBOL(sym, "")
-#define EXPORT_SYMBOL_GPL(sym)		_EXPORT_SYMBOL(sym, "GPL")
+#define EXPORT_SYMBOL(sym)		__EXPORT_SYMBOL(sym, "", DEFAULT_SYMBOL_NAMESPACE)
+#define EXPORT_SYMBOL_GPL(sym)		__EXPORT_SYMBOL(sym, "GPL", DEFAULT_SYMBOL_NAMESPACE)
 #define EXPORT_SYMBOL_NS(sym, ns)	__EXPORT_SYMBOL(sym, "", ns)
 #define EXPORT_SYMBOL_NS_GPL(sym, ns)	__EXPORT_SYMBOL(sym, "GPL", ns)
 
-- 
2.17.1

Re: [PATCH] module: Allow DEFAULT_SYMBOL_NAMESPACE be set after export.h included

[Andy Shevchenko]

On Sat, Dec 28, 2024 at 06:43:28PM +0000, David Laight wrote:
> Commit ceb8bf2ceaa77 ("module: Convert default symbol namespace to string
> literal") changed DEFAULT_SYMBOL_NAMESPACE to be a string literal.
> However the conditional definition of _EXPORT_SYMBOL() was left in.
> 
> Instead just default DEFAULT_SYMBOL_NAMESPACE to "" and remove the
> extra _EXPORT_SYMBOL() wrapper.
> 
> This lets DEFAULT_SYMBOL_NAMESPACE be defined after export.h is included.

> Fixes fd57a3325a779 ("i2c: designware: Move exports to I2C_DW namespaces")

Incorrect format, and this should be a tag.

...

This patch in a different form had been already submitted by Uwe. So, guys, fix
the documentation or clarify it and when you agree on the approach, choose the
patch to review. No Ack till that. Andi, FYI.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] module: Allow DEFAULT_SYMBOL_NAMESPACE be set after export.h included

[David Laight]

On Sat, 28 Dec 2024 23:38:49 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Sat, Dec 28, 2024 at 06:43:28PM +0000, David Laight wrote:
> > Commit ceb8bf2ceaa77 ("module: Convert default symbol namespace to string
> > literal") changed DEFAULT_SYMBOL_NAMESPACE to be a string literal.
> > However the conditional definition of _EXPORT_SYMBOL() was left in.
> > 
> > Instead just default DEFAULT_SYMBOL_NAMESPACE to "" and remove the
> > extra _EXPORT_SYMBOL() wrapper.
> > 
> > This lets DEFAULT_SYMBOL_NAMESPACE be defined after export.h is included.  
> 
> > Fixes fd57a3325a779 ("i2c: designware: Move exports to I2C_DW namespaces")  
> 
> Incorrect format, and this should be a tag.

Except that it doesn't want to be picked up by the back-port bots.
At least not in this form - since the changes to export.h that remove
the __stringify() don't really want back-porting.

> 
> ...
> 
> This patch in a different form had been already submitted by Uwe.

Did that move the DEFAULT_SYMBOL_NAMESPACE define to the top of the file?
That can be back-ported provided the " are removed.

The simplification to export.h (which is what I was trying to do)
can then be done after the other patches.

> So, guys, fix
> the documentation or clarify it and when you agree on the approach, choose the
> patch to review. No Ack till that. Andi, FYI.
> 

I had another thought overnight - which is more changes.
Instead of using DEFAULT_SYMBOL_NAMESPACE in EXPORT_SYMBOL() use it as
a default for EXPORT_SYMBOL_NS().
So you have something like:
#define EXPORT_SYMBOL_NS(sym, ...) _EXPORT_SYMBOL_NS(sym, __VA_ARGS__, DEFAULT_SYMBOL_NAMESPACE)
#define _EXPORT_SYMBOL_NS(sym, ns, ...) __EXPORT_SYMBOL(sym, "", ns)

That requires that all the EXPORT_SYMBOL(sym) in files that define DEFAULT_SYMBOL_NAMESPACE
be changed to EXPORT_SYMBOL_NS(sym).
But it doesn't require a default definition of DEFAULT_SYMBOL_NAMESPACE and lets
it be defined in a more obvious part of the source file.

	David

Re: [PATCH] module: Allow DEFAULT_SYMBOL_NAMESPACE be set after export.h included

[Linus Torvalds]

On Sat, 28 Dec 2024 at 10:43, David Laight <david.laight.linux@gmail.com> wrote:
>
> Instead just default DEFAULT_SYMBOL_NAMESPACE to "" and remove the
> extra _EXPORT_SYMBOL() wrapper.
>
> This lets DEFAULT_SYMBOL_NAMESPACE be defined after export.h is included.

Grr. This is horribly ugly.

I think the i2c code should just be fixed to use the proper "define
namespace early".

I will also note that 'sparse' has a notion of a "weak define", where
you can set a default value for a preprocessor symbol, but if it gets
redefined by the user (or already has a definition), sparse won't
complain about it, and just use the strong one.

That would have been lovely, and we could have had a

   #weak_define DEFAULT_SYMBOL_NAMESPACE ""

and this wouldn't be the ugly mess it is.

I wish the regular C preprocessor could do the same. Oh well. Since it
doesn't, I really think i2c should just be fixed, and we shouldn't try
to deal with i2c having done things wrong.

                  Linus

Re: [PATCH] module: Allow DEFAULT_SYMBOL_NAMESPACE be set after export.h included

[David Laight]

On Sat, 28 Dec 2024 15:29:24 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, 28 Dec 2024 at 10:43, David Laight <david.laight.linux@gmail.com> wrote:
> >
> > Instead just default DEFAULT_SYMBOL_NAMESPACE to "" and remove the
> > extra _EXPORT_SYMBOL() wrapper.
> >
> > This lets DEFAULT_SYMBOL_NAMESPACE be defined after export.h is included.  
> 
> Grr. This is horribly ugly.

I thought it was a neater 'ugly' than the current definitions in export.h

> I think the i2c code should just be fixed to use the proper "define
> namespace early".

The i2c changes were needed because I found the code wouldn't compile.
It is pretty easy mistake to make and will happen again.

and does - I missed drivers/pwm/pwm-lpss.c drivers/hwmon/nct6775-core.c
and drivers/pwm/pwm-lpss.c

I guess those files could be fixed by moving the definition 'early'.

> I will also note that 'sparse' has a notion of a "weak define", where
> you can set a default value for a preprocessor symbol, but if it gets
> redefined by the user (or already has a definition), sparse won't
> complain about it, and just use the strong one.
> 
> That would have been lovely, and we could have had a
> 
>    #weak_define DEFAULT_SYMBOL_NAMESPACE ""
> 
> and this wouldn't be the ugly mess it is.
> 
> I wish the regular C preprocessor could do the same. Oh well. Since it
> doesn't, I really think i2c should just be fixed, and we shouldn't try
> to deal with i2c having done things wrong.

What you really need is the preprocessor to support a ?: type operator
in an expansion. Then you can have (DEFAULT_SYMBOL_NAMESPACE ?: "") in
the expansion of EXPORT_SYMBOL().

> 
>                   Linus

Re: [PATCH] module: Allow DEFAULT_SYMBOL_NAMESPACE be set after export.h included

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1726 bytes --]

On Sun, Dec 29, 2024 at 12:59:36AM +0000, David Laight wrote:
> On Sat, 28 Dec 2024 15:29:24 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > On Sat, 28 Dec 2024 at 10:43, David Laight <david.laight.linux@gmail.com> wrote:
> > >
> > > Instead just default DEFAULT_SYMBOL_NAMESPACE to "" and remove the
> > > extra _EXPORT_SYMBOL() wrapper.
> > >
> > > This lets DEFAULT_SYMBOL_NAMESPACE be defined after export.h is included.  
> > 
> > Grr. This is horribly ugly.
> 
> I thought it was a neater 'ugly' than the current definitions in export.h
> 
> > I think the i2c code should just be fixed to use the proper "define
> > namespace early".
> 
> The i2c changes were needed because I found the code wouldn't compile.
> It is pretty easy mistake to make and will happen again.

There is
https://lore.kernel.org/linux-i2c/20241203173640.1648939-2-u.kleine-koenig@baylibre.com
that moves the DEFAULT_SYMBOL_NAMESPACE above the #include block for the
i2c driver. Though it seems I missed ...master.c. (I'll address that.)

> and does - I missed drivers/pwm/pwm-lpss.c drivers/hwmon/nct6775-core.c
> and drivers/pwm/pwm-lpss.c

drivers/pwm/pwm-lpss.c is addressed by
https://lore.kernel.org/linux-pwm/cover.1733245406.git.ukleinek@kernel.org

the hwmon driver is addressed by
https://lore.kernel.org/linux-hwmon/20241203173149.1648456-2-u.kleine-koenig@baylibre.com
(and applied in next)

There is also drivers/gpio/gpio-idio-16.c (which I guess you intended to
list instead of the pwm driver twice), which I sent a patch for at
https://lore.kernel.org/linux-gpio/20241203172631.1647792-2-u.kleine-koenig@baylibre.com
(also already applied in next).

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] module: Allow DEFAULT_SYMBOL_NAMESPACE be set after export.h included

[David Laight]

On Mon, 30 Dec 2024 10:42:55 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> On Sun, Dec 29, 2024 at 12:59:36AM +0000, David Laight wrote:
> > On Sat, 28 Dec 2024 15:29:24 -0800
> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >   
> > > On Sat, 28 Dec 2024 at 10:43, David Laight <david.laight.linux@gmail.com> wrote:  
> > > >
> > > > Instead just default DEFAULT_SYMBOL_NAMESPACE to "" and remove the
> > > > extra _EXPORT_SYMBOL() wrapper.
> > > >
> > > > This lets DEFAULT_SYMBOL_NAMESPACE be defined after export.h is included.    
> > > 
> > > Grr. This is horribly ugly.  
> > 
> > I thought it was a neater 'ugly' than the current definitions in export.h
> >   
> > > I think the i2c code should just be fixed to use the proper "define
> > > namespace early".  
> > 
> > The i2c changes were needed because I found the code wouldn't compile.
> > It is pretty easy mistake to make and will happen again.  
> 
> There is
> https://lore.kernel.org/linux-i2c/20241203173640.1648939-2-u.kleine-koenig@baylibre.com
> that moves the DEFAULT_SYMBOL_NAMESPACE above the #include block for the
> i2c driver. Though it seems I missed ...master.c. (I'll address that.)
> 
> > and does - I missed drivers/pwm/pwm-lpss.c drivers/hwmon/nct6775-core.c
> > and drivers/pwm/pwm-lpss.c  
> 
> drivers/pwm/pwm-lpss.c is addressed by
> https://lore.kernel.org/linux-pwm/cover.1733245406.git.ukleinek@kernel.org
> 
> the hwmon driver is addressed by
> https://lore.kernel.org/linux-hwmon/20241203173149.1648456-2-u.kleine-koenig@baylibre.com
> (and applied in next)
> 
> There is also drivers/gpio/gpio-idio-16.c (which I guess you intended to
> list instead of the pwm driver twice), which I sent a patch for at
> https://lore.kernel.org/linux-gpio/20241203172631.1647792-2-u.kleine-koenig@baylibre.com
> (also already applied in next).


With all those applied it is probably worth applying my change to export.h
(which is all I really wanted to do - until the build failures.)

diff --git a/include/linux/export.h b/include/linux/export.h
index 2633df4d31e6..6cea1c3982cd 100644
--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -59,14 +59,12 @@

 #endif

-#ifdef DEFAULT_SYMBOL_NAMESPACE
-#define _EXPORT_SYMBOL(sym, license)   __EXPORT_SYMBOL(sym, license, DEFAULT_SYMBOL_NAMESPACE)
-#else
-#define _EXPORT_SYMBOL(sym, license)   __EXPORT_SYMBOL(sym, license, "")
+#ifndef DEFAULT_SYMBOL_NAMESPACE
+#define DEFAULT_SYMBOL_NAMESPACE ""
 #endif

-#define EXPORT_SYMBOL(sym)             _EXPORT_SYMBOL(sym, "")
-#define EXPORT_SYMBOL_GPL(sym)         _EXPORT_SYMBOL(sym, "GPL")
+#define EXPORT_SYMBOL(sym)             __EXPORT_SYMBOL(sym, "", DEFAULT_SYMBOL_NAMESPACE)
+#define EXPORT_SYMBOL_GPL(sym)         __EXPORT_SYMBOL(sym, "GPL", DEFAULT_SYMBOL_NAMESPACE)
 #define EXPORT_SYMBOL_NS(sym, ns)      __EXPORT_SYMBOL(sym, "", ns)
 #define EXPORT_SYMBOL_NS_GPL(sym, ns)  __EXPORT_SYMBOL(sym, "GPL", ns)

It would be 'nice' to get that into 6.13 (along with the other changes that
remove __stringify()) - but it is getting late in the rc cycle now.

Whether it is better to define DEFAULT_SYMBOL_NAMESPACE at the top of the
file or after the includes is another matter.
If the file is a module then it really makes sense to put the definition
with all the other module-related definitions.
The fact that it needs a #undef is annoying, but not the end of the world.

	David

Re: [PATCH] module: Allow DEFAULT_SYMBOL_NAMESPACE be set after export.h included

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3630 bytes --]

Hello David,

On Mon, Dec 30, 2024 at 12:03:03PM +0000, David Laight wrote:
> On Mon, 30 Dec 2024 10:42:55 +0100
> Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:
> 
> > On Sun, Dec 29, 2024 at 12:59:36AM +0000, David Laight wrote:
> > > On Sat, 28 Dec 2024 15:29:24 -0800
> > > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > >   
> > > > On Sat, 28 Dec 2024 at 10:43, David Laight <david.laight.linux@gmail.com> wrote:  
> > > > >
> > > > > Instead just default DEFAULT_SYMBOL_NAMESPACE to "" and remove the
> > > > > extra _EXPORT_SYMBOL() wrapper.
> > > > >
> > > > > This lets DEFAULT_SYMBOL_NAMESPACE be defined after export.h is included.    
> > > > 
> > > > Grr. This is horribly ugly.  
> > > 
> > > I thought it was a neater 'ugly' than the current definitions in export.h
> > >   
> > > > I think the i2c code should just be fixed to use the proper "define
> > > > namespace early".  
> > > 
> > > The i2c changes were needed because I found the code wouldn't compile.
> > > It is pretty easy mistake to make and will happen again.  
> > 
> > There is
> > https://lore.kernel.org/linux-i2c/20241203173640.1648939-2-u.kleine-koenig@baylibre.com
> > that moves the DEFAULT_SYMBOL_NAMESPACE above the #include block for the
> > i2c driver. Though it seems I missed ...master.c. (I'll address that.)
> > 
> > > and does - I missed drivers/pwm/pwm-lpss.c drivers/hwmon/nct6775-core.c
> > > and drivers/pwm/pwm-lpss.c  
> > 
> > drivers/pwm/pwm-lpss.c is addressed by
> > https://lore.kernel.org/linux-pwm/cover.1733245406.git.ukleinek@kernel.org
> > 
> > the hwmon driver is addressed by
> > https://lore.kernel.org/linux-hwmon/20241203173149.1648456-2-u.kleine-koenig@baylibre.com
> > (and applied in next)
> > 
> > There is also drivers/gpio/gpio-idio-16.c (which I guess you intended to
> > list instead of the pwm driver twice), which I sent a patch for at
> > https://lore.kernel.org/linux-gpio/20241203172631.1647792-2-u.kleine-koenig@baylibre.com
> > (also already applied in next).
> 
> 
> With all those applied it is probably worth applying my change to export.h
> (which is all I really wanted to do - until the build failures.)
> 
> diff --git a/include/linux/export.h b/include/linux/export.h
> index 2633df4d31e6..6cea1c3982cd 100644
> --- a/include/linux/export.h
> +++ b/include/linux/export.h
> @@ -59,14 +59,12 @@
> 
>  #endif
> 
> -#ifdef DEFAULT_SYMBOL_NAMESPACE
> -#define _EXPORT_SYMBOL(sym, license)   __EXPORT_SYMBOL(sym, license, DEFAULT_SYMBOL_NAMESPACE)

If you keep the above definition (without the #ifdef), you don't need to
touch the definitions of EXPORT_SYMBOL and EXPORT_SYMBOL_GPL below.
Probably a matter of taste.

> -#else
> -#define _EXPORT_SYMBOL(sym, license)   __EXPORT_SYMBOL(sym, license, "")
> +#ifndef DEFAULT_SYMBOL_NAMESPACE
> +#define DEFAULT_SYMBOL_NAMESPACE ""
>  #endif
> 
> -#define EXPORT_SYMBOL(sym)             _EXPORT_SYMBOL(sym, "")
> -#define EXPORT_SYMBOL_GPL(sym)         _EXPORT_SYMBOL(sym, "GPL")
> +#define EXPORT_SYMBOL(sym)             __EXPORT_SYMBOL(sym, "", DEFAULT_SYMBOL_NAMESPACE)
> +#define EXPORT_SYMBOL_GPL(sym)         __EXPORT_SYMBOL(sym, "GPL", DEFAULT_SYMBOL_NAMESPACE)
>  #define EXPORT_SYMBOL_NS(sym, ns)      __EXPORT_SYMBOL(sym, "", ns)
>  #define EXPORT_SYMBOL_NS_GPL(sym, ns)  __EXPORT_SYMBOL(sym, "GPL", ns)
> 
> It would be 'nice' to get that into 6.13 (along with the other changes that
> remove __stringify()) - but it is getting late in the rc cycle now.

I'm pretty sure we won't get all my changes into v6.13.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] module: Allow DEFAULT_SYMBOL_NAMESPACE be set after export.h included

[David Laight]

On Mon, 30 Dec 2024 13:54:45 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> Hello David,
> 
> On Mon, Dec 30, 2024 at 12:03:03PM +0000, David Laight wrote:
> > On Mon, 30 Dec 2024 10:42:55 +0100
> > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:
> >   
> > > On Sun, Dec 29, 2024 at 12:59:36AM +0000, David Laight wrote:  
> > > > On Sat, 28 Dec 2024 15:29:24 -0800
> > > > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > > >     
> > > > > On Sat, 28 Dec 2024 at 10:43, David Laight <david.laight.linux@gmail.com> wrote:    
> > > > > >
> > > > > > Instead just default DEFAULT_SYMBOL_NAMESPACE to "" and remove the
> > > > > > extra _EXPORT_SYMBOL() wrapper.
> > > > > >
> > > > > > This lets DEFAULT_SYMBOL_NAMESPACE be defined after export.h is included.      
> > > > > 
> > > > > Grr. This is horribly ugly.    
> > > > 
> > > > I thought it was a neater 'ugly' than the current definitions in export.h
> > > >     
> > > > > I think the i2c code should just be fixed to use the proper "define
> > > > > namespace early".    
> > > > 
> > > > The i2c changes were needed because I found the code wouldn't compile.
> > > > It is pretty easy mistake to make and will happen again.    
> > > 
> > > There is
> > > https://lore.kernel.org/linux-i2c/20241203173640.1648939-2-u.kleine-koenig@baylibre.com
> > > that moves the DEFAULT_SYMBOL_NAMESPACE above the #include block for the
> > > i2c driver. Though it seems I missed ...master.c. (I'll address that.)
> > >   
> > > > and does - I missed drivers/pwm/pwm-lpss.c drivers/hwmon/nct6775-core.c
> > > > and drivers/pwm/pwm-lpss.c    
> > > 
> > > drivers/pwm/pwm-lpss.c is addressed by
> > > https://lore.kernel.org/linux-pwm/cover.1733245406.git.ukleinek@kernel.org
> > > 
> > > the hwmon driver is addressed by
> > > https://lore.kernel.org/linux-hwmon/20241203173149.1648456-2-u.kleine-koenig@baylibre.com
> > > (and applied in next)
> > > 
> > > There is also drivers/gpio/gpio-idio-16.c (which I guess you intended to
> > > list instead of the pwm driver twice), which I sent a patch for at
> > > https://lore.kernel.org/linux-gpio/20241203172631.1647792-2-u.kleine-koenig@baylibre.com
> > > (also already applied in next).  
> > 
> > 
> > With all those applied it is probably worth applying my change to export.h
> > (which is all I really wanted to do - until the build failures.)
> > 
> > diff --git a/include/linux/export.h b/include/linux/export.h
> > index 2633df4d31e6..6cea1c3982cd 100644
> > --- a/include/linux/export.h
> > +++ b/include/linux/export.h
> > @@ -59,14 +59,12 @@
> > 
> >  #endif
> > 
> > -#ifdef DEFAULT_SYMBOL_NAMESPACE
> > -#define _EXPORT_SYMBOL(sym, license)   __EXPORT_SYMBOL(sym, license, DEFAULT_SYMBOL_NAMESPACE)  
> 
> If you keep the above definition (without the #ifdef), you don't need to
> touch the definitions of EXPORT_SYMBOL and EXPORT_SYMBOL_GPL below.
> Probably a matter of taste.

The extra wrapper just makes it harder to read (and will be immeasurably slower to
compile).

> > -#else
> > -#define _EXPORT_SYMBOL(sym, license)   __EXPORT_SYMBOL(sym, license, "")
> > +#ifndef DEFAULT_SYMBOL_NAMESPACE
> > +#define DEFAULT_SYMBOL_NAMESPACE ""
> >  #endif
> > 
> > -#define EXPORT_SYMBOL(sym)             _EXPORT_SYMBOL(sym, "")
> > -#define EXPORT_SYMBOL_GPL(sym)         _EXPORT_SYMBOL(sym, "GPL")
> > +#define EXPORT_SYMBOL(sym)             __EXPORT_SYMBOL(sym, "", DEFAULT_SYMBOL_NAMESPACE)
> > +#define EXPORT_SYMBOL_GPL(sym)         __EXPORT_SYMBOL(sym, "GPL", DEFAULT_SYMBOL_NAMESPACE)
> >  #define EXPORT_SYMBOL_NS(sym, ns)      __EXPORT_SYMBOL(sym, "", ns)
> >  #define EXPORT_SYMBOL_NS_GPL(sym, ns)  __EXPORT_SYMBOL(sym, "GPL", ns)
> > 
> > It would be 'nice' to get that into 6.13 (along with the other changes that
> > remove __stringify()) - but it is getting late in the rc cycle now.  
> 
> I'm pretty sure we won't get all my changes into v6.13.

Indeed: Linus would have to like them and 'just apply them' :-)

	David

> 
> Best regards
> Uwe

Re: [PATCH] module: Allow DEFAULT_SYMBOL_NAMESPACE be set after export.h included

[Masahiro Yamada]

On Sun, Dec 29, 2024 at 9:59 AM David Laight
<david.laight.linux@gmail.com> wrote:
>
> On Sat, 28 Dec 2024 15:29:24 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> > On Sat, 28 Dec 2024 at 10:43, David Laight <david.laight.linux@gmail.com> wrote:
> > >
> > > Instead just default DEFAULT_SYMBOL_NAMESPACE to "" and remove the
> > > extra _EXPORT_SYMBOL() wrapper.
> > >
> > > This lets DEFAULT_SYMBOL_NAMESPACE be defined after export.h is included.
> >
> > Grr. This is horribly ugly.
>
> I thought it was a neater 'ugly' than the current definitions in export.h
>
> > I think the i2c code should just be fixed to use the proper "define
> > namespace early".
>
> The i2c changes were needed because I found the code wouldn't compile.
> It is pretty easy mistake to make and will happen again.


Agree.

Currently, the compilation still succeeds, and the empty string ""
is used instead of the specified namespace, silently.
It is difficult to notice this mistake.

So, I like the change for include/linux/export.h
since it causes a compile error if
DEFAULT_SYMBOL_NAMESPACE is defined after
the header inclusion.

Perhaps, we can add a comment about how
to fix the issue.


/*
 * If you override DEFAULT_SYMBOL_NAMESPACE, please define it at the very top
 * of the source file, before any header inclusion.
 */
#ifndef DEFAULT_SYMBOL_NAMESPACE
#define DEFAULT_SYMBOL_NAMESPACE ""
#endif









>
> and does - I missed drivers/pwm/pwm-lpss.c drivers/hwmon/nct6775-core.c
> and drivers/pwm/pwm-lpss.c

OK, this patch breaks the compilation, and we can notice the mistake.
This is good.



> I guess those files could be fixed by moving the definition 'early'.
>
> > I will also note that 'sparse' has a notion of a "weak define", where
> > you can set a default value for a preprocessor symbol, but if it gets
> > redefined by the user (or already has a definition), sparse won't
> > complain about it, and just use the strong one.
> >
> > That would have been lovely, and we could have had a
> >
> >    #weak_define DEFAULT_SYMBOL_NAMESPACE ""
> >
> > and this wouldn't be the ugly mess it is.
> >
> > I wish the regular C preprocessor could do the same. Oh well. Since it
> > doesn't, I really think i2c should just be fixed, and we shouldn't try
> > to deal with i2c having done things wrong.
>
> What you really need is the preprocessor to support a ?: type operator
> in an expansion. Then you can have (DEFAULT_SYMBOL_NAMESPACE ?: "") in
> the expansion of EXPORT_SYMBOL().
>
> >
> >                   Linus
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH] module: Allow DEFAULT_SYMBOL_NAMESPACE be set after export.h included

[kernel test robot]

Hi David,

kernel test robot noticed the following build warnings:

[auto build test WARNING on andi-shyti/i2c/i2c-host]
[also build test WARNING on linus/master v6.13-rc4 next-20241220]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Laight/module-Allow-DEFAULT_SYMBOL_NAMESPACE-be-set-after-export-h-included/20241229-024441
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20241228184328.5ced280b%40dsl-u17-10
patch subject: [PATCH] module: Allow DEFAULT_SYMBOL_NAMESPACE be set after export.h included
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20241229/202412290825.cKFjKHQf-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241229/202412290825.cKFjKHQf-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412290825.cKFjKHQf-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/pwm/pwm-lpss.c:22: warning: "DEFAULT_SYMBOL_NAMESPACE" redefined
      22 | #define DEFAULT_SYMBOL_NAMESPACE "PWM_LPSS"
         | 
   In file included from include/linux/linkage.h:7,
                    from arch/x86/include/asm/cache.h:5,
                    from include/linux/cache.h:6,
                    from arch/x86/include/asm/current.h:10,
                    from include/linux/sched.h:12,
                    from include/linux/delay.h:13,
                    from drivers/pwm/pwm-lpss.c:14:
   include/linux/export.h:63: note: this is the location of the previous definition
      63 | #define DEFAULT_SYMBOL_NAMESPACE ""
         | 


vim +/DEFAULT_SYMBOL_NAMESPACE +22 drivers/pwm/pwm-lpss.c

093e00bb3f82f3 Alan Cox        2014-04-18  21  
ceb8bf2ceaa77f Masahiro Yamada 2024-12-03 @22  #define DEFAULT_SYMBOL_NAMESPACE "PWM_LPSS"
a3682d2fe3c36c Andy Shevchenko 2022-09-27  23  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] module: Allow DEFAULT_SYMBOL_NAMESPACE be set after export.h included

[kernel test robot]

Hi David,

kernel test robot noticed the following build warnings:

[auto build test WARNING on andi-shyti/i2c/i2c-host]
[also build test WARNING on linus/master v6.13-rc4 next-20241220]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Laight/module-Allow-DEFAULT_SYMBOL_NAMESPACE-be-set-after-export-h-included/20241229-024441
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20241228184328.5ced280b%40dsl-u17-10
patch subject: [PATCH] module: Allow DEFAULT_SYMBOL_NAMESPACE be set after export.h included
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20241229/202412291114.MzNzqKpo-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241229/202412291114.MzNzqKpo-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412291114.MzNzqKpo-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/pwm/pwm-lpss.c:22:9: warning: 'DEFAULT_SYMBOL_NAMESPACE' macro redefined [-Wmacro-redefined]
      22 | #define DEFAULT_SYMBOL_NAMESPACE "PWM_LPSS"
         |         ^
   include/linux/export.h:63:9: note: previous definition is here
      63 | #define DEFAULT_SYMBOL_NAMESPACE ""
         |         ^
   1 warning generated.


vim +/DEFAULT_SYMBOL_NAMESPACE +22 drivers/pwm/pwm-lpss.c

093e00bb3f82f3 Alan Cox        2014-04-18  21  
ceb8bf2ceaa77f Masahiro Yamada 2024-12-03 @22  #define DEFAULT_SYMBOL_NAMESPACE "PWM_LPSS"
a3682d2fe3c36c Andy Shevchenko 2022-09-27  23  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] module: Allow DEFAULT_SYMBOL_NAMESPACE be set after export.h included

[Masahiro Yamada]

On Sun, Dec 29, 2024 at 3:43 AM David Laight
<david.laight.linux@gmail.com> wrote:
>
> Commit ceb8bf2ceaa77 ("module: Convert default symbol namespace to string
> literal") changed DEFAULT_SYMBOL_NAMESPACE to be a string literal.

Why is ceb8bf2ceaa77 related?

Even before ceb8bf2ceaa77, this was broken.





--
Best Regards
Masahiro Yamada