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

[PATCH RFC v3 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 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 | 235 ++++++++++++++++++++++++++++++++++----------
 3 files changed, 184 insertions(+), 55 deletions(-)
---
base-commit: 0e35b9b6ec0ffcc5e23cbdec09f5c622ad532b53
change-id: 20260626-ls2kbmc-mod-5209193009b2

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

[PATCH RFC v3 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 | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
index 408056bfb2fe757a5bde43775a483a48352e706d..27f6e096404d67459038a0607378057ec7ef69ab 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;
+	else
+		goto invalid_mode;
 
-	ret = kstrtoint(strsep(&mode, "x"), 10, &pd->width);
+	ret = kstrtouint(strsep(&mode, "x"), 10, &pd->width);
 	if (ret)
 		return ret;
+	if (mode == NULL)
+		goto invalid_mode;
 
-	ret = kstrtoint(strsep(&mode, "-"), 10, &pd->height);
+	ret = kstrtouint(strsep(&mode, "-"), 10, &pd->height);
 	if (ret)
 		return ret;
+	if (mode == NULL)
+		goto invalid_mode;
 
 	ret = kstrtoint(strsep(&mode, "@"), 10, &depth);
 	if (ret)
 		return ret;
+	if (mode == NULL)
+		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 -EINVAL;
 }
 
 static int ls2k_bmc_probe(struct pci_dev *dev, const struct pci_device_id *id)

-- 
2.49.0

[PATCH RFC v3 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 27f6e096404d67459038a0607378057ec7ef69ab..b02e4955e9b04f517892a18b2ef103b5e481a238 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 -EINVAL;
 }
 
+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 v3 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 b02e4955e9b04f517892a18b2ef103b5e481a238..a5465c42a77b8b7b81e3ad787d6036679c6ba6df 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 v3 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 a5465c42a77b8b7b81e3ad787d6036679c6ba6df..1466b314fc4e577fe5e31404444648b5b0447ebb 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 v3 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.

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 v3 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 v3 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 | 158 +++++++++++++++++++++++++++++++-------------
 1 file changed, 111 insertions(+), 47 deletions(-)

diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
index 1466b314fc4e577fe5e31404444648b5b0447ebb..5a2644ff07fee05f4c8cbc54cb62abc2358c1820 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,90 @@ 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_chip_get_fwnode(struct gpio_chip *chip)
+{
+	if (chip->fwnode)
+		return chip->fwnode;
+	else if (chip->parent)
+		return chip->parent->fwnode;
+	else
+		return NULL;
+}
+
+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_chip_get_fwnode(gc);
+	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;
+	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 relelecting 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 gpio_chip *gc;
+	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);
+	}
+
+	gc = gpio_device_get_chip(gdev);
+
+	ls2k_bmc_swnode_properties[0] = PROPERTY_ENTRY_GPIO("gpio",
+		gpio_chip_get_fwnode(gc), 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 +461,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 +478,32 @@ 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 v3 4/7] mfd: ls2kbmc: Cancel the work queue on removal

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The `stop_machine` callback executes operations that sleep (`pci_iomap`) and performs excessive busy-waiting (over 10 seconds), which will trigger kernel panics and hardware watchdogs.
- [High] The driver dynamically modifies the globally shared `ls2k_bmc_cells` array during `probe()` by assigning a stack-allocated variable to its `platform_data` field, violating MFD subsystem guidelines and causing data races.
- [High] The debouncing logic in the interrupt handler uses an uninitialized static variable `last_jiffies`, causing all interrupts to be ignored for the first ~5 minutes of uptime and creating a data race between multiple device instances.
- [High] The driver unconditionally unregisters the ACPI GSI immediately after successfully requesting its Linux IRQ, destroying the mapping while it is actively in use.
- [High] The shared interrupt handler unconditionally schedules work and returns `IRQ_HANDLED` without verifying if its device generated the interrupt, violating `IRQF_SHARED` semantics.
--

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

mfd: ls2kbmc: Cancel the work queue on removal

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.

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index a5465c42a77b8..1466b314fc4e5 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) {

[ ... ]

> static struct mfd_cell ls2k_bmc_cells[] = {
[ ... ]
> ls2k_bmc_cells[LS2K_BMC_DISPLAY].platform_data = &pd;
> ls2k_bmc_cells[LS2K_BMC_DISPLAY].pdata_size = sizeof(pd);

[Severity: High]
This is a pre-existing issue, but could dynamically modifying the global
ls2k_bmc_cells array during probe() with a stack-allocated variable create a
data race?

The MFD subsystem mandates that mfd_cell arrays must be declared as static
const to prevent local amendments. Assigning a local stack variable to the
global array can cause a data race if multiple instances are probed
concurrently, potentially overwriting another device's platform data with a
dangling pointer.

[ ... ]

> static unsigned long last_jiffies;
> ...
> if (time_after(jiffies, last_jiffies + LS2K_BMC_INT_INTERVAL)) {

[Severity: High]
This isn't a bug introduced by this patch, but is it possible that the
uninitialized static last_jiffies variable causes all interrupts to be
ignored for the first 5 minutes of uptime?

Since last_jiffies is implicitly initialized to 0 and jiffies starts at
-300*HZ, time_after() evaluates to false until jiffies wraps past 0.

Furthermore, does using a static variable here introduce a data race between
multiple device instances?

[ ... ]

> stop_machine(ls2k_bmc_recover_pci_data, ddata, NULL);
[ ... ]
> base = pci_iomap(parent, 0, LS7A_PCI_CFG_SIZE);
[ ... ]
> mdelay(LS2K_BMC_RESET_WAIT_TIME);

[Severity: Critical]
This is a pre-existing issue, but would calling stop_machine() here trigger a
regression by halting the system?

The ls2k_bmc_events_fn() invokes stop_machine(), which moves execution to an
atomic context with all CPUs halted and IRQs disabled.

Inside this callback, it calls ls2k_bmc_pcie_is_connected() which can sleep
via pci_iomap(). It then busy-waits for 10 seconds with mdelay().

Will halting the entire system with interrupts disabled for 10 seconds
reliably trigger hardware watchdogs and RCU stall panics?

[ ... ]

> acpi_unregister_gsi(LS2K_BMC_RESET_GPIO_GSI);

[Severity: High]
This isn't a bug introduced by this patch, but does unconditionally
unregistering the ACPI GSI immediately after successfully requesting its
Linux IRQ destroy the mapping while it is actively in use?

Tearing down the hardware-to-Linux IRQ domain mapping and freeing the
underlying IRQ descriptor guarantees the interrupt will not be delivered.

Will this also trigger a warning and a use-after-free regression during
device removal when the active IRQ is later freed?

[ ... ]

> if (time_after(jiffies, last_jiffies + LS2K_BMC_INT_INTERVAL)) {
> 	schedule_work(&ddata->bmc_reset_work);
> 	last_jiffies = jiffies;
> }
>
> return IRQ_HANDLED;

[Severity: High]
This is a pre-existing issue, but will unconditionally returning IRQ_HANDLED
without verifying the hardware source violate IRQF_SHARED semantics?

The handler is registered with IRQF_SHARED, but it unconditionally returns
IRQ_HANDLED and schedules bmc_reset_work without checking any hardware
status.

If another device on the shared line fires an interrupt, will the BMC driver
intercept it, falsely claim it was handled, and trigger the 10-second system
lockup via its workqueue?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-ls2kbmc-mod-v3-0-ef718636e78e@gmail.com?part=4

Re: [PATCH RFC v3 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] An explicit 10-second delay is executed inside a stop_machine() context, halting the system and triggering the hard lockup detector.
- [High] Dynamic amendment of a global static, non-const mfd_cell array with a stack-allocated variable leaves a dangling pointer.
- [High] Premature disposal of the GPIO IRQ mapping by calling acpi_unregister_gsi() immediately after requesting it.
--

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

mfd: ls2kbmc: Able to be compiled as a module

This commit changes the MFD_LS2K_BMC_CORE Kconfig option from a boolean
to a tristate. This allows the Loongson-2K Board Management Controller
driver to be built and loaded dynamically as a kernel module.

> 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"
>  	depends on PCI && ACPI_GENERIC_GSI
>  	select MFD_CORE
>  	help

Since this patch enables module unloading for the ls2k-bmc driver, I reviewed
the existing driver code and found a few pre-existing regressions that are
impacted by or related to this change.

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

[Severity: High]
This isn't a bug introduced by this patch, but does this dynamically amend a
global non-const mfd_cell array with a stack-allocated variable?

Because ls2k_bmc_cells is a shared global static array, if two BMC devices
are probed concurrently, thread A and thread B could race to write their local
stack pointers into this global array. Thread A might pass thread B's stack
pointer to devm_mfd_add_devices(). 

If thread B has already returned, could thread A read from a popped stack
frame and yield garbage resolution data?

Does this also violate the MFD subsystem rule that mfd_cell arrays must be
static const?

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

[Severity: High]
This is a pre-existing issue, but does this prematurely dispose of the GPIO
IRQ mapping?

In ls2k_bmc_init(), acpi_register_gsi() maps an IRQ and devm_request_irq()
requests it. Immediately after, acpi_unregister_gsi() is called on the exact
same IRQ number. This invokes irq_dispose_mapping(), which tears down the
IRQ descriptor while the action is still linked.

Since this patch turns the driver into a module, module unloading is now
possible. When the module is unloaded, devm_irq_release() will fire on the
disposed IRQ. 

Could this lead to a use-after-free or inadvertently free an unrelated IRQ
that reused the same descriptor?

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

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

[Severity: Critical]
This is a pre-existing issue, but does this execute a 10-second delay inside
a stop_machine() context?

The stop_machine() function halts all CPUs in the system with local interrupts
disabled to serialize execution. Inside ls2k_bmc_recover_pci_data(), there is
an unconditional mdelay() for 10 seconds.

Could spinning a CPU with interrupts disabled for over 10 seconds trigger a
hard lockup panic and disrupt system timekeeping and networking?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-ls2kbmc-mod-v3-0-ef718636e78e@gmail.com?part=6

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

[sashiko-bot]

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

New issues:
- [High] The newly added `mode == NULL` check for the depth token strictly rejects valid mode strings that do not contain an `@` character (refresh rate).

Pre-existing issues:
- [High] The static global `mfd_cell` array is dynamically mutated during probe using a local stack pointer, causing a data race and violating MFD guidelines.
- [High] Missing NULL check for the parent PCI device leads to a guaranteed kernel crash on VM passthrough or root bus topologies.
- [High] Integer overflow during framebuffer stride calculation allows a maliciously crafted stride of zero.
- [High] The shared interrupt handler unconditionally returns `IRQ_HANDLED` without checking if the interrupt actually originated from its device.
- [High] The driver executes extremely long busy-waits (e.g., `mdelay(10000)`) inside a `stop_machine()` callback, which runs in an atomic context with interrupts disabled on all CPUs.
--

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

mfd: ls2kbmc: Make a copy when parsing mode string

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>

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index 408056bfb2fe7..27f6e096404d6 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, "@"), 10, &depth);
>  	if (ret)
>  		return ret;
> +	if (mode == NULL)
> +		goto invalid_mode;

[Severity: High]
Does this new check break backward compatibility?

If the resolution string doesn't contain a refresh rate (e.g.
"video=1920x1080-32"), strsep() will return the remaining string for depth,
and set mode to NULL.

The kstrtoint() succeeds, but then the new "if (mode == NULL)" check will
trigger, unconditionally rejecting valid mode strings without an explicit
refresh rate.

>  
>  	pd->stride = pd->width * depth / 8;

[Severity: High]
This is a pre-existing issue, but does this calculation check for integer
overflow?

If a compromised BMC hardware endpoint supplies maliciously large values
(e.g., width = 0x40000000 and depth = 8), this calculation could wrap
around to 0, resulting in a zero-size framebuffer mapping.


I also noticed some pre-existing issues in other parts of the driver:

[Severity: High]
This is a pre-existing issue, but does ls2k_bmc_probe() dynamically mutate a
global static 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);
    ...
}

Assigning a local stack pointer to a mutable global static MFD cell array
creates a data race and a use-after-free hazard if multiple instances of
this driver probe concurrently.


[Severity: High]
This is a pre-existing issue, but is it possible for the parent pointer to
be NULL in ls2k_bmc_save_pci_data()?

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_save_pci_data() {
    struct pci_dev *parent = pdev->bus->self;
    ...
    pci_read_config_dword(parent, PCI_COMMAND, &ddata->bridge_pci_data.pci_command);
    ...
}

If the device is connected directly to a root bus (e.g., passed through via
VFIO to a VM), pdev->bus->self will be NULL, which would cause a kernel
panic due to a NULL pointer dereference.


[Severity: High]
This is a pre-existing issue, but does the shared interrupt handler check
the hardware to confirm the interrupt's provenance?

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

Because it is registered with IRQF_SHARED, if another device on the same
PCI IRQ line fires an interrupt, the kernel assumes this driver handled it,
potentially starving the other device and spuriously triggering a BMC reset.


[Severity: High]
This is a pre-existing issue, but does this code execute an extremely long
busy-wait in an atomic context?

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_recover_pci_data() {
    ...
    /* Waiting for U-Boot and DDR ready */
    mdelay(LS2K_BMC_RESET_WAIT_TIME);
    ...
}

The BMC reset workqueue calls stop_machine(ls2k_bmc_recover_pci_data, ...).
Inside this callback, mdelay() busy-waits for 10 seconds. Since
stop_machine() runs in an atomic context with interrupts disabled on all
CPUs, this can cause a system freeze and a hard lockup/NMI watchdog panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-ls2kbmc-mod-v3-0-ef718636e78e@gmail.com?part=1

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

[sashiko-bot]

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

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

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

mfd: ls2kbmc: Sanity check for the connected pci port

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

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

[ ... ]

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

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

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

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

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

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

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

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

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

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

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

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

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

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-ls2kbmc-mod-v3-0-ef718636e78e@gmail.com?part=2

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

[sashiko-bot]

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

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

Pre-existing issues:
- [High] ipmi_si_ls2k_init() ignores the return value of platform_driver_register() and unconditionally sets ls2k_registered = true.
- [High] The MFD driver dynamically mutates a shared static global `mfd_cell` array using a stack pointer, violating MFD subsystem rules and causing a data race.
- [High] The driver executes a massive busy-wait of up to 13 seconds inside `stop_machine()`, triggering hard lockups.
--

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

ipmi: ls2k: Relax the dependency to its mfd driver

This commit changes the dependency of the IPMI driver on its MFD core
driver from select to imply. This is intended to allow the MFD driver to
be built as a module while the IPMI driver remains built-in or part of
ipmi_si.

> There is no strong dependency between the IPMI driver and its mfd
> driver. Although the IPMI driver will not work without the mfd driver,
> it is not a hard dependency. The IPMI driver can actually be compiled
> without the mfd driver, and it will just fail to probe. When the mfd
> driver is loaded, the IPMI driver will probe successfully. Therefore,
> the dependency of the IPMI driver on its mfd driver should be relaxed
> to "imply" from "select". This will allow the mfd driver to be compiled
> as a module and the IPMI driver to be compiled as a part of the ipmi_si
> module.

[Severity: Medium]
Is this description accurate regarding compiling the MFD driver as a module?
Looking at drivers/mfd/Kconfig, MFD_LS2K_BMC_CORE is defined as a bool,
meaning it can only be built-in (=y) or disabled (=n). Since a bool
cannot be compiled as a module, will this change actually allow modular
builds 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
>  	help
>  	  Provides a driver for Loongson-2K IPMI interfaces.

[Severity: High]
This is a pre-existing issue, but does ipmi_si_ls2k_init() properly
handle registration failures?

drivers/char/ipmi/ipmi_si_ls2k.c:ipmi_si_ls2k_init() {
    platform_driver_register(&ipmi_ls2k_platform_driver);
    ls2k_registered = true;
}

If resource constraints like OOM cause platform_driver_register() to fail,
ls2k_registered is still set to true. During module unload or driver
shutdown, ipmi_si_ls2k_shutdown() checks this flag and calls
platform_driver_unregister() on an unregistered driver, which can trigger
a driver core WARNING.

[Severity: High]
This is a pre-existing issue, but does the MFD driver dynamically mutate a
shared global cell array using 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);
    ...
}

MFD subsystem rules mandate that cell arrays must be static const. Because
this array is not const and is mutated at runtime with a local stack
variable pointer (pd), concurrent PCI probes of the BMC device could
overwrite each other's pointers in the global array. This could cause
mfd_add_devices() to clone garbage or freed stack memory from another
thread.

[Severity: High]
This is a pre-existing issue, but does the driver execute a massive
busy-wait inside stop_machine()?

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

Inside the callback:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_recover_pci_data() {
    for (i = LS7A_BAR0_CHECK_MAX_TIMES; i > 0 ; i--) {
        if (!ls2k_bmc_bar0_addr_is_set(parent))
            break;
        mdelay(1);
    }
    ...
    /* Waiting for U-Boot and DDR ready */
    mdelay(LS2K_BMC_RESET_WAIT_TIME);
}

stop_machine() disables interrupts on all CPUs. When an interrupt schedules
the BMC reset work, ls2k_bmc_recover_pci_data() is called in atomic context
with IRQs disabled system-wide. The callback executes a 2-second polling
loop and then an unconditional 10-second wait. Halting all CPUs with
interrupts disabled for up to 13 seconds can trigger NMI hard lockup panics
or RCU stalls.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-ls2kbmc-mod-v3-0-ef718636e78e@gmail.com?part=5

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

[sashiko-bot]

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

New issues:
- [Medium] Replacing the VT switch with update_screen() breaks display recovery under X/Wayland graphics modes.

Pre-existing issues:
- [Critical] Execution of 10+ second delays and sleeping functions inside stop_machine() guarantees an NMI watchdog panic.
- [High] Dynamic modification of a global static mfd_cell array during probe creates a data race and dangling pointer.
- [High] Missing cleanup for bmc_reset_work leads to a use-after-free on device removal.
- [High] Unconditional IRQ_HANDLED return breaks spurious IRQ detection and maliciously reacts to shared interrupts.
- [High] The GSI mapping is unconditionally destroyed on the success path via acpi_unregister_gsi().
--

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

mfd: ls2kbmc: Redraw using exported functions

This commit modifies the VT redraw mechanism in the ls2k-bmc-core driver to
use update_screen() within a console_lock() instead of switching virtual
terminals. It aims to trigger the redraw of the current vt using exported
functions.

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index b02e4955e9b04..a5465c42a77b8 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);

[Severity: Medium]
Will this regression break display recovery under X/Wayland graphics modes?

In text mode, this repaints the framebuffer memory. However, in graphics mode
(KD_GRAPHICS), update_screen() explicitly skips do_update_region() and
notify_update(vc).

As a result, user-space display servers like Xorg or Wayland are never
notified of the hardware reset, meaning they will not be prompted to
re-acquire DRM master and modeset the restored display controller.

> +		console_unlock();
>  	}
>  }

[Severity: Critical]
This isn't a bug introduced by this patch, but does the recovery process
trigger an NMI watchdog panic by sleeping inside stop_machine()?

The work item calls stop_machine(), which disables interrupts on all CPUs:

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

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

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_recover_pci_data() {
    ...
    /* Waiting for U-Boot and DDR ready */
    mdelay(LS2K_BMC_RESET_WAIT_TIME);
    ...
}

Halting all CPUs with interrupts disabled for over 10 seconds will
unconditionally trigger the kernel's hard lockup detector. Additionally, is it
safe to sleep by calling pci_iomap() while in this atomic context?

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing cleanup for
bmc_reset_work that leads to a use-after-free on device removal?

The driver initializes and schedules the work, but structurally lacks a
.remove callback to invoke cancel_work_sync():

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

Since the PCI driver relies purely on devres APIs for cleanup and does not
provide a .remove callback or a devm_add_action handler, the work item will
remain in the workqueue after the ddata structure is freed during module
unload.

[Severity: High]
This isn't a bug introduced by this patch, but how does the driver handle the
data race and dangling pointer created by modifying the global static mfd_cell
array during probe?

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

Modifying a global struct in a probe function introduces a data race if
multiple device instances probe concurrently. The pointer saved into the
global struct (&pd) points to a local stack variable, leaving a dangling
pointer in global state after the probe function returns. This also violates
the MFD subsystem guidelines which require mfd_cell arrays to be static const.

[Severity: High]
This isn't a bug introduced by this patch, but wouldn't the unconditional
IRQ_HANDLED return break spurious IRQ detection?

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_interrupt() {
    ...
    return IRQ_HANDLED;
}

The handler is registered as a shared interrupt:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_init() {
    ...
    devm_request_irq(dev, ddata->bmc_pci_data.irq, ls2k_bmc_interrupt,
                   IRQF_SHARED | IRQF_TRIGGER_FALLING, "ls2kbmc gpio", ddata);
    ...
}

Because it does not check any device-specific registers to verify the source,
every interrupt from another device sharing the line will cause this driver
to falsely schedule a BMC reset recovery and freeze the system via
stop_machine().

[Severity: High]
This isn't a bug introduced by this patch, but is it intended to destroy the
GSI mapping unconditionally on the success path?

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

The hardware interrupt becomes unreachable because its Linux IRQ mapping is
destroyed via acpi_unregister_gsi() immediately after being requested
successfully. This severs the interrupt mapping and prevents the GPIO
interrupt from ever firing.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-ls2kbmc-mod-v3-0-ef718636e78e@gmail.com?part=3

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

[sashiko-bot]

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

New issues:
- [High] Unprotected access to `gpio_chip` without a NULL check leads to a potential NULL pointer dereference or use-after-free.
- [Medium] The ACPI memory resource search for the GPIO controller only checks the first memory descriptor, ignoring any subsequent ones.

Pre-existing issues:
- [High] The shared interrupt handler unconditionally returns IRQ_HANDLED without checking the hardware status, violating shared IRQ semantics.
--

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

mfd: ls2kbmc: Capture the reset event of BMC through GPIO
    
This patch modifies the driver to find its reset GPIO via ACPI or a fallback 
software node, rather than directly accessing the GPIO registers.

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

[ ... ]

> +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_chip_get_fwnode(gc);
> +	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;
> +	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;

[Severity: Medium]
Will this correctly identify the GPIO controller if the register base is
defined as the second or later memory resource in the ACPI DSDT?

The list of memory resources is queried, but only the first element is
evaluated with list_first_entry_or_null() instead of iterating through the
entire list with list_for_each_entry().

> +
> +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)
> +{

[ ... ]

> +	gc = gpio_device_get_chip(gdev);
> +
> +	ls2k_bmc_swnode_properties[0] = PROPERTY_ENTRY_GPIO("gpio",
> +		gpio_chip_get_fwnode(gc), LS2K_BMC_RESET_GPIO, GPIO_ACTIVE_HIGH);

[Severity: High]
Does this code risk a NULL pointer dereference?

gpio_device_get_chip() can return NULL if the underlying GPIO controller is
concurrently unregistered.

Passing this potentially NULL gc pointer to gpio_chip_get_fwnode() results in
an immediate unconditional dereference of chip->fwnode inside that function.

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

[ ... ]

> @@ -402,44 +478,32 @@ static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)

[ ... ]

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

[Severity: High]
This is a pre-existing issue, but does ls2k_bmc_interrupt() properly handle
being shared?

The interrupt is requested with IRQF_SHARED, but ls2k_bmc_interrupt()
unconditionally returns IRQ_HANDLED and schedules its work without reading
a hardware register to verify it was the actual source of the interrupt.

When another device sharing this IRQ line fires an interrupt, wouldn't this
erroneously reset the BMC and suppress spurious interrupt detection for the
other devices?

> +
> +	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;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-ls2kbmc-mod-v3-0-ef718636e78e@gmail.com?part=7

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

[Lee Jones]

On Thu, 09 Jul 2026, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [High] Unprotected access to `gpio_chip` without a NULL check leads to a potential NULL pointer dereference or use-after-free.
> - [Medium] The ACPI memory resource search for the GPIO controller only checks the first memory descriptor, ignoring any subsequent ones.

Looks like there are a few of these to consider.

I'm unmarking this for review for now.

> Pre-existing issues:
> - [High] The shared interrupt handler unconditionally returns IRQ_HANDLED without checking the hardware status, violating shared IRQ semantics.
> --
> 
> --- Patch [7]: [PATCH RFC v3 7/7] mfd: ls2kbmc: Capture the reset event of BMC through GPIO ---
> commit a2c13d408a04d53aa90ec63a2b31ea6450fb493e
> Author: Miao Wang <shankerwangmiao@gmail.com>
> 
> mfd: ls2kbmc: Capture the reset event of BMC through GPIO
>     
> This patch modifies the driver to find its reset GPIO via ACPI or a fallback 
> software node, rather than directly accessing the GPIO registers.
> 
> > diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> > index 1466b314fc4e5..5a2644ff07fee 100644
> > --- a/drivers/mfd/ls2k-bmc-core.c
> > +++ b/drivers/mfd/ls2k-bmc-core.c
> 
> [ ... ]
> 
> > +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_chip_get_fwnode(gc);
> > +	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;
> > +	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;
> 
> [Severity: Medium]
> Will this correctly identify the GPIO controller if the register base is
> defined as the second or later memory resource in the ACPI DSDT?
> 
> The list of memory resources is queried, but only the first element is
> evaluated with list_first_entry_or_null() instead of iterating through the
> entire list with list_for_each_entry().
> 
> > +
> > +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)
> > +{
> 
> [ ... ]
> 
> > +	gc = gpio_device_get_chip(gdev);
> > +
> > +	ls2k_bmc_swnode_properties[0] = PROPERTY_ENTRY_GPIO("gpio",
> > +		gpio_chip_get_fwnode(gc), LS2K_BMC_RESET_GPIO, GPIO_ACTIVE_HIGH);
> 
> [Severity: High]
> Does this code risk a NULL pointer dereference?
> 
> gpio_device_get_chip() can return NULL if the underlying GPIO controller is
> concurrently unregistered.
> 
> Passing this potentially NULL gc pointer to gpio_chip_get_fwnode() results in
> an immediate unconditional dereference of chip->fwnode inside that function.
> 
> > +
> > +	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);
> > +}
> 
> [ ... ]
> 
> > @@ -402,44 +478,32 @@ static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
> 
> [ ... ]
> 
> > -	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);
> 
> [Severity: High]
> This is a pre-existing issue, but does ls2k_bmc_interrupt() properly handle
> being shared?
> 
> The interrupt is requested with IRQF_SHARED, but ls2k_bmc_interrupt()
> unconditionally returns IRQ_HANDLED and schedules its work without reading
> a hardware register to verify it was the actual source of the interrupt.
> 
> When another device sharing this IRQ line fires an interrupt, wouldn't this
> erroneously reset the BMC and suppress spurious interrupt detection for the
> other devices?
> 
> > +
> > +	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;
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260710-ls2kbmc-mod-v3-0-ef718636e78e@gmail.com?part=7

-- 
Lee Jones

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

[Lee Jones]

Why is this v3 set still RFC?  What comment are you requesting?

On Fri, 10 Jul 2026, Miao Wang via B4 Relay wrote:

> 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 | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index 408056bfb2fe757a5bde43775a483a48352e706d..27f6e096404d67459038a0607378057ec7ef69ab 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;
> +	else
> +		goto invalid_mode;
>  
> -	ret = kstrtoint(strsep(&mode, "x"), 10, &pd->width);
> +	ret = kstrtouint(strsep(&mode, "x"), 10, &pd->width);
>  	if (ret)
>  		return ret;
> +	if (mode == NULL)
> +		goto invalid_mode;
>  
> -	ret = kstrtoint(strsep(&mode, "-"), 10, &pd->height);
> +	ret = kstrtouint(strsep(&mode, "-"), 10, &pd->height);
>  	if (ret)
>  		return ret;
> +	if (mode == NULL)
> +		goto invalid_mode;
>  
>  	ret = kstrtoint(strsep(&mode, "@"), 10, &depth);
>  	if (ret)
>  		return ret;
> +	if (mode == NULL)
> +		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 -EINVAL;
>  }
>  
>  static int ls2k_bmc_probe(struct pci_dev *dev, const struct pci_device_id *id)
> 
> -- 
> 2.49.0
> 
> 

-- 
Lee Jones

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

[Miao Wang]

Hi,

> 2026年7月23日 19:29，Lee Jones <lee@kernel.org> 写道：
> 
> On Thu, 09 Jul 2026, sashiko-bot@kernel.org wrote:
> 
>> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>> 
>> New issues:
>> - [High] Unprotected access to `gpio_chip` without a NULL check leads to a potential NULL pointer dereference or use-after-free.
>> - [Medium] The ACPI memory resource search for the GPIO controller only checks the first memory descriptor, ignoring any subsequent ones.
> 
> Looks like there are a few of these to consider.

Many thanks for your response. I was preparing a v4 to fix the first issue
found by the bot. However, I believe that the second issue is false positive,
since the memory descriptors (if more than one) in _CRS are ordered and the
position is meaningful for the driver. As a result, only checking the first
descriptor is acceptable.

> 
> I'm unmarking this for review for now.

I still want review for other improvement suggestions. Thanks in advance.

Cheers,

Miao Wang

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

[Miao Wang]

Hi,

> 2026年7月23日 19:29，Lee Jones <lee@kernel.org> 写道：
> 
> Why is this v3 set still RFC?  What comment are you requesting?

The reason why this is still in RFC is that I am still waiting
for response and discussion from Loongson personals on the
proposal that adding a DSDT entry for the ls2kbmc device that
describes the GPIO pin associated with the device to signal
the reset event of that BMC device. As pointed by Ruoyao,
such a proposal will need to be discussed with Loongson. I have
CC-ed the patch to the relevant maintainers from Loongson but
failed to receive any response.

Moreover, I am also in need of comments on the backup method
of searching the GPIO device and pin when such a DSDT entry
is not available.

Cheers,

Miao Wang

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

[Miao Wang]

> 2026年7月23日 20:02，Miao Wang <shankerwangmiao@gmail.com> 写道：
> 
> Hi,
> 
>> 2026年7月23日 19:29，Lee Jones <lee@kernel.org> 写道：
>> 
>> Why is this v3 set still RFC?  What comment are you requesting?
> 
> The reason why this is still in RFC is that I am still waiting
> for response and discussion from Loongson personals on the
> proposal that adding a DSDT entry for the ls2kbmc device that
> describes the GPIO pin associated with the device to signal
> the reset event of that BMC device. As pointed by Ruoyao,
> such a proposal will need to be discussed with Loongson. I have
> CC-ed the patch to the relevant maintainers from Loongson but
> failed to receive any response.
> 
> Moreover, I am also in need of comments on the backup method
> of searching the GPIO device and pin when such a DSDT entry
> is not available.
> 

To complement, the part that I'm in need of comments is the
7-th patch of this series. The first 6 patches are focusing on
fixing the existing issues in this driver. I thought that they
could be managed to be applied altogether so that the driver
would become a good, ready-to-use state. If the first 6 patches
are good to be applied and we still receive no response from
Loongson personnels, can I split the series into 2 parts?

Cheers,

Miao Wang

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

[Lee Jones]

On Thu, 23 Jul 2026, Miao Wang wrote:

> 
> 
> > 2026年7月23日 20:02，Miao Wang <shankerwangmiao@gmail.com> 写道：
> > 
> > Hi,
> > 
> >> 2026年7月23日 19:29，Lee Jones <lee@kernel.org> 写道：
> >> 
> >> Why is this v3 set still RFC?  What comment are you requesting?
> > 
> > The reason why this is still in RFC is that I am still waiting
> > for response and discussion from Loongson personals on the
> > proposal that adding a DSDT entry for the ls2kbmc device that
> > describes the GPIO pin associated with the device to signal
> > the reset event of that BMC device. As pointed by Ruoyao,
> > such a proposal will need to be discussed with Loongson. I have
> > CC-ed the patch to the relevant maintainers from Loongson but
> > failed to receive any response.
> > 
> > Moreover, I am also in need of comments on the backup method
> > of searching the GPIO device and pin when such a DSDT entry
> > is not available.
> > 
> 
> To complement, the part that I'm in need of comments is the
> 7-th patch of this series. The first 6 patches are focusing on
> fixing the existing issues in this driver. I thought that they
> could be managed to be applied altogether so that the driver
> would become a good, ready-to-use state. If the first 6 patches
> are good to be applied and we still receive no response from
> Loongson personnels, can I split the series into 2 parts?

I don't see any reason why not.

-- 
Lee Jones

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

[Lee Jones]

On Thu, 23 Jul 2026, Miao Wang wrote:

> Hi,
> 
> > 2026年7月23日 19:29，Lee Jones <lee@kernel.org> 写道：
> > 
> > On Thu, 09 Jul 2026, sashiko-bot@kernel.org wrote:
> > 
> >> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> >> 
> >> New issues:
> >> - [High] Unprotected access to `gpio_chip` without a NULL check leads to a potential NULL pointer dereference or use-after-free.
> >> - [Medium] The ACPI memory resource search for the GPIO controller only checks the first memory descriptor, ignoring any subsequent ones.
> > 
> > Looks like there are a few of these to consider.
> 
> Many thanks for your response. I was preparing a v4 to fix the first issue
> found by the bot. However, I believe that the second issue is false positive,
> since the memory descriptors (if more than one) in _CRS are ordered and the
> position is meaningful for the driver. As a result, only checking the first
> descriptor is acceptable.
> 
> > 
> > I'm unmarking this for review for now.
> 
> I still want review for other improvement suggestions. Thanks in advance.

Due to extreme workload, I need a clean or justified Sashiko pass first.

-- 
Lee Jones

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

[Miao Wang]

Hi,

> 2026年7月23日 21:11，Lee Jones <lee@kernel.org> 写道：
> 
> On Thu, 23 Jul 2026, Miao Wang wrote:
> 
>> Hi,
>> 
>>> 2026年7月23日 19:29，Lee Jones <lee@kernel.org> 写道：
>>> 
>>> On Thu, 09 Jul 2026, sashiko-bot@kernel.org wrote:
>>> 
>>>> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>>>> 
>>>> New issues:
>>>> - [High] Unprotected access to `gpio_chip` without a NULL check leads to a potential NULL pointer dereference or use-after-free.
>>>> - [Medium] The ACPI memory resource search for the GPIO controller only checks the first memory descriptor, ignoring any subsequent ones.
>>> 
>>> Looks like there are a few of these to consider.
>> 
>> Many thanks for your response. I was preparing a v4 to fix the first issue
>> found by the bot. However, I believe that the second issue is false positive,
>> since the memory descriptors (if more than one) in _CRS are ordered and the
>> position is meaningful for the driver. As a result, only checking the first
>> descriptor is acceptable.
>> 
>>> 
>>> I'm unmarking this for review for now.
>> 
>> I still want review for other improvement suggestions. Thanks in advance.
> 
> Due to extreme workload, I need a clean or justified Sashiko pass first.
> 
So is there any guides that I can refer to to mark for false positives?

Cheers,

Miao Wang

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

[Huacai Chen]

Hi, Miao,

On Fri, Jul 10, 2026 at 1:24 AM Miao Wang via B4 Relay
<devnull+shankerwangmiao.gmail.com@kernel.org> wrote:
>
> 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.
You can replace the redraw function, but I don't think it is necessary
to make the bmc driver modular.

BMC core, IPMI and simpledrm display are usually (if not always)
supposed to work as early as possible.


Huacai

>
> 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 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 | 235 ++++++++++++++++++++++++++++++++++----------
>  3 files changed, 184 insertions(+), 55 deletions(-)
> ---
> base-commit: 0e35b9b6ec0ffcc5e23cbdec09f5c622ad532b53
> change-id: 20260626-ls2kbmc-mod-5209193009b2
>
> Best regards,
> --
> Miao Wang <shankerwangmiao@gmail.com>
>
>

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

[Miao Wang]

Hi,

> 2026年7月24日 16:55，Huacai Chen <chenhuacai@kernel.org> 写道：
> 
> Hi, Miao,
> 
> On Fri, Jul 10, 2026 at 1:24 AM Miao Wang via B4 Relay
> <devnull+shankerwangmiao.gmail.com@kernel.org> wrote:
>> 
>> 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.
> You can replace the redraw function, but I don't think it is necessary
> to make the bmc driver modular.
> 
> BMC core, IPMI and simpledrm display are usually (if not always)
> supposed to work as early as possible.

I believe that it should be the user's decision to choose whether to
compile a module into the kernel or as a module and it would be better
if we can provide the possibilities for the user to choose from.
Additionally, I don't think these modules are supposed to work that
early. The mfd module provide two functions, the display and the ipmi
device. In the aspect of graphical display, without this module, the
user can still see the output during booting on their monitors, since
efifb is working, providing a basic display function. In the aspect of
the ipmi device, I don't think the lack of such device will influence
the boot of the system, since it is a common practice to compile ipmi
device drivers as modules on other architectures. As a result, neither
of the two functions are required to be loaded that early and it is
reasonable to at lease leave the choice to compile it as a module
to the user.

Cheers,

Miao Wang

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

[Lee Jones]

On Thu, 23 Jul 2026, Miao Wang wrote:

> Hi,
> 
> > 2026年7月23日 21:11，Lee Jones <lee@kernel.org> 写道：
> > 
> > On Thu, 23 Jul 2026, Miao Wang wrote:
> > 
> >> Hi,
> >> 
> >>> 2026年7月23日 19:29，Lee Jones <lee@kernel.org> 写道：
> >>> 
> >>> On Thu, 09 Jul 2026, sashiko-bot@kernel.org wrote:
> >>> 
> >>>> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> >>>> 
> >>>> New issues:
> >>>> - [High] Unprotected access to `gpio_chip` without a NULL check leads to a potential NULL pointer dereference or use-after-free.
> >>>> - [Medium] The ACPI memory resource search for the GPIO controller only checks the first memory descriptor, ignoring any subsequent ones.
> >>> 
> >>> Looks like there are a few of these to consider.
> >> 
> >> Many thanks for your response. I was preparing a v4 to fix the first issue
> >> found by the bot. However, I believe that the second issue is false positive,
> >> since the memory descriptors (if more than one) in _CRS are ordered and the
> >> position is meaningful for the driver. As a result, only checking the first
> >> descriptor is acceptable.
> >> 
> >>> 
> >>> I'm unmarking this for review for now.
> >> 
> >> I still want review for other improvement suggestions. Thanks in advance.
> > 
> > Due to extreme workload, I need a clean or justified Sashiko pass first.
> > 
> So is there any guides that I can refer to to mark for false positives?

Most people reply to the feedback and state the fact, with a little
background or additional information of course.

-- 
Lee Jones

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

[Huacai Chen]

On Fri, Jul 24, 2026 at 5:28 PM Miao Wang <shankerwangmiao@gmail.com> wrote:
>
> Hi,
>
> > 2026年7月24日 16:55，Huacai Chen <chenhuacai@kernel.org> 写道：
> >
> > Hi, Miao,
> >
> > On Fri, Jul 10, 2026 at 1:24 AM Miao Wang via B4 Relay
> > <devnull+shankerwangmiao.gmail.com@kernel.org> wrote:
> >>
> >> 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.
> > You can replace the redraw function, but I don't think it is necessary
> > to make the bmc driver modular.
> >
> > BMC core, IPMI and simpledrm display are usually (if not always)
> > supposed to work as early as possible.
>
> I believe that it should be the user's decision to choose whether to
> compile a module into the kernel or as a module and it would be better
> if we can provide the possibilities for the user to choose from.
> Additionally, I don't think these modules are supposed to work that
> early. The mfd module provide two functions, the display and the ipmi
> device. In the aspect of graphical display, without this module, the
> user can still see the output during booting on their monitors, since
> efifb is working, providing a basic display function. In the aspect of
> the ipmi device, I don't think the lack of such device will influence
> the boot of the system, since it is a common practice to compile ipmi
> device drivers as modules on other architectures. As a result, neither
> of the two functions are required to be loaded that early and it is
> reasonable to at lease leave the choice to compile it as a module
> to the user.
Flexibility is not always useful, if a config doesn't provide good
effect, then it just increases complexity and makes maintenance more
difficult.

Huacai

>
> Cheers,
>
> Miao Wang
>
>

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

[Miao Wang]

Hi,

> 2026年7月31日 16:07，Huacai Chen <chenhuacai@kernel.org> 写道：
> 
> On Fri, Jul 24, 2026 at 5:28 PM Miao Wang <shankerwangmiao@gmail.com> wrote:
>> 
>> Hi,
>> 
>>> 2026年7月24日 16:55，Huacai Chen <chenhuacai@kernel.org> 写道：
>>> 
>>> Hi, Miao,
>>> 
>>> On Fri, Jul 10, 2026 at 1:24 AM Miao Wang via B4 Relay
>>> <devnull+shankerwangmiao.gmail.com@kernel.org> wrote:
>>>> 
>>>> 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.
>>> You can replace the redraw function, but I don't think it is necessary
>>> to make the bmc driver modular.
>>> 
>>> BMC core, IPMI and simpledrm display are usually (if not always)
>>> supposed to work as early as possible.
>> 
>> I believe that it should be the user's decision to choose whether to
>> compile a module into the kernel or as a module and it would be better
>> if we can provide the possibilities for the user to choose from.
>> Additionally, I don't think these modules are supposed to work that
>> early. The mfd module provide two functions, the display and the ipmi
>> device. In the aspect of graphical display, without this module, the
>> user can still see the output during booting on their monitors, since
>> efifb is working, providing a basic display function. In the aspect of
>> the ipmi device, I don't think the lack of such device will influence
>> the boot of the system, since it is a common practice to compile ipmi
>> device drivers as modules on other architectures. As a result, neither
>> of the two functions are required to be loaded that early and it is
>> reasonable to at lease leave the choice to compile it as a module
>> to the user.
> Flexibility is not always useful, if a config doesn't provide good
> effect, then it just increases complexity and makes maintenance more
> difficult.

I should emphasize that to allow this driver to be a module, there is
no such increase on maintenance. Moreover, not all loongarch machines
are requiring this driver. Especially only a part of the server models
are quipped with this BMC. Comparing with other architectures, the
driver for BMC are normally compiled as a module, such as mgag200 for
iLO from HPE and iDRAC from DELL, hibmc_drm for Kunpeng server from
Huawei. None of these BMC drivers requiring to be compiled into the
kernel. I cannot see there is any reason keeping the driver from being
allowed to be compiled as a module. I also do not think it will bring
any significant maintenance burden. Implementing correct cleanup code
should be necessary instead of burden.

Cheers,

Miao Wang

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

[Huacai Chen]

On Fri, Jul 31, 2026 at 4:24 PM Miao Wang <shankerwangmiao@gmail.com> wrote:
>
> Hi,
>
> > 2026年7月31日 16:07，Huacai Chen <chenhuacai@kernel.org> 写道：
> >
> > On Fri, Jul 24, 2026 at 5:28 PM Miao Wang <shankerwangmiao@gmail.com> wrote:
> >>
> >> Hi,
> >>
> >>> 2026年7月24日 16:55，Huacai Chen <chenhuacai@kernel.org> 写道：
> >>>
> >>> Hi, Miao,
> >>>
> >>> On Fri, Jul 10, 2026 at 1:24 AM Miao Wang via B4 Relay
> >>> <devnull+shankerwangmiao.gmail.com@kernel.org> wrote:
> >>>>
> >>>> 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.
> >>> You can replace the redraw function, but I don't think it is necessary
> >>> to make the bmc driver modular.
> >>>
> >>> BMC core, IPMI and simpledrm display are usually (if not always)
> >>> supposed to work as early as possible.
> >>
> >> I believe that it should be the user's decision to choose whether to
> >> compile a module into the kernel or as a module and it would be better
> >> if we can provide the possibilities for the user to choose from.
> >> Additionally, I don't think these modules are supposed to work that
> >> early. The mfd module provide two functions, the display and the ipmi
> >> device. In the aspect of graphical display, without this module, the
> >> user can still see the output during booting on their monitors, since
> >> efifb is working, providing a basic display function. In the aspect of
> >> the ipmi device, I don't think the lack of such device will influence
> >> the boot of the system, since it is a common practice to compile ipmi
> >> device drivers as modules on other architectures. As a result, neither
> >> of the two functions are required to be loaded that early and it is
> >> reasonable to at lease leave the choice to compile it as a module
> >> to the user.
> > Flexibility is not always useful, if a config doesn't provide good
> > effect, then it just increases complexity and makes maintenance more
> > difficult.
>
> I should emphasize that to allow this driver to be a module, there is
> no such increase on maintenance. Moreover, not all loongarch machines
> are requiring this driver. Especially only a part of the server models
> are quipped with this BMC. Comparing with other architectures, the
> driver for BMC are normally compiled as a module, such as mgag200 for
> iLO from HPE and iDRAC from DELL, hibmc_drm for Kunpeng server from
> Huawei. None of these BMC drivers requiring to be compiled into the
> kernel. I cannot see there is any reason keeping the driver from being
> allowed to be compiled as a module. I also do not think it will bring
> any significant maintenance burden. Implementing correct cleanup code
> should be necessary instead of burden.
Can we split into two series, one fix bugs and the others make bmc modular?

Otherwise I don't think we can reach a consensus in the near future.

Huacai

>
> Cheers,
>
> Miao Wang
>
>

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

[Miao Wang]

Hi,

> 2026年8月3日 21:45，Huacai Chen <chenhuacai@kernel.org> 写道：
> 
> On Fri, Jul 31, 2026 at 4:24 PM Miao Wang <shankerwangmiao@gmail.com> wrote:
>> 
>> Hi,
>> 
>>> 2026年7月31日 16:07，Huacai Chen <chenhuacai@kernel.org> 写道：
>>> 
>>> On Fri, Jul 24, 2026 at 5:28 PM Miao Wang <shankerwangmiao@gmail.com> wrote:
>>>> 
>>>> Hi,
>>>> 
>>>>> 2026年7月24日 16:55，Huacai Chen <chenhuacai@kernel.org> 写道：
>>>>> 
>>>>> Hi, Miao,
>>>>> 
>>>>> On Fri, Jul 10, 2026 at 1:24 AM Miao Wang via B4 Relay
>>>>> <devnull+shankerwangmiao.gmail.com@kernel.org> wrote:
>>>>>> 
>>>>>> 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.
>>>>> You can replace the redraw function, but I don't think it is necessary
>>>>> to make the bmc driver modular.
>>>>> 
>>>>> BMC core, IPMI and simpledrm display are usually (if not always)
>>>>> supposed to work as early as possible.
>>>> 
>>>> I believe that it should be the user's decision to choose whether to
>>>> compile a module into the kernel or as a module and it would be better
>>>> if we can provide the possibilities for the user to choose from.
>>>> Additionally, I don't think these modules are supposed to work that
>>>> early. The mfd module provide two functions, the display and the ipmi
>>>> device. In the aspect of graphical display, without this module, the
>>>> user can still see the output during booting on their monitors, since
>>>> efifb is working, providing a basic display function. In the aspect of
>>>> the ipmi device, I don't think the lack of such device will influence
>>>> the boot of the system, since it is a common practice to compile ipmi
>>>> device drivers as modules on other architectures. As a result, neither
>>>> of the two functions are required to be loaded that early and it is
>>>> reasonable to at lease leave the choice to compile it as a module
>>>> to the user.
>>> Flexibility is not always useful, if a config doesn't provide good
>>> effect, then it just increases complexity and makes maintenance more
>>> difficult.
>> 
>> I should emphasize that to allow this driver to be a module, there is
>> no such increase on maintenance. Moreover, not all loongarch machines
>> are requiring this driver. Especially only a part of the server models
>> are quipped with this BMC. Comparing with other architectures, the
>> driver for BMC are normally compiled as a module, such as mgag200 for
>> iLO from HPE and iDRAC from DELL, hibmc_drm for Kunpeng server from
>> Huawei. None of these BMC drivers requiring to be compiled into the
>> kernel. I cannot see there is any reason keeping the driver from being
>> allowed to be compiled as a module. I also do not think it will bring
>> any significant maintenance burden. Implementing correct cleanup code
>> should be necessary instead of burden.
> Can we split into two series, one fix bugs and the others make bmc modular?
> 
> Otherwise I don't think we can reach a consensus in the near future.

I accept different opinions on design trade-offs. However, I don't think
you have provided enough excuses to remain this driver as built-in,
since I believe normally in kernel, most non-core drivers are all able
to be compiled as a module. I also provided some examples from devices
with similar functions. As a result, I'll not split this series before
there is indeed a strong reason against allowing this driver to be
compiled as a module or we may have a great benefit if we force this
module to be compiled built-in.

Cheers,

Miao Wang

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

[Miao Wang]

Hi, Jones

> 2026年7月26日 05:10，Lee Jones <lee@kernel.org> 写道：
> 
> On Thu, 23 Jul 2026, Miao Wang wrote:
> 
>> Hi,
>> 
>>> 2026年7月23日 21:11，Lee Jones <lee@kernel.org> 写道：
>>> 
>>> On Thu, 23 Jul 2026, Miao Wang wrote:
>>> 
>>>> Hi,
>>>> 
>>>>> 2026年7月23日 19:29，Lee Jones <lee@kernel.org> 写道：
>>>>> 
>>>>> On Thu, 09 Jul 2026, sashiko-bot@kernel.org wrote:
>>>>> 
>>>>>> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>>>>>> 
>>>>>> New issues:
>>>>>> - [High] Unprotected access to `gpio_chip` without a NULL check leads to a potential NULL pointer dereference or use-after-free.
>>>>>> - [Medium] The ACPI memory resource search for the GPIO controller only checks the first memory descriptor, ignoring any subsequent ones.
>>>>> 
>>>>> Looks like there are a few of these to consider.
>>>> 
>>>> Many thanks for your response. I was preparing a v4 to fix the first issue
>>>> found by the bot. However, I believe that the second issue is false positive,
>>>> since the memory descriptors (if more than one) in _CRS are ordered and the
>>>> position is meaningful for the driver. As a result, only checking the first
>>>> descriptor is acceptable.
>>>> 
>>>>> 
>>>>> I'm unmarking this for review for now.
>>>> 
>>>> I still want review for other improvement suggestions. Thanks in advance.
>>> 
>>> Due to extreme workload, I need a clean or justified Sashiko pass first.
>>> 
>> So is there any guides that I can refer to to mark for false positives?
> 
> Most people reply to the feedback and state the fact, with a little
> background or additional information of course.

I have successfully made the V5 pass the review from Sashiko.

Cheers,

Miao Wang

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

[Huacai Chen]

On Tue, Aug 4, 2026 at 12:09 AM Miao Wang <shankerwangmiao@gmail.com> wrote:
>
> Hi,
>
> > 2026年8月3日 21:45，Huacai Chen <chenhuacai@kernel.org> 写道：
> >
> > On Fri, Jul 31, 2026 at 4:24 PM Miao Wang <shankerwangmiao@gmail.com> wrote:
> >>
> >> Hi,
> >>
> >>> 2026年7月31日 16:07，Huacai Chen <chenhuacai@kernel.org> 写道：
> >>>
> >>> On Fri, Jul 24, 2026 at 5:28 PM Miao Wang <shankerwangmiao@gmail.com> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>>> 2026年7月24日 16:55，Huacai Chen <chenhuacai@kernel.org> 写道：
> >>>>>
> >>>>> Hi, Miao,
> >>>>>
> >>>>> On Fri, Jul 10, 2026 at 1:24 AM Miao Wang via B4 Relay
> >>>>> <devnull+shankerwangmiao.gmail.com@kernel.org> wrote:
> >>>>>>
> >>>>>> 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.
> >>>>> You can replace the redraw function, but I don't think it is necessary
> >>>>> to make the bmc driver modular.
> >>>>>
> >>>>> BMC core, IPMI and simpledrm display are usually (if not always)
> >>>>> supposed to work as early as possible.
> >>>>
> >>>> I believe that it should be the user's decision to choose whether to
> >>>> compile a module into the kernel or as a module and it would be better
> >>>> if we can provide the possibilities for the user to choose from.
> >>>> Additionally, I don't think these modules are supposed to work that
> >>>> early. The mfd module provide two functions, the display and the ipmi
> >>>> device. In the aspect of graphical display, without this module, the
> >>>> user can still see the output during booting on their monitors, since
> >>>> efifb is working, providing a basic display function. In the aspect of
> >>>> the ipmi device, I don't think the lack of such device will influence
> >>>> the boot of the system, since it is a common practice to compile ipmi
> >>>> device drivers as modules on other architectures. As a result, neither
> >>>> of the two functions are required to be loaded that early and it is
> >>>> reasonable to at lease leave the choice to compile it as a module
> >>>> to the user.
> >>> Flexibility is not always useful, if a config doesn't provide good
> >>> effect, then it just increases complexity and makes maintenance more
> >>> difficult.
> >>
> >> I should emphasize that to allow this driver to be a module, there is
> >> no such increase on maintenance. Moreover, not all loongarch machines
> >> are requiring this driver. Especially only a part of the server models
> >> are quipped with this BMC. Comparing with other architectures, the
> >> driver for BMC are normally compiled as a module, such as mgag200 for
> >> iLO from HPE and iDRAC from DELL, hibmc_drm for Kunpeng server from
> >> Huawei. None of these BMC drivers requiring to be compiled into the
> >> kernel. I cannot see there is any reason keeping the driver from being
> >> allowed to be compiled as a module. I also do not think it will bring
> >> any significant maintenance burden. Implementing correct cleanup code
> >> should be necessary instead of burden.
> > Can we split into two series, one fix bugs and the others make bmc modular?
> >
> > Otherwise I don't think we can reach a consensus in the near future.
>
> I accept different opinions on design trade-offs. However, I don't think
> you have provided enough excuses to remain this driver as built-in,
> since I believe normally in kernel, most non-core drivers are all able
> to be compiled as a module. I also provided some examples from devices
> with similar functions. As a result, I'll not split this series before
> there is indeed a strong reason against allowing this driver to be
> compiled as a module or we may have a great benefit if we force this
> module to be compiled built-in.
At least you should remove the first patch from this series. Binbin
Zhou has been fixing it:
https://lore.kernel.org/loongarch/20260616115530.4012675-1-zhoubinbin@loongson.cn/
https://lore.kernel.org/loongarch/20260624085550.1508771-1-zhoubinbin@loongson.cn/
https://lore.kernel.org/loongarch/20260706021909.2346535-1-zhoubinbin@loongson.cn/

Huacai

>
> Cheers,
>
> Miao Wang

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

[Miao Wang]

Hi, Huacai

> 2026年8月4日 22:33，Huacai Chen <chenhuacai@kernel.org> 写道：
> 
> On Tue, Aug 4, 2026 at 12:09 AM Miao Wang <shankerwangmiao@gmail.com> wrote:
>> 
>> Hi,
>> 
>>> 2026年8月3日 21:45，Huacai Chen <chenhuacai@kernel.org> 写道：
>>> 
>>> On Fri, Jul 31, 2026 at 4:24 PM Miao Wang <shankerwangmiao@gmail.com> wrote:
>>>> 
>>>> Hi,
>>>> 
>>>>> 2026年7月31日 16:07，Huacai Chen <chenhuacai@kernel.org> 写道：
>>>>> 
>>>>> On Fri, Jul 24, 2026 at 5:28 PM Miao Wang <shankerwangmiao@gmail.com> wrote:
>>>>>> 
>>>>>> Hi,
>>>>>> 
>>>>>>> 2026年7月24日 16:55，Huacai Chen <chenhuacai@kernel.org> 写道：
>>>>>>> 
>>>>>>> Hi, Miao,
>>>>>>> 
>>>>>>> On Fri, Jul 10, 2026 at 1:24 AM Miao Wang via B4 Relay
>>>>>>> <devnull+shankerwangmiao.gmail.com@kernel.org> wrote:
>>>>>>>> 
>>>>>>>> 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.
>>>>>>> You can replace the redraw function, but I don't think it is necessary
>>>>>>> to make the bmc driver modular.
>>>>>>> 
>>>>>>> BMC core, IPMI and simpledrm display are usually (if not always)
>>>>>>> supposed to work as early as possible.
>>>>>> 
>>>>>> I believe that it should be the user's decision to choose whether to
>>>>>> compile a module into the kernel or as a module and it would be better
>>>>>> if we can provide the possibilities for the user to choose from.
>>>>>> Additionally, I don't think these modules are supposed to work that
>>>>>> early. The mfd module provide two functions, the display and the ipmi
>>>>>> device. In the aspect of graphical display, without this module, the
>>>>>> user can still see the output during booting on their monitors, since
>>>>>> efifb is working, providing a basic display function. In the aspect of
>>>>>> the ipmi device, I don't think the lack of such device will influence
>>>>>> the boot of the system, since it is a common practice to compile ipmi
>>>>>> device drivers as modules on other architectures. As a result, neither
>>>>>> of the two functions are required to be loaded that early and it is
>>>>>> reasonable to at lease leave the choice to compile it as a module
>>>>>> to the user.
>>>>> Flexibility is not always useful, if a config doesn't provide good
>>>>> effect, then it just increases complexity and makes maintenance more
>>>>> difficult.
>>>> 
>>>> I should emphasize that to allow this driver to be a module, there is
>>>> no such increase on maintenance. Moreover, not all loongarch machines
>>>> are requiring this driver. Especially only a part of the server models
>>>> are quipped with this BMC. Comparing with other architectures, the
>>>> driver for BMC are normally compiled as a module, such as mgag200 for
>>>> iLO from HPE and iDRAC from DELL, hibmc_drm for Kunpeng server from
>>>> Huawei. None of these BMC drivers requiring to be compiled into the
>>>> kernel. I cannot see there is any reason keeping the driver from being
>>>> allowed to be compiled as a module. I also do not think it will bring
>>>> any significant maintenance burden. Implementing correct cleanup code
>>>> should be necessary instead of burden.
>>> Can we split into two series, one fix bugs and the others make bmc modular?
>>> 
>>> Otherwise I don't think we can reach a consensus in the near future.
>> 
>> I accept different opinions on design trade-offs. However, I don't think
>> you have provided enough excuses to remain this driver as built-in,
>> since I believe normally in kernel, most non-core drivers are all able
>> to be compiled as a module. I also provided some examples from devices
>> with similar functions. As a result, I'll not split this series before
>> there is indeed a strong reason against allowing this driver to be
>> compiled as a module or we may have a great benefit if we force this
>> module to be compiled built-in.
> At least you should remove the first patch from this series. Binbin
> Zhou has been fixing it:
> https://lore.kernel.org/loongarch/20260616115530.4012675-1-zhoubinbin@loongson.cn/
> https://lore.kernel.org/loongarch/20260624085550.1508771-1-zhoubinbin@loongson.cn/
> https://lore.kernel.org/loongarch/20260706021909.2346535-1-zhoubinbin@loongson.cn/


Sorry for not noticing Binbin's patch which had already been addressing the
same issue. However, I believe the fix in my series has covered more issues
than Binbin's patch and thus should be better in the following aspects:

1. Since the length of the resolution string is short enough, there is no
   need to allocate buffer from the heap. The buffer can be allocated 
   directly on the stack. The most possible resolution string might be
   "video=4294967295x4294967295-4294967295@4294967295M\0", which is 51 bytes,
   and allocating 64 bytes on the stack would be enough.
2. My patch enhances the validation of the mode string. strsep(&ptr, sep) may
   set ptr to NULL if sep is not found, which will cause later strsep()
   invocations returning NULL, which is not expected by kstrtoxx() functions.
3. My patch further guards string manipulation functions by adding the ending
   '\0' to the end of the string buffer, to prevent strsep() from overrunning
   the end of the buffer.
4. My patch also addresses the type of the data. The fields in the struct
   simplefb_platform_data are unsigned and thus kstrtouint() should be used
   instead.
 
If the BMC device is under the control of an attacker somehow and the attacker
is feeding malicious resolution string to the host kernel, then the second and
the third issue will cause memory issues in the host kernel, which might be
dangerous.

I wonder if Binbin is willing to address previous issues in that patch. If
Binbin is willing to do so, then I will be pleased to drop the first patch
from my series. If Binbin agrees, that patch can also be absorbed and replaced
by the first patch in my series.

Cheers,

Miao Wang