Re: [PATCH RFC v7 6/8] mfd: ls2kbmc: Able to be compiled as a module

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

New issues:
- [High] Making the driver a loadable module exposes a missing hardware interrupt teardown on module unload, risking spurious interrupt storms.
- [Low] The commit message violates MFD subsystem formatting and naming rules.

Pre-existing issues:
- [Critical] The driver invokes `stop_machine()` to execute a 10-second blocking delay and perform `ioremap()` (via `pci_iomap()`), leading to guaranteed NMI watchdogs, deadlocks, and system stalls.
- [High] The driver modifies a global static `mfd_cell` array during probe with a pointer to a stack-allocated variable, violating the MFD `static const` rule and creating a race condition.
- [High] The driver unconditionally destroys its GSI interrupt mapping immediately after successfully requesting it, breaking BMC reset detection.
- [High] A generic PCI driver bypasses resource discovery to hardcode a SoC physical MMIO address.
--

--- Patch [6]: [PATCH RFC v7 6/8] mfd: ls2kbmc: Able to be compiled as a module ---
commit e0d6c702d53af0c3fc808b9dac2ceb61eef6aaf1
Author: Miao Wang <[email protected]>

mfd: ls2kbmc: Able to be compiled as a module
    
Make ls2kmc able to be compiled as a module

> mfd: ls2kbmc: Able to be compiled as a module
> 
> Make ls2kmc able to be compiled as a module

[Severity: Low]
This isn't a bug introduced by this patch, but there is a typo (ls2kmc) in
the commit body, and the prefix uses a mismatched driver name (ls2kbmc
instead of ls2k-bmc).

Does it make sense to update this to strictly follow the MFD subsystem Git
history categorization rules?

> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 763ce6a34782b..a7a9f97af248c 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2494,7 +2494,7 @@ config MFD_LOONGSON_SE
>  	  which will forward them to the corresponding engine.
>  
>  config MFD_LS2K_BMC_CORE
> -	bool "Loongson-2K Board Management Controller Support"
> +	tristate "Loongson-2K Board Management Controller Support"

[Severity: High]
Does making this driver a loadable module expose a missing hardware
interrupt teardown?

Looking at drivers/mfd/ls2k-bmc-core.c, ls2k_bmc_init() explicitly enables
the hardware GPIO interrupt:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_init() {
    ...
    writel(val | BIT(LS2K_BMC_RESET_GPIO), gpio_base + LOONGSON_GPIO_INTEN);
    ...
}

However, the driver lacks a .remove callback to clean up the hardware state
when the module is unloaded:

static struct pci_driver ls2k_bmc_driver = {
    ...
};

If the module is unloaded, could the hardware remain armed and lead to
spurious interrupt storms if a BMC reset occurs subsequently?


[Severity: Critical]
This is a pre-existing issue, but could this driver cause system deadlocks
by invoking stop_machine() to execute a blocking delay and perform ioremap()?

Looking at ls2k_bmc_events_fn(), it calls stop_machine():

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_events_fn() {
    ...
    stop_machine(ls2k_bmc_recover_pci_data, ddata, NULL);
    ...
}

Inside ls2k_bmc_recover_pci_data(), it performs a 10-second busy wait:

    mdelay(LS2K_BMC_RESET_WAIT_TIME);

Additionally, in the same atomic context, it calls pci_iomap() which invokes
ioremap() inside ls2k_bmc_pcie_is_connected():

    base = pci_iomap(parent, 0, LS7A_PCI_CFG_SIZE);

Could calling a 10-second delay and ioremap() (which might send IPIs for
TLB shootdowns) inside a stop_machine() context cause NMI watchdogs and
system deadlocks?


[Severity: High]
This isn't a bug introduced by this patch, but is it safe to modify a global
static mfd_cell array during probe with a pointer to a stack-allocated
variable?

The MFD subsystem requires mfd_cell arrays to be static const, but it is
defined as mutable:

static struct mfd_cell ls2k_bmc_cells[] = {

During ls2k_bmc_probe(), it dynamically assigns the address of a local stack
variable pd:

    ls2k_bmc_cells[LS2K_BMC_DISPLAY].platform_data = &pd;

When probe() returns, does this leave a dangling pointer in the global array,
potentially causing a data race if concurrent probes occur?


[Severity: High]
This is a pre-existing issue, but does the initialization sequence
unconditionally destroy its GSI interrupt mapping after successfully
requesting it?

In ls2k_bmc_init(), it calls acpi_unregister_gsi() in the success path:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_init() {
    ...
    acpi_unregister_gsi(LS2K_BMC_RESET_GPIO_GSI);
    ...
}

Will this completely dispose of the IRQ mapping, preventing BMC reset
interrupts from being routed?


[Severity: High]
This isn't a bug introduced by this patch, but shouldn't a generic PCI
driver rely on standard resource discovery instead of hardcoding a physical
MMIO address?

In ls2k_bmc_init(), it calls ioremap() on a hardcoded constant:

    gpio_base = ioremap(LOONGSON_GPIO_REG_BASE, LOONGSON_GPIO_REG_SIZE);

Could this cause resource conflicts and break platform portability by bypassing
mechanisms like PCI BARs?

>  	depends on PCI && ACPI_GENERIC_GSI
>  	select MFD_CORE
>  	help

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6