Re: [PATCH RFC v6 4/7] mfd: ls2kbmc: Cancel the work queue on removal

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

Pre-existing issues:
- [Critical] The work queue callback executes a prolonged 10-second busy-wait delay (`mdelay(10000)`) inside `stop_machine`, triggering an immediate NMI watchdog panic.
- [High] Calling `acpi_unregister_gsi` immediately after requesting the IRQ destroys the IRQ mapping while it is actively in use.
- [High] Dynamically modifying the global `mfd_cell` array with a stack-allocated platform data pointer violates MFD guidelines and creates a race condition and use-after-free vulnerability.
- [High] The interrupt rate-limiting logic uses a 0-initialized shared static variable, causing global cross-device interference and silently ignoring all interrupts for the first ~6 minutes of system uptime.
--

--- Patch [4]: [PATCH RFC v6 4/7] mfd: ls2kbmc: Cancel the work queue on removal ---
commit fad0e9633fb652b609b571c7eefab49f76d46696
Author: Miao Wang <[email protected]>

    mfd: ls2kbmc: Cancel the work queue on removal
    
    When the device is being removeed, the work queue should be canceled to
    avoid any pending work to be executed after the device is removed.

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index e12f2ca2fbef3..e7bebfe696ac4 100644
> --- a/drivers/mfd/ls2k-bmc-core.c
> +++ b/drivers/mfd/ls2k-bmc-core.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but does this destroy the IRQ mapping while
it is still actively in use?

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_init() {
...
	gpio_irq = acpi_register_gsi(NULL, LS2K_BMC_RESET_GPIO_GSI, ACPI_EDGE_SENSITIVE,
				     ACPI_ACTIVE_LOW);
	if (gpio_irq < 0)
		return gpio_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;
}

Calling acpi_unregister_gsi() immediately after requesting the IRQ destroys
the mapping via irq_dispose_mapping(), which permanently breaks the BMC
reset GPIO interrupt handler on every initialization.

[Severity: High]
This is a pre-existing issue, but does this create a use-after-free by saving
a stack pointer into a global array?

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_probe() {
	struct simplefb_platform_data pd;
...
	ret = ls2k_bmc_parse_mode(dev, &pd);
	if (ret)
		return ret;

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

Writing the stack-allocated &pd pointer to the global ls2k_bmc_cells array
leaves a dangling pointer once probe() returns. Furthermore, modifying a
global mfd_cell array at runtime violates MFD subsystem rules, which require
cell arrays to be static const.

[Severity: High]
This is a pre-existing issue, but will this silently drop interrupts for
the first several minutes of system uptime?

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)) {
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Since jiffies is initialized to -300 * HZ at boot and the shared static
last_jiffies is 0, time_after() evaluates to false until jiffies crosses
the positive threshold. Doesn't this result in all hardware interrupts being
ignored for approximately the first 6 minutes of system uptime? Also, using
a shared static state causes interference if there are multiple device
instances.

[Severity: Critical]
This is a pre-existing issue, but does this cause an NMI watchdog panic?

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_events_fn() {
...
	/*
	 * The PCI-E is lost when the BMC resets, at which point access to the PCI-E
	 * from other CPUs is suspended to prevent a crash.
	 */
	stop_machine(ls2k_bmc_recover_pci_data, ddata, NULL);
        ^^^^^^^^^^^^

Which executes ls2k_bmc_recover_pci_data() on all CPUs with local interrupts
disabled:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_recover_pci_data() {
...
	/* Check if PCI-E is connected */
	if (!ls2k_bmc_pcie_is_connected(parent, ddata))
		return false;

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

LS2K_BMC_RESET_WAIT_TIME is 10000. Invoking mdelay(10000) inside
stop_machine() forces the entire system to stall with interrupts disabled for
10 seconds. Will this trigger the NMI watchdog hard lockup detector and
result in a kernel panic?

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