[PATCH 0/2] pwm: lpss: module namespace fixes

[PATCH 0/2] pwm: lpss: module namespace fixes

[Uwe Kleine-König]

Hello,

while working on a patch quite similar to
https://lore.kernel.org/all/20241203102124.3147478-4-masahiroy@kernel.org/
I noticed that the module name space in drivers/pwm/pwm-lpss.c isn't
used. When I fixed that (patch #2) the intel pinctrl driver was unhappy
with

	WARNING: modpost: module pinctrl-intel uses symbol devm_pwm_lpss_probe from namespace PWM_LPSS, but does not import it.

so the first patch prevents this from happening.

This conflicts with ceb8bf2ceaa77fe222fe8fe32cb7789c9099ddf1 that is
currently in Linus' tree. I'll take care about that.

Best regards
Uwe

Uwe Kleine-König (2):
  pwm: lpss: Move namespace import into a header
  pwm: lpss: Define DEFAULT_SYMBOL_NAMESPACE earlier

 drivers/pwm/pwm-lpss-pci.c                 | 2 --
 drivers/pwm/pwm-lpss-platform.c            | 2 --
 drivers/pwm/pwm-lpss.c                     | 4 ++--
 include/linux/platform_data/x86/pwm-lpss.h | 7 +++++++
 4 files changed, 9 insertions(+), 6 deletions(-)

base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
-- 
2.45.2

[PATCH 1/2] pwm: lpss: Move namespace import into a header

[Uwe Kleine-König]

Each user of the exported symbols related to the pwm-lpss driver needs
to import the matching namespace. So this can just be done in the header
together with the prototypes.

This fixes drivers/pinctrl/intel/pinctrl-intel.c which failed to import
that namespace before. (However this didn't hurt because the pwm-lpss
module namespace isn't used; see the next commit.)

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-lpss-pci.c                 | 2 --
 drivers/pwm/pwm-lpss-platform.c            | 2 --
 include/linux/platform_data/x86/pwm-lpss.h | 7 +++++++
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-lpss-pci.c b/drivers/pwm/pwm-lpss-pci.c
index f7ece2809e6b..8615c44c1034 100644
--- a/drivers/pwm/pwm-lpss-pci.c
+++ b/drivers/pwm/pwm-lpss-pci.c
@@ -8,7 +8,6 @@
  */
 
 #include <linux/kernel.h>
-#include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/pm_runtime.h>
 
@@ -70,4 +69,3 @@ module_pci_driver(pwm_lpss_driver_pci);
 
 MODULE_DESCRIPTION("PWM PCI driver for Intel LPSS");
 MODULE_LICENSE("GPL v2");
-MODULE_IMPORT_NS(PWM_LPSS);
diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-platform.c
index 5130238a4567..3de1ab2cff54 100644
--- a/drivers/pwm/pwm-lpss-platform.c
+++ b/drivers/pwm/pwm-lpss-platform.c
@@ -9,7 +9,6 @@
 
 #include <linux/kernel.h>
 #include <linux/mod_devicetable.h>
-#include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
@@ -78,5 +77,4 @@ module_platform_driver(pwm_lpss_driver_platform);
 
 MODULE_DESCRIPTION("PWM platform driver for Intel LPSS");
 MODULE_LICENSE("GPL v2");
-MODULE_IMPORT_NS(PWM_LPSS);
 MODULE_ALIAS("platform:pwm-lpss");
diff --git a/include/linux/platform_data/x86/pwm-lpss.h b/include/linux/platform_data/x86/pwm-lpss.h
index 752c06b47cc8..0a1025f6cd58 100644
--- a/include/linux/platform_data/x86/pwm-lpss.h
+++ b/include/linux/platform_data/x86/pwm-lpss.h
@@ -4,6 +4,7 @@
 #ifndef __PLATFORM_DATA_X86_PWM_LPSS_H
 #define __PLATFORM_DATA_X86_PWM_LPSS_H
 
+#include <linux/module.h>
 #include <linux/types.h>
 
 struct device;
@@ -30,4 +31,10 @@ struct pwm_lpss_boardinfo {
 struct pwm_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base,
 				     const struct pwm_lpss_boardinfo *info);
 
+/*
+ * The above function and the pwm_lpss_boardinfo variables in
+ * drivers/pwm/pwm-lpss.h are defined in the PWM_LPSS namespace.
+ */
+MODULE_IMPORT_NS(PWM_LPSS);
+
 #endif	/* __PLATFORM_DATA_X86_PWM_LPSS_H */
-- 
2.45.2

Re: [PATCH 1/2] pwm: lpss: Move namespace import into a header

[Andy Shevchenko]

On Tue, Dec 03, 2024 at 06:16:14PM +0100, Uwe Kleine-König wrote:
> Each user of the exported symbols related to the pwm-lpss driver needs
> to import the matching namespace. So this can just be done in the header
> together with the prototypes.
> 
> This fixes drivers/pinctrl/intel/pinctrl-intel.c which failed to import
> that namespace before. (However this didn't hurt because the pwm-lpss
> module namespace isn't used; see the next commit.)

I disagree on this change, I think it had been discussed already.

The header must not provide any module importing features as it effectively
diminishes the point of namespace. Any (ab)user can include just a header and
be okay with that.

Besides that, you should have based this on recent changes in the NS area of
the module symbols, i.e. module namespaces needs to be provided as string
literals.

See last few commits in Linus' tree as of today.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] pwm: lpss: Move namespace import into a header

[Uwe Kleine-König]

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

Hello Andy,

On Tue, Dec 03, 2024 at 09:12:36PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 03, 2024 at 06:16:14PM +0100, Uwe Kleine-König wrote:
> > Each user of the exported symbols related to the pwm-lpss driver needs
> > to import the matching namespace. So this can just be done in the header
> > together with the prototypes.
> > 
> > This fixes drivers/pinctrl/intel/pinctrl-intel.c which failed to import
> > that namespace before. (However this didn't hurt because the pwm-lpss
> > module namespace isn't used; see the next commit.)
> 
> I disagree on this change, I think it had been discussed already.

Who discussed this? If I was involved, I don't remember. So if you have
a link, that would be great.

> The header must not provide any module importing features as it effectively
> diminishes the point of namespace. Any (ab)user can include just a header and
> be okay with that.

Huh? Any abuser can also just do the IMPORT_NS statement? Module
namespaces are not a safeguard against bad code? So I don't see why
making it simple for the regular users should be the wrong choice.

Actually I think this is very elegant because this way all needs to use
these symbols (i.e. prototype and namespace) are in a single place and
users just do the #include and get all the preconditions.

> Besides that, you should have based this on recent changes in the NS area of
> the module symbols, i.e. module namespaces needs to be provided as string
> literals.

I coordinated my patch set with the pwm maintainer and he is ok with it
:-) That's why I wrote "This conflicts with
ceb8bf2ceaa77fe222fe8fe32cb7789c9099ddf1 that is currently in Linus'
tree. I'll take care about that." in the cover letter.

Best regards
Uwe

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

Re: [PATCH 1/2] pwm: lpss: Move namespace import into a header

[Andy Shevchenko]

On Tue, Dec 03, 2024 at 10:32:17PM +0100, Uwe Kleine-König wrote:
> On Tue, Dec 03, 2024 at 09:12:36PM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 03, 2024 at 06:16:14PM +0100, Uwe Kleine-König wrote:
> > > Each user of the exported symbols related to the pwm-lpss driver needs
> > > to import the matching namespace. So this can just be done in the header
> > > together with the prototypes.
> > > 
> > > This fixes drivers/pinctrl/intel/pinctrl-intel.c which failed to import
> > > that namespace before. (However this didn't hurt because the pwm-lpss
> > > module namespace isn't used; see the next commit.)
> > 
> > I disagree on this change, I think it had been discussed already.
> 
> Who discussed this? If I was involved, I don't remember. So if you have
> a link, that would be great.

Sure, https://lore.kernel.org/linux-pwm/20220908135658.64463-1-andriy.shevchenko@linux.intel.com/
you were a participant there.

> > The header must not provide any module importing features as it effectively
> > diminishes the point of namespace. Any (ab)user can include just a header and
> > be okay with that.
> 
> Huh? Any abuser can also just do the IMPORT_NS statement? Module
> namespaces are not a safeguard against bad code? So I don't see why
> making it simple for the regular users should be the wrong choice.
> 
> Actually I think this is very elegant because this way all needs to use
> these symbols (i.e. prototype and namespace) are in a single place and
> users just do the #include and get all the preconditions.

Recent LWN https://lwn.net/Articles/998221/ article describes in more details
what I implied under abuser here.

> > Besides that, you should have based this on recent changes in the NS area of
> > the module symbols, i.e. module namespaces needs to be provided as string
> > literals.
> 
> I coordinated my patch set with the pwm maintainer and he is ok with it
> :-) That's why I wrote "This conflicts with
> ceb8bf2ceaa77fe222fe8fe32cb7789c9099ddf1 that is currently in Linus'
> tree. I'll take care about that." in the cover letter.

Other subsystems simply backmerged those changes. Note, at any time one may
consider origin/master as an immutable (at the point of a given change).
It might be better (if no urgent need) to wait for rc2 and merge it before
doing other changes.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] pwm: lpss: Move namespace import into a header

[Uwe Kleine-König]

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

Hello Andy,

On Wed, Dec 04, 2024 at 12:28:23AM +0200, Andy Shevchenko wrote:
> On Tue, Dec 03, 2024 at 10:32:17PM +0100, Uwe Kleine-König wrote:
> > On Tue, Dec 03, 2024 at 09:12:36PM +0200, Andy Shevchenko wrote:
> > > On Tue, Dec 03, 2024 at 06:16:14PM +0100, Uwe Kleine-König wrote:
> > > > Each user of the exported symbols related to the pwm-lpss driver needs
> > > > to import the matching namespace. So this can just be done in the header
> > > > together with the prototypes.
> > > > 
> > > > This fixes drivers/pinctrl/intel/pinctrl-intel.c which failed to import
> > > > that namespace before. (However this didn't hurt because the pwm-lpss
> > > > module namespace isn't used; see the next commit.)
> > > 
> > > I disagree on this change, I think it had been discussed already.
> > 
> > Who discussed this? If I was involved, I don't remember. So if you have
> > a link, that would be great.
> 
> Sure, https://lore.kernel.org/linux-pwm/20220908135658.64463-1-andriy.shevchenko@linux.intel.com/
> you were a participant there.

Ah that. When I read "it had been discussed already" I expected a
discussion with an agreement in the end. Let me note that you chose the
more complicated way back then (and I let you) and the result is that
the pinctrl driver didn't get the (up to now not yet) needed import.

> > > The header must not provide any module importing features as it effectively
> > > diminishes the point of namespace. Any (ab)user can include just a header and
> > > be okay with that.
> > 
> > Huh? Any abuser can also just do the IMPORT_NS statement? Module
> > namespaces are not a safeguard against bad code? So I don't see why
> > making it simple for the regular users should be the wrong choice.
> > 
> > Actually I think this is very elegant because this way all needs to use
> > these symbols (i.e. prototype and namespace) are in a single place and
> > users just do the #include and get all the preconditions.
> 
> Recent LWN https://lwn.net/Articles/998221/ article describes in more details
> what I implied under abuser here.

Ok. But I don't see how this supports your case. Independent of where
the import for a given namespace happens, you can see the used
namespaces in modinfo and that is the only place where suspicious
imports can be noted reliably. And MODULE_foo isn't relevant here as the
namespace of the pwm-lpss driver combo is used in several modules.

I thought the point of namespaces is grouping of symbols and reduction
of the set of globally available symbols to speed up module loading.
I didn't have "duplicate IMPORT_NS statements to make it harder for bad
out-of-tree code" on my radar and that shouldn't be a motivation if the
price is that in-tree code faces the same constraints IMHO.

And I'm pretty sure, we won't prevent a bad actor from successfully
using a symbol they should not, just because they need an include *and*
an import.

Am I missing something?

> > > Besides that, you should have based this on recent changes in the NS area of
> > > the module symbols, i.e. module namespaces needs to be provided as string
> > > literals.
> > 
> > I coordinated my patch set with the pwm maintainer and he is ok with it
> > :-) That's why I wrote "This conflicts with
> > ceb8bf2ceaa77fe222fe8fe32cb7789c9099ddf1 that is currently in Linus'
> > tree. I'll take care about that." in the cover letter.
> 
> Other subsystems simply backmerged those changes. Note, at any time one may
> consider origin/master as an immutable (at the point of a given change).
> It might be better (if no urgent need) to wait for rc2 and merge it before
> doing other changes.

I'm well aware of my options. Thanks
Uwe

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

Re: [PATCH 1/2] pwm: lpss: Move namespace import into a header

[Andy Shevchenko]

On Wed, Dec 04, 2024 at 12:38:49AM +0100, Uwe Kleine-König wrote:
> On Wed, Dec 04, 2024 at 12:28:23AM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 03, 2024 at 10:32:17PM +0100, Uwe Kleine-König wrote:
> > > On Tue, Dec 03, 2024 at 09:12:36PM +0200, Andy Shevchenko wrote:
> > > > On Tue, Dec 03, 2024 at 06:16:14PM +0100, Uwe Kleine-König wrote:

...

> > > > The header must not provide any module importing features as it effectively
> > > > diminishes the point of namespace. Any (ab)user can include just a header and
> > > > be okay with that.
> > > 
> > > Huh? Any abuser can also just do the IMPORT_NS statement? Module
> > > namespaces are not a safeguard against bad code? So I don't see why
> > > making it simple for the regular users should be the wrong choice.
> > > 
> > > Actually I think this is very elegant because this way all needs to use
> > > these symbols (i.e. prototype and namespace) are in a single place and
> > > users just do the #include and get all the preconditions.
> > 
> > Recent LWN https://lwn.net/Articles/998221/ article describes in more details
> > what I implied under abuser here.
> 
> Ok. But I don't see how this supports your case. Independent of where
> the import for a given namespace happens, you can see the used
> namespaces in modinfo and that is the only place where suspicious
> imports can be noted reliably. And MODULE_foo isn't relevant here as the
> namespace of the pwm-lpss driver combo is used in several modules.

The initial point that the (ab)users will be visible at review stage.
Yes, runtime restrictions are more difficult to enforce.

> I thought the point of namespaces is grouping of symbols and reduction
> of the set of globally available symbols to speed up module loading.
> I didn't have "duplicate IMPORT_NS statements to make it harder for bad
> out-of-tree code" on my radar and that shouldn't be a motivation if the
> price is that in-tree code faces the same constraints IMHO.
> 
> And I'm pretty sure, we won't prevent a bad actor from successfully
> using a symbol they should not, just because they need an include *and*
> an import.
> 
> Am I missing something?

They may not need the include actually, as they can just tell the compiler with
extern keyword to look for a prototype somewhere else. In any case let's
return to the main topic here. My proposal is to add the respective module
namespace import into pin control driver.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] pwm: lpss: Move namespace import into a header

[Uwe Kleine-König]

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

Hello Linus,

On Tue, Dec 03, 2024 at 09:12:36PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 03, 2024 at 06:16:14PM +0100, Uwe Kleine-König wrote:
> > Each user of the exported symbols related to the pwm-lpss driver needs
> > to import the matching namespace. So this can just be done in the header
> > together with the prototypes.
> > 
> > This fixes drivers/pinctrl/intel/pinctrl-intel.c which failed to import
> > that namespace before. (However this didn't hurt because the pwm-lpss
> > module namespace isn't used; see the next commit.)
> 
> I disagree on this change, I think it had been discussed already.
> 
> The header must not provide any module importing features as it effectively
> diminishes the point of namespace. Any (ab)user can include just a header and
> be okay with that.

Andy and I disagree about this change. If you happen to not have the
patch in your inbox, see
https://lore.kernel.org/linux-pwm/3a99048a52aeee356d01dbf7f2f06e6e0826ed78.1733245406.git.ukleinek@kernel.org/
.

Would you please volunteer as the impartial judge here as you're the
upstream maintainer of drivers/pinctrl/intel/pinctrl-intel.c?

The TL;DR; is: Do you prefer a single MODULE_IMPORT_NS() in a header, or
should every consumer driver explicitly have its own MODULE_IMPORT_NS()
invokation?  I won't repeat the arguments because I think I cannot
present them without making my opinion too obvious, so please see the
discussion between Andy and me in the above linked thread.

Best regards and thanks,
Uwe

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

Re: [PATCH 1/2] pwm: lpss: Move namespace import into a header

[Linus Walleij]

On Mon, Dec 16, 2024 at 9:46 AM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:

> Andy and I disagree about this change. If you happen to not have the
> patch in your inbox, see
> https://lore.kernel.org/linux-pwm/3a99048a52aeee356d01dbf7f2f06e6e0826ed78.1733245406.git.ukleinek@kernel.org/
>
> Would you please volunteer as the impartial judge here as you're the
> upstream maintainer of drivers/pinctrl/intel/pinctrl-intel.c?

What about those wise lines from Torvalds in
Documentation/process/management-style.rst:

  The name of the game is to **avoid** having to make a decision.  In
  particular, if somebody tells you "choose (a) or (b), we really need you
  to decide on this", you're in trouble as a manager.  The people you
  manage had better know the details better than you, so if they come to
  you for a technical decision, you're screwed.  You're clearly not
  competent to make that decision for them.

> The TL;DR; is: Do you prefer a single MODULE_IMPORT_NS() in a header, or
> should every consumer driver explicitly have its own MODULE_IMPORT_NS()
> invokation?

Intuitively I personally prefer using macros like that in every instance
(in every consumer driver). But it's not like I can give any good reason
for it, just intuition and what I would do.

Yours,
Linus Walleij

[PATCH 2/2] pwm: lpss: Define DEFAULT_SYMBOL_NAMESPACE earlier

[Uwe Kleine-König]

DEFAULT_SYMBOL_NAMESPACE must be already defined when <linux/export.h>
is included. So move the define above the include block.

With the DEFAULT_SYMBOL_NAMESPACE being defined too late, the exported
symbols end up in the default namespace. So the modules making use of
the symbols defined in pwm-lpss.c can import these just fine and just
import the the PWM_LPSS namespace without any gain.

Fixes: a3682d2fe3c3 ("pwm: lpss: Move exported symbols to PWM_LPSS namespace")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-lpss.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 867e2bc8c601..9519d1b06869 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -10,6 +10,8 @@
  * Author: Alan Cox <alan@linux.intel.com>
  */
 
+#define DEFAULT_SYMBOL_NAMESPACE PWM_LPSS
+
 #include <linux/bits.h>
 #include <linux/delay.h>
 #include <linux/io.h>
@@ -19,8 +21,6 @@
 #include <linux/pm_runtime.h>
 #include <linux/time.h>
 
-#define DEFAULT_SYMBOL_NAMESPACE PWM_LPSS
-
 #include "pwm-lpss.h"
 
 #define PWM				0x00000000
-- 
2.45.2

Re: [PATCH 2/2] pwm: lpss: Define DEFAULT_SYMBOL_NAMESPACE earlier

[Andy Shevchenko]

On Tue, Dec 03, 2024 at 06:16:15PM +0100, Uwe Kleine-König wrote:
> DEFAULT_SYMBOL_NAMESPACE must be already defined when <linux/export.h>
> is included. So move the define above the include block.
> 
> With the DEFAULT_SYMBOL_NAMESPACE being defined too late, the exported
> symbols end up in the default namespace. So the modules making use of
> the symbols defined in pwm-lpss.c can import these just fine and just
> import the the PWM_LPSS namespace without any gain.

Documentation disagrees with you. But, you are right, a lurking bug is here as
we need to undef existing define.

> -#define DEFAULT_SYMBOL_NAMESPACE PWM_LPSS

-- 
With Best Regards,
Andy Shevchenko