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?