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

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

Pre-existing issues:
- [Critical] The driver executes a 10+ second busy-wait and a sleeping func=
tion inside a `stop_machine()` context, severely stalling the entire system.
- [High] `acpi_unregister_gsi()` is erroneously called immediately after re=
questing the IRQ, destroying the mapping and breaking interrupt delivery.
- [Medium] The global `mfd_cell` array is dynamically modified using a stac=
k-allocated pointer and lacks the `const` qualifier, violating MFD subsyste=
m guidelines.
--

--- Patch [6]: [PATCH RFC v5 6/7] mfd: ls2kbmc: Able to be compiled as a mo=
dule ---
commit 998b814232bcf86316d4b7feaee8e3da166c0a9e
Author: Miao Wang <[email protected]>

mfd: ls2kbmc: Able to be compiled as a module

Make ls2kmc able to be compiled as a module by changing the Kconfig option
from bool to tristate.

> 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.
> =20
>  config MFD_LS2K_BMC_CORE
> -	bool "Loongson-2K Board Management Controller Support"
> +	tristate "Loongson-2K Board Management Controller Support"

[Severity: High]
This is a pre-existing issue, but enabling this driver to be built as a
module exposes it during module unload or driver unbind.

Does this code in ls2k_bmc_init() destroy the GSI mapping while the IRQ
is still in use?

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

Calling acpi_unregister_gsi() unconditionally right after devm_request_irq()
succeeds disposes of the mapping. Hardware interrupts will never reach the
registered handler. Furthermore, during module unload or driver unbind,
devm_free_irq() will attempt to free an IRQ whose mapping and descriptor
have already been disposed, potentially causing a use-after-free or kernel
warning.

[Severity: Medium]
This is a pre-existing issue, but unbinding and rebinding the module could
trigger a race condition here.

Does this assign the address of a local stack variable to a global array?

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_probe() {
    ...
    ls2k_bmc_cells[LS2K_BMC_DISPLAY].platform_data =3D &pd;
    ...
}

Once probe returns, the platform_data pointer points to freed stack memory.
If another instance of the device binds, or if the driver unbinds and
rebinds, this dangling pointer could be used or overwritten concurrently.
Additionally, modifying a global mfd_cell array dynamically violates MFD
subsystem guidelines which require it to be static const.

[Severity: Critical]
This isn't a bug introduced by this patch, but there is a severe system
stall issue in the event handler.

Does this sequence execute a 10,000ms delay and call a sleeping function
inside an atomic context?

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

Inside the callback, it invokes a 10-second busy wait:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_recover_pci_data() {
    ...
    mdelay(LS2K_BMC_RESET_WAIT_TIME);
    ...
}

It also calls a function that can sleep:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_pcie_is_connected() {
    ...
    base =3D pci_iomap(parent, 0, LS7A_PCI_CFG_SIZE);
    ...
}

The workqueue handler ls2k_bmc_events_fn() calls stop_machine(), which
runs ls2k_bmc_recover_pci_data() while all other CPUs spin with local
interrupts disabled. Stalling the entire system with interrupts disabled
for over 10 seconds can lead to system-wide lockups, RCU stalls, and NMI
watchdog panics. Also, pci_iomap() can acquire locks and sleep, which is
unsafe inside stop_machine().

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260804-ls2kbmc-mo=
[email protected]?part=3D6