[PATCH] regmap: do not call regmap_debugfs_init() from regmap_attach_dev()

[PATCH] regmap: do not call regmap_debugfs_init() from regmap_attach_dev()

[Matthias Schiffer]

regmap_debugfs_init() should never be called twice for the same regmap,
as it initializes various fields of the regmap struct, including list
heads and mutices. A visible symptom are messages like:

    debugfs: Directory 'dummy-iomuxc-gpr@20e4000' with parent 'regmap'
    already present!

This happened whenever regmap_attach_dev() was called for an existing
regmap. Remove the call from regmap_attach_dev() and change
__regmap_init() so that regmap_debugfs_init() is called exactly once.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
Fixes: 9b947a13e7f6 ("regmap: use debugfs even when no device")
---
 drivers/base/regmap/regmap.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)


As mentioned in my previous mail [1], I believe that the duplicate call to
regmap_debugfs_init() is not the only issue with regmap_attach_dev().

Every single user of regmap_attach_dev() outside of __regmap_init() is
using it to attach a device to a syscon regmap obtained using one of the
syscon_*() functions. AIUI, syscon regmaps do not belong to a single
device, so calling regmap_attach_dev() seems wrong to me. My (possibly
lacking) understanding of the semantics aside, the fact that
regmap_attach_dev() is setting fields on the shared regmap without any
kind of locking is at least suspicious.

Maybe regmap_attach_dev() shouldn't be exported from the regmap code at
all, removing all calls to the functions from drivers?


[1] https://lkml.org/lkml/2021/7/19/500


diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index fe3e38dd5324..27625a1330ac 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -630,8 +630,6 @@ int regmap_attach_dev(struct device *dev, struct regmap *map,
 	if (ret)
 		return ret;
 
-	regmap_debugfs_init(map);
-
 	/* Add a devres resource for dev_get_regmap() */
 	m = devres_alloc(dev_get_regmap_release, sizeof(*m), GFP_KERNEL);
 	if (!m) {
@@ -1192,10 +1190,10 @@ struct regmap *__regmap_init(struct device *dev,
 		ret = regmap_attach_dev(dev, map, config);
 		if (ret != 0)
 			goto err_regcache;
-	} else {
-		regmap_debugfs_init(map);
 	}
 
+	regmap_debugfs_init(map);
+
 	return map;
 
 err_regcache:
-- 
2.17.1

Re: [PATCH] regmap: do not call regmap_debugfs_init() from regmap_attach_dev()

[Mark Brown]

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

On Mon, Jul 26, 2021 at 09:36:27AM +0200, Matthias Schiffer wrote:
> regmap_debugfs_init() should never be called twice for the same regmap,
> as it initializes various fields of the regmap struct, including list
> heads and mutices. A visible symptom are messages like:
> 
>     debugfs: Directory 'dummy-iomuxc-gpr@20e4000' with parent 'regmap'
>     already present!
> 
> This happened whenever regmap_attach_dev() was called for an existing
> regmap. Remove the call from regmap_attach_dev() and change
> __regmap_init() so that regmap_debugfs_init() is called exactly once.

The use case for regmap_attach_dev() is that there was no device when
the regmap was initially instantiated due to it running very early, we
want to attach the device when we figure out what it is which includes
setting up the debugfs stuff.  Whatever is managing to call this with
the same device as has already been set is clearly not that use case.

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

Re: [PATCH] regmap: do not call regmap_debugfs_init() from regmap_attach_dev()

[Matthias Schiffer]

On Mon, 2021-07-26 at 12:47 +0100, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Mon, Jul 26, 2021 at 09:36:27AM +0200, Matthias Schiffer wrote:
> > regmap_debugfs_init() should never be called twice for the same regmap,
> > as it initializes various fields of the regmap struct, including list
> > heads and mutices. A visible symptom are messages like:
> > 
> >     debugfs: Directory 'dummy-iomuxc-gpr@20e4000' with parent 'regmap'
> >     already present!
> > 
> > This happened whenever regmap_attach_dev() was called for an existing
> > regmap. Remove the call from regmap_attach_dev() and change
> > __regmap_init() so that regmap_debugfs_init() is called exactly once.
> 
> The use case for regmap_attach_dev() is that there was no device when
> the regmap was initially instantiated due to it running very early, we
> want to attach the device when we figure out what it is which includes
> setting up the debugfs stuff.  Whatever is managing to call this with
> the same device as has already been set is clearly not that use case.
> 
> 

Hi Mark,

I'm not talking about a case where regmap_attach_dev() is called when
there is already a device attached; as far as I can tell such a thing
does not happen in current kernel code.

Please have a look at the commit in the Fixes: tag. The duplicate
regmap_debugfs_init() happens even when no device was passed in
__regmap_init(), so the regmap_attach_dev() is the first time a device
it attached.

Re: [PATCH] regmap: do not call regmap_debugfs_init() from regmap_attach_dev()

[Mark Brown]

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

On Mon, Jul 26, 2021 at 02:01:51PM +0200, Matthias Schiffer wrote:
> On Mon, 2021-07-26 at 12:47 +0100, Mark Brown wrote:

> > The use case for regmap_attach_dev() is that there was no device when
> > the regmap was initially instantiated due to it running very early, we
> > want to attach the device when we figure out what it is which includes
> > setting up the debugfs stuff.  Whatever is managing to call this with
> > the same device as has already been set is clearly not that use case.

> I'm not talking about a case where regmap_attach_dev() is called when
> there is already a device attached; as far as I can tell such a thing
> does not happen in current kernel code.

So in that case how are we managing to create a debugfs file with the
same name given (which was the problem you were reporting) that the
device name is embedded in the name of the debugfs file?

> Please have a look at the commit in the Fixes: tag. The duplicate
> regmap_debugfs_init() happens even when no device was passed in
> __regmap_init(), so the regmap_attach_dev() is the first time a device
> it attached.

That's not what your patch says it's fixing, your patch says it's
fixing an attempt to recreate the same directory as we had originally
(we should probably clean up the one with no device but that's not what
your commit does).  I think what you need to look at here is that we
store map->debugfs_name and don't overwrite it when the device is
supplied.

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

Re: [PATCH] regmap: do not call regmap_debugfs_init() from regmap_attach_dev()

[Matthias Schiffer]

On Mon, 2021-07-26 at 13:11 +0100, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Mon, Jul 26, 2021 at 02:01:51PM +0200, Matthias Schiffer wrote:
> > On Mon, 2021-07-26 at 12:47 +0100, Mark Brown wrote:
> > > The use case for regmap_attach_dev() is that there was no device when
> > > the regmap was initially instantiated due to it running very early, we
> > > want to attach the device when we figure out what it is which includes
> > > setting up the debugfs stuff.  Whatever is managing to call this with
> > > the same device as has already been set is clearly not that use case.
> > I'm not talking about a case where regmap_attach_dev() is called when
> > there is already a device attached; as far as I can tell such a thing
> > does not happen in current kernel code.
> 
> So in that case how are we managing to create a debugfs file with the
> same name given (which was the problem you were reporting) that the
> device name is embedded in the name of the debugfs file?
> 
> > Please have a look at the commit in the Fixes: tag. The duplicate
> > regmap_debugfs_init() happens even when no device was passed in
> > __regmap_init(), so the regmap_attach_dev() is the first time a device
> > it attached.
> 
> That's not what your patch says it's fixing, your patch says it's
> fixing an attempt to recreate the same directory as we had originally
> (we should probably clean up the one with no device but that's not what
> your commit does).  I think what you need to look at here is that we
> store map->debugfs_name and don't overwrite it when the device is
> supplied.
> 

That would be fine if regmap_debugfs_init() didn't do a lot more than
just create the debugfs directory. I'm more concerned about the mutex
and list head initialization that is happening on an already
initialized structure. I haven't looked in detail what the mutex and
list head are used for, but I assume bad things™ are going to happen
when someone is already holding the mutex or using the list.

Re: [PATCH] regmap: do not call regmap_debugfs_init() from regmap_attach_dev()

[Mark Brown]

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

On Mon, Jul 26, 2021 at 02:18:42PM +0200, Matthias Schiffer wrote:
> On Mon, 2021-07-26 at 13:11 +0100, Mark Brown wrote:

> > That's not what your patch says it's fixing, your patch says it's
> > fixing an attempt to recreate the same directory as we had originally
> > (we should probably clean up the one with no device but that's not what
> > your commit does).  I think what you need to look at here is that we
> > store map->debugfs_name and don't overwrite it when the device is
> > supplied.

> That would be fine if regmap_debugfs_init() didn't do a lot more than
> just create the debugfs directory. I'm more concerned about the mutex

The whole point here is to move the debugfs directory so if any fix
stops that happening it's not really viable.  If we knew that devices
were definitely going to have a device bound we could just defer till
the device is bound but it's not clear to me that that will always
happen.

> and list head initialization that is happening on an already
> initialized structure. I haven't looked in detail what the mutex and
> list head are used for, but I assume bad things™ are going to happen
> when someone is already holding the mutex or using the list.

They're used to cache information on where registers are located in the
debugfs files so seeks work much faster on large register maps, they
won't be doing anything if userspace isn't up yet which should really be
the case for anything that's initializing early enough that it needed to
have a regmap prior to the driver model being up.  You're right that
there is a potential issue there though, but that can be handled
separately.

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

Re: (EXT) Re: [PATCH] regmap: do not call regmap_debugfs_init() from regmap_attach_dev()

[Matthias Schiffer]

On Mon, 2021-07-26 at 19:48 +0100, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Mon, Jul 26, 2021 at 02:18:42PM +0200, Matthias Schiffer wrote:
> > On Mon, 2021-07-26 at 13:11 +0100, Mark Brown wrote:
> > > That's not what your patch says it's fixing, your patch says it's
> > > fixing an attempt to recreate the same directory as we had originally
> > > (we should probably clean up the one with no device but that's not what
> > > your commit does).  I think what you need to look at here is that we
> > > store map->debugfs_name and don't overwrite it when the device is
> > > supplied.
> > That would be fine if regmap_debugfs_init() didn't do a lot more than
> > just create the debugfs directory. I'm more concerned about the mutex
> 
> The whole point here is to move the debugfs directory so if any fix
> stops that happening it's not really viable.

Looking at the history, I assume this already broke with cffa4b2122f5
("regmap: debugfs: Fix a memory leak when calling regmap_attach_dev").
This is why the kernel is trying to recreate the "dummy" debugfs
directory on my system when regmap_attach_dev() is called by imx-
pinctrl.

I'm not convinced that the behaviour before that commit was strictly
better - when regmap_debugfs_init() was called for the second time, the
new debugfs paths would be created, but the old ones were never
removed, they just leaked.


>
>   If we knew that devices
> were definitely going to have a device bound we could just defer till
> the device is bound but it's not clear to me that that will always
> happen.

Right, there are definitely cases where that's not happening - the
mentioned syscon driver is a prime example, as it creates regmaps that
don't belong to a single device, but are shared between different
drivers. In most cases, nobody ever binds a device to these regmaps.

The thing on which I need clarification is whether it is okay to bind a
device to these shared regmaps at all:

There is nothing preventing two different drivers from calling
regmap_attach_dev() on the same regmap (AFAICT, this is actually
happening when both imx_rproc and reset-imx7 are enabled, as both use
the same syscon "SRC").

There is also nothing preventing one driver from calling
regmap_attach_dev() while another is accessing the regmap.

What I'm trying to find out here is if there are any legitimate users
of regmap_attach_dev(). If there aren't any, we can remove the API and
don't need to fix it.


> 
> > and list head initialization that is happening on an already
> > initialized structure. I haven't looked in detail what the mutex and
> > list head are used for, but I assume bad things™ are going to happen
> > when someone is already holding the mutex or using the list.
> 
> They're used to cache information on where registers are located in the
> debugfs files so seeks work much faster on large register maps, they
> won't be doing anything if userspace isn't up yet which should really be
> the case for anything that's initializing early enough that it needed to
> have a regmap prior to the driver model being up.  You're right that
> there is a potential issue there though, but that can be handled
> separately.
> 

Re: (EXT) Re: [PATCH] regmap: do not call regmap_debugfs_init() from regmap_attach_dev()

[Mark Brown]

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

On Tue, Jul 27, 2021 at 02:24:17PM +0200, Matthias Schiffer wrote:
> On Mon, 2021-07-26 at 19:48 +0100, Mark Brown wrote:

> > The whole point here is to move the debugfs directory so if any fix
> > stops that happening it's not really viable.

> Looking at the history, I assume this already broke with cffa4b2122f5
> ("regmap: debugfs: Fix a memory leak when calling regmap_attach_dev").
> This is why the kernel is trying to recreate the "dummy" debugfs
> directory on my system when regmap_attach_dev() is called by imx-
> pinctrl.

Right, before that we'd just overwrite the existing name.

> I'm not convinced that the behaviour before that commit was strictly
> better - when regmap_debugfs_init() was called for the second time, the
> new debugfs paths would be created, but the old ones were never
> removed, they just leaked.

There's definitely a memory leak, although unless you're instantiating a
lot of these devices it's going to be hard to notice.

> The thing on which I need clarification is whether it is okay to bind a
> device to these shared regmaps at all:

> There is nothing preventing two different drivers from calling
> regmap_attach_dev() on the same regmap (AFAICT, this is actually
> happening when both imx_rproc and reset-imx7 are enabled, as both use
> the same syscon "SRC").

It's OK for one device to do it, but it should probably be the core
syscon code not some random driver that happens to talk to the syscon.
All the current users look at least somewhat suspicious unless they
somehow coordinate with each other in ways that I can't determine.

> What I'm trying to find out here is if there are any legitimate users
> of regmap_attach_dev(). If there aren't any, we can remove the API and
> don't need to fix it.

There's a definite use case for it.  What's probably more interesting
is if we have any users that create regmaps without a device, currently
I can't seem to find any though it's possible my greps weren't good
enough to spot them.  

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

Re: [PATCH] regmap: do not call regmap_debugfs_init() from regmap_attach_dev()

[Matthias Schiffer]

[adding syscon maintainers to CC again]

On Tue, 2021-07-27 at 18:08 +0100, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Jul 27, 2021 at 02:24:17PM +0200, Matthias Schiffer wrote:
> > On Mon, 2021-07-26 at 19:48 +0100, Mark Brown wrote:
> > > The whole point here is to move the debugfs directory so if any fix
> > > stops that happening it's not really viable.
> > Looking at the history, I assume this already broke with cffa4b2122f5
> > ("regmap: debugfs: Fix a memory leak when calling regmap_attach_dev").
> > This is why the kernel is trying to recreate the "dummy" debugfs
> > directory on my system when regmap_attach_dev() is called by imx-
> > pinctrl.
> 
> Right, before that we'd just overwrite the existing name.
> 
> > I'm not convinced that the behaviour before that commit was strictly
> > better - when regmap_debugfs_init() was called for the second time, the
> > new debugfs paths would be created, but the old ones were never
> > removed, they just leaked.
> 
> There's definitely a memory leak, although unless you're instantiating a
> lot of these devices it's going to be hard to notice.
> 
> > The thing on which I need clarification is whether it is okay to bind a
> > device to these shared regmaps at all:
> > There is nothing preventing two different drivers from calling
> > regmap_attach_dev() on the same regmap (AFAICT, this is actually
> > happening when both imx_rproc and reset-imx7 are enabled, as both use
> > the same syscon "SRC").
> 
> It's OK for one device to do it, but it should probably be the core
> syscon code not some random driver that happens to talk to the syscon.
> All the current users look at least somewhat suspicious unless they
> somehow coordinate with each other in ways that I can't determine.

The core syscon driver doesn't create a device anymore for regmaps
obtained using syscon_node_to_regmap() etc. since bdb0066df96e ("mfd:
syscon: Decouple syscon interface from platform devices") - so there is
no device the regmap could be bound to here.

(in fact, the platform_driver part of the syscon driver could have been
removed a while ago as suggested in that commit's description, as
nothing is using it anymore sice ~2018 - I'll send a patch to do that
later)

With no device available to own the regmap, leaving it unset seems
better to me than allowing an arbitrary consumer of the syscon regmap
to claim ownership.

> 
> > What I'm trying to find out here is if there are any legitimate users
> > of regmap_attach_dev(). If there aren't any, we can remove the API and
> > don't need to fix it.
> 
> There's a definite use case for it.  What's probably more interesting
> is if we have any users that create regmaps without a device, currently
> I can't seem to find any though it's possible my greps weren't good
> enough to spot them.  

`git grep 'regmap_init[^(]*(NULL'` gives me 5 places, one of them being
in the mentioned syscon driver (which in turn is used by 200~300 of
other drivers). There might be others where NULL isn't passed directly.

Re: [PATCH] regmap: do not call regmap_debugfs_init() from regmap_attach_dev()

[Mark Brown]

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

On Mon, Jul 26, 2021 at 09:36:27AM +0200, Matthias Schiffer wrote:

> lacking) understanding of the semantics aside, the fact that
> regmap_attach_dev() is setting fields on the shared regmap without any
> kind of locking is at least suspicious.

BTW there shouldn't be any harm in adding locking, but we're really
hoping that it shouldn't be required here as the caller ought to be
doing coordination which means things are single threaded anyway so we
didn't bother.

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