Re: [PATCH RFC v6 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:
- [High] Immediate disposal of IRQ mapping breaks GPIO interrupt and causes panic on module unload.
- [High] Dynamic mutation of global MFD cell array with a local stack pointer violates MFD API rules.
- [High] System-wide 13-second stall with interrupts disabled inside stop_machine() callback.
--

--- Patch [6]: [PATCH RFC v6 6/7] mfd: ls2kbmc: Able to be compiled as a module ---
commit 9c6121767eaa76246cc34b4848b6602e7e2a88b2
Author: Miao Wang <[email protected]>

mfd: ls2kbmc: Able to be compiled as a module

Make ls2kmc able to be compiled as a module

> 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]
This isn't a bug introduced by this patch, but by allowing this driver to
be built as a module, it exposes an unbind and unload crash path.

In drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_init(), the code unregisters the
GSI immediately after requesting the IRQ:

    ret = devm_request_irq(ddata->dev, gpio_irq, ls2k_bmc_interrupt,
                           IRQF_SHARED | IRQF_TRIGGER_FALLING, "ls2kbmc gpio", ddata);
    if (ret)
        dev_err(ddata->dev, "Failed to request LS2KBMC GPIO IRQ %d.\n", gpio_irq);

    acpi_unregister_gsi(LS2K_BMC_RESET_GPIO_GSI);
    return ret;

Does this unconditional call to acpi_unregister_gsi() free the IRQ
descriptor while the interrupt is still actively requested? If the module
is unloaded, will the devres cleanup call free_irq() on a destroyed IRQ
descriptor and cause a kernel panic?

[Severity: High]
This is also a pre-existing issue, but it violates MFD subsystem rules
regarding static const cell arrays.

In drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_probe(), the driver mutates the
global ls2k_bmc_cells array:

    struct simplefb_platform_data pd;
    ...
    ls2k_bmc_cells[LS2K_BMC_DISPLAY].platform_data = &pd;
    ls2k_bmc_cells[LS2K_BMC_DISPLAY].pdata_size = sizeof(pd);

Since ls2k_bmc_cells is not declared const, does mutating this global
array with a local stack pointer leave a dangling pointer once the probe
function returns? Would concurrent probes of multiple BMC PCI devices
result in a structural data race?

[Severity: High]
This is another pre-existing issue that might cause severe instability.

In drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_events_fn(), the driver calls
stop_machine():

    stop_machine(ls2k_bmc_recover_pci_data, ddata, NULL);

Inside the callback ls2k_bmc_recover_pci_data(), the code performs a long
delay:

    /* Waiting for U-Boot and DDR ready */
    mdelay(LS2K_BMC_RESET_WAIT_TIME);

Since stop_machine() halts all CPUs and disables local interrupts, will
delaying for 10+ seconds inside this callback trigger the NMI hard lockup
watchdog and panic the host kernel?

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

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