Re: [PATCH RFC v4 5/7] ipmi: ls2k: Relax the dependency to its mfd driver

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

New issues:
- [Medium] The commit message incorrectly claims the Kconfig change allows the MFD driver to be compiled as a module, but the target config is a boolean.

Pre-existing issues:
- [High] The MFD core driver dynamically modifies a non-const global `mfd_cell` array with a stack-allocated pointer during probe.
--

--- Patch [5]: [PATCH RFC v4 5/7] ipmi: ls2k: Relax the dependency to its mfd driver ---
commit df8dbca48cc9588013370bfdb3946a4662d09444
Author: Miao Wang <[email protected]>

ipmi: ls2k: Relax the dependency to its mfd driver

There is no strong dependency between the IPMI driver and its mfd
driver. Although the IPMI driver will not work without the mfd driver,
it is not a hard dependency. The IPMI driver can actually be compiled
without the mfd driver, and it will just fail to probe. When the mfd
driver is loaded, the IPMI driver will probe successfully.

> Therefore,
> the dependency of the IPMI driver on its mfd driver should be relaxed
> to "imply" from "select". This will allow the mfd driver to be compiled
> as a module and the IPMI driver to be compiled as a part of the ipmi_si
> module.

[Severity: Medium]
Is this description accurate regarding the mfd driver being compiled as a
module?

Looking at drivers/mfd/Kconfig, MFD_LS2K_BMC_CORE is defined as a bool
rather than a tristate:

config MFD_LS2K_BMC_CORE
	bool "Loongson-2K Board Management Controller Support"

Since it is a bool, it can only evaluate to built-in or disabled, so it cannot
be compiled as a module.

diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
index 669f760001974..f456e3e894169 100644
--- a/drivers/char/ipmi/Kconfig
+++ b/drivers/char/ipmi/Kconfig
@@ -87,7 +87,7 @@ config IPMI_IPMB
 config IPMI_LS2K
 	bool 'Loongson-2K IPMI interface'
 	depends on LOONGARCH
-	select MFD_LS2K_BMC_CORE
+	imply MFD_LS2K_BMC_CORE

[Severity: High]
This is a pre-existing issue, but does mutating the global mfd_cell array
during probe create a dangling pointer and potential data race?

Looking at ls2k_bmc_probe() in drivers/mfd/ls2k-bmc-core.c, a local stack
variable is assigned to the platform_data pointer of the global array:

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

The array itself is declared as static instead of static const:

static struct mfd_cell ls2k_bmc_cells[] = {
    ...
};

While devm_mfd_add_devices() deep-copies the platform data, this leaves a
dangling pointer to a destroyed stack frame in the global array after the probe
function returns.

If multiple such devices are probed concurrently, could this also result in a
data race on the shared global array?

 	help
 	  Provides a driver for Loongson-2K IPMI interfaces.

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