Re: [PATCH RFC v3 5/7] ipmi: ls2k: Relax the dependency to its mfd driver

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

New issues:
- [Medium] The commit message incorrectly claims the change allows the MFD core driver to be compiled as a module, but the target config is a boolean.

Pre-existing issues:
- [High] ipmi_si_ls2k_init() ignores the return value of platform_driver_register() and unconditionally sets ls2k_registered = true.
- [High] The MFD driver dynamically mutates a shared static global `mfd_cell` array using a stack pointer, violating MFD subsystem rules and causing a data race.
- [High] The driver executes a massive busy-wait of up to 13 seconds inside `stop_machine()`, triggering hard lockups.
--

--- Patch [5]: [PATCH RFC v3 5/7] ipmi: ls2k: Relax the dependency to its mfd driver ---
commit 8c0e83d6cff2cc10a75154a2292052f88af85328
Author: Miao Wang <[email protected]>

ipmi: ls2k: Relax the dependency to its mfd driver

This commit changes the dependency of the IPMI driver on its MFD core
driver from select to imply. This is intended to allow the MFD driver to
be built as a module while the IPMI driver remains built-in or part of
ipmi_si.

> There is no strong dependency between the IPMI driver and its mfd
> driver. Although the IPMI driver will not work without the mfd driver,
> it is not a hard dependency. The IPMI driver can actually be compiled
> without the mfd driver, and it will just fail to probe. When the mfd
> driver is loaded, the IPMI driver will probe successfully. Therefore,
> the dependency of the IPMI driver on its mfd driver should be relaxed
> to "imply" from "select". This will allow the mfd driver to be compiled
> as a module and the IPMI driver to be compiled as a part of the ipmi_si
> module.

[Severity: Medium]
Is this description accurate regarding compiling the MFD driver as a module?
Looking at drivers/mfd/Kconfig, MFD_LS2K_BMC_CORE is defined as a bool,
meaning it can only be built-in (=y) or disabled (=n). Since a bool
cannot be compiled as a module, will this change actually allow modular
builds of the MFD driver?

> diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
> index 669f760001974..f456e3e894169 100644
> --- a/drivers/char/ipmi/Kconfig
> +++ b/drivers/char/ipmi/Kconfig
> @@ -87,7 +87,7 @@ config IPMI_IPMB
>  config IPMI_LS2K
>  	bool 'Loongson-2K IPMI interface'
>  	depends on LOONGARCH
> -	select MFD_LS2K_BMC_CORE
> +	imply MFD_LS2K_BMC_CORE
>  	help
>  	  Provides a driver for Loongson-2K IPMI interfaces.

[Severity: High]
This is a pre-existing issue, but does ipmi_si_ls2k_init() properly
handle registration failures?

drivers/char/ipmi/ipmi_si_ls2k.c:ipmi_si_ls2k_init() {
    platform_driver_register(&ipmi_ls2k_platform_driver);
    ls2k_registered = true;
}

If resource constraints like OOM cause platform_driver_register() to fail,
ls2k_registered is still set to true. During module unload or driver
shutdown, ipmi_si_ls2k_shutdown() checks this flag and calls
platform_driver_unregister() on an unregistered driver, which can trigger
a driver core WARNING.

[Severity: High]
This is a pre-existing issue, but does the MFD driver dynamically mutate a
shared global cell array using a stack pointer?

drivers/mfd/ls2k-bmc-core.c:
static struct mfd_cell ls2k_bmc_cells[] = {
...

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

MFD subsystem rules mandate that cell arrays must be static const. Because
this array is not const and is mutated at runtime with a local stack
variable pointer (pd), concurrent PCI probes of the BMC device could
overwrite each other's pointers in the global array. This could cause
mfd_add_devices() to clone garbage or freed stack memory from another
thread.

[Severity: High]
This is a pre-existing issue, but does the driver execute a massive
busy-wait inside stop_machine()?

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

Inside the callback:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_recover_pci_data() {
    for (i = LS7A_BAR0_CHECK_MAX_TIMES; i > 0 ; i--) {
        if (!ls2k_bmc_bar0_addr_is_set(parent))
            break;
        mdelay(1);
    }
    ...
    /* Waiting for U-Boot and DDR ready */
    mdelay(LS2K_BMC_RESET_WAIT_TIME);
}

stop_machine() disables interrupts on all CPUs. When an interrupt schedules
the BMC reset work, ls2k_bmc_recover_pci_data() is called in atomic context
with IRQs disabled system-wide. The callback executes a 2-second polling
loop and then an unconditional 10-second wait. Halting all CPUs with
interrupts disabled for up to 13 seconds can trigger NMI hard lockup panics
or RCU stalls.

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