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

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

Pre-existing issues:
- [High] System Hang / Watchdog Panic due to 13+ second mdelay() in stop_machine()
- [High] Broken hardware interrupt due to immediate GSI unregistration
- [Medium] MFD Subsystem Violation: Dynamic modification of a global static MFD cell array
- [Medium] Concurrency Bug: Interrupt handler uses a static local variable for rate-limiting
--

--- Patch [6]: [PATCH RFC v4 6/7] mfd: ls2kbmc: Able to be compiled as a module ---
commit 25dc6cbaebb11df8a5a5578847da95bd91aa28f3
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"
>  	depends on PCI && ACPI_GENERIC_GSI
>  	select MFD_CORE
>  	help

Since this patch allows the driver to be compiled as a module, it exposes
several pre-existing issues in drivers/mfd/ls2k-bmc-core.c.

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

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

[Severity: High]
This is a pre-existing issue, but does this trigger a watchdog panic?
Calling mdelay() for a long time inside a stop_machine() callback halts all
CPUs with local interrupts disabled. Can this destroy system latency, RCU
state, and inevitably trigger a kernel panic due to the Hard Lockup Detector?

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

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

[Severity: Medium]
This isn't a bug introduced by this patch, but does this dynamically modify a
global static array using a local stack variable? The MFD subsystem mandates 
that mfd_cell arrays must be static const.

Modifying the global array with a stack pointer leaves a dangling pointer
after the probe returns, and introduces a race condition if multiple devices
are probed concurrently.

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_init() {
    ...
	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;
}

[Severity: High]
This is a pre-existing issue, but does this immediately tear down the hardware
interrupt mapping on the success path? Unregistering the GSI calls
irq_dispose_mapping(), which destroys the mapping between the hardware
interrupt and the Linux IRQ number.

Will this prevent the interrupt from ever firing?

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_interrupt() {
    ...
	struct ls2k_bmc_ddata *ddata = arg;
	static unsigned long last_jiffies;

	if (system_state != SYSTEM_RUNNING)
		return IRQ_HANDLED;

	/* Skip interrupt in LS2K_BMC_INT_INTERVAL */
	if (time_after(jiffies, last_jiffies + LS2K_BMC_INT_INTERVAL)) {
		schedule_work(&ddata->bmc_reset_work);
		last_jiffies = jiffies;
	}
    ...
}

[Severity: Medium]
This is a pre-existing issue, but does this use shared state across all device
instances to rate-limit interrupts globally instead of per-device?

If multiple BMC devices exist in a system, an interrupt on one device will
erroneously lock out interrupt handling on all other BMC devices for the
interval. Can this state be tracked within the device-specific private
structure instead?

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