Some Qualcomm PMICs integrate a SDAM device, internally located in
a specific address range reachable through SPMI communication.
Instead of using the parent SPMI device (the main PMIC) as a kind
of syscon in this driver, register a new SPMI sub-device for SDAM
and initialize its own regmap with this sub-device's specific base
address, retrieved from the devicetree.
This allows to stop manually adding the register base address to
every R/W call in this driver, as this can be, and is now, handled
by the regmap API instead.
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Link: https://lore.kernel.org/r/20250722101317.76729-3-angelogioacchino.delregno@collabora.com
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD
Link: https://lore.kernel.org/r/20250730112645.542179-3-angelogioacchino.delregno@collabora.com
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
drivers/nvmem/qcom-spmi-sdam.c | 37 ++++++++++++++++++++++++----------
1 file changed, 26 insertions(+), 11 deletions(-)
diff --git a/drivers/nvmem/qcom-spmi-sdam.c b/drivers/nvmem/qcom-spmi-sdam.c
index 4f1cca6eab71..9a4be20dfa9f 100644
--- a/drivers/nvmem/qcom-spmi-sdam.c
+++ b/drivers/nvmem/qcom-spmi-sdam.c
@@ -9,6 +9,7 @@
#include <linux/nvmem-provider.h>
#include <linux/platform_device.h>
#include <linux/regmap.h>
+#include <linux/spmi.h>
#define SDAM_MEM_START 0x40
#define REGISTER_MAP_ID 0x40
@@ -20,7 +21,6 @@
struct sdam_chip {
struct regmap *regmap;
struct nvmem_config sdam_config;
- unsigned int base;
unsigned int size;
};
@@ -73,7 +73,7 @@ static int sdam_read(void *priv, unsigned int offset, void *val,
return -EINVAL;
}
- rc = regmap_bulk_read(sdam->regmap, sdam->base + offset, val, bytes);
+ rc = regmap_bulk_read(sdam->regmap, offset, val, bytes);
if (rc < 0)
dev_err(dev, "Failed to read SDAM offset %#x len=%zd, rc=%d\n",
offset, bytes, rc);
@@ -100,7 +100,7 @@ static int sdam_write(void *priv, unsigned int offset, void *val,
return -EINVAL;
}
- rc = regmap_bulk_write(sdam->regmap, sdam->base + offset, val, bytes);
+ rc = regmap_bulk_write(sdam->regmap, offset, val, bytes);
if (rc < 0)
dev_err(dev, "Failed to write SDAM offset %#x len=%zd, rc=%d\n",
offset, bytes, rc);
@@ -110,8 +110,17 @@ static int sdam_write(void *priv, unsigned int offset, void *val,
static int sdam_probe(struct platform_device *pdev)
{
+ struct regmap_config sdam_regmap_config = {
+ .reg_bits = 16,
+ .val_bits = 8,
+ .max_register = 0x100,
+ .fast_io = true,
+ };
struct sdam_chip *sdam;
struct nvmem_device *nvmem;
+ struct spmi_device *sparent;
+ struct spmi_subdevice *sub_sdev;
+ struct device *dev = &pdev->dev;
unsigned int val;
int rc;
@@ -119,19 +128,24 @@ static int sdam_probe(struct platform_device *pdev)
if (!sdam)
return -ENOMEM;
- sdam->regmap = dev_get_regmap(pdev->dev.parent, NULL);
- if (!sdam->regmap) {
- dev_err(&pdev->dev, "Failed to get regmap handle\n");
- return -ENXIO;
- }
+ sparent = to_spmi_device(dev->parent);
+ sub_sdev = devm_spmi_subdevice_alloc_and_add(dev, sparent);
+ if (IS_ERR(sub_sdev))
+ return PTR_ERR(sub_sdev);
- rc = of_property_read_u32(pdev->dev.of_node, "reg", &sdam->base);
+ rc = of_property_read_u32(dev->of_node, "reg", &sdam_regmap_config.reg_base);
if (rc < 0) {
dev_err(&pdev->dev, "Failed to get SDAM base, rc=%d\n", rc);
return -EINVAL;
}
- rc = regmap_read(sdam->regmap, sdam->base + SDAM_SIZE, &val);
+ sdam->regmap = devm_regmap_init_spmi_ext(&sub_sdev->sdev, &sdam_regmap_config);
+ if (IS_ERR(sdam->regmap)) {
+ dev_err(&pdev->dev, "Failed to get regmap handle\n");
+ return PTR_ERR(sdam->regmap);
+ }
+
+ rc = regmap_read(sdam->regmap, SDAM_SIZE, &val);
if (rc < 0) {
dev_err(&pdev->dev, "Failed to read SDAM_SIZE rc=%d\n", rc);
return -EINVAL;
@@ -159,7 +173,7 @@ static int sdam_probe(struct platform_device *pdev)
}
dev_dbg(&pdev->dev,
"SDAM base=%#x size=%u registered successfully\n",
- sdam->base, sdam->size);
+ sdam_regmap_config.reg_base, sdam->size);
return 0;
}
@@ -181,3 +195,4 @@ module_platform_driver(sdam_driver);
MODULE_DESCRIPTION("QCOM SPMI SDAM driver");
MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS("SPMI");
--
2.51.0Hello,
On Tue, Sep 16, 2025 at 10:44:40AM +0200, AngeloGioacchino Del Regno wrote:
> @@ -119,19 +128,24 @@ static int sdam_probe(struct platform_device *pdev)
> if (!sdam)
> return -ENOMEM;
>
> - sdam->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> - if (!sdam->regmap) {
> - dev_err(&pdev->dev, "Failed to get regmap handle\n");
> - return -ENXIO;
> - }
> + sparent = to_spmi_device(dev->parent);
> + sub_sdev = devm_spmi_subdevice_alloc_and_add(dev, sparent);
> + if (IS_ERR(sub_sdev))
> + return PTR_ERR(sub_sdev);
>
> - rc = of_property_read_u32(pdev->dev.of_node, "reg", &sdam->base);
> + rc = of_property_read_u32(dev->of_node, "reg", &sdam_regmap_config.reg_base);
It's a bit ugly that you pass the address of an unsigned int as u32*.
But this isn't new, so fine for me. (Also for all Linux archs we have
sizeof(unsigned int) == 4, so AFAICT it's safe anyhow.)
> if (rc < 0) {
> dev_err(&pdev->dev, "Failed to get SDAM base, rc=%d\n", rc);
> return -EINVAL;
> }
>
> - rc = regmap_read(sdam->regmap, sdam->base + SDAM_SIZE, &val);
> + sdam->regmap = devm_regmap_init_spmi_ext(&sub_sdev->sdev, &sdam_regmap_config);
> + if (IS_ERR(sdam->regmap)) {
> + dev_err(&pdev->dev, "Failed to get regmap handle\n");
dev_err_probe()
> + return PTR_ERR(sdam->regmap);
> + }
> +
> + rc = regmap_read(sdam->regmap, SDAM_SIZE, &val);
> if (rc < 0) {
> dev_err(&pdev->dev, "Failed to read SDAM_SIZE rc=%d\n", rc);
> return -EINVAL;
> @@ -159,7 +173,7 @@ static int sdam_probe(struct platform_device *pdev)
> }
> dev_dbg(&pdev->dev,
> "SDAM base=%#x size=%u registered successfully\n",
> - sdam->base, sdam->size);
> + sdam_regmap_config.reg_base, sdam->size);
>
> return 0;
> }
> @@ -181,3 +195,4 @@ module_platform_driver(sdam_driver);
>
> MODULE_DESCRIPTION("QCOM SPMI SDAM driver");
> MODULE_LICENSE("GPL v2");
> +MODULE_IMPORT_NS("SPMI");
If it's exactly the files that #include <linux/spmi.h> should have that
namespace import, you can put the MODULE_IMPORT_NS into that header.
Best regards
Uwe
On Tue, Sep 16, 2025 at 03:24:56PM +0200, Uwe Kleine-König wrote:
> On Tue, Sep 16, 2025 at 10:44:40AM +0200, AngeloGioacchino Del Regno wrote:
...
> > +MODULE_IMPORT_NS("SPMI");
>
> If it's exactly the files that #include <linux/spmi.h> should have that
> namespace import, you can put the MODULE_IMPORT_NS into that header.
Which makes anyone to import namespace even if they just want to use some types
out of the header. This is not good solution generally speaking. Also this will
diminish one of the purposes of _NS variants of MODULE*/EXPORT*, i.e. make it
invisible that some of the code may become an abuser of the API just by someone
include the header (for a reason or by a mistake).
--
With Best Regards,
Andy Shevchenko
On Tue, Sep 16, 2025 at 04:35:35PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 16, 2025 at 03:24:56PM +0200, Uwe Kleine-König wrote:
> > On Tue, Sep 16, 2025 at 10:44:40AM +0200, AngeloGioacchino Del Regno wrote:
>
> ...
>
> > > +MODULE_IMPORT_NS("SPMI");
> >
> > If it's exactly the files that #include <linux/spmi.h> should have that
> > namespace import, you can put the MODULE_IMPORT_NS into that header.
>
> Which makes anyone to import namespace even if they just want to use some types
> out of the header.
Notice that I carefully formulated my suggestion to cope for this case.
> This is not good solution generally speaking. Also this will
> diminish one of the purposes of _NS variants of MODULE*/EXPORT*, i.e. make it
> invisible that some of the code may become an abuser of the API just by someone
> include the header (for a reason or by a mistake).
Yeah, opinions differ. In my eyes it's quite elegant.
Best regards
Uwe
On Tue, Sep 16, 2025 at 6:11 PM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
> On Tue, Sep 16, 2025 at 04:35:35PM +0300, Andy Shevchenko wrote:
> > On Tue, Sep 16, 2025 at 03:24:56PM +0200, Uwe Kleine-König wrote:
> > > On Tue, Sep 16, 2025 at 10:44:40AM +0200, AngeloGioacchino Del Regno wrote:
...
> > > > +MODULE_IMPORT_NS("SPMI");
> > >
> > > If it's exactly the files that #include <linux/spmi.h> should have that
> > > namespace import, you can put the MODULE_IMPORT_NS into that header.
> >
> > Which makes anyone to import namespace even if they just want to use some types
> > out of the header.
>
> Notice that I carefully formulated my suggestion to cope for this case.
And I carefully answered. Your proposal won't prevent _other_ files to
use the same header in the future without needing a namespace to be
imported.
> > This is not good solution generally speaking. Also this will
> > diminish one of the purposes of _NS variants of MODULE*/EXPORT*, i.e. make it
> > invisible that some of the code may become an abuser of the API just by someone
> > include the header (for a reason or by a mistake).
>
> Yeah, opinions differ. In my eyes it's quite elegant.
It's not a pure opinion, it has a technical background that I
explained. The explicit usage of MODULE_IMPORT_NS() is better than
some header somewhere that might even be included by another and be
proxied to the code that doesn't need / want to have this namespace to
be present. Puting MODULE_IMPORT_NS() into a _header_ is a minefield
for the future.
--
With Best Regards,
Andy Shevchenko
Hello Andy,
On Tue, Sep 16, 2025 at 07:20:20PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 16, 2025 at 6:11 PM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> > On Tue, Sep 16, 2025 at 04:35:35PM +0300, Andy Shevchenko wrote:
> > > On Tue, Sep 16, 2025 at 03:24:56PM +0200, Uwe Kleine-König wrote:
> > > > On Tue, Sep 16, 2025 at 10:44:40AM +0200, AngeloGioacchino Del Regno wrote:
>
> ...
>
> > > > > +MODULE_IMPORT_NS("SPMI");
> > > >
> > > > If it's exactly the files that #include <linux/spmi.h> should have that
> > > > namespace import, you can put the MODULE_IMPORT_NS into that header.
> > >
> > > Which makes anyone to import namespace even if they just want to use some types
> > > out of the header.
> >
> > Notice that I carefully formulated my suggestion to cope for this case.
>
> And I carefully answered.
I tend to disagree. If that anyone only wants some type from the header
but not the namespace, the if part of my statement isn't given any more.
Still your reply felt like objection while logically it's not.
> Your proposal won't prevent _other_ files to
> use the same header in the future without needing a namespace to be
> imported.
Oh yes. But that's true for every change: If you change it further you
have to cope for what is already there.
> > > This is not good solution generally speaking. Also this will
> > > diminish one of the purposes of _NS variants of MODULE*/EXPORT*, i.e. make it
> > > invisible that some of the code may become an abuser of the API just by someone
> > > include the header (for a reason or by a mistake).
> >
> > Yeah, opinions differ. In my eyes it's quite elegant.
>
> It's not a pure opinion,
That's your opinion :-)
> it has a technical background that I
> explained. The explicit usage of MODULE_IMPORT_NS() is better than
> some header somewhere that might even be included by another and be
> proxied to the code that doesn't need / want to have this namespace to
> be present. Puting MODULE_IMPORT_NS() into a _header_ is a minefield
> for the future.
Well, for a deliberate abuser the hurdle to have to add the explicit
MODULE_IMPORT_NS() isn't that big. And a mistaken abuser won't explode,
just generate a few bytes overhead that can be fixed when noticed.
In my opinion that is an ok cost for the added elegance.
Best regards
Uwe
On Wed, Sep 17, 2025 at 02:47:22PM +0200, Uwe Kleine-König wrote:
> On Tue, Sep 16, 2025 at 07:20:20PM +0300, Andy Shevchenko wrote:
> > On Tue, Sep 16, 2025 at 6:11 PM Uwe Kleine-König
> > <u.kleine-koenig@baylibre.com> wrote:
> > > On Tue, Sep 16, 2025 at 04:35:35PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Sep 16, 2025 at 03:24:56PM +0200, Uwe Kleine-König wrote:
> > > > > On Tue, Sep 16, 2025 at 10:44:40AM +0200, AngeloGioacchino Del Regno wrote:
...
> > > > > > +MODULE_IMPORT_NS("SPMI");
> > > > >
> > > > > If it's exactly the files that #include <linux/spmi.h> should have that
> > > > > namespace import, you can put the MODULE_IMPORT_NS into that header.
> > > >
> > > > Which makes anyone to import namespace even if they just want to use some types
> > > > out of the header.
> > >
> > > Notice that I carefully formulated my suggestion to cope for this case.
> >
> > And I carefully answered.
>
> I tend to disagree. If that anyone only wants some type from the header
> but not the namespace, the if part of my statement isn't given any more.
> Still your reply felt like objection while logically it's not.
You assumed that in case that the header that is *currently* included in the
users, may be the one that used by the same users that needs an imported
namespace. Okay, *now* (or today) it's not a problem, but *in the future* it
might be *when* one wants to use *just* types from it.
I don't think this is likely to happen, but in general including something
"by default" is not a good idea. That's what I'm objecting to.
> > Your proposal won't prevent _other_ files to
> > use the same header in the future without needing a namespace to be
> > imported.
>
> Oh yes. But that's true for every change: If you change it further you
> have to cope for what is already there.
>
> > > > This is not good solution generally speaking. Also this will
> > > > diminish one of the purposes of _NS variants of MODULE*/EXPORT*, i.e. make it
> > > > invisible that some of the code may become an abuser of the API just by someone
> > > > include the header (for a reason or by a mistake).
> > >
> > > Yeah, opinions differ. In my eyes it's quite elegant.
> >
> > It's not a pure opinion,
>
> That's your opinion :-)
All we said is just set of opinions. Facts are provided by scientific
experiments.
> > it has a technical background that I
> > explained. The explicit usage of MODULE_IMPORT_NS() is better than
> > some header somewhere that might even be included by another and be
> > proxied to the code that doesn't need / want to have this namespace to
> > be present. Puting MODULE_IMPORT_NS() into a _header_ is a minefield
> > for the future.
>
> Well, for a deliberate abuser the hurdle to have to add the explicit
> MODULE_IMPORT_NS() isn't that big. And a mistaken abuser won't explode,
> just generate a few bytes overhead that can be fixed when noticed.
>
> In my opinion that is an ok cost for the added elegance.
I tend to disagree. The practice to include (be lazy) something just in case is
a bad practice. Developer has to know what they are doing. We have already too
much bad code in the kernel and opening new ways for more "vibe:ish" coding is
a road to accumulated issues in the future.
I,o.w. I principally disagree on putting MODULE_IMPORT_NS() into the header
file.
--
With Best Regards,
Andy Shevchenko
On Thu, Sep 18, 2025 at 10:00:29PM +0300, Andy Shevchenko wrote: > I,o.w. I principally disagree on putting MODULE_IMPORT_NS() into the header > file. Yes, please never do that, it defeats the purpose of module namespaces completly. If you don't want to have module namespaces, don't use them for your subsytem. Don't use them and then make them moot by putting MODULE_IMPORT_NS() in the .h file for the symbols as that's pointless. thanks, greg k-h
On 9/19/25 8:59 AM, Greg KH wrote: > On Thu, Sep 18, 2025 at 10:00:29PM +0300, Andy Shevchenko wrote: >> I,o.w. I principally disagree on putting MODULE_IMPORT_NS() into the header >> file. > > Yes, please never do that, it defeats the purpose of module namespaces > completly. If you don't want to have module namespaces, don't use them > for your subsytem. Don't use them and then make them moot by putting > MODULE_IMPORT_NS() in the .h file for the symbols as that's pointless. > > thanks, > > greg k-h Could someone suggest some additional explanation to add to Documentation/core-api/symbol-namespaces.rst to explain the reasoning behind this? Right now, the only part of that document that say _why_ we have module namespces says: That is useful for documentation purposes (think of the SUBSYSTEM_DEBUG namespace) as well as for limiting the availability of a set of symbols for use in other parts of the kernel. So I don't see the connection between this explanation and and: [Putting MODULE_IMPORT_NS() into the header] defeats the purpose of module namespaces completely. I am guilty of putting it in a header, so if I need to fix that I would like to actually understand why first. Andy has mentioned something about potential abuses, but without any example, I haven't been able to understand what this would actually actually look like. Or maybe there is some other reason that Greg is thinking of that hasn't been mentioned yet?
On Fri, Sep 19, 2025 at 10:05:28AM -0500, David Lechner wrote: > On 9/19/25 8:59 AM, Greg KH wrote: > > On Thu, Sep 18, 2025 at 10:00:29PM +0300, Andy Shevchenko wrote: > >> I,o.w. I principally disagree on putting MODULE_IMPORT_NS() into the header > >> file. > > > > Yes, please never do that, it defeats the purpose of module namespaces > > completly. If you don't want to have module namespaces, don't use them > > for your subsytem. Don't use them and then make them moot by putting > > MODULE_IMPORT_NS() in the .h file for the symbols as that's pointless. > > > > thanks, > > > > greg k-h > > > Could someone suggest some additional explanation to add to > Documentation/core-api/symbol-namespaces.rst to explain the > reasoning behind this? > > Right now, the only part of that document that say _why_ we have > module namespces says: > > That is useful for documentation purposes (think of the > SUBSYSTEM_DEBUG namespace) as well as for limiting the > availability of a set of symbols for use in other parts > of the kernel. > > So I don't see the connection between this explanation and and: > > [Putting MODULE_IMPORT_NS() into the header] defeats > the purpose of module namespaces completely. > > I am guilty of putting it in a header, so if I need to fix that > I would like to actually understand why first. Andy has mentioned > something about potential abuses, but without any example, I haven't > been able to understand what this would actually actually look like. > Or maybe there is some other reason that Greg is thinking of that > hasn't been mentioned yet? Let me turn it around, _why_ would you want your exports in a namespace at all if you just are putting a MODULE_IMPORT_NS() in the .h file at the same time? What is this giving you at all compared to just a normal MODULE_EXPORT() marking for your exports? I know what it gives me when I don't put it in a .h file, but I think that might be different from what you are thinking here :) thanks, greg k-h
Hello Greg,
On Fri, Sep 19, 2025 at 05:13:35PM +0200, Greg KH wrote:
> On Fri, Sep 19, 2025 at 10:05:28AM -0500, David Lechner wrote:
> > On 9/19/25 8:59 AM, Greg KH wrote:
> > > On Thu, Sep 18, 2025 at 10:00:29PM +0300, Andy Shevchenko wrote:
> > >> I,o.w. I principally disagree on putting MODULE_IMPORT_NS() into the header
> > >> file.
> > >
> > > Yes, please never do that, it defeats the purpose of module namespaces
> > > completly. If you don't want to have module namespaces, don't use them
> > > for your subsytem. Don't use them and then make them moot by putting
> > > MODULE_IMPORT_NS() in the .h file for the symbols as that's pointless.
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> >
> > Could someone suggest some additional explanation to add to
> > Documentation/core-api/symbol-namespaces.rst to explain the
> > reasoning behind this?
> >
> > Right now, the only part of that document that say _why_ we have
> > module namespces says:
> >
> > That is useful for documentation purposes (think of the
> > SUBSYSTEM_DEBUG namespace) as well as for limiting the
> > availability of a set of symbols for use in other parts
> > of the kernel.
> >
> > So I don't see the connection between this explanation and and:
> >
> > [Putting MODULE_IMPORT_NS() into the header] defeats
> > the purpose of module namespaces completely.
> >
> > I am guilty of putting it in a header, so if I need to fix that
> > I would like to actually understand why first. Andy has mentioned
> > something about potential abuses, but without any example, I haven't
> > been able to understand what this would actually actually look like.
> > Or maybe there is some other reason that Greg is thinking of that
> > hasn't been mentioned yet?
>
> Let me turn it around, _why_ would you want your exports in a namespace
> at all if you just are putting a MODULE_IMPORT_NS() in the .h file at
> the same time? What is this giving you at all compared to just a normal
> MODULE_EXPORT() marking for your exports?
>
> I know what it gives me when I don't put it in a .h file, but I think
> that might be different from what you are thinking here :)
For me (who still today thinks it's elegant to put the MODULE_IMPORT_NS
in a header next to the declarations of the symbols in that namespace)
it's the documentation thing quoted above (e.g. modinfo lists the used
namespaces) and to unclutter the global namespace.
The latter was essentially the motivation to introduce symbol
namespaces, see the cover letter from back then[1].
Personally I don't see the relevance of making it harder for abusers.
A non-GPL module should be stopped by the symbol being exported using
EXPORT_SYMBOL_GPL() with or without namespaces. And for a binary
distribution of a module the need to add the MODULE_IMPORT_NS statement
in the source code is a quite low barrier for the evil binary module
distributor that IMHO doesn't justify to burden all regular users of a
namespace to have to do two things (#include + MODULE_IMPORT_NS) instead
of only one (include which implies the MODULE_IMPORT_NS) to make use of
said namespace.
And for GPL code: Who is actually an abuser of say devm_pwm_get()?
In my eyes any module should be free to use that function. And if it's
not sensible to do so, I would expect this to be discovered even without
the needed MODULE_IMPORT_NS("PWM") in the code. And if it's not
discovered that's fine for me, too. For me that's the spirit of Open
Source: If someone finds devm_pwm_get() useful, let them use it. Please
even tell me if you use it in a way I didn't expect, I'm glad to hear
about it.
I'm probably missing something.
Best regards
Uwe
On 9/19/25 10:13 AM, Greg KH wrote: > On Fri, Sep 19, 2025 at 10:05:28AM -0500, David Lechner wrote: >> On 9/19/25 8:59 AM, Greg KH wrote: >>> On Thu, Sep 18, 2025 at 10:00:29PM +0300, Andy Shevchenko wrote: >>>> I,o.w. I principally disagree on putting MODULE_IMPORT_NS() into the header >>>> file. >>> >>> Yes, please never do that, it defeats the purpose of module namespaces >>> completly. If you don't want to have module namespaces, don't use them >>> for your subsytem. Don't use them and then make them moot by putting >>> MODULE_IMPORT_NS() in the .h file for the symbols as that's pointless. >>> >>> thanks, >>> >>> greg k-h >> >> >> Could someone suggest some additional explanation to add to >> Documentation/core-api/symbol-namespaces.rst to explain the >> reasoning behind this? >> >> Right now, the only part of that document that say _why_ we have >> module namespces says: >> >> That is useful for documentation purposes (think of the >> SUBSYSTEM_DEBUG namespace) as well as for limiting the >> availability of a set of symbols for use in other parts >> of the kernel. >> >> So I don't see the connection between this explanation and and: >> >> [Putting MODULE_IMPORT_NS() into the header] defeats >> the purpose of module namespaces completely. >> >> I am guilty of putting it in a header, so if I need to fix that >> I would like to actually understand why first. Andy has mentioned >> something about potential abuses, but without any example, I haven't >> been able to understand what this would actually actually look like. >> Or maybe there is some other reason that Greg is thinking of that >> hasn't been mentioned yet? > > Let me turn it around, _why_ would you want your exports in a namespace > at all if you just are putting a MODULE_IMPORT_NS() in the .h file at > the same time? What is this giving you at all compared to just a normal > MODULE_EXPORT() marking for your exports? > > I know what it gives me when I don't put it in a .h file, but I think > that might be different from what you are thinking here :) > > thanks, > > greg k-h Up to now, my (naive) understanding was that the point module namespaces is to reduce the number of symbols in the global namespace because having too many symbols there was starting to cause problems. So moving symbols to another namespace was a "good thing".
On Fri, Sep 19, 2025 at 10:20:29AM -0500, David Lechner wrote:
> On 9/19/25 10:13 AM, Greg KH wrote:
> > On Fri, Sep 19, 2025 at 10:05:28AM -0500, David Lechner wrote:
> >> On 9/19/25 8:59 AM, Greg KH wrote:
> >>> On Thu, Sep 18, 2025 at 10:00:29PM +0300, Andy Shevchenko wrote:
> >>>> I,o.w. I principally disagree on putting MODULE_IMPORT_NS() into the header
> >>>> file.
> >>>
> >>> Yes, please never do that, it defeats the purpose of module namespaces
> >>> completly. If you don't want to have module namespaces, don't use them
> >>> for your subsytem. Don't use them and then make them moot by putting
> >>> MODULE_IMPORT_NS() in the .h file for the symbols as that's pointless.
> >>>
> >>> thanks,
> >>>
> >>> greg k-h
> >>
> >>
> >> Could someone suggest some additional explanation to add to
> >> Documentation/core-api/symbol-namespaces.rst to explain the
> >> reasoning behind this?
> >>
> >> Right now, the only part of that document that say _why_ we have
> >> module namespces says:
> >>
> >> That is useful for documentation purposes (think of the
> >> SUBSYSTEM_DEBUG namespace) as well as for limiting the
> >> availability of a set of symbols for use in other parts
> >> of the kernel.
> >>
> >> So I don't see the connection between this explanation and and:
> >>
> >> [Putting MODULE_IMPORT_NS() into the header] defeats
> >> the purpose of module namespaces completely.
> >>
> >> I am guilty of putting it in a header, so if I need to fix that
> >> I would like to actually understand why first. Andy has mentioned
> >> something about potential abuses, but without any example, I haven't
> >> been able to understand what this would actually actually look like.
> >> Or maybe there is some other reason that Greg is thinking of that
> >> hasn't been mentioned yet?
> >
> > Let me turn it around, _why_ would you want your exports in a namespace
> > at all if you just are putting a MODULE_IMPORT_NS() in the .h file at
> > the same time? What is this giving you at all compared to just a normal
> > MODULE_EXPORT() marking for your exports?
> >
> > I know what it gives me when I don't put it in a .h file, but I think
> > that might be different from what you are thinking here :)
> >
> > thanks,
> >
> > greg k-h
>
> Up to now, my (naive) understanding was that the point module namespaces
> is to reduce the number of symbols in the global namespace because having
> too many symbols there was starting to cause problems. So moving symbols
> to another namespace was a "good thing".
Yes, it is a "good thing" overall, but by just making all of your
symbols in a namespace, and then including it in the .h file, that does
the same exact thing as before (i.e. anyone that includes that .h file
puts the symbols into the global namespace with that prefix.)
Ideally, the goal was to be able to easily see in a module, what symbol
namespaces they depend on, which requires them to put MODULE_IMPORT_NS()
in the module to get access to those symbols. dmabuf has done this very
well, making it obvious to the maintainers of that subsystem that they
should be paying attention to those users.
For other "tiny" subsystems, it just slots away their symbols so that no
one else should ever be using them, and it makes it blindingly obvious
if they do. For example, the usb-storage symbols, anyone that does:
MODULE_IMPORT_NS("USB_STORAGE");
had better be living in drivers/usb/storage/ otherwise I need to have a
word with those offenders :)
So it's a way of "tidying" up things, and to make things more explicit
than just having to rely on searching a tree and looking for .h include
usage. Right now, you are kind of defeating that by just allowing a .h
to be included and you don't get any benifit of being able to watch out
for who is actually using those symbols overall.
Hope this helps,
greg k-h
Il 16/09/25 18:20, Andy Shevchenko ha scritto:
> On Tue, Sep 16, 2025 at 6:11 PM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
>> On Tue, Sep 16, 2025 at 04:35:35PM +0300, Andy Shevchenko wrote:
>>> On Tue, Sep 16, 2025 at 03:24:56PM +0200, Uwe Kleine-König wrote:
>>>> On Tue, Sep 16, 2025 at 10:44:40AM +0200, AngeloGioacchino Del Regno wrote:
>
> ...
>
>>>>> +MODULE_IMPORT_NS("SPMI");
>>>>
>>>> If it's exactly the files that #include <linux/spmi.h> should have that
>>>> namespace import, you can put the MODULE_IMPORT_NS into that header.
>>>
>>> Which makes anyone to import namespace even if they just want to use some types
>>> out of the header.
>>
>> Notice that I carefully formulated my suggestion to cope for this case.
>
> And I carefully answered. Your proposal won't prevent _other_ files to
> use the same header in the future without needing a namespace to be
> imported.
>
>>> This is not good solution generally speaking. Also this will
>>> diminish one of the purposes of _NS variants of MODULE*/EXPORT*, i.e. make it
>>> invisible that some of the code may become an abuser of the API just by someone
>>> include the header (for a reason or by a mistake).
>>
>> Yeah, opinions differ. In my eyes it's quite elegant.
>
> It's not a pure opinion, it has a technical background that I
> explained. The explicit usage of MODULE_IMPORT_NS() is better than
> some header somewhere that might even be included by another and be
> proxied to the code that doesn't need / want to have this namespace to
> be present. Puting MODULE_IMPORT_NS() into a _header_ is a minefield
> for the future.
>
Uwe, thanks for your review - much appreciated.
Even though I get your point... Sorry, but here I do agree with Andy, and I think
he explained some of the reasons pretty well.
Cheers,
Angelo
© 2016 - 2025 Red Hat, Inc.