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

[Matthias Schiffer <matthias.schiffer@ew.tq-group.com>]

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.
>