Re: [PATCH v2 3/7] mfd: Add a shared BCMBCA PMC3 PMB transport

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mfd: bcmbca_pmc3_pmb: debugfs stall, WARN_ON, and HW read in bcmbca_pmb_regmap_init()
- [Low] mfd: bcmbca_pmc3_pmb: mfd_cell match data and unused drvdata in bcmbca_pmb_probe()
--

commit cbd0d7381d66545c4a7f2cd3781134426406ab49
Author: Linus Walleij <linusw@kernel.org>

mfd: Add a shared BCMBCA PMC3 PMB transport

This commit adds an MFD driver for the Broadcom BCMBCA third generation Power
Management Controller (PMC3) Processor Monitor Bus (PMB) on BCM6846, exposing
a shared regmap to child devices while managing hardware keyhole semaphore
locking and recovery.

>     mfd: Add a shared BCMBCA PMC3 PMB transport
>
>     Add an MFD driver for shared access to the Processor Monitor Bus (PMB)
>     of the third generation Power Management Controller (PMC3), initially
>     for BCM6846.
>
>     Expose an uncached regmap to all children, encoding the bus, device and
>     word offset in each register address. Keep keyhole semaphore acquisition
>     and transaction recovery inside the MFD driver.
>
>     A software timeout does not abort a transaction. If the keyhole is still
>     busy, retain its hardware lock and reject new callers until delayed work
>     observes completion and releases it.
>
>     The keyhole register layout differs from older Broadcom PMB controllers.
>     Match only BCM6846 for now.
>
>     Signed-off-by: Linus Walleij <linusw@kernel.org>

> diff --git a/drivers/mfd/bcmbca_pmc3_pmb.c b/drivers/mfd/bcmbca_pmc3_pmb.c
> new file mode 100644
> index 0000000000000..837143409ce9c
> --- /dev/null
> +++ b/drivers/mfd/bcmbca_pmc3_pmb.c
[ ... ]
> +struct bcmbca_pmb_data {
> +	const struct mfd_cell *cells;
> +	unsigned int num_cells;
> +};
[ ... ]
> +static bool bcmbca_pmb_precious_reg(struct device *dev, unsigned int reg)
> +{
> +	/* Do not let debugfs probe unimplemented or firmware-owned registers. */
> +	return true;
> +}
> +
> +static struct regmap *bcmbca_pmb_regmap_init(struct bcmbca_pmb *pmb)
> +{
> +	const struct regmap_config config = {
> +		.reg_bits = 32,
> +		.val_bits = 32,
> +		.reg_stride = 1,
> +		.max_register = BCMBCA_PMB_REG(3, 255, 65535),
> +		.cache_type = REGCACHE_NONE,
> +		.precious_reg = bcmbca_pmb_precious_reg,
> +		.reg_read = bcmbca_pmb_reg_read,
> +		.reg_write = bcmbca_pmb_reg_write,
> +		.lock = bcmbca_pmb_regmap_lock,
> +		.unlock = bcmbca_pmb_regmap_unlock,
> +		.can_sleep = true,
> +		.lock_arg = pmb,
> +	};
> +
> +	return devm_regmap_init(pmb->dev, NULL, pmb, &config);
> +}

[Severity: Medium]
What happens when CONFIG_DEBUG_FS is enabled and a user reads the regmap
debugfs files (/sys/kernel/debug/regmap/<dev>/registers, range, or access)
with max_register set to BCMBCA_PMB_REG(3, 255, 65535) (0x03ffffff) and
bcmbca_pmb_precious_reg() returning true for all registers?

Because config.max_register is non-zero and readable_reg is unset,
regmap_debugfs_init() creates the registers, range, and access files.

When reading the registers or range file, regmap_debugfs_get_dump_start()
scans from 0 to 0x03ffffff (67,108,864 iterations) under map->cache_lock.
Because every register is marked precious, regmap_printable() returns false
for every address, leaving map->debugfs_off_cache empty and triggering a
WARN_ON on every read:

drivers/base/regmap/regmap-debugfs.c:regmap_debugfs_get_dump_start()
	WARN_ON(list_empty(&map->debugfs_off_cache));
	ret = base;

Furthermore, regmap_debugfs_get_dump_start() then returns base (0) to
regmap_read_debugfs(), which enters its loop at start_reg = 0 without checking
regmap_printable(map, start_reg) and performs a hardware PMB read of register
0 (bus 0, device 0, offset 0) on the first iteration:

drivers/base/regmap/regmap-debugfs.c:regmap_read_debugfs()
	for (i = start_reg; i >= 0 && i <= to;
	     i = regmap_next_readable_reg(map, i)) {
		...
		ret = regmap_read(map, i, &val);

Similarly, reading the access debugfs file causes regmap_access_show() to
iterate over all 67,108,864 addresses and call seq_printf() for each one,
repeatedly doubling the seq_file buffer until kvzalloc() fails with -ENOMEM.

Should max_register be left unset in bcmbca_pmb_regmap_init() (removing the
need for bcmbca_pmb_precious_reg()) so regmap_debugfs_init() does not create
the registers, range, and access debugfs files for this sparse 26-bit address
space?

[ ... ]
> +static int bcmbca_pmb_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct bcmbca_pmb_data *data = device_get_match_data(dev);
[ ... ]
> +	platform_set_drvdata(pdev, pmb);
> +	pmb->regmap = bcmbca_pmb_regmap_init(pmb);
> +	if (IS_ERR(pmb->regmap))
> +		return dev_err_probe(dev, PTR_ERR(pmb->regmap), "failed to register PMB regmap\n");
> +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, data->cells,
> +				   data->num_cells, NULL, 0, NULL);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to register PMB functions\n");
> +
> +	return 0;
> +}
> +
> +static const struct mfd_cell bcm6846_pmb_cells[] = {
> +	{
> +		.name = "bcm6846-pmb-clock",
> +		.of_compatible = "brcm,bcm6846-pmb-clock",
> +	},
> +	{
> +		.name = "bcm6846-pmb-reset",
> +		.of_compatible = "brcm,bcm6846-pmb-reset",
> +	},
> +	{
> +		.name = "bcm6846-pmb-pvtmon",
> +		.of_compatible = "brcm,bcm6846-pmb-pvtmon",
> +	},
> +};
> +
> +static const struct bcmbca_pmb_data bcm6846_pmb_data = {
> +	.cells = bcm6846_pmb_cells,
> +	.num_cells = ARRAY_SIZE(bcm6846_pmb_cells),
> +};
> +
> +static const struct of_device_id bcmbca_pmb_match[] = {
> +	{ .compatible = "brcm,bcm6846-pmb", .data = &bcm6846_pmb_data },
> +	{ }
> +};

[Severity: Low]
This isn't a bug, but in MFD drivers, passing mfd_cell arrays through the
.data field of struct of_device_id is discouraged in favor of either passing
bcm6846_pmb_cells and ARRAY_SIZE(bcm6846_pmb_cells) directly to
devm_mfd_add_devices() while only one compatible is matched, or storing a
device ID enum in .data and selecting the static const mfd_cell array with a
switch in bcmbca_pmb_probe().

Also, are platform_set_drvdata(pdev, pmb) and struct bcmbca_pmb::regmap needed
in bcmbca_pmb_probe(), given that struct bcmbca_pmb is private to
drivers/mfd/bcmbca_pmc3_pmb.c and child drivers look up the regmap via
dev_get_regmap(dev->parent, NULL)?

Finally, could the commit subject include the driver prefix to match MFD
conventions (for example, "mfd: bcmbca-pmb: Add a shared BCMBCA PMC3 PMB
transport")?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101-xg6846b-bcmbca-xrdp-reset-v2-0-455fe9eed5ee@kernel.org?part=3