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: <eb27b79ce46bde0202a4e7b047a3aaec8338fb6d.camel@ew.tq-group.com>
Subject: 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>
Cc:     linux-kernel@vger.kernel.org, Dong Aisheng <aisheng.dong@nxp.com>,
        Fabio Estevam <festevam@gmail.com>,
        Shawn Guo <shawnguo@kernel.org>,
        Stefan Agner <stefan@agner.ch>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        NXP Linux Team <linux-imx@nxp.com>,
        linux-arm-kernel@lists.infradead.org,
        Lee Jones <lee.jones@linaro.org>, Arnd Bergmann <arnd@arndb.de>
Date:   Mon, 19 Jul 2021 15:53:38 +0200
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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