Re: [PATCH RFC v3 4/7] 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 `stop_machine` callback executes operations that sleep (`pci_iomap`) and performs excessive busy-waiting (over 10 seconds), which will trigger kernel panics and hardware watchdogs.
- [High] The driver dynamically modifies the globally shared `ls2k_bmc_cells` array during `probe()` by assigning a stack-allocated variable to its `platform_data` field, violating MFD subsystem guidelines and causing data races.
- [High] The debouncing logic in the interrupt handler uses an uninitialized static variable `last_jiffies`, causing all interrupts to be ignored for the first ~5 minutes of uptime and creating a data race between multiple device instances.
- [High] The driver unconditionally unregisters the ACPI GSI immediately after successfully requesting its Linux IRQ, destroying the mapping while it is actively in use.
- [High] The shared interrupt handler unconditionally schedules work and returns `IRQ_HANDLED` without verifying if its device generated the interrupt, violating `IRQF_SHARED` semantics.
--

--- Patch [4]: [PATCH RFC v3 4/7] mfd: ls2kbmc: Cancel the work queue on removal ---
commit c019cbf289f699805da0540d5ca947d9e1feb7fd
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 a5465c42a77b8..1466b314fc4e5 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) {

[ ... ]

> static struct mfd_cell ls2k_bmc_cells[] = {
[ ... ]
> ls2k_bmc_cells[LS2K_BMC_DISPLAY].platform_data = &pd;
> ls2k_bmc_cells[LS2K_BMC_DISPLAY].pdata_size = sizeof(pd);

[Severity: High]
This is a pre-existing issue, but could dynamically modifying the global
ls2k_bmc_cells array during probe() with a stack-allocated variable create a
data race?

The MFD subsystem mandates that mfd_cell arrays must be declared as static
const to prevent local amendments. Assigning a local stack variable to the
global array can cause a data race if multiple instances are probed
concurrently, potentially overwriting another device's platform data with a
dangling pointer.

[ ... ]

> static unsigned long last_jiffies;
> ...
> if (time_after(jiffies, last_jiffies + LS2K_BMC_INT_INTERVAL)) {

[Severity: High]
This isn't a bug introduced by this patch, but is it possible that the
uninitialized static last_jiffies variable causes all interrupts to be
ignored for the first 5 minutes of uptime?

Since last_jiffies is implicitly initialized to 0 and jiffies starts at
-300*HZ, time_after() evaluates to false until jiffies wraps past 0.

Furthermore, does using a static variable here introduce a data race between
multiple device instances?

[ ... ]

> stop_machine(ls2k_bmc_recover_pci_data, ddata, NULL);
[ ... ]
> base = pci_iomap(parent, 0, LS7A_PCI_CFG_SIZE);
[ ... ]
> mdelay(LS2K_BMC_RESET_WAIT_TIME);

[Severity: Critical]
This is a pre-existing issue, but would calling stop_machine() here trigger a
regression by halting the system?

The ls2k_bmc_events_fn() invokes stop_machine(), which moves execution to an
atomic context with all CPUs halted and IRQs disabled.

Inside this callback, it calls ls2k_bmc_pcie_is_connected() which can sleep
via pci_iomap(). It then busy-waits for 10 seconds with mdelay().

Will halting the entire system with interrupts disabled for 10 seconds
reliably trigger hardware watchdogs and RCU stall panics?

[ ... ]

> acpi_unregister_gsi(LS2K_BMC_RESET_GPIO_GSI);

[Severity: High]
This isn't a bug introduced by this patch, but does unconditionally
unregistering the ACPI GSI immediately after successfully requesting its
Linux IRQ destroy the mapping while it is actively in use?

Tearing down the hardware-to-Linux IRQ domain mapping and freeing the
underlying IRQ descriptor guarantees the interrupt will not be delivered.

Will this also trigger a warning and a use-after-free regression during
device removal when the active IRQ is later freed?

[ ... ]

> 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 will unconditionally returning IRQ_HANDLED
without verifying the hardware source violate IRQF_SHARED semantics?

The handler is registered with IRQF_SHARED, but it unconditionally returns
IRQ_HANDLED and schedules bmc_reset_work without checking any hardware
status.

If another device on the shared line fires an interrupt, will the BMC driver
intercept it, falsely claim it was handled, and trigger the 10-second system
lockup via its workqueue?

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