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

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

Pre-existing issues:
- [High] The driver mutates a global static `mfd_cell` array with a stack-allocated platform data pointer, violating MFD rules and causing a race condition and potential use-after-free.
- [High] `acpi_unregister_gsi()` is unconditionally called immediately after successfully requesting the GPIO interrupt, destroying the active IRQ mapping.
- [High] A 10-second busy-wait (`mdelay`) is executed inside `stop_machine()`, which will reliably trigger the NMI watchdog and panic or freeze the system.
--

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

When the device is being removed, 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);

[Severity: High]
This isn't a bug introduced by this patch, but looking at
ls2k_bmc_events_fn():

	stop_machine(ls2k_bmc_recover_pci_data, ddata, NULL);

Which then initiates a highly atomic context where all CPUs are halted, and
ls2k_bmc_recover_pci_data() executes:

	mdelay(LS2K_BMC_RESET_WAIT_TIME);

Does a 10-second busy-wait inside stop_machine() trigger the NMI watchdog
and freeze the system, since it spins with local interrupts disabled on all
CPUs?

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

[Severity: High]
This isn't a bug introduced by this patch, but later in ls2k_bmc_init(),
acpi_unregister_gsi() is unconditionally called:

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

Since this executes unconditionally on the success path after requesting
the IRQ, does it destroy the active IRQ mapping? When the driver is
unbound, will devm's automated free_irq() then operate on a disposed IRQ
descriptor?

[Severity: High]
This isn't a bug introduced by this patch, but looking at
ls2k_bmc_probe():

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

Does mutating the global static ls2k_bmc_cells array with a
stack-allocated platform data pointer (&pd) leave a dangling pointer in
global state after the probe returns?

Could this also cause a data race if multiple device instances probe
concurrently? MFD subsystem rules require mfd_cell arrays to be static
const, and dynamic platform data should not be passed this way.

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