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

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

Pre-existing issues:
- [Critical] The driver executes a 10-second busy wait (`mdelay`) inside a `stop_machine` context, stalling all CPUs and likely triggering the NMI watchdog.
- [High] The driver dynamically modifies a global non-const `mfd_cell` array and assigns a pointer to a local stack variable to its `platform_data` field.
--

--- Patch [2]: [PATCH RFC v4 2/7] mfd: ls2kbmc: Sanity check for the connected pci port ---
commit ec9f316b181d1a4fc9b0daee3548a4f8a40ef766
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. 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);

[Severity: High]
This isn't a bug introduced by this patch, but looking further down in
ls2k_bmc_probe(), does assigning a pointer to a local stack variable to the
platform_data field of a global mfd_cell array create a dangling pointer once
the probe function returns?

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

This modifies a global non-const mfd_cell array, which violates MFD subsystem
guidelines requiring mfd_cell arrays to be static const. If multiple devices
bind to this driver, could concurrent probes race to modify this global array
and cause the wrong platform data to be copied?

[Severity: Critical]
This is a pre-existing issue, but since the commit message mentions the
recovery procedure, does the 10-second mdelay() inside the recovery
procedure's stop_machine callback stall all CPUs and trigger the NMI
hardlockup watchdog?

The hardware triggers a BMC reset, which queues the work that calls
stop_machine():

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

Inside the callback, the code executes a 10-second busy wait:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_recover_pci_data() {
    ...
    mdelay(LS2K_BMC_RESET_WAIT_TIME);
    ...
}

Because stop_machine() halts all other CPUs with interrupts disabled, and the
executing CPU also runs with local interrupts disabled, will this 10-second
delay cause a kernel panic?

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