Messages in this thread | | | Subject | Re: (EXT) Re: [PATCH] regmap: do not call regmap_debugfs_init() from regmap_attach_dev() | From | Matthias Schiffer <> | Date | Tue, 27 Jul 2021 14:24:17 +0200 |
| |
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. >
| |