lkml.org 
[lkml]   [2021]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: (EXT) Re: [PATCH] regmap: do not call regmap_debugfs_init() from regmap_attach_dev()
From
Date
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.
>



\
 
 \ /
  Last update: 2021-07-27 14:25    [W:0.091 / U:0.176 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site