[PATCH RFC v5 0/7] mfd: ls2kbmc: multiple fixes for this driver

[PATCH RFC v5 0/7] mfd: ls2kbmc: multiple fixes for this driver

[Miao Wang via B4 Relay]

Previously, the driver has been introduced to support the Loongson 2K
BMC running on the Loongson Servers, which is essential to prevent
the system from hanging when the BMC is being reset and the default
efi-framebuffer is being used. However, there are some drawbacks in the
driver.

Firstly, the driver tries to read and write to the connected PCI-E host
controller registers, assuming that the BMC is connected to LS7A PCI-E
host controller. This assumption should be true for real products, but
to prevent from accidentally reading and writing to the wrong PCI-E host
controller, this driver should be modified to check this before
accessing the registers.

Secondly, the driver uses non-exported functions to tell the vt
subsystem to redraw the screen, preventing the driver from being
compiling as a module. This can be fixed by using the exported
functions instead.

Thirdly, the driver directly accesses the GPIO controller registers
using hard-coded addresses, which might conflict with the loaded GPIO
controller driver for the same GPIO controller. This is fixed in this
series by using the GPIO subsystem APIs instead. However, legacy GPIO
APIs have to be used in this fixed to correctly request a GPIO
descriptor from the GPIO subsystem, which might be further discussed
to find a better solution.

Finally, there is a minor issue in the driver where it changes the
mode string describing the screen resolution during probing, which
prevents the device from being probed again if -EPROBE_DEFER is
returned by the probe function.

I have tested the changes in this series on a single-socket Loongson
3C6000 server with a Loongson 2K BMC, and the driver works as expected
when the corresponding GPIO driver is additionally loaded.

Signed-off-by: Miao Wang <shankerwangmiao@gmail.com>
---
Changes in v5:
- Address issues found by the Sashiko AI review bot
  - Maintain the compatibility with possible unexpected mode string when
    parsing, although such mode string is actually not expected, to
    satisfy the AI bot's concern.
  - Add a comment to point out that the adjustment of the Kconfig entry
    for ls2kbmc is in the following patch, to satisfy the AI bot's
    concern.
- Link to v4: https://lore.kernel.org/r/20260731-ls2kbmc-mod-v4-0-d201502ba239@gmail.com

Changes in v4:
- Use a better way to get the GPIO device fwnode.
- Add a comment to describe a problem found by AI bot which is actually
  intended.
- Link to v3: https://lore.kernel.org/r/20260710-ls2kbmc-mod-v3-0-ef718636e78e@gmail.com

Changes in v3:
- Check the return value of devm_add_action_or_reset when registering
  the cleanup hook of the work queue
- Use swnode to create the link between the device to the GPIO chip,
  and prevent borrowing the legacy GPIO APIs
- Link to v2: https://lore.kernel.org/r/20260708-ls2kbmc-mod-v2-0-2afdd1741766@gmail.com

Changes in v2:
- Several fixes suggested by the Sashiko AI review bot
- Add a cleanup function for the wq on removal of the device
- Relax the reverse dependency from CONFIG_IPMI_LS2K to
  CONFIG_MFD_LS2K_BMC_CORE to allow the driver to be built as a module
- Link to v1: https://lore.kernel.org/r/20260708-ls2kbmc-mod-v1-0-c344bf5defa3@gmail.com

---
Miao Wang (7):
      mfd: ls2kbmc: Make a copy when parsing mode string
      mfd: ls2kbmc: Sanity check for the connected pci port
      mfd: ls2kbmc: Redraw using exported functions
      mfd: ls2kbmc: Cancel the work queue on removal
      ipmi: ls2k: Relax the dependency to its mfd driver
      mfd: ls2kbmc: Able to be compiled as a module
      mfd: ls2kbmc: Capture the reset event of BMC through GPIO

 drivers/char/ipmi/Kconfig   |   2 +-
 drivers/mfd/Kconfig         |   2 +-
 drivers/mfd/ls2k-bmc-core.c | 246 ++++++++++++++++++++++++++++++++++----------
 3 files changed, 192 insertions(+), 58 deletions(-)
---
base-commit: 11028ab62899e4191e074ee364c712b77823a9c4
change-id: 20260626-ls2kbmc-mod-5209193009b2

Best regards,
-- 
Miao Wang <shankerwangmiao@gmail.com>

[PATCH RFC v5 0/7] mfd: ls2kbmc: multiple fixes for this driver

[Miao Wang]

Previously, the driver has been introduced to support the Loongson 2K
BMC running on the Loongson Servers, which is essential to prevent
the system from hanging when the BMC is being reset and the default
efi-framebuffer is being used. However, there are some drawbacks in the
driver.

Firstly, the driver tries to read and write to the connected PCI-E host
controller registers, assuming that the BMC is connected to LS7A PCI-E
host controller. This assumption should be true for real products, but
to prevent from accidentally reading and writing to the wrong PCI-E host
controller, this driver should be modified to check this before
accessing the registers.

Secondly, the driver uses non-exported functions to tell the vt
subsystem to redraw the screen, preventing the driver from being
compiling as a module. This can be fixed by using the exported
functions instead.

Thirdly, the driver directly accesses the GPIO controller registers
using hard-coded addresses, which might conflict with the loaded GPIO
controller driver for the same GPIO controller. This is fixed in this
series by using the GPIO subsystem APIs instead. However, legacy GPIO
APIs have to be used in this fixed to correctly request a GPIO
descriptor from the GPIO subsystem, which might be further discussed
to find a better solution.

Finally, there is a minor issue in the driver where it changes the
mode string describing the screen resolution during probing, which
prevents the device from being probed again if -EPROBE_DEFER is
returned by the probe function.

I have tested the changes in this series on a single-socket Loongson
3C6000 server with a Loongson 2K BMC, and the driver works as expected
when the corresponding GPIO driver is additionally loaded.

Signed-off-by: Miao Wang <shankerwangmiao@gmail.com>
---
Changes in v5:
- Address issues found by the Sashiko AI review bot
  - Maintain the compatibility with possible unexpected mode string when
    parsing, although such mode string is actually not expected, to
    satisfy the AI bot's concern.
  - Add a comment to point out that the adjustment of the Kconfig entry
    for ls2kbmc is in the following patch, to satisfy the AI bot's
    concern.
- Link to v4: https://lore.kernel.org/r/20260731-ls2kbmc-mod-v4-0-d201502ba239@gmail.com

Changes in v4:
- Use a better way to get the GPIO device fwnode.
- Add a comment to describe a problem found by AI bot which is actually
  intended.
- Link to v3: https://lore.kernel.org/r/20260710-ls2kbmc-mod-v3-0-ef718636e78e@gmail.com

Changes in v3:
- Check the return value of devm_add_action_or_reset when registering
  the cleanup hook of the work queue
- Use swnode to create the link between the device to the GPIO chip,
  and prevent borrowing the legacy GPIO APIs
- Link to v2: https://lore.kernel.org/r/20260708-ls2kbmc-mod-v2-0-2afdd1741766@gmail.com

Changes in v2:
- Several fixes suggested by the Sashiko AI review bot
- Add a cleanup function for the wq on removal of the device
- Relax the reverse dependency from CONFIG_IPMI_LS2K to
  CONFIG_MFD_LS2K_BMC_CORE to allow the driver to be built as a module
- Link to v1: https://lore.kernel.org/r/20260708-ls2kbmc-mod-v1-0-c344bf5defa3@gmail.com

---
Miao Wang (7):
      mfd: ls2kbmc: Make a copy when parsing mode string
      mfd: ls2kbmc: Sanity check for the connected pci port
      mfd: ls2kbmc: Redraw using exported functions
      mfd: ls2kbmc: Cancel the work queue on removal
      ipmi: ls2k: Relax the dependency to its mfd driver
      mfd: ls2kbmc: Able to be compiled as a module
      mfd: ls2kbmc: Capture the reset event of BMC through GPIO

 drivers/char/ipmi/Kconfig   |   2 +-
 drivers/mfd/Kconfig         |   2 +-
 drivers/mfd/ls2k-bmc-core.c | 246 ++++++++++++++++++++++++++++++++++----------
 3 files changed, 192 insertions(+), 58 deletions(-)
---
base-commit: 11028ab62899e4191e074ee364c712b77823a9c4
change-id: 20260626-ls2kbmc-mod-5209193009b2

Best regards,
-- 
Miao Wang <shankerwangmiao@gmail.com>

[PATCH RFC v5 1/7] mfd: ls2kbmc: Make a copy when parsing mode string

[Miao Wang via B4 Relay]

From: Miao Wang <shankerwangmiao@gmail.com>

When parsing the mode string from BMC, the string is manipulated
in-place with strsep(), preventing from parsing it again. Make a copy of
the original string and manipulate the copy instead to fix this.

Fixes: 0d64f6d1ffe9 ("mfd: ls2kbmc: Introduce Loongson-2K BMC core driver")
Signed-off-by: Miao Wang <shankerwangmiao@gmail.com>
---
 drivers/mfd/ls2k-bmc-core.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
index 408056bfb2fe757a5bde43775a483a48352e706d..fc9695eedd3662ac92ff116811bb07bff1c994ea 100644
--- a/drivers/mfd/ls2k-bmc-core.c
+++ b/drivers/mfd/ls2k-bmc-core.c
@@ -427,34 +427,54 @@ static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
  */
 static int ls2k_bmc_parse_mode(struct pci_dev *pdev, struct simplefb_platform_data *pd)
 {
-	char *mode;
+	/* Assume 64 bytes is enough for the resolution string */
+	char mode_buf[64], mode_buf_orig[64];
+	char *mode = mode_buf;
+	const void __iomem *mode_base;
 	int depth, ret;
 
 	/* The last 16M of PCI BAR0 is used to store the resolution string. */
-	mode = devm_ioremap(&pdev->dev, pci_resource_start(pdev, 0) + SZ_16M, SZ_16M);
-	if (!mode)
+	mode_base = ioremap(pci_resource_start(pdev, 0) + SZ_16M,
+			    sizeof(mode_buf));
+	if (!mode_base)
 		return -ENOMEM;
+	memcpy_fromio(mode_buf, mode_base, sizeof(mode_buf) - 1);
+	mode_buf[sizeof(mode_buf) - 1] = '\0';
+	iounmap((void __iomem *)mode_base);
+	memcpy(mode_buf_orig, mode_buf, sizeof(mode_buf_orig));
 
 	/* The resolution field starts with the flag "video=". */
 	if (!strncmp(mode, "video=", 6))
 		mode = mode + 6;
 
-	ret = kstrtoint(strsep(&mode, "x"), 10, &pd->width);
+	ret = kstrtouint(strsep(&mode, "x"), 10, &pd->width);
 	if (ret)
-		return ret;
+		goto invalid_mode;
 
-	ret = kstrtoint(strsep(&mode, "-"), 10, &pd->height);
+	if (mode == NULL) {
+		ret = -EINVAL;
+		goto invalid_mode;
+	}
+	ret = kstrtouint(strsep(&mode, "-"), 10, &pd->height);
 	if (ret)
-		return ret;
+		goto invalid_mode;
 
+	if (mode == NULL) {
+		ret = -EINVAL;
+		goto invalid_mode;
+	}
 	ret = kstrtoint(strsep(&mode, "@"), 10, &depth);
 	if (ret)
-		return ret;
+		goto invalid_mode;
 
 	pd->stride = pd->width * depth / 8;
 	pd->format = depth == 32 ? "a8r8g8b8" : "r5g6b5";
 
 	return 0;
+
+invalid_mode:
+	dev_err(&pdev->dev, "Invalid resolution string: %s\n", mode_buf_orig);
+	return ret;
 }
 
 static int ls2k_bmc_probe(struct pci_dev *dev, const struct pci_device_id *id)

-- 
2.49.0

[PATCH RFC v5 1/7] mfd: ls2kbmc: Make a copy when parsing mode string

[Miao Wang]

When parsing the mode string from BMC, the string is manipulated
in-place with strsep(), preventing from parsing it again. Make a copy of
the original string and manipulate the copy instead to fix this.

Fixes: 0d64f6d1ffe9 ("mfd: ls2kbmc: Introduce Loongson-2K BMC core driver")
Signed-off-by: Miao Wang <shankerwangmiao@gmail.com>
---
 drivers/mfd/ls2k-bmc-core.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
index 408056bfb2fe757a5bde43775a483a48352e706d..fc9695eedd3662ac92ff116811bb07bff1c994ea 100644
--- a/drivers/mfd/ls2k-bmc-core.c
+++ b/drivers/mfd/ls2k-bmc-core.c
@@ -427,34 +427,54 @@ static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
  */
 static int ls2k_bmc_parse_mode(struct pci_dev *pdev, struct simplefb_platform_data *pd)
 {
-	char *mode;
+	/* Assume 64 bytes is enough for the resolution string */
+	char mode_buf[64], mode_buf_orig[64];
+	char *mode = mode_buf;
+	const void __iomem *mode_base;
 	int depth, ret;
 
 	/* The last 16M of PCI BAR0 is used to store the resolution string. */
-	mode = devm_ioremap(&pdev->dev, pci_resource_start(pdev, 0) + SZ_16M, SZ_16M);
-	if (!mode)
+	mode_base = ioremap(pci_resource_start(pdev, 0) + SZ_16M,
+			    sizeof(mode_buf));
+	if (!mode_base)
 		return -ENOMEM;
+	memcpy_fromio(mode_buf, mode_base, sizeof(mode_buf) - 1);
+	mode_buf[sizeof(mode_buf) - 1] = '\0';
+	iounmap((void __iomem *)mode_base);
+	memcpy(mode_buf_orig, mode_buf, sizeof(mode_buf_orig));
 
 	/* The resolution field starts with the flag "video=". */
 	if (!strncmp(mode, "video=", 6))
 		mode = mode + 6;
 
-	ret = kstrtoint(strsep(&mode, "x"), 10, &pd->width);
+	ret = kstrtouint(strsep(&mode, "x"), 10, &pd->width);
 	if (ret)
-		return ret;
+		goto invalid_mode;
 
-	ret = kstrtoint(strsep(&mode, "-"), 10, &pd->height);
+	if (mode == NULL) {
+		ret = -EINVAL;
+		goto invalid_mode;
+	}
+	ret = kstrtouint(strsep(&mode, "-"), 10, &pd->height);
 	if (ret)
-		return ret;
+		goto invalid_mode;
 
+	if (mode == NULL) {
+		ret = -EINVAL;
+		goto invalid_mode;
+	}
 	ret = kstrtoint(strsep(&mode, "@"), 10, &depth);
 	if (ret)
-		return ret;
+		goto invalid_mode;
 
 	pd->stride = pd->width * depth / 8;
 	pd->format = depth == 32 ? "a8r8g8b8" : "r5g6b5";
 
 	return 0;
+
+invalid_mode:
+	dev_err(&pdev->dev, "Invalid resolution string: %s\n", mode_buf_orig);
+	return ret;
 }
 
 static int ls2k_bmc_probe(struct pci_dev *dev, const struct pci_device_id *id)

-- 
2.49.0

Re: [PATCH RFC v5 1/7] mfd: ls2kbmc: Make a copy when parsing mode string

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver illegally modifies a shared static `mfd_cell` array (`ls2k_bmc_cells`) during probe, assigning a pointer to a stack-allocated variable.
- [High] Massive `mdelay` (10+ seconds) inside a `stop_machine` callback will freeze the system and trigger hard lockups.
- [High] Video mode dimensions parsed from untrusted BMC memory are not bounds-checked, causing an out-of-bounds write in the DRM subsystem.
--

--- Patch [1]: [PATCH RFC v5 1/7] mfd: ls2kbmc: Make a copy when parsing mode string ---
commit 04a85f9ac065d47d2e55cbd8e495ab417bf78739
Author: Miao Wang <shankerwangmiao@gmail.com>
Subject: mfd: ls2kbmc: Make a copy when parsing mode string

This commit resolves an issue where the resolution string mapped from
the BMC was being manipulated in-place by strsep(). It creates a copy
of the string to parse instead.

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index 408056bfb2fe7..fc9695eedd366 100644
> --- a/drivers/mfd/ls2k-bmc-core.c
> +++ b/drivers/mfd/ls2k-bmc-core.c
[ ... ]
> @@ -427,34 +427,54 @@ static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
[ ... ]
> -	ret = kstrtoint(strsep(&mode, "x"), 10, &pd->width);
> +	ret = kstrtouint(strsep(&mode, "x"), 10, &pd->width);
>  	if (ret)
> -		return ret;
> +		goto invalid_mode;
>  
> -	ret = kstrtoint(strsep(&mode, "-"), 10, &pd->height);
> +	if (mode == NULL) {
> +		ret = -EINVAL;
> +		goto invalid_mode;
> +	}
> +	ret = kstrtouint(strsep(&mode, "-"), 10, &pd->height);
>  	if (ret)
> -		return ret;
> +		goto invalid_mode;
>  
> +	if (mode == NULL) {
> +		ret = -EINVAL;
> +		goto invalid_mode;
> +	}
>  	ret = kstrtoint(strsep(&mode, "@"), 10, &depth);
>  	if (ret)
> -		return ret;
> +		goto invalid_mode;
>  
>  	pd->stride = pd->width * depth / 8;
>  	pd->format = depth == 32 ? "a8r8g8b8" : "r5g6b5";
>  
>  	return 0;

[Severity: High]
This isn't a bug introduced by this patch, but does this parsing logic need
bounds checking for the dimensions provided by the BMC?

The parsed width, height, and stride are used to configure the framebuffer.
However, the backing memory is capped at a fixed 4MB size:

static struct resource ls2k_display_resources[] = {
        DEFINE_RES_MEM_NAMED(LS2K_DISPLAY_RES_START, SZ_4M, "simpledrm-res"),
};

If a compromised or misconfigured BMC provides abnormally large dimensions,
could this lead to out-of-bounds writes past the 4MB limit when the
framebuffer is cleared or rendered?

> +
> +invalid_mode:
> +	dev_err(&pdev->dev, "Invalid resolution string: %s\n", mode_buf_orig);
> +	return ret;
>  }
>  
>  static int ls2k_bmc_probe(struct pci_dev *dev, const struct pci_device_id *id)

[Severity: High]
This is a pre-existing issue, but does ls2k_bmc_probe() leave a dangling
pointer by assigning a stack variable to a global array?

The driver modifies the global static array ls2k_bmc_cells during probe:

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

Since pd is local to the stack during probe, the global array is left with a
dangling pointer once the function returns. Could this cause a stack
use-after-free if another device instance probes concurrently or accesses
the array later? Additionally, shouldn't MFD cell arrays be static const?

[Severity: High]
This is another pre-existing issue, but does ls2k_bmc_events_fn() trigger
hard lockups by sleeping inside stop_machine()?

When a BMC reset occurs, the driver executes ls2k_bmc_events_fn(), which
calls stop_machine() to freeze all CPUs with local interrupts disabled:

ls2k_bmc_events_fn() {
        ...
        stop_machine(ls2k_bmc_recover_pci_data, ddata, NULL);
        ...
}

Inside ls2k_bmc_recover_pci_data(), the code waits for the BMC to become
ready using a 10-second busy loop:

ls2k_bmc_recover_pci_data() {
        ...
        mdelay(LS2K_BMC_RESET_WAIT_TIME);
        ...
}

Does holding all CPUs with interrupts disabled for 10 seconds
unconditionally trigger NMI watchdogs and RCU stall panics?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804-ls2kbmc-mod-v5-0-e6bc5cdd9a93@gmail.com?part=1

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

[Miao Wang via B4 Relay]

From: Miao Wang <shankerwangmiao@gmail.com>

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.

Fixes: d952bba3fbb5 ("mfd: ls2kbmc: Add Loongson-2K BMC reset function support")
Signed-off-by: Miao Wang <shankerwangmiao@gmail.com>
---
 drivers/mfd/ls2k-bmc-core.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
index fc9695eedd3662ac92ff116811bb07bff1c994ea..a9a30611dde9e31cd86ff67b1d6e02bf67b34755 100644
--- a/drivers/mfd/ls2k-bmc-core.c
+++ b/drivers/mfd/ls2k-bmc-core.c
@@ -35,6 +35,15 @@
 #define LS2K_IPMI3_RES_START		(LS2K_IPMI2_RES_START + LS2K_IPMI_RES_SIZE)
 #define LS2K_IPMI4_RES_START		(LS2K_IPMI3_RES_START + LS2K_IPMI_RES_SIZE)
 
+/* LS7A port Device IDs */
+#define DEV_LS7A1K_PCIE_PORT0	0x7a09
+#define DEV_LS7A1K_PCIE_PORT1	0x7a19
+#define DEV_LS7A1K_PCIE_PORT2	0x7a29
+#define DEV_LS7A2K_PCIE_PORT0	0x7a39
+#define DEV_LS7A2K_PCIE_PORT1	0x7a49
+#define DEV_LS7A2K_PCIE_PORT2	0x7a59
+#define DEV_LS7A2K_PCIE_PORT3	0x7a69
+
 #define LS7A_PCI_CFG_SIZE		0x100
 
 /* LS7A bridge registers */
@@ -477,6 +486,24 @@ static int ls2k_bmc_parse_mode(struct pci_dev *pdev, struct simplefb_platform_da
 	return ret;
 }
 
+static const struct pci_device_id ls7a_ports[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A1K_PCIE_PORT0) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A1K_PCIE_PORT1) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A1K_PCIE_PORT2) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A2K_PCIE_PORT0) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A2K_PCIE_PORT1) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A2K_PCIE_PORT2) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A2K_PCIE_PORT3) },
+	{ }
+};
+
+static bool ls2k_check_parent(struct pci_dev *dev)
+{
+	struct pci_dev *parent = dev->bus->self;
+
+	return parent && pci_match_id(ls7a_ports, parent) != NULL;
+}
+
 static int ls2k_bmc_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	struct simplefb_platform_data pd;
@@ -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;

-- 
2.49.0

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

[Miao Wang]

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.

Fixes: d952bba3fbb5 ("mfd: ls2kbmc: Add Loongson-2K BMC reset function support")
Signed-off-by: Miao Wang <shankerwangmiao@gmail.com>
---
 drivers/mfd/ls2k-bmc-core.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
index fc9695eedd3662ac92ff116811bb07bff1c994ea..a9a30611dde9e31cd86ff67b1d6e02bf67b34755 100644
--- a/drivers/mfd/ls2k-bmc-core.c
+++ b/drivers/mfd/ls2k-bmc-core.c
@@ -35,6 +35,15 @@
 #define LS2K_IPMI3_RES_START		(LS2K_IPMI2_RES_START + LS2K_IPMI_RES_SIZE)
 #define LS2K_IPMI4_RES_START		(LS2K_IPMI3_RES_START + LS2K_IPMI_RES_SIZE)
 
+/* LS7A port Device IDs */
+#define DEV_LS7A1K_PCIE_PORT0	0x7a09
+#define DEV_LS7A1K_PCIE_PORT1	0x7a19
+#define DEV_LS7A1K_PCIE_PORT2	0x7a29
+#define DEV_LS7A2K_PCIE_PORT0	0x7a39
+#define DEV_LS7A2K_PCIE_PORT1	0x7a49
+#define DEV_LS7A2K_PCIE_PORT2	0x7a59
+#define DEV_LS7A2K_PCIE_PORT3	0x7a69
+
 #define LS7A_PCI_CFG_SIZE		0x100
 
 /* LS7A bridge registers */
@@ -477,6 +486,24 @@ static int ls2k_bmc_parse_mode(struct pci_dev *pdev, struct simplefb_platform_da
 	return ret;
 }
 
+static const struct pci_device_id ls7a_ports[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A1K_PCIE_PORT0) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A1K_PCIE_PORT1) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A1K_PCIE_PORT2) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A2K_PCIE_PORT0) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A2K_PCIE_PORT1) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A2K_PCIE_PORT2) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A2K_PCIE_PORT3) },
+	{ }
+};
+
+static bool ls2k_check_parent(struct pci_dev *dev)
+{
+	struct pci_dev *parent = dev->bus->self;
+
+	return parent && pci_match_id(ls7a_ports, parent) != NULL;
+}
+
 static int ls2k_bmc_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	struct simplefb_platform_data pd;
@@ -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;

-- 
2.49.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver dynamically modifies the shared static `ls2k_bmc_cells` global `mfd_cell` array during probe, passing a pointer to a stack-allocated variable as platform data.
- [High] The driver schedules a work item (`bmc_reset_work`) but never cancels it during device removal or probe failure, leading to a use-after-free.
- [High] Unconditional destruction of the GPIO IRQ mapping immediately after successful registration.
- [High] A 10-second busy-wait delay inside a `stop_machine` callback will cause system lockups.
- [High] Calling `pci_iomap` inside a `stop_machine` callback will cause a scheduling while atomic panic.
--

--- Patch [2]: [PATCH RFC v5 2/7] mfd: ls2kbmc: Sanity check for the connected pci port ---
commit 0c7508c9325b7e4f23816a0998da4d8f6069eb25
Author: Miao Wang <shankerwangmiao@gmail.com>

mfd: ls2kbmc: Sanity check for the connected pci port

This commit adds a sanity check to ensure the parent device of the BMC
is an LS7A PCIe port. It prevents accidental operations on non-LS7A ports
during the BMC recovery procedure by checking the parent device ID against
a list of supported LS7A port IDs.

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index fc9695eedd366..a9a30611dde9e 100644
> --- a/drivers/mfd/ls2k-bmc-core.c
> +++ b/drivers/mfd/ls2k-bmc-core.c
[ ... ]
> @@ -477,6 +486,24 @@ static int ls2k_bmc_parse_mode(struct pci_dev *pdev, struct simplefb_platform_da
>  	return ret;
>  }
>  
> +static const struct pci_device_id ls7a_ports[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A1K_PCIE_PORT0) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A1K_PCIE_PORT1) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A1K_PCIE_PORT2) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A2K_PCIE_PORT0) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A2K_PCIE_PORT1) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A2K_PCIE_PORT2) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A2K_PCIE_PORT3) },
> +	{ }
> +};
> +
> +static bool ls2k_check_parent(struct pci_dev *dev)
> +{
> +	struct pci_dev *parent = dev->bus->self;
> +
> +	return parent && pci_match_id(ls7a_ports, parent) != NULL;
> +}
> +
>  static int ls2k_bmc_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  {
>  	struct simplefb_platform_data pd;
> @@ -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 is a pre-existing issue, but does this driver dynamically modify the
shared static global mfd_cell array during probe?

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);
...
}

The mfd_cell array is declared globally and lacks the const qualifier.
Can this cause a data race during concurrent probing that corrupts global
state? Also, passing a pointer to a stack-allocated variable as platform
data could leave a dangling pointer once probe completes.

[Severity: High]
This is a pre-existing issue, but does the driver schedule a work item that
is never canceled during device removal or probe failure?

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

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_events_fn() {
...
    struct ls2k_bmc_ddata *ddata = container_of(work, struct ls2k_bmc_ddata, bmc_reset_work);
    stop_machine(ls2k_bmc_recover_pci_data, ddata, NULL);
...
}

If the device is unbound, devres will automatically free ddata. If the
workqueue is initialized and the IRQ is requested without adding a teardown
action (e.g., via devm_add_action_or_reset) to cancel the work, could the
asynchronous work still run and access freed memory?

[Severity: High]
This is a pre-existing issue, but does the initialization path unconditionally
destroy the GPIO IRQ mapping immediately after successful registration?

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

Could this prevent the GPIO reset interrupt from ever being delivered because
its underlying GSI mapping is destroyed, permanently breaking the BMC reset
watchdog functionality?

[Severity: High]
This is a pre-existing issue, but does the recovery path execute a 10-second
busy-wait delay while the system is in a fully atomic, stopped state?

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

The BMC reset event triggers ls2k_bmc_events_fn, which calls stop_machine().
This runs ls2k_bmc_recover_pci_data with all other CPUs halted and interrupts
disabled. Could this 10-second busy-wait loop cause system lockups, NMI
watchdog panics, or RCU stall warnings?

[Severity: High]
This is a pre-existing issue, but does the PCIe connection check initiate a
sleeping operation while the system is in an atomic context?

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_pcie_is_connected() {
...
    base = pci_iomap(parent, 0, LS7A_PCI_CFG_SIZE);
...
}

Since stop_machine() runs ls2k_bmc_recover_pci_data atomically, which then
invokes ls2k_bmc_pcie_is_connected(), calling pci_iomap() here allocates
virtual memory and acquires sleeping locks. Could this result in a
scheduling while atomic or sleeping in atomic context kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804-ls2kbmc-mod-v5-0-e6bc5cdd9a93@gmail.com?part=2

[PATCH RFC v5 3/7] mfd: ls2kbmc: Redraw using exported functions

[Miao Wang via B4 Relay]

From: Miao Wang <shankerwangmiao@gmail.com>

Use update_screen, i.e. redraw_screen() to trigger the redraw of the
current vt.

Fixes: d952bba3fbb5 ("mfd: ls2kbmc: Add Loongson-2K BMC reset function support")
Signed-off-by: Miao Wang <shankerwangmiao@gmail.com>
---
 drivers/mfd/ls2k-bmc-core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
index a9a30611dde9e31cd86ff67b1d6e02bf67b34755..e12f2ca2fbef360ceda14ee6fc64f500e4944120 100644
--- a/drivers/mfd/ls2k-bmc-core.c
+++ b/drivers/mfd/ls2k-bmc-core.c
@@ -25,6 +25,7 @@
 #include <linux/platform_device.h>
 #include <linux/stop_machine.h>
 #include <linux/vt_kern.h>
+#include <linux/console.h>
 
 /* LS2K BMC resources */
 #define LS2K_DISPLAY_RES_START		(SZ_16M + SZ_2M)
@@ -310,7 +311,9 @@ static void ls2k_bmc_events_fn(struct work_struct *work)
 
 	if (IS_ENABLED(CONFIG_VT)) {
 		/* Re-push the display due to previous PCI-E loss. */
-		set_console(vt_move_to_console(MAX_NR_CONSOLES - 1, 1));
+		console_lock();
+		update_screen(vc_cons[fg_console].d);
+		console_unlock();
 	}
 }
 

-- 
2.49.0

[PATCH RFC v5 3/7] mfd: ls2kbmc: Redraw using exported functions

[Miao Wang]

Use update_screen, i.e. redraw_screen() to trigger the redraw of the
current vt.

Fixes: d952bba3fbb5 ("mfd: ls2kbmc: Add Loongson-2K BMC reset function support")
Signed-off-by: Miao Wang <shankerwangmiao@gmail.com>
---
 drivers/mfd/ls2k-bmc-core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
index a9a30611dde9e31cd86ff67b1d6e02bf67b34755..e12f2ca2fbef360ceda14ee6fc64f500e4944120 100644
--- a/drivers/mfd/ls2k-bmc-core.c
+++ b/drivers/mfd/ls2k-bmc-core.c
@@ -25,6 +25,7 @@
 #include <linux/platform_device.h>
 #include <linux/stop_machine.h>
 #include <linux/vt_kern.h>
+#include <linux/console.h>
 
 /* LS2K BMC resources */
 #define LS2K_DISPLAY_RES_START		(SZ_16M + SZ_2M)
@@ -310,7 +311,9 @@ static void ls2k_bmc_events_fn(struct work_struct *work)
 
 	if (IS_ENABLED(CONFIG_VT)) {
 		/* Re-push the display due to previous PCI-E loss. */
-		set_console(vt_move_to_console(MAX_NR_CONSOLES - 1, 1));
+		console_lock();
+		update_screen(vc_cons[fg_console].d);
+		console_unlock();
 	}
 }
 

-- 
2.49.0

Re: [PATCH RFC v5 3/7] mfd: ls2kbmc: Redraw using exported functions

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing cancellation of `bmc_reset_work` on driver unbind leads to a use-after-free.
- [High] The global `mfd_cell` array is dynamically mutated with a pointer to a stack-allocated variable.
- [High] Uninitialized static variable `last_jiffies` prevents interrupt handling for the first ~6 minutes of uptime and incorrectly shares state globally.
- [High] The driver executes an excessively long busy-wait (`mdelay(10000)`) and calls sleepable functions inside a `stop_machine()` context, severely freezing the system.
- [High] The driver registers its interrupt handler with `IRQF_SHARED` but unconditionally returns `IRQ_HANDLED` without verifying device interrupt ownership.
- [High] PCI-E recovery failures are ignored, leading to a potential system crash when unconditionally redrawing the VT screen.
- [High] Premature disposal of the GPIO GSI mapping immediately after requesting the IRQ.
--

--- Patch [3]: [PATCH RFC v5 3/7] mfd: ls2kbmc: Redraw using exported functions ---
commit 5d480fc6e39faac5ce62f0c2c72ba001afde5343
Author: Miao Wang <shankerwangmiao@gmail.com>
Subject: mfd: ls2kbmc: Redraw using exported functions

This commit modifies the ls2k-bmc driver to use update_screen() inside
console_lock() to redraw the virtual terminal after a PCI-E loss, replacing
the previous usage of set_console().

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index a9a30611dde9e..e12f2ca2fbef3 100644
> --- a/drivers/mfd/ls2k-bmc-core.c
> +++ b/drivers/mfd/ls2k-bmc-core.c
[ ... ]
> @@ -310,7 +311,9 @@ static void ls2k_bmc_events_fn(struct work_struct *work)
>  
>  	if (IS_ENABLED(CONFIG_VT)) {
>  		/* Re-push the display due to previous PCI-E loss. */
> -		set_console(vt_move_to_console(MAX_NR_CONSOLES - 1, 1));
> +		console_lock();
> +		update_screen(vc_cons[fg_console].d);
> +		console_unlock();
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but does ignoring the return value of
stop_machine() here lead to a system crash?

ls2k_bmc_recover_pci_data() returns false (0) on error instead of a negative
error code, which makes stop_machine() return 0 on failure. The caller ignores
this and unconditionally calls update_screen(), which writes to the
framebuffer.

If the PCI-E link recovery failed, won't accessing the dead PCI-E memory fatally
crash the system?

[Severity: High]
This is a pre-existing issue, but are we performing long busy-waits and calling
sleepable functions inside a stop_machine() atomic context?

stop_machine() executes its callback on one CPU while globally disabling local
interrupts on all other CPUs.

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_recover_pci_data() {
    ...
    if (!ls2k_bmc_pcie_is_connected(parent, ddata))
    ...
    mdelay(LS2K_BMC_RESET_WAIT_TIME);
    ...
}

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_pcie_is_connected() {
    ...
    base = pci_iomap(parent, 0, LS7A_PCI_CFG_SIZE);
    ...
}

Will the 10-second mdelay() trigger NMI hardlockup detectors or RCU stall
warnings?

Additionally, won't calling pci_iomap() (which can sleep) in this atomic context
violate core kernel rules?

[Severity: High]
This is a pre-existing issue, but does the uninitialized static variable
last_jiffies prevent interrupt handling for the first several minutes of uptime?

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_interrupt() {
    ...
    static unsigned long last_jiffies;
    ...
    if (time_after(jiffies, last_jiffies + LS2K_BMC_INT_INTERVAL)) {
    ...
}

last_jiffies is implicitly initialized to 0, and jiffies starts at -300 * HZ.
This makes the time_after() check remain false for the first ~6 minutes of
uptime.

If the BMC resets during this period, will the PCI-E recovery logic be skipped,
causing subsequent CPU accesses to crash the system?

Also, being static, doesn't this incorrectly share rate-limiting across all
device instances?

[Severity: High]
This is a pre-existing issue, but does this shared interrupt handler
unconditionally return IRQ_HANDLED without verifying ownership?

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

    return IRQ_HANDLED;
}

Since the IRQ is registered with IRQF_SHARED, if an unrelated device sharing
this line triggers an interrupt, will ls2k_bmc_interrupt() blindly claim it?

Could this schedule bmc_reset_work and trigger the 10-second stop_machine()
freeze for completely unrelated hardware events?

[Severity: High]
This is a pre-existing issue, but does the lack of a workqueue cancellation lead
to a use-after-free on driver unbind?

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

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_driver {
    .name = "ls2k-bmc",
    .id_table = ls2k_bmc_devices,
    .probe = ls2k_bmc_probe,
};

The driver has no .remove callback or devm teardown action registered to cancel
the work queue upon device unbind.

When the device is unbound, devm will free the IRQ and ddata structure. If any
pending work in the workqueue executes, won't it dereference the freed ddata?
Does this violate the MFD subsystem guidelines requiring workqueue shutdown?

[Severity: High]
This is a pre-existing issue, but is the global mfd_cell array being dynamically
mutated with a stack pointer?

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

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);
    ...
}

The ls2k_bmc_cells array lacks the const qualifier, allowing dynamic mutation.
If multiple instances probe concurrently, won't they race to write their own
stack pointers to the shared global array?

Could this result in one instance copying platform data from another's
potentially exited stack frame? Also, doesn't this explicitly violate the MFD
subsystem rule requiring static const cell arrays?

[Severity: High]
This is a pre-existing issue, but is the GPIO GSI mapping disposed of
prematurely?

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

This is called immediately after requesting the IRQ. Doesn't unregistering the
GSI tear down the interrupt mapping in the IRQ domain?

Will this cause the hardware interrupt to be disconnected and never fire,
defeating the BMC reset recovery mechanism?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804-ls2kbmc-mod-v5-0-e6bc5cdd9a93@gmail.com?part=3

[PATCH RFC v5 4/7] mfd: ls2kbmc: Cancel the work queue on removal

[Miao Wang via B4 Relay]

From: Miao Wang <shankerwangmiao@gmail.com>

When the device is being removeed, the work queue should be canceled to
avoid any pending work to be executed after the device is removed.

Fixes: d952bba3fbb5 ("mfd: ls2kbmc: Add Loongson-2K BMC reset function support")
Signed-off-by: Miao Wang <shankerwangmiao@gmail.com>
---
 drivers/mfd/ls2k-bmc-core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
index e12f2ca2fbef360ceda14ee6fc64f500e4944120..e7bebfe696ac4554eb6283279811a27ed865ab73 100644
--- a/drivers/mfd/ls2k-bmc-core.c
+++ b/drivers/mfd/ls2k-bmc-core.c
@@ -375,6 +375,12 @@ static void ls2k_bmc_save_pci_data(struct pci_dev *pdev, struct ls2k_bmc_ddata *
 	pci_read_config_dword(pdev, PCI_INTERRUPT_LINE, &ddata->bmc_pci_data.interrupt_line);
 }
 
+static void ls2k_bmc_cancel_wq(void *data)
+{
+	struct ls2k_bmc_ddata *ddata = data;
+	(void) cancel_work_sync(&ddata->bmc_reset_work);
+}
+
 static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
 {
 	struct pci_dev *pdev = to_pci_dev(ddata->dev);
@@ -385,6 +391,10 @@ static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
 
 	INIT_WORK(&ddata->bmc_reset_work, ls2k_bmc_events_fn);
 
+	ret = devm_add_action_or_reset(ddata->dev, ls2k_bmc_cancel_wq, ddata);
+	if (ret)
+		return ret;
+
 	ret = devm_request_irq(&pdev->dev, pdev->irq, ls2k_bmc_interrupt,
 			       IRQF_SHARED | IRQF_TRIGGER_FALLING, "ls2kbmc pcie", ddata);
 	if (ret) {

-- 
2.49.0

[PATCH RFC v5 4/7] mfd: ls2kbmc: Cancel the work queue on removal

[Miao Wang]

When the device is being removeed, the work queue should be canceled to
avoid any pending work to be executed after the device is removed.

Fixes: d952bba3fbb5 ("mfd: ls2kbmc: Add Loongson-2K BMC reset function support")
Signed-off-by: Miao Wang <shankerwangmiao@gmail.com>
---
 drivers/mfd/ls2k-bmc-core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
index e12f2ca2fbef360ceda14ee6fc64f500e4944120..e7bebfe696ac4554eb6283279811a27ed865ab73 100644
--- a/drivers/mfd/ls2k-bmc-core.c
+++ b/drivers/mfd/ls2k-bmc-core.c
@@ -375,6 +375,12 @@ static void ls2k_bmc_save_pci_data(struct pci_dev *pdev, struct ls2k_bmc_ddata *
 	pci_read_config_dword(pdev, PCI_INTERRUPT_LINE, &ddata->bmc_pci_data.interrupt_line);
 }
 
+static void ls2k_bmc_cancel_wq(void *data)
+{
+	struct ls2k_bmc_ddata *ddata = data;
+	(void) cancel_work_sync(&ddata->bmc_reset_work);
+}
+
 static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
 {
 	struct pci_dev *pdev = to_pci_dev(ddata->dev);
@@ -385,6 +391,10 @@ static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
 
 	INIT_WORK(&ddata->bmc_reset_work, ls2k_bmc_events_fn);
 
+	ret = devm_add_action_or_reset(ddata->dev, ls2k_bmc_cancel_wq, ddata);
+	if (ret)
+		return ret;
+
 	ret = devm_request_irq(&pdev->dev, pdev->irq, ls2k_bmc_interrupt,
 			       IRQF_SHARED | IRQF_TRIGGER_FALLING, "ls2kbmc pcie", ddata);
 	if (ret) {

-- 
2.49.0

Re: [PATCH RFC v5 4/7] mfd: ls2kbmc: Cancel the work queue on removal

[sashiko-bot]

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 variable, 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 instead of per-device state, causing cross-device interference and unprotected data races.
--

--- Patch [4]: [PATCH RFC v5 4/7] mfd: ls2kbmc: Cancel the work queue on removal ---
commit 65ab5d9b597ea3db5f87628df49d0fa3a7bc805f
Author: Miao Wang <shankerwangmiao@gmail.com>

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 *ddata)
>  
>  	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 context
trigger a scheduling while atomic bug?

>  
> +	ret = devm_add_action_or_reset(ddata->dev, ls2k_bmc_cancel_wq, ddata);
> +	if (ret)
> +		return ret;
> +
>  	ret = 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 races?

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 = 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 different
CPUs race to read and update this static variable? Should this state be moved
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 crashes 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[] = { ... };

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 = &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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804-ls2kbmc-mod-v5-0-e6bc5cdd9a93@gmail.com?part=4

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

[Miao Wang via B4 Relay]

From: Miao Wang <shankerwangmiao@gmail.com>

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. The adjustment to Kconfig for the mfd driver will be introduced
in the later patch in this series.

Fixes: d46651d4e3c0 ("ipmi: Add Loongson-2K BMC support")
Signed-off-by: Miao Wang <shankerwangmiao@gmail.com>
---
 drivers/char/ipmi/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
index 669f7600019747bcd2b37563477cf336f19a0407..f456e3e89416932f0d21bb742153a503aeb24267 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
 	help
 	  Provides a driver for Loongson-2K IPMI interfaces.
 

-- 
2.49.0

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

[Miao Wang]

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. The adjustment to Kconfig for the mfd driver will be introduced
in the later patch in this series.

Fixes: d46651d4e3c0 ("ipmi: Add Loongson-2K BMC support")
Signed-off-by: Miao Wang <shankerwangmiao@gmail.com>
---
 drivers/char/ipmi/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
index 669f7600019747bcd2b37563477cf336f19a0407..f456e3e89416932f0d21bb742153a503aeb24267 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
 	help
 	  Provides a driver for Loongson-2K IPMI interfaces.
 

-- 
2.49.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Blind dereference of platform device resources without checking if resources exist, leading to a potential NULL pointer dereference.
--

--- Patch [5]: [PATCH RFC v5 5/7] ipmi: ls2k: Relax the dependency to its mfd driver ---
commit 41124ab927989929db3ae83837ad0e4e74e88d2e
Author: Miao Wang <shankerwangmiao@gmail.com>

ipmi: ls2k: Relax the dependency to its mfd driver

This commit changes the Kconfig dependency for the Loongson-2K IPMI
interface from select to imply for MFD_LS2K_BMC_CORE. This allows
the IPMI driver to be compiled independently of the MFD driver.

> 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 and was not introduced by the patch under
review, but since this change allows the IPMI driver to probe independently
of the MFD driver, I noticed a potential regression with resource handling.

Looking at ipmi_ls2k_probe() in drivers/char/ipmi/ipmi_si_ls2k.c:

    io.addr_data = pdev->resource[0].start;
    io.regspacing = resource_size(&pdev->resource[0]);

Does this code blindly dereference the platform device resources without
checking if they exist?

If the platform device is instantiated without resources (such as manually
via sysfs, or due to a misconfigured MFD core or device tree), would
pdev->resource be NULL and cause a kernel panic?

Would it be safer to verify pdev->num_resources > 0 or use
platform_get_resource() before accessing pdev->resource[0]?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804-ls2kbmc-mod-v5-0-e6bc5cdd9a93@gmail.com?part=5

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

[Corey Minyard]

On Tue, Aug 04, 2026 at 12:55:53AM +0800, Miao Wang via B4 Relay wrote:
> From: Miao Wang <shankerwangmiao@gmail.com>
> 
> 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. The adjustment to Kconfig for the mfd driver will be introduced
> in the later patch in this series.

I don't think that's what "imply" is for.  Imply seems to be for if
there is another subsystem that can use this subsystem, but doesn't
require it to exist.

For instance:

config SENSORS_NPCM7XX
        tristate "Nuvoton NPCM750 and compatible PWM and Fan controllers"
        imply THERMAL

The fan controller will work fine without the thermal subsystem; you
can control the fan speed without it.  But the thermal subsystem is the
logical user of this.  I looked at many of these things like this.

In the IPMI case, the IPMI driver is useless without the mfd part.  So
there's no point in compiling the IPMI part of this if the mfd part is
not there.

I could be wrong, but I can't see why you would want to do this.

-corey

> 
> Fixes: d46651d4e3c0 ("ipmi: Add Loongson-2K BMC support")
> Signed-off-by: Miao Wang <shankerwangmiao@gmail.com>
> ---
>  drivers/char/ipmi/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
> index 669f7600019747bcd2b37563477cf336f19a0407..f456e3e89416932f0d21bb742153a503aeb24267 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
>  	help
>  	  Provides a driver for Loongson-2K IPMI interfaces.
>  
> 
> -- 
> 2.49.0
> 
> 

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

[Miao Wang]

Hi,

> 2026年8月4日 04:46，Corey Minyard <corey@minyard.net> 写道：
> 
> On Tue, Aug 04, 2026 at 12:55:53AM +0800, Miao Wang via B4 Relay wrote:
>> From: Miao Wang <shankerwangmiao@gmail.com>
>> 
>> 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. The adjustment to Kconfig for the mfd driver will be introduced
>> in the later patch in this series.
> 
> I don't think that's what "imply" is for.  Imply seems to be for if
> there is another subsystem that can use this subsystem, but doesn't
> require it to exist.
> 
> For instance:
> 
> config SENSORS_NPCM7XX
>        tristate "Nuvoton NPCM750 and compatible PWM and Fan controllers"
>        imply THERMAL
> 
> The fan controller will work fine without the thermal subsystem; you
> can control the fan speed without it.  But the thermal subsystem is the
> logical user of this.  I looked at many of these things like this.
> 
> In the IPMI case, the IPMI driver is useless without the mfd part.  So
> there's no point in compiling the IPMI part of this if the mfd part is
> not there.
> 
> I could be wrong, but I can't see why you would want to do this.

The mfd part and the IPMI part loosely depend on each other. Without the
IPMI part, the mfd part can still work to handle the display part.
Without the mfd part, the IPMI part is indeed useless, but it will not
generate compiling errors or other runtime errors. In the runtime,
the IPMI part can be actually loaded earlier than the mfd part. As a
result, their dependency is not that strong.

The reason why I want to change this is that "select" here requires the
mfd part should also be compiled as built-in (i.e. = y), since the
type of the configure entry IPMI_LS2K is bool. However, I cannot see
there is no other reason preventing the mfd driver from compiling as
a module. This patch series will introduce a minor fix, after which
the mfd driver will be capable to be compiled as a module.

When the type of MFD_LS2K_BMC_CORE is changed to tristate, the "select"
here will prevent selecting =m for MFD_LS2K_BMC_CORE. I thus believe
that "select" here should be also changed.

Any suggestions on declaring the dependency of the both parts?

Cheers,

Miao Wang

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

[Corey Minyard]

On Tue, Aug 04, 2026 at 05:40:14PM +0800, Miao Wang wrote:
> Hi,
> 
> > 2026年8月4日 04:46，Corey Minyard <corey@minyard.net> 写道：
> > 
> > On Tue, Aug 04, 2026 at 12:55:53AM +0800, Miao Wang via B4 Relay wrote:
> >> From: Miao Wang <shankerwangmiao@gmail.com>
> >> 
> >> 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. The adjustment to Kconfig for the mfd driver will be introduced
> >> in the later patch in this series.
> > 
> > I don't think that's what "imply" is for.  Imply seems to be for if
> > there is another subsystem that can use this subsystem, but doesn't
> > require it to exist.
> > 
> > For instance:
> > 
> > config SENSORS_NPCM7XX
> >        tristate "Nuvoton NPCM750 and compatible PWM and Fan controllers"
> >        imply THERMAL
> > 
> > The fan controller will work fine without the thermal subsystem; you
> > can control the fan speed without it.  But the thermal subsystem is the
> > logical user of this.  I looked at many of these things like this.
> > 
> > In the IPMI case, the IPMI driver is useless without the mfd part.  So
> > there's no point in compiling the IPMI part of this if the mfd part is
> > not there.
> > 
> > I could be wrong, but I can't see why you would want to do this.
> 
> The mfd part and the IPMI part loosely depend on each other. Without the
> IPMI part, the mfd part can still work to handle the display part.
> Without the mfd part, the IPMI part is indeed useless, but it will not
> generate compiling errors or other runtime errors. In the runtime,
> the IPMI part can be actually loaded earlier than the mfd part. As a
> result, their dependency is not that strong.

The operational dependency is strong, which is what I think you want to
convey here.

> 
> The reason why I want to change this is that "select" here requires the
> mfd part should also be compiled as built-in (i.e. = y), since the
> type of the configure entry IPMI_LS2K is bool. However, I cannot see
> there is no other reason preventing the mfd driver from compiling as
> a module. This patch series will introduce a minor fix, after which
> the mfd driver will be capable to be compiled as a module.
> 
> When the type of MFD_LS2K_BMC_CORE is changed to tristate, the "select"
> here will prevent selecting =m for MFD_LS2K_BMC_CORE. I thus believe
> that "select" here should be also changed.
> 
> Any suggestions on declaring the dependency of the both parts?

Ok, I understand now.

Why can't the IPMI part be compiled as a module?  Making that
module-capable would be the right fix, I think.  I can't see
why that wouldn't work.  IIRC, it was bool because the mfd part
was bool.

If you changed to imply, you would need a depends on the mfd core, BTW.

Also, IPMI_LS2K needs a "depends on IPMI_SI" either way.  I missed that
earlier.  Could you add that?

Thanks,

-corey

> 
> Cheers,
> 
> Miao Wang

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

[Miao Wang]

Hi,

> 2026年8月4日 19:39，Corey Minyard <corey@minyard.net> 写道：
> 
> On Tue, Aug 04, 2026 at 05:40:14PM +0800, Miao Wang wrote:
>> Hi,
>> 
>>> 2026年8月4日 04:46，Corey Minyard <corey@minyard.net> 写道：
>>> 
>>> On Tue, Aug 04, 2026 at 12:55:53AM +0800, Miao Wang via B4 Relay wrote:
>>>> From: Miao Wang <shankerwangmiao@gmail.com>
>>>> 
>>>> 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. The adjustment to Kconfig for the mfd driver will be introduced
>>>> in the later patch in this series.
>>> 
>>> I don't think that's what "imply" is for.  Imply seems to be for if
>>> there is another subsystem that can use this subsystem, but doesn't
>>> require it to exist.
>>> 
>>> For instance:
>>> 
>>> config SENSORS_NPCM7XX
>>>       tristate "Nuvoton NPCM750 and compatible PWM and Fan controllers"
>>>       imply THERMAL
>>> 
>>> The fan controller will work fine without the thermal subsystem; you
>>> can control the fan speed without it.  But the thermal subsystem is the
>>> logical user of this.  I looked at many of these things like this.
>>> 
>>> In the IPMI case, the IPMI driver is useless without the mfd part.  So
>>> there's no point in compiling the IPMI part of this if the mfd part is
>>> not there.
>>> 
>>> I could be wrong, but I can't see why you would want to do this.
>> 
>> The mfd part and the IPMI part loosely depend on each other. Without the
>> IPMI part, the mfd part can still work to handle the display part.
>> Without the mfd part, the IPMI part is indeed useless, but it will not
>> generate compiling errors or other runtime errors. In the runtime,
>> the IPMI part can be actually loaded earlier than the mfd part. As a
>> result, their dependency is not that strong.
> 
> The operational dependency is strong, which is what I think you want to
> convey here.
> 
>> 
>> The reason why I want to change this is that "select" here requires the
>> mfd part should also be compiled as built-in (i.e. = y), since the
>> type of the configure entry IPMI_LS2K is bool. However, I cannot see
>> there is no other reason preventing the mfd driver from compiling as
>> a module. This patch series will introduce a minor fix, after which
>> the mfd driver will be capable to be compiled as a module.
>> 
>> When the type of MFD_LS2K_BMC_CORE is changed to tristate, the "select"
>> here will prevent selecting =m for MFD_LS2K_BMC_CORE. I thus believe
>> that "select" here should be also changed.
>> 
>> Any suggestions on declaring the dependency of the both parts?
> 
> Ok, I understand now.
> 
> Why can't the IPMI part be compiled as a module?  Making that
> module-capable would be the right fix, I think.  I can't see
> why that wouldn't work.  IIRC, it was bool because the mfd part
> was bool.

I would have agree with you if the IPMI part was a normal driver.
However, the IPMI part is actually a part of ipmi_si. As a result,
the IPMI part can actually be compiled as a module, but as a part
of ipmi_si. That is why the configure entry IPMI_LS2K is bool rather
than tristate. I don't think it is trivial to convert IPMI_LS2K from
bool to tristate.

> If you changed to imply, you would need a depends on the mfd core, BTW.

If imply is used here, and there is "select MFD_CORE" in MFD_LS2K_BMC_CORE,
I think there is no need to add "depends on MFD_CORE" to IPMI_LS2K, if I
understand it correctly.

I don't insist using imply here, if there is a better way to express the
relation between both drivers.

> Also, IPMI_LS2K needs a "depends on IPMI_SI" either way.  I missed that
> earlier.  Could you add that?

If I understand it correctly, there seems to be no need to add the
dependency from the MFD driver to the driver for the actual sub-functions.
Examples are:

- MFD_MENF21BMC and SENSORS_MENF21BMC_HWMON, LEDS_MENF21BMC, 
  MENF21BMC_WATCHDOG: The later three declared depends on the former MFD
  driver
- MFD_INTEL_M10_BMC_CORE and FPGA_M10_BMC_SEC_UPDATE, 
  SENSORS_INTEL_M10_BMC_HWMON: The later two declared depends on the
  former MFD driver.
- MFD_STMFX and PINCTRL_STMFX: The later declared select the former
  MFD driver.

To summarize, as shown in the current mfd drivers, there is no need to
declare dependency to the drivers providing the sub-functions in the
mfd drivers, and the drivers of the sub functions should "depends on" or
"select" the mfd driver. So if IPMI_LS2K could be tristate, then it would
be reasonable to select MFD_LS2K_BMC_CORE or depends on MFD_LS2K_BMC_CORE.


Regarding the fact that IPMI_LS2K is bool and it is not an independent
module but a part of ipmi_si, it comes into my mind that we may choose
to adjust the dependency from ipmi_si. That is to remove "select
MFD_LS2K_BMC_CORE" from IPMI_LS2K, and add "select MFD_LS2K_BMC_CORE
if IPMI_LS2K" to IPMI_SI.

I wonder if such a change would be appropriate.

Cheers,

Miao Wang

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

[Corey Minyard]

On Tue, Aug 04, 2026 at 08:27:11PM +0800, Miao Wang wrote:
> Hi,
> 
> > 2026年8月4日 19:39，Corey Minyard <corey@minyard.net> 写道：
> > 
> > On Tue, Aug 04, 2026 at 05:40:14PM +0800, Miao Wang wrote:
> >> Hi,
> >> 
> >>> 2026年8月4日 04:46，Corey Minyard <corey@minyard.net> 写道：
> >>> 
> >>> On Tue, Aug 04, 2026 at 12:55:53AM +0800, Miao Wang via B4 Relay wrote:
> >>>> From: Miao Wang <shankerwangmiao@gmail.com>
> >>>> 
> >>>> 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. The adjustment to Kconfig for the mfd driver will be introduced
> >>>> in the later patch in this series.
> >>> 
> >>> I don't think that's what "imply" is for.  Imply seems to be for if
> >>> there is another subsystem that can use this subsystem, but doesn't
> >>> require it to exist.
> >>> 
> >>> For instance:
> >>> 
> >>> config SENSORS_NPCM7XX
> >>>       tristate "Nuvoton NPCM750 and compatible PWM and Fan controllers"
> >>>       imply THERMAL
> >>> 
> >>> The fan controller will work fine without the thermal subsystem; you
> >>> can control the fan speed without it.  But the thermal subsystem is the
> >>> logical user of this.  I looked at many of these things like this.
> >>> 
> >>> In the IPMI case, the IPMI driver is useless without the mfd part.  So
> >>> there's no point in compiling the IPMI part of this if the mfd part is
> >>> not there.
> >>> 
> >>> I could be wrong, but I can't see why you would want to do this.
> >> 
> >> The mfd part and the IPMI part loosely depend on each other. Without the
> >> IPMI part, the mfd part can still work to handle the display part.
> >> Without the mfd part, the IPMI part is indeed useless, but it will not
> >> generate compiling errors or other runtime errors. In the runtime,
> >> the IPMI part can be actually loaded earlier than the mfd part. As a
> >> result, their dependency is not that strong.
> > 
> > The operational dependency is strong, which is what I think you want to
> > convey here.
> > 
> >> 
> >> The reason why I want to change this is that "select" here requires the
> >> mfd part should also be compiled as built-in (i.e. = y), since the
> >> type of the configure entry IPMI_LS2K is bool. However, I cannot see
> >> there is no other reason preventing the mfd driver from compiling as
> >> a module. This patch series will introduce a minor fix, after which
> >> the mfd driver will be capable to be compiled as a module.
> >> 
> >> When the type of MFD_LS2K_BMC_CORE is changed to tristate, the "select"
> >> here will prevent selecting =m for MFD_LS2K_BMC_CORE. I thus believe
> >> that "select" here should be also changed.
> >> 
> >> Any suggestions on declaring the dependency of the both parts?
> > 
> > Ok, I understand now.
> > 
> > Why can't the IPMI part be compiled as a module?  Making that
> > module-capable would be the right fix, I think.  I can't see
> > why that wouldn't work.  IIRC, it was bool because the mfd part
> > was bool.
> 
> I would have agree with you if the IPMI part was a normal driver.
> However, the IPMI part is actually a part of ipmi_si. As a result,
> the IPMI part can actually be compiled as a module, but as a part
> of ipmi_si. That is why the configure entry IPMI_LS2K is bool rather
> than tristate. I don't think it is trivial to convert IPMI_LS2K from
> bool to tristate.

Yes, you are right it would be non-trivial.  It would take some redesign
of the ipmi_si module.  You are right here, probably not worth it.

> 
> > If you changed to imply, you would need a depends on the mfd core, BTW.
> 
> If imply is used here, and there is "select MFD_CORE" in MFD_LS2K_BMC_CORE,
> I think there is no need to add "depends on MFD_CORE" to IPMI_LS2K, if I
> understand it correctly.

If imply doesn't force MFD_LS2K_BMC_CORE to be selected, then it's not
going to force MFD_CORE to be selected.

> 
> I don't insist using imply here, if there is a better way to express the
> relation between both drivers.
> 
> > Also, IPMI_LS2K needs a "depends on IPMI_SI" either way.  I missed that
> > earlier.  Could you add that?
> 
> If I understand it correctly, there seems to be no need to add the
> dependency from the MFD driver to the driver for the actual sub-functions.
> Examples are:

There absolutely is.  You can select IPMI_LS2K without selecting
IPMI_SI.  It would compile without it, and it just wouldn't do anything,
but it would be confusing to be able to select something and it not do
anything.

> 
> - MFD_MENF21BMC and SENSORS_MENF21BMC_HWMON, LEDS_MENF21BMC, 
>   MENF21BMC_WATCHDOG: The later three declared depends on the former MFD
>   driver
> - MFD_INTEL_M10_BMC_CORE and FPGA_M10_BMC_SEC_UPDATE, 
>   SENSORS_INTEL_M10_BMC_HWMON: The later two declared depends on the
>   former MFD driver.
> - MFD_STMFX and PINCTRL_STMFX: The later declared select the former
>   MFD driver.
> 
> To summarize, as shown in the current mfd drivers, there is no need to
> declare dependency to the drivers providing the sub-functions in the
> mfd drivers, and the drivers of the sub functions should "depends on" or
> "select" the mfd driver. So if IPMI_LS2K could be tristate, then it would
> be reasonable to select MFD_LS2K_BMC_CORE or depends on MFD_LS2K_BMC_CORE.
> 
> 
> Regarding the fact that IPMI_LS2K is bool and it is not an independent
> module but a part of ipmi_si, it comes into my mind that we may choose
> to adjust the dependency from ipmi_si. That is to remove "select
> MFD_LS2K_BMC_CORE" from IPMI_LS2K, and add "select MFD_LS2K_BMC_CORE
> if IPMI_LS2K" to IPMI_SI.
> 
> I wonder if such a change would be appropriate.

That would be a good solution if it works.  Thanks for working with
me on this.

-corey

> 
> Cheers,
> 
> Miao Wang
> 

[PATCH RFC v5 6/7] mfd: ls2kbmc: Able to be compiled as a module

[Miao Wang via B4 Relay]

From: Miao Wang <shankerwangmiao@gmail.com>

Make ls2kmc able to be compiled as a module

Signed-off-by: Miao Wang <shankerwangmiao@gmail.com>
---
 drivers/mfd/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 763ce6a34782bdd5d2b1a4d840c75b040092d83e..a7a9f97af248c88489dc1203a1ba05f2ce4827df 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2494,7 +2494,7 @@ config MFD_LOONGSON_SE
 	  which will forward them to the corresponding engine.
 
 config MFD_LS2K_BMC_CORE
-	bool "Loongson-2K Board Management Controller Support"
+	tristate "Loongson-2K Board Management Controller Support"
 	depends on PCI && ACPI_GENERIC_GSI
 	select MFD_CORE
 	help

-- 
2.49.0

[PATCH RFC v5 6/7] mfd: ls2kbmc: Able to be compiled as a module

[Miao Wang]

Make ls2kmc able to be compiled as a module

Signed-off-by: Miao Wang <shankerwangmiao@gmail.com>
---
 drivers/mfd/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 763ce6a34782bdd5d2b1a4d840c75b040092d83e..a7a9f97af248c88489dc1203a1ba05f2ce4827df 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2494,7 +2494,7 @@ config MFD_LOONGSON_SE
 	  which will forward them to the corresponding engine.
 
 config MFD_LS2K_BMC_CORE
-	bool "Loongson-2K Board Management Controller Support"
+	tristate "Loongson-2K Board Management Controller Support"
 	depends on PCI && ACPI_GENERIC_GSI
 	select MFD_CORE
 	help

-- 
2.49.0

Re: [PATCH RFC v5 6/7] mfd: ls2kbmc: Able to be compiled as a module

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The driver executes a 10+ second busy-wait and a sleeping function inside a `stop_machine()` context, severely stalling the entire system.
- [High] `acpi_unregister_gsi()` is erroneously called immediately after requesting the IRQ, destroying the mapping and breaking interrupt delivery.
- [Medium] The global `mfd_cell` array is dynamically modified using a stack-allocated pointer and lacks the `const` qualifier, violating MFD subsystem guidelines.
--

--- Patch [6]: [PATCH RFC v5 6/7] mfd: ls2kbmc: Able to be compiled as a module ---
commit 998b814232bcf86316d4b7feaee8e3da166c0a9e
Author: Miao Wang <shankerwangmiao@gmail.com>

mfd: ls2kbmc: Able to be compiled as a module

Make ls2kmc able to be compiled as a module by changing the Kconfig option
from bool to tristate.

> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 763ce6a34782b..a7a9f97af248c 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
[ ... ]
> @@ -2494,7 +2494,7 @@ config MFD_LOONGSON_SE
>  	  which will forward them to the corresponding engine.
>  
>  config MFD_LS2K_BMC_CORE
> -	bool "Loongson-2K Board Management Controller Support"
> +	tristate "Loongson-2K Board Management Controller Support"

[Severity: High]
This is a pre-existing issue, but enabling this driver to be built as a
module exposes it during module unload or driver unbind.

Does this code in ls2k_bmc_init() destroy the GSI mapping while the IRQ
is still in use?

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

Calling acpi_unregister_gsi() unconditionally right after devm_request_irq()
succeeds disposes of the mapping. Hardware interrupts will never reach the
registered handler. Furthermore, during module unload or driver unbind,
devm_free_irq() will attempt to free an IRQ whose mapping and descriptor
have already been disposed, potentially causing a use-after-free or kernel
warning.

[Severity: Medium]
This is a pre-existing issue, but unbinding and rebinding the module could
trigger a race condition here.

Does this assign the address of a local stack variable to a global array?

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

Once probe returns, the platform_data pointer points to freed stack memory.
If another instance of the device binds, or if the driver unbinds and
rebinds, this dangling pointer could be used or overwritten concurrently.
Additionally, modifying a global mfd_cell array dynamically violates MFD
subsystem guidelines which require it to be static const.

[Severity: Critical]
This isn't a bug introduced by this patch, but there is a severe system
stall issue in the event handler.

Does this sequence execute a 10,000ms delay and call a sleeping function
inside an atomic context?

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

Inside the callback, it invokes a 10-second busy wait:

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

It also calls a function that can sleep:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_pcie_is_connected() {
    ...
    base = pci_iomap(parent, 0, LS7A_PCI_CFG_SIZE);
    ...
}

The workqueue handler ls2k_bmc_events_fn() calls stop_machine(), which
runs ls2k_bmc_recover_pci_data() while all other CPUs spin with local
interrupts disabled. Stalling the entire system with interrupts disabled
for over 10 seconds can lead to system-wide lockups, RCU stalls, and NMI
watchdog panics. Also, pci_iomap() can acquire locks and sleep, which is
unsafe inside stop_machine().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804-ls2kbmc-mod-v5-0-e6bc5cdd9a93@gmail.com?part=6

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

[Miao Wang via B4 Relay]

From: Miao Wang <shankerwangmiao@gmail.com>

The reset event of BMC is captured through GPIO. However, this driver
bypasses the GPIO framework and directly accesses the GPIO controller
through the fixed address. When the same GPIO controller is also
exposed through ACPI and probed by the corresponding GPIO driver,
there would be a conflict between the two drivers.

This patch will try to find the GPIO through declared GPIO pin in the
_CRS resources of the ACPI node. If no such delaration is found, the
driver will fall back to search for the correct GPIO controller and pin
according to the fixed address and pin number. A possible DSDT
declaration for the GPIO pin might be as follows:

    Device (BMC0) {
        Name (_ADR, ...) // Match the PCI address of the BMC device
        // \_SB.GPO1 is the ACPI path of the GPIO controller
        Name (_CRS, ResourceTemplate () {
            GpioInt (Edge, ActiveLow, Exclusive, PullNone, 0,
                     "\\_SB.GPO1", 0) {
                14 // 14 is the GPIO pin number
            }
    }

Signed-off-by: Miao Wang <shankerwangmiao@gmail.com>
---
 drivers/mfd/ls2k-bmc-core.c | 163 +++++++++++++++++++++++++++++++-------------
 1 file changed, 116 insertions(+), 47 deletions(-)

diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
index e7bebfe696ac4554eb6283279811a27ed865ab73..97a666a136fe41ed4268cb70770201533aebb1d5 100644
--- a/drivers/mfd/ls2k-bmc-core.c
+++ b/drivers/mfd/ls2k-bmc-core.c
@@ -26,6 +26,10 @@
 #include <linux/stop_machine.h>
 #include <linux/vt_kern.h>
 #include <linux/console.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/property.h>
+#include <linux/gpio/machine.h>
 
 /* LS2K BMC resources */
 #define LS2K_DISPLAY_RES_START		(SZ_16M + SZ_2M)
@@ -81,18 +85,6 @@
 
 #define PCI_REG_STRIDE			0x4
 
-#define LS2K_BMC_RESET_GPIO		14
-#define LOONGSON_GPIO_REG_BASE		0x1FE00500
-#define LOONGSON_GPIO_REG_SIZE		0x18
-#define LOONGSON_GPIO_OEN		0x0
-#define LOONGSON_GPIO_FUNC		0x4
-#define LOONGSON_GPIO_INTPOL		0x10
-#define LOONGSON_GPIO_INTEN		0x14
-
-#define LOONGSON_IO_INT_BASE		16
-#define LS2K_BMC_RESET_GPIO_INT_VEC	(LS2K_BMC_RESET_GPIO % 8)
-#define LS2K_BMC_RESET_GPIO_GSI		(LOONGSON_IO_INT_BASE + LS2K_BMC_RESET_GPIO_INT_VEC)
-
 enum {
 	LS2K_BMC_DISPLAY,
 	LS2K_BMC_IPMI0,
@@ -186,6 +178,7 @@ struct ls2k_bmc_ddata {
 	struct work_struct bmc_reset_work;
 	struct ls2k_bmc_pci_data bmc_pci_data;
 	struct ls2k_bmc_bridge_pci_data bridge_pci_data;
+	struct gpio_desc *reset_gpio;
 };
 
 static bool ls2k_bmc_bar0_addr_is_set(struct pci_dev *pdev)
@@ -375,6 +368,91 @@ static void ls2k_bmc_save_pci_data(struct pci_dev *pdev, struct ls2k_bmc_ddata *
 	pci_read_config_dword(pdev, PCI_INTERRUPT_LINE, &ddata->bmc_pci_data.interrupt_line);
 }
 
+static struct fwnode_handle *gpio_dev_get_fwnode(struct gpio_device *gdev)
+{
+	struct device *gdev_dev;
+
+	gdev_dev = gpio_device_to_device(gdev);
+	return dev_fwnode(gdev_dev);
+}
+
+static int ls2k_bmc_gpiochip_find(struct gpio_chip *gc, const void *data)
+{
+	struct acpi_device *adev;
+	struct list_head resource_list;
+	struct resource_entry *rentry;
+	struct fwnode_handle *fwnode = gpio_dev_get_fwnode(gc->gpiodev);
+	phys_addr_t start_addr = (phys_addr_t) data;
+	int ret, found = 0;
+
+	if (!is_acpi_node(fwnode))
+		goto out;
+
+	adev = to_acpi_device_node(fwnode);
+	if (!adev)
+		goto out;
+
+	INIT_LIST_HEAD(&resource_list);
+
+	ret = acpi_dev_get_memory_resources(adev, &resource_list);
+	if (ret < 0)
+		goto out;
+	/*
+	 * ACPI memory resources are ordered and only the first one is
+	 * considered by the driver of the expected GPIO controller. So
+	 * here we also only check the first one to see if it matches the
+	 * expected address.
+	 */
+	rentry = list_first_entry_or_null(&resource_list, struct resource_entry, node);
+	if (!rentry)
+		goto free_resource_list;
+	if (rentry->res->start == start_addr)
+		found = 1;
+
+free_resource_list:
+	acpi_dev_free_resource_list(&resource_list);
+out:
+	return found;
+}
+
+static struct gpio_desc *ls2k_bmc_find_gpio(struct ls2k_bmc_ddata *ddata)
+{
+	/*
+	 * In conventional way, the GPIO should be obtained through ACPI or
+	 * device tree. However, when the information is not available,
+	 * we should find the GPIO according to the convention of the server
+	 * boards with LS2K BMC, the gpio signal reflecting the reset event
+	 * of the BMC should be connected to pin 14 of the GPIO input of
+	 * the first CPU node. The address of that GPIO controller is fixed.
+	 */
+	static const phys_addr_t LOONGSON_GPIO_REG_BASE = 0x1FE00500;
+	static const unsigned int LS2K_BMC_RESET_GPIO = 14;
+	int ret;
+	struct gpio_device *gdev __free(gpio_device_put) = NULL;
+	struct property_entry ls2k_bmc_swnode_properties[2] = { 0 };
+
+	dev_dbg(ddata->dev, "Searching for GPIO chip at address %pa\n", &LOONGSON_GPIO_REG_BASE);
+
+	gdev = gpio_device_find((void *)LOONGSON_GPIO_REG_BASE, ls2k_bmc_gpiochip_find);
+
+	if (!gdev) {
+		dev_dbg(ddata->dev, "cannot find GPIO chip at address %pa, deferring\n",
+			&LOONGSON_GPIO_REG_BASE);
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	ls2k_bmc_swnode_properties[0] = PROPERTY_ENTRY_GPIO("gpio",
+		gpio_dev_get_fwnode(gdev), LS2K_BMC_RESET_GPIO, GPIO_ACTIVE_HIGH);
+
+	ret = device_create_managed_software_node(ddata->dev, ls2k_bmc_swnode_properties, NULL);
+	if (ret) {
+		dev_err(ddata->dev, "Failed to create software node for GPIO reset: %d\n", ret);
+		return ERR_PTR(ret);
+	}
+
+	return devm_gpiod_get_index(ddata->dev, NULL, 0, GPIOD_IN);
+}
+
 static void ls2k_bmc_cancel_wq(void *data)
 {
 	struct ls2k_bmc_ddata *ddata = data;
@@ -384,8 +462,7 @@ static void ls2k_bmc_cancel_wq(void *data)
 static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
 {
 	struct pci_dev *pdev = to_pci_dev(ddata->dev);
-	void __iomem *gpio_base;
-	int gpio_irq, ret, val;
+	int gpio_irq, ret;
 
 	ls2k_bmc_save_pci_data(pdev, ddata);
 
@@ -402,44 +479,36 @@ static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
 		return ret;
 	}
 
-	gpio_base = ioremap(LOONGSON_GPIO_REG_BASE, LOONGSON_GPIO_REG_SIZE);
-	if (!gpio_base)
-		return -ENOMEM;
-
-	/* Disable GPIO output */
-	val = readl(gpio_base + LOONGSON_GPIO_OEN);
-	writel(val | BIT(LS2K_BMC_RESET_GPIO), gpio_base + LOONGSON_GPIO_OEN);
-
-	/* Enable GPIO functionality */
-	val = readl(gpio_base + LOONGSON_GPIO_FUNC);
-	writel(val & ~BIT(LS2K_BMC_RESET_GPIO), gpio_base + LOONGSON_GPIO_FUNC);
-
-	/* Set GPIO interrupts to low-level active */
-	val = readl(gpio_base + LOONGSON_GPIO_INTPOL);
-	writel(val & ~BIT(LS2K_BMC_RESET_GPIO), gpio_base + LOONGSON_GPIO_INTPOL);
-
-	/* Enable GPIO interrupts */
-	val = readl(gpio_base + LOONGSON_GPIO_INTEN);
-	writel(val | BIT(LS2K_BMC_RESET_GPIO), gpio_base + LOONGSON_GPIO_INTEN);
+	ddata->reset_gpio = devm_gpiod_get_index_optional(&pdev->dev, NULL, 0, GPIOD_IN);
+	if (IS_ERR(ddata->reset_gpio)) {
+		ret = PTR_ERR(ddata->reset_gpio);
+		ddata->reset_gpio = NULL;
+		return dev_err_probe(ddata->dev, ret, "Failed to get GPIO pin for reset signal\n");
+	}
+	if (ddata->reset_gpio == NULL) {
+		ddata->reset_gpio = ls2k_bmc_find_gpio(ddata);
+		if (IS_ERR(ddata->reset_gpio)) {
+			ret = PTR_ERR(ddata->reset_gpio);
+			ddata->reset_gpio = NULL;
+			return dev_err_probe(ddata->dev, ret,
+					     "Failed to find GPIO pin for reset signal\n");
+		}
+	}
 
-	iounmap(gpio_base);
+	gpio_irq = gpiod_to_irq(ddata->reset_gpio);
 
-	/*
-	 * Since gpio_chip->to_irq is not implemented in the Loongson-3 GPIO driver,
-	 * acpi_register_gsi() is used to obtain the GPIO IRQ. The GPIO interrupt is a
-	 * watchdog interrupt that is triggered when the BMC resets.
-	 */
-	gpio_irq = acpi_register_gsi(NULL, LS2K_BMC_RESET_GPIO_GSI, ACPI_EDGE_SENSITIVE,
-				     ACPI_ACTIVE_LOW);
 	if (gpio_irq < 0)
-		return gpio_irq;
+		return dev_err_probe(ddata->dev, gpio_irq,
+				     "Failed to get IRQ for GPIO reset signal input\n");
 
-	ret = devm_request_irq(ddata->dev, gpio_irq, ls2k_bmc_interrupt,
-			       IRQF_SHARED | IRQF_TRIGGER_FALLING, "ls2kbmc gpio", ddata);
-	if (ret)
-		dev_err(ddata->dev, "Failed to request LS2KBMC GPIO IRQ %d.\n", gpio_irq);
+	ret = devm_request_irq(&pdev->dev, gpio_irq, ls2k_bmc_interrupt,
+			       IRQF_SHARED | IRQF_TRIGGER_FALLING, "ls2kbmc reset", ddata);
+
+	if (ret != 0)
+		return dev_err_probe(ddata->dev, ret,
+				     "Failed to request IRQ %d for GPIO reset signal input.\n",
+				     gpio_irq);
 
-	acpi_unregister_gsi(LS2K_BMC_RESET_GPIO_GSI);
 	return ret;
 }
 

-- 
2.49.0

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

[Miao Wang]

The reset event of BMC is captured through GPIO. However, this driver
bypasses the GPIO framework and directly accesses the GPIO controller
through the fixed address. When the same GPIO controller is also
exposed through ACPI and probed by the corresponding GPIO driver,
there would be a conflict between the two drivers.

This patch will try to find the GPIO through declared GPIO pin in the
_CRS resources of the ACPI node. If no such delaration is found, the
driver will fall back to search for the correct GPIO controller and pin
according to the fixed address and pin number. A possible DSDT
declaration for the GPIO pin might be as follows:

    Device (BMC0) {
        Name (_ADR, ...) // Match the PCI address of the BMC device
        // \_SB.GPO1 is the ACPI path of the GPIO controller
        Name (_CRS, ResourceTemplate () {
            GpioInt (Edge, ActiveLow, Exclusive, PullNone, 0,
                     "\\_SB.GPO1", 0) {
                14 // 14 is the GPIO pin number
            }
    }

Signed-off-by: Miao Wang <shankerwangmiao@gmail.com>
---
 drivers/mfd/ls2k-bmc-core.c | 163 +++++++++++++++++++++++++++++++-------------
 1 file changed, 116 insertions(+), 47 deletions(-)

diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
index e7bebfe696ac4554eb6283279811a27ed865ab73..97a666a136fe41ed4268cb70770201533aebb1d5 100644
--- a/drivers/mfd/ls2k-bmc-core.c
+++ b/drivers/mfd/ls2k-bmc-core.c
@@ -26,6 +26,10 @@
 #include <linux/stop_machine.h>
 #include <linux/vt_kern.h>
 #include <linux/console.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/property.h>
+#include <linux/gpio/machine.h>
 
 /* LS2K BMC resources */
 #define LS2K_DISPLAY_RES_START		(SZ_16M + SZ_2M)
@@ -81,18 +85,6 @@
 
 #define PCI_REG_STRIDE			0x4
 
-#define LS2K_BMC_RESET_GPIO		14
-#define LOONGSON_GPIO_REG_BASE		0x1FE00500
-#define LOONGSON_GPIO_REG_SIZE		0x18
-#define LOONGSON_GPIO_OEN		0x0
-#define LOONGSON_GPIO_FUNC		0x4
-#define LOONGSON_GPIO_INTPOL		0x10
-#define LOONGSON_GPIO_INTEN		0x14
-
-#define LOONGSON_IO_INT_BASE		16
-#define LS2K_BMC_RESET_GPIO_INT_VEC	(LS2K_BMC_RESET_GPIO % 8)
-#define LS2K_BMC_RESET_GPIO_GSI		(LOONGSON_IO_INT_BASE + LS2K_BMC_RESET_GPIO_INT_VEC)
-
 enum {
 	LS2K_BMC_DISPLAY,
 	LS2K_BMC_IPMI0,
@@ -186,6 +178,7 @@ struct ls2k_bmc_ddata {
 	struct work_struct bmc_reset_work;
 	struct ls2k_bmc_pci_data bmc_pci_data;
 	struct ls2k_bmc_bridge_pci_data bridge_pci_data;
+	struct gpio_desc *reset_gpio;
 };
 
 static bool ls2k_bmc_bar0_addr_is_set(struct pci_dev *pdev)
@@ -375,6 +368,91 @@ static void ls2k_bmc_save_pci_data(struct pci_dev *pdev, struct ls2k_bmc_ddata *
 	pci_read_config_dword(pdev, PCI_INTERRUPT_LINE, &ddata->bmc_pci_data.interrupt_line);
 }
 
+static struct fwnode_handle *gpio_dev_get_fwnode(struct gpio_device *gdev)
+{
+	struct device *gdev_dev;
+
+	gdev_dev = gpio_device_to_device(gdev);
+	return dev_fwnode(gdev_dev);
+}
+
+static int ls2k_bmc_gpiochip_find(struct gpio_chip *gc, const void *data)
+{
+	struct acpi_device *adev;
+	struct list_head resource_list;
+	struct resource_entry *rentry;
+	struct fwnode_handle *fwnode = gpio_dev_get_fwnode(gc->gpiodev);
+	phys_addr_t start_addr = (phys_addr_t) data;
+	int ret, found = 0;
+
+	if (!is_acpi_node(fwnode))
+		goto out;
+
+	adev = to_acpi_device_node(fwnode);
+	if (!adev)
+		goto out;
+
+	INIT_LIST_HEAD(&resource_list);
+
+	ret = acpi_dev_get_memory_resources(adev, &resource_list);
+	if (ret < 0)
+		goto out;
+	/*
+	 * ACPI memory resources are ordered and only the first one is
+	 * considered by the driver of the expected GPIO controller. So
+	 * here we also only check the first one to see if it matches the
+	 * expected address.
+	 */
+	rentry = list_first_entry_or_null(&resource_list, struct resource_entry, node);
+	if (!rentry)
+		goto free_resource_list;
+	if (rentry->res->start == start_addr)
+		found = 1;
+
+free_resource_list:
+	acpi_dev_free_resource_list(&resource_list);
+out:
+	return found;
+}
+
+static struct gpio_desc *ls2k_bmc_find_gpio(struct ls2k_bmc_ddata *ddata)
+{
+	/*
+	 * In conventional way, the GPIO should be obtained through ACPI or
+	 * device tree. However, when the information is not available,
+	 * we should find the GPIO according to the convention of the server
+	 * boards with LS2K BMC, the gpio signal reflecting the reset event
+	 * of the BMC should be connected to pin 14 of the GPIO input of
+	 * the first CPU node. The address of that GPIO controller is fixed.
+	 */
+	static const phys_addr_t LOONGSON_GPIO_REG_BASE = 0x1FE00500;
+	static const unsigned int LS2K_BMC_RESET_GPIO = 14;
+	int ret;
+	struct gpio_device *gdev __free(gpio_device_put) = NULL;
+	struct property_entry ls2k_bmc_swnode_properties[2] = { 0 };
+
+	dev_dbg(ddata->dev, "Searching for GPIO chip at address %pa\n", &LOONGSON_GPIO_REG_BASE);
+
+	gdev = gpio_device_find((void *)LOONGSON_GPIO_REG_BASE, ls2k_bmc_gpiochip_find);
+
+	if (!gdev) {
+		dev_dbg(ddata->dev, "cannot find GPIO chip at address %pa, deferring\n",
+			&LOONGSON_GPIO_REG_BASE);
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	ls2k_bmc_swnode_properties[0] = PROPERTY_ENTRY_GPIO("gpio",
+		gpio_dev_get_fwnode(gdev), LS2K_BMC_RESET_GPIO, GPIO_ACTIVE_HIGH);
+
+	ret = device_create_managed_software_node(ddata->dev, ls2k_bmc_swnode_properties, NULL);
+	if (ret) {
+		dev_err(ddata->dev, "Failed to create software node for GPIO reset: %d\n", ret);
+		return ERR_PTR(ret);
+	}
+
+	return devm_gpiod_get_index(ddata->dev, NULL, 0, GPIOD_IN);
+}
+
 static void ls2k_bmc_cancel_wq(void *data)
 {
 	struct ls2k_bmc_ddata *ddata = data;
@@ -384,8 +462,7 @@ static void ls2k_bmc_cancel_wq(void *data)
 static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
 {
 	struct pci_dev *pdev = to_pci_dev(ddata->dev);
-	void __iomem *gpio_base;
-	int gpio_irq, ret, val;
+	int gpio_irq, ret;
 
 	ls2k_bmc_save_pci_data(pdev, ddata);
 
@@ -402,44 +479,36 @@ static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
 		return ret;
 	}
 
-	gpio_base = ioremap(LOONGSON_GPIO_REG_BASE, LOONGSON_GPIO_REG_SIZE);
-	if (!gpio_base)
-		return -ENOMEM;
-
-	/* Disable GPIO output */
-	val = readl(gpio_base + LOONGSON_GPIO_OEN);
-	writel(val | BIT(LS2K_BMC_RESET_GPIO), gpio_base + LOONGSON_GPIO_OEN);
-
-	/* Enable GPIO functionality */
-	val = readl(gpio_base + LOONGSON_GPIO_FUNC);
-	writel(val & ~BIT(LS2K_BMC_RESET_GPIO), gpio_base + LOONGSON_GPIO_FUNC);
-
-	/* Set GPIO interrupts to low-level active */
-	val = readl(gpio_base + LOONGSON_GPIO_INTPOL);
-	writel(val & ~BIT(LS2K_BMC_RESET_GPIO), gpio_base + LOONGSON_GPIO_INTPOL);
-
-	/* Enable GPIO interrupts */
-	val = readl(gpio_base + LOONGSON_GPIO_INTEN);
-	writel(val | BIT(LS2K_BMC_RESET_GPIO), gpio_base + LOONGSON_GPIO_INTEN);
+	ddata->reset_gpio = devm_gpiod_get_index_optional(&pdev->dev, NULL, 0, GPIOD_IN);
+	if (IS_ERR(ddata->reset_gpio)) {
+		ret = PTR_ERR(ddata->reset_gpio);
+		ddata->reset_gpio = NULL;
+		return dev_err_probe(ddata->dev, ret, "Failed to get GPIO pin for reset signal\n");
+	}
+	if (ddata->reset_gpio == NULL) {
+		ddata->reset_gpio = ls2k_bmc_find_gpio(ddata);
+		if (IS_ERR(ddata->reset_gpio)) {
+			ret = PTR_ERR(ddata->reset_gpio);
+			ddata->reset_gpio = NULL;
+			return dev_err_probe(ddata->dev, ret,
+					     "Failed to find GPIO pin for reset signal\n");
+		}
+	}
 
-	iounmap(gpio_base);
+	gpio_irq = gpiod_to_irq(ddata->reset_gpio);
 
-	/*
-	 * Since gpio_chip->to_irq is not implemented in the Loongson-3 GPIO driver,
-	 * acpi_register_gsi() is used to obtain the GPIO IRQ. The GPIO interrupt is a
-	 * watchdog interrupt that is triggered when the BMC resets.
-	 */
-	gpio_irq = acpi_register_gsi(NULL, LS2K_BMC_RESET_GPIO_GSI, ACPI_EDGE_SENSITIVE,
-				     ACPI_ACTIVE_LOW);
 	if (gpio_irq < 0)
-		return gpio_irq;
+		return dev_err_probe(ddata->dev, gpio_irq,
+				     "Failed to get IRQ for GPIO reset signal input\n");
 
-	ret = devm_request_irq(ddata->dev, gpio_irq, ls2k_bmc_interrupt,
-			       IRQF_SHARED | IRQF_TRIGGER_FALLING, "ls2kbmc gpio", ddata);
-	if (ret)
-		dev_err(ddata->dev, "Failed to request LS2KBMC GPIO IRQ %d.\n", gpio_irq);
+	ret = devm_request_irq(&pdev->dev, gpio_irq, ls2k_bmc_interrupt,
+			       IRQF_SHARED | IRQF_TRIGGER_FALLING, "ls2kbmc reset", ddata);
+
+	if (ret != 0)
+		return dev_err_probe(ddata->dev, ret,
+				     "Failed to request IRQ %d for GPIO reset signal input.\n",
+				     gpio_irq);
 
-	acpi_unregister_gsi(LS2K_BMC_RESET_GPIO_GSI);
 	return ret;
 }
 

-- 
2.49.0

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

[sashiko-bot]

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

New issues:
- [Low] Casting between `phys_addr_t` and `void *` causes compiler warnings and potential data truncation on 32-bit platforms.

Pre-existing issues:
- [High] The driver dynamically modifies the global `ls2k_bmc_cells` array during probe, violating the `static const` requirement and creating a race condition.
--

--- Patch [7]: [PATCH RFC v5 7/7] mfd: ls2kbmc: Capture the reset event of BMC through GPIO ---
commit 535a8683905c93577231c2208f8f76431cb9049d
Author: Miao Wang <shankerwangmiao@gmail.com>

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

This patch modifies the driver to capture the BMC reset event through the
GPIO framework rather than directly accessing the controller via a fixed
address. It attempts to find the GPIO pin via ACPI _CRS resources, falling
back to a fixed address and pin number if not found. This helps prevent
conflicts when the same GPIO controller is probed by an ACPI GPIO driver.

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index e7bebfe696ac4..97a666a136fe4 100644
> --- a/drivers/mfd/ls2k-bmc-core.c
> +++ b/drivers/mfd/ls2k-bmc-core.c

[ ... ]

> @@ -375,6 +368,91 @@ static void ls2k_bmc_save_pci_data(struct pci_dev *pdev, struct ls2k_bmc_ddata *
>  	pci_read_config_dword(pdev, PCI_INTERRUPT_LINE, &ddata->bmc_pci_data.interrupt_line);
>  }
>  
> +static struct fwnode_handle *gpio_dev_get_fwnode(struct gpio_device *gdev)
> +{
> +	struct device *gdev_dev;
> +
> +	gdev_dev = gpio_device_to_device(gdev);
> +	return dev_fwnode(gdev_dev);
> +}
> +
> +static int ls2k_bmc_gpiochip_find(struct gpio_chip *gc, const void *data)
> +{
> +	struct acpi_device *adev;
> +	struct list_head resource_list;
> +	struct resource_entry *rentry;
> +	struct fwnode_handle *fwnode = gpio_dev_get_fwnode(gc->gpiodev);
> +	phys_addr_t start_addr = (phys_addr_t) data;

[Severity: Low]
Does casting data from a void pointer back to a phys_addr_t cause data
truncation on 32-bit platforms where phys_addr_t is a 64-bit type?

Can this also trigger pointer-to-int cast warnings during compilation?

> +	int ret, found = 0;
> +

[ ... ]

> +static struct gpio_desc *ls2k_bmc_find_gpio(struct ls2k_bmc_ddata *ddata)
> +{
> +	/*
> +	 * In conventional way, the GPIO should be obtained through ACPI or
> +	 * device tree. However, when the information is not available,
> +	 * we should find the GPIO according to the convention of the server
> +	 * boards with LS2K BMC, the gpio signal reflecting the reset event
> +	 * of the BMC should be connected to pin 14 of the GPIO input of
> +	 * the first CPU node. The address of that GPIO controller is fixed.
> +	 */
> +	static const phys_addr_t LOONGSON_GPIO_REG_BASE = 0x1FE00500;
> +	static const unsigned int LS2K_BMC_RESET_GPIO = 14;
> +	int ret;
> +	struct gpio_device *gdev __free(gpio_device_put) = NULL;
> +	struct property_entry ls2k_bmc_swnode_properties[2] = { 0 };
> +
> +	dev_dbg(ddata->dev, "Searching for GPIO chip at address %pa\n", &LOONGSON_GPIO_REG_BASE);
> +
> +	gdev = gpio_device_find((void *)LOONGSON_GPIO_REG_BASE, ls2k_bmc_gpiochip_find);

[Severity: Low]
Can passing the 64-bit LOONGSON_GPIO_REG_BASE by casting it to a void pointer
truncate the upper 32 bits on 32-bit architectures?

Does this also trigger int-to-pointer cast warnings during the build?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does ls2k_bmc_probe()
dynamically modify the global ls2k_bmc_cells array during probe?

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);
    ...
}

The MFD subsystem guide requires mfd_cell arrays to be static const.

Does this assignment create a race condition on concurrent probes (if multiple
PCI devices were present) since instances would overwrite the shared
platform_data pointer?

Additionally, since pd is a stack variable, does this leave a dangling pointer
in the global array after ls2k_bmc_probe() returns?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804-ls2kbmc-mod-v5-0-e6bc5cdd9a93@gmail.com?part=7