Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <75670688fbec759e8ab8b42356a16cca465dc430.camel@ew.tq-group.com>
Subject: Re: Duplicate calls to regmap_debugfs_init() through
 regmap_attach_dev()
From:   Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
To:     Mark Brown <broonie@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Lee Jones <lee.jones@linaro.org>, Arnd Bergmann <arnd@arndb.de>
Cc:     linux-kernel@vger.kernel.org, Dong Aisheng <aisheng.dong@nxp.com>,
        linux-arm-kernel@lists.infradead.org
Date:   Fri, 23 Jul 2021 09:34:26 +0200
In-Reply-To: <eb27b79ce46bde0202a4e7b047a3aaec8338fb6d.camel@ew.tq-group.com>
References: <eb27b79ce46bde0202a4e7b047a3aaec8338fb6d.camel@ew.tq-group.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 2021-07-19 at 15:53 +0200, Matthias Schiffer wrote:
> Hi everyone,
> 
> I hope I got the right list of maintainers for this issue, which seems
> to be rooted in the interaction between regmap, syscon and pinctrl-imx.
> 
> With recent kernels (observed on v5.10.y, but the code doesn't look
> significantly different on master/next) I've seen the following message
> on boot on i.MX6UL SoCs:
> 
> > debugfs: Directory 'dummy-iomuxc-gpr@20e4000' with parent 'regmap' already present!
> 
> I've tracked this down to this piece of code in the pinctrl-imx driver:
> 
> > 		gpr = syscon_regmap_lookup_by_compatible(info->gpr_compatible);
> > 		if (!IS_ERR(gpr))
> > 			regmap_attach_dev(&pdev->dev, gpr, &config);
> 
> __regmap_init() (called by syscon_regmap_lookup_by_compatible()) has:
> 
> > 	if (dev) {
> > 		ret = regmap_attach_dev(dev, map, config);
> > 		if (ret != 0)
> > 			goto err_regcache;
> > 	} else {
> > 		regmap_debugfs_init(map);
> > 	}
> 
> As dev is NULL in this call, regmap_debugfs_init() will be called.
> 
> pinctrl-imx then calls regmap_attach_dev(), which calls
> regmap_debugfs_init() again. Unless I'm missing something, this is very
> problematic: regmap_debugfs_init() does a lot more than just adding
> debugfs files - it also initializes list heads and mutices in the
> regmap structure.
> 
> It seems to me that there is no correct way to use regmap_attach_dev()
> from outside of __regmap_init(). In particular on a syscon regmap that
> may be shared between different drivers, setting map->dev looks wrong
> to me.
> 
> The total number of drivers that call regmap_attach_dev() is very low
> (I count 5), but all of them use it on a syscon regmap. Some of them
> perform further operations on the regmap as if they owned it, like
> modifying the cache configuration.
> 
> While not directly related, could anyone tell me why the locking around
> syscon_list in the syscon driver is correct (or if it is in fact
> incorrect)? It looks to me like two tasks might call
> device_node_get_regmap() at the same time, leading to two concurrent
> constructions of the same syscon regmap.
> 
> Kind regards,
> Matthias


Another question regarding the syscon driver: Does the syscon platform
device still have any use after bdb0066df96e ("mfd: syscon: Decouple
syscon interface from platform devices")? All exported syscon functions
use the regmaps stored in the global "syscon_list", which are
compeletely independent of the devices handled by syscon_probe().

As the syscon platform_driver doesn't do anything, it seems to me like
that part could just be removed, leaving only the handling of shared
regmaps. Maybe that code should live under drivers/base/regmap instead
of drivers/mfd?