Re: [PATCH RFC v5 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:
- [High] Dynamic amendment of the global `mfd_cell` array using a stack var=
iable, violating MFD subsystem guidelines and risking data races.
- [High] Premature unregistration of the ACPI GSI mapping immediately after=
 requesting the IRQ, effectively destroying the mapping while in use.
- [High] The BMC recovery function executes inside `stop_machine()` causing=
 a 10-second hard stall and attempting to sleep via `pci_iomap()`.
- [Medium] Rate-limiting in IRQ handler uses a global static variable inste=
ad of per-device state, causing cross-device interference and unprotected d=
ata races.
--

--- Patch [4]: [PATCH RFC v5 4/7] mfd: ls2kbmc: Cancel the work queue on re=
moval ---
commit 65ab5d9b597ea3db5f87628df49d0fa3a7bc805f
Author: Miao Wang <[email protected]>

mfd: ls2kbmc: Cancel the work queue on removal

This commit cancels the BMC reset work queue upon device removal. It uses
devm_add_action_or_reset() to ensure any pending reset work is aborted
before the device goes away, avoiding use-after-free or execution after
removal.

> 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
[ ... ]
> @@ -385,6 +391,10 @@ static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddat=
a)
> =20
>  	INIT_WORK(&ddata->bmc_reset_work, ls2k_bmc_events_fn);

[Severity: High]
This is a pre-existing issue, but does scheduling ls2k_bmc_events_fn()
lead to a hard stall and attempt to sleep in an atomic context?

Looking at the work handler, it executes the recovery function inside
stop_machine():

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

Because stop_machine() executes with local interrupts strictly disabled
on all CPUs, the 10-second delay in ls2k_bmc_recover_pci_data():

    mdelay(LS2K_BMC_RESET_WAIT_TIME);

could trigger NMI watchdogs and hard lockups. Additionally, it calls
ls2k_bmc_pcie_is_connected(), which uses pci_iomap() and ioremap(). Does
calling blocking memory allocation and mutex functions in this atomic conte=
xt
trigger a scheduling while atomic bug?

> =20
> +	ret =3D devm_add_action_or_reset(ddata->dev, ls2k_bmc_cancel_wq, ddata);
> +	if (ret)
> +		return ret;
> +
>  	ret =3D devm_request_irq(&pdev->dev, pdev->irq, ls2k_bmc_interrupt,
>  			       IRQF_SHARED | IRQF_TRIGGER_FALLING, "ls2kbmc pcie", ddata);

[Severity: Medium]
This is a pre-existing issue, but does the rate-limiting inside this shared
interrupt handler cause cross-device interference and unprotected data race=
s?

The handler locklessly checks and updates a static variable:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_interrupt() {
    static unsigned long last_jiffies;
    ...
    if (time_after(jiffies, last_jiffies + LS2K_BMC_INT_INTERVAL)) {
        schedule_work(&ddata->bmc_reset_work);
        last_jiffies =3D jiffies;
    }
}

If multiple devices exist, they share the same rate-limit state. Since both=
 a
PCI IRQ and a GPIO IRQ share this handler, can concurrent firings on differ=
ent
CPUs race to read and update this static variable? Should this state be mov=
ed
to the per-device ls2k_bmc_ddata structure and protected by a spinlock?

[Severity: High]
This isn't a bug introduced by this patch, but does the initialization
unconditionally destroy the GSI mapping while the IRQ is actively armed?

Looking further down in ls2k_bmc_init(), right after the IRQ is successfully
requested via devm_request_irq(), the GSI is unregistered:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_init() {
    ...
    acpi_unregister_gsi(LS2K_BMC_RESET_GPIO_GSI);
    return ret;
}

Because acpi_unregister_gsi() calls irq_dispose_mapping(), does this destroy
the IRQ descriptor and break interrupt delivery, potentially causing crashe=
s if
the IRQ fires?

[Severity: High]
This is a pre-existing issue, but does ls2k_bmc_probe() violate the MFD
subsystem guidelines by mutating a global mfd_cell array and leaving a
dangling pointer?

The global array is defined as a mutable array instead of static const:

drivers/mfd/ls2k-bmc-core.c:
static struct mfd_cell ls2k_bmc_cells[] =3D { ... };

In the probe function, the address of a local stack variable is assigned to
this global array:

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

Because this modifies a shared global array, can probing multiple devices
concurrently cause a data race? Also, does assigning a stack-allocated
pointer leave a dangling pointer after the probe completes?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260804-ls2kbmc-mo=
[email protected]?part=3D4