Re: [PATCH RFC v4 7/7] mfd: ls2kbmc: Capture the reset event of BMC through GPIO

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

Pre-existing issues:
- [High] Dynamic platform data pointer assigned to a shared static MFD cell array.
- [High] Data race and cross-device interference on static `last_jiffies` variable in the interrupt handler.
--

--- Patch [7]: [PATCH RFC v4 7/7] mfd: ls2kbmc: Capture the reset event of BMC through GPIO ---
commit a8353dcca0807b3f06c4230ec30411f5eac5f0e8
Author: Miao Wang <[email protected]>

mfd: ls2kbmc: Capture the reset event of BMC through GPIO

This commit updates the driver to capture the BMC reset event through the
GPIO framework instead of bypassing it. It attempts to find the GPIO pin via
ACPI resources, and falls back to a hardcoded controller and pin if an ACPI
declaration is not found.

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index 1466b314fc4e5..48ce7197c7744 100644
> --- a/drivers/mfd/ls2k-bmc-core.c
> +++ b/drivers/mfd/ls2k-bmc-core.c

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but assigning dynamic platform
data to a statically allocated global MFD cell array in ls2k_bmc_probe()
seems unsafe:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_probe() {
	...
	struct simplefb_platform_data pd;
	...
	ret = ls2k_bmc_parse_mode(dev, &pd);
	if (ret)
		return ret;

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

This violates the MFD subsystem API contract which requires mfd_cell arrays
to be static const. Since ls2k_bmc_cells is shared across all instances
of the driver, concurrent probing of multiple PCI devices will cause a data
race where one thread overwrites the platform_data pointer of the other.

Furthermore, pd is a local stack variable, meaning the global array retains
a dangling pointer after ls2k_bmc_probe() returns.

Should the cells array be defined as static const and the platform data be
passed using a dynamically allocated copy or another mechanism?

[Severity: High]
This is a pre-existing issue, but I noticed a data race in how
last_jiffies is defined in ls2k_bmc_interrupt():

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_interrupt() {
	...
	static unsigned long last_jiffies;

	if (system_state != SYSTEM_RUNNING)
		return IRQ_HANDLED;

	/* Skip interrupt in LS2K_BMC_INT_INTERVAL */
	if (time_after(jiffies, last_jiffies + LS2K_BMC_INT_INTERVAL)) {
		schedule_work(&ddata->bmc_reset_work);
		last_jiffies = jiffies;
	}
	...
}

Because this interrupt handler is registered with IRQF_SHARED and
last_jiffies is a shared static variable, concurrent interrupts on
different CPUs or multiple BMC devices will cause a data race.

Could this static state be moved to the per-device struct ls2k_bmc_ddata
structure to prevent cross-device interference and ensure proper isolation?

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