Duplicate calls to regmap_debugfs_init() through regmap_attach_dev()

Duplicate calls to regmap_debugfs_init() through regmap_attach_dev()

[Matthias Schiffer]

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

Re: Duplicate calls to regmap_debugfs_init() through regmap_attach_dev()

[Matthias Schiffer]

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?

Re: Duplicate calls to regmap_debugfs_init() through regmap_attach_dev()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 695 bytes --]

On Mon, Jul 19, 2021 at 03:53:38PM +0200, Matthias Schiffer wrote:

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

That's not recent but anyway...

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

Yes, trying to set the device on a regmap that already has a device is
not a good idea, if the syscon code is doing it transparently as part of
lookup then syscon users shouldn't do it by hand.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]