Re: [PATCH 1/4] bcma: support driver specific quirks from soc pdata

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Synthesized write to BCMA_IOCTL inadvertently clears bits 17-31 of the SHIM Control register.
- [High] Missing `block_read` and `block_write` callbacks in `bcma_host_soc_ops_brcm_shim` will cause a NULL pointer dereference when CONFIG_BCMA_BLOCKIO is enabled.

Pre-existing issues:
- [High] The bcm47xx_wdt watchdog timer is not canceled when the platform device is unregistered during bus teardown, leading to a Use-After-Free when the software timer expires.
--

commit dfe73fc3413d87faa9dc228ee7a188e34676dd31
Author: Alessio Ferri <alessio.ferri@mythread.it>

bcma: support driver specific quirks from soc pdata

This commit adds support for BMIPS xDSL SoCs by instantiating bcma-host-soc
programmatically and routing accesses through a SHIM Control register. It
also introduces big-endian accessors for SoCs where the AXI backplane
sits on a big-endian peripheral bus.

> diff --git a/drivers/bcma/host_soc.c b/drivers/bcma/host_soc.c
> index 20b1816c570b2..f39129fb9cf29 100644
> --- a/drivers/bcma/host_soc.c
> +++ b/drivers/bcma/host_soc.c
[ ... ]
> @@ -165,6 +166,195 @@ static const struct bcma_host_ops bcma_host_soc_ops = {
[ ... ]
> +static void bcma_host_soc_synth_awrite32(struct bcma_device *core,
> +					 u16 offset, u32 value)
> +{
> +	void __iomem *ctrl_reg = bcma_host_soc_shim_ctrl_reg(core);
> +	u32 cur, new_val;
> +
> +	if (ctrl_reg) {
> +		switch (offset) {
> +		case BCMA_IOCTL:
> +			/* SICF low 16 bits == BCMA_IOCTL. Preserve
> +			 * SICF_WOC_CORE_RESET (the RESET_CTL view) so an
> +			 * IOCTL write does not accidentally release reset.
> +			 */
> +			cur = ioread32be(ctrl_reg);
> +			new_val = (value & 0xFFFF) |
> +				  (cur & SICF_WOC_CORE_RESET);

[Severity: High]
Does this synthesized write inadvertently clear bits 17-31 of the SHIM Control
register?

By only keeping SICF_WOC_CORE_RESET (bit 16) from the current register value,
all other bits from 17 to 31 will be dropped. If the hardware or firmware
relies on any configuration or state in those upper bits, this operation could
lead to register state corruption. Would a mask like (cur & ~0xFFFF) be better
to preserve all unmanaged bits?

> +			iowrite32be(new_val, ctrl_reg);
> +			pr_debug("bcma: synth IOCTL core=0x%x SHIM %08x->%08x (req %08x)\n",
> +				 core->id.id, cur, new_val, value);
> +			return;
[ ... ]
> +static const struct bcma_host_ops bcma_host_soc_ops_brcm_shim = {
> +	.read8		= bcma_host_soc_read8,
> +	.read16		= bcma_host_soc_read16_be,
> +	.read32		= bcma_host_soc_read32_be,
> +	.write8		= bcma_host_soc_write8,
> +	.write16	= bcma_host_soc_write16_be,
> +	.write32	= bcma_host_soc_write32_be,
> +	.aread32	= bcma_host_soc_aread32_be,
> +	.awrite32	= bcma_host_soc_awrite32_be,
> +};

[Severity: High]
Can omitting block_read and block_write callbacks here cause a null pointer
dereference when CONFIG_BCMA_BLOCKIO is enabled?

If a driver like b43 configured in PIO mode calls the bcma_block_read() or
bcma_block_write() inline wrappers, those functions unconditionally call
core->bus->ops->block_read without checking if the pointer is null. Might
these functions need to be safely stubbed out?

> +
>  int __init bcma_host_soc_register(struct bcma_soc *soc)
>  {
[ ... ]
> @@ -245,7 +443,9 @@ static void bcma_host_soc_remove(struct platform_device *pdev)
>  	struct bcma_bus *bus = platform_get_drvdata(pdev);
>  
>  	bcma_bus_unregister(bus);

[Severity: High]
This is a pre-existing issue, but does unregistering the bus cause a
use-after-free with the bcm47xx_wdt watchdog timer?

Calling bcma_bus_unregister() unbinds the bcm47xx_wdt driver. Since the
driver lacks a remove function, it relies on devm_watchdog_unregister_device
for cleanup. However, it does not set WDOG_STOP_ON_UNREGISTER, so if the
watchdog was active, its software timer remains running.

Because the memory holding the timer resides in the platform data, which is
freed immediately upon unregister, will the timer dereference freed memory
when it expires?

> -	iounmap(bus->mmio);
> +	/* bus->mmio is devm-managed; shim_iomem is borrowed from the
> +	 * parent bridge driver and must not be unmapped here.
> +	 */
>  	platform_set_drvdata(pdev, NULL);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529-add-bcm6362-wlan-v1-0-722242777f58@gmail.com?part=1