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

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

Pre-existing issues:
- [Critical] The reset workqueue runs `stop_machine()` containing a 10-second `mdelay()`, freezing the entire system with local interrupts disabled.
- [High] The driver dynamically modifies the global `ls2k_bmc_cells` array during `probe()` to set `platform_data` to a stack-allocated pointer, violating MFD guidelines.
- [High] The shared IRQ handler blindly returns IRQ_HANDLED without verifying the interrupt source, preventing spurious interrupt detection.
- [High] The driver unconditionally unregisters the ACPI GSI immediately after successfully requesting the GPIO IRQ, destroying the active IRQ mapping.
- [Medium] The `last_jiffies` variable in the interrupt handler is declared as `static`, inappropriately sharing rate-limiting state globally across all devices and interrupt lines.
--

--- Patch [4]: [PATCH RFC v7 4/8] mfd: ls2kbmc: Cancel the work queue on removal ---
commit c07197c793489ef5a3fafe072398e2f5a992c868
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 06ec8143d8cdb..5eea53f134215 100644
> --- a/drivers/mfd/ls2k-bmc-core.c
> +++ b/drivers/mfd/ls2k-bmc-core.c
> @@ -375,6 +375,12 @@ static void ls2k_bmc_save_pci_data(struct pci_dev *pdev, struct ls2k_bmc_ddata *
>  	pci_read_config_dword(pdev, PCI_INTERRUPT_LINE, &ddata->bmc_pci_data.interrupt_line);
>  }
>  
> +static void ls2k_bmc_cancel_wq(void *data)
> +{
> +	struct ls2k_bmc_ddata *ddata = data;
> +	(void) cancel_work_sync(&ddata->bmc_reset_work);
> +}
> +
>  static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
>  {
>  	struct pci_dev *pdev = to_pci_dev(ddata->dev);
> @@ -385,6 +391,10 @@ static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
>  
>  	INIT_WORK(&ddata->bmc_reset_work, ls2k_bmc_events_fn);
>  
> +	ret = devm_add_action_or_reset(ddata->dev, ls2k_bmc_cancel_wq, ddata);
> +	if (ret)
> +		return ret;
> +
>  	ret = devm_request_irq(&pdev->dev, pdev->irq, ls2k_bmc_interrupt,
>  			       IRQF_SHARED | IRQF_TRIGGER_FALLING, "ls2kbmc pcie", ddata);
>  	if (ret) {

While reviewing this patch, I noticed a few pre-existing issues in the driver:

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;
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to assign a local
stack variable to a global structure?

The ls2k_bmc_cells array is global, but the platform_data is pointing to pd,
which is allocated on the stack. Once ls2k_bmc_probe() returns, wouldn't this
leave a dangling pointer in the global array?

Also, dynamically modifying global cell arrays appears to violate MFD subsystem
guidelines.


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() {
    ...
    /* Waiting for U-Boot and DDR ready */
    mdelay(LS2K_BMC_RESET_WAIT_TIME);
    if (!ls2k_bmc_bar0_addr_is_set(parent))
        return false;
    ...
}

[Severity: Critical]
This is a pre-existing issue, but can this 10-second delay cause a system
panic?

Since stop_machine() parks all CPUs and disables local interrupts, won't
executing mdelay(10000) inside ls2k_bmc_recover_pci_data() trigger the NMI
watchdog or cause a hard lockup?


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 isn't a bug introduced by this patch, but should last_jiffies be
declared as static here?

Because the variable is static, its state is shared globally across both the
PCI-E and GPIO IRQ lines, and across all instances of the device. Does this
create a data race if interrupts fire concurrently, or cause one interrupt
line to inappropriately throttle the other?


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

    return IRQ_HANDLED;
}

[Severity: High]
This is a pre-existing issue, but does this handler correctly check if the
hardware actually generated the interrupt?

Both the PCI-E and GPIO IRQs are registered with IRQF_SHARED. Since this
function unconditionally returns IRQ_HANDLED without checking hardware
registers, would it suppress the spurious interrupt detector and cause an
interrupt storm if another device on the same shared IRQ line fires?


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;
}

[Severity: High]
This isn't a bug introduced by this patch, but does acpi_unregister_gsi()
destroy the active IRQ mapping?

If acpi_unregister_gsi() calls irq_dispose_mapping(), won't the underlying
descriptor be freed while the IRQ handler remains registered? Will incoming
hardware interrupts fail to be delivered, or could devm_free_irq() attempt a
double-free during device teardown?

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