Re: [PATCH RFC v3 2/7] mfd: ls2kbmc: Sanity check for the connected pci port

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

Pre-existing issues:
- [Critical] The BMC reset recovery procedure executes inside `stop_machine()` with an unconditional 10-second delay, which will trigger a system hard lockup.
- [High] The `ls2k_bmc_cells` array is declared as a non-const global variable and is dynamically modified during probe to pass stack-allocated `platform_data`, violating MFD subsystem guidelines and causing a data race.
- [High] The `bmc_reset_work` workqueue is not cancelled on device unbind or probe failure, leading to a potential use-after-free.
- [High] The driver unregisters the GPIO ACPI GSI immediately after requesting the IRQ, rendering the IRQ mapping invalid and breaking reset detection.
--

--- Patch [2]: [PATCH RFC v3 2/7] mfd: ls2kbmc: Sanity check for the connected pci port ---
commit a57e58240c51cbe6832a32a400a0d6d415c3c375
Author: Miao Wang <[email protected]>

mfd: ls2kbmc: Sanity check for the connected pci port

When the bmc resets, the recovery procedure require to reconfigure the
parent device. The driver assumes that the parent device should be LS7A.
Add a sanity check on initialization to ensure this and prevent from
accidentally operating on non-LS7A ports.

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index 27f6e096404d6..b02e4955e9b04 100644
> --- a/drivers/mfd/ls2k-bmc-core.c
> +++ b/drivers/mfd/ls2k-bmc-core.c

[ ... ]

> @@ -488,6 +515,11 @@ static int ls2k_bmc_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	if (ret)
>  		return ret;
>  
> +	if (!ls2k_check_parent(dev)) {
> +		dev_err(&dev->dev, "Expected to be connected to LS7A PCI-E port\n");
> +		return -ENODEV;
> +	}
> +
>  	ddata = devm_kzalloc(&dev->dev, sizeof(*ddata), GFP_KERNEL);
>  	if (!ddata)
>  		return -ENOMEM;

[Severity: High]
This isn't a bug introduced by this patch, but later in ls2k_bmc_probe() the
driver dynamically modifies a global array:

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

Does this violate the MFD subsystem guidelines which require mfd_cell arrays
to be static const? Since this mutates a global array with a pointer to a
stack-allocated variable, could this cause a data race or data corruption if
multiple instances of the driver probe concurrently?

[Severity: High]
This is a pre-existing issue, but I noticed the driver doesn't seem to clean
up the workqueue in error paths or on teardown:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_init() {
    ...
    INIT_WORK(&ddata->bmc_reset_work, ls2k_bmc_events_fn);
    ...
}

Since devm_add_action_or_reset() isn't used to register a teardown action for
cancel_work_sync(), and there is no .remove callback, will this workqueue
remain active or queued after devres frees ddata on device unbind, leading to
a use-after-free?

[Severity: Critical]
This is a pre-existing issue, but is it safe to use mdelay() for 10 seconds
inside a stop_machine() callback?

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() {
    ...
    mdelay(LS2K_BMC_RESET_WAIT_TIME);
    ...
}

Since stop_machine() halts all CPUs and disables interrupts, will spending 10
seconds here inevitably trigger the NMI watchdog (hard lockup detector) and
cause a kernel panic?

[Severity: High]
This is a pre-existing issue, but the driver unregisters the GPIO ACPI GSI
immediately after requesting the IRQ:

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

Does calling acpi_unregister_gsi() here invoke irq_dispose_mapping() and
destroy the interrupt mapping before it can be used, breaking the reset
detection?

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