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

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

New issues:
- [High] Overwriting an error pointer with NULL before evaluating PTR_ERR() silently masks probe failures and skips interrupt setup.

Pre-existing issues:
- [High] Dynamic mutation of the global `mfd_cell` array violates MFD guidelines and creates race conditions.
- [High] The shared interrupt handler unconditionally returns IRQ_HANDLED without hardware verification, defeating spurious interrupt detection.
--

--- Patch [8]: [PATCH RFC v7 8/8] mfd: ls2kbmc: Capture the reset event of BMC through GPIO ---
commit 6a359b995e90c1c237ad1e2bbc3df19694c36a32
Author: Miao Wang <[email protected]>

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

The reset event of BMC is captured through GPIO. This patch tries to find
the GPIO through declared GPIO pin in the _CRS resources of the ACPI node,
falling back to a fixed address if not found.

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index 5eea53f134215..fd2142d4991a4 100644
> --- a/drivers/mfd/ls2k-bmc-core.c
> +++ b/drivers/mfd/ls2k-bmc-core.c

[ ... ]

> @@ -402,44 +470,35 @@ static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
>  		return ret;
>  	}
>  
> -	gpio_base = ioremap(LOONGSON_GPIO_REG_BASE, LOONGSON_GPIO_REG_SIZE);

[ ... ]

> -	/* 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)) {
> +		ddata->reset_gpio = NULL;
> +		return dev_err_probe(ddata->dev, PTR_ERR(ddata->reset_gpio),
> +				     "Failed to get GPIO pin for reset signal\n");
> +	}

[Severity: High]
Does this error path unintentionally return 0 (success) on failure?

Because ddata->reset_gpio is explicitly set to NULL right before calling
PTR_ERR(ddata->reset_gpio), the error code passed to dev_err_probe()
evaluates to 0. This masks probe failures (like -EPROBE_DEFER) and causes
ls2k_bmc_init() to succeed without configuring the interrupt.

> +	if (ddata->reset_gpio == NULL) {
> +		ddata->reset_gpio = ls2k_bmc_find_gpio(ddata);
> +		if (IS_ERR(ddata->reset_gpio)) {
> +			ddata->reset_gpio = NULL;
> +			return dev_err_probe(ddata->dev, PTR_ERR(ddata->reset_gpio),
> +					     "Failed to find GPIO pin for reset signal\n");
> +		}
> +	}

[Severity: High]
Is this fallback path subject to the same pointer overwrite issue?

Setting ddata->reset_gpio to NULL before calling PTR_ERR(ddata->reset_gpio)
will also mask the error code returned from ls2k_bmc_find_gpio().

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but looking at ls2k_bmc_probe():

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

Does this dynamically mutate the global ls2k_bmc_cells array with a pointer
to a stack-allocated variable?

The MFD subsystem guidelines require mfd_cell arrays to be static const.
Mutating this globally could create a data race if multiple driver instances
probe concurrently, and it leaves a dangling pointer in the global structure
after the probe function returns.

[Severity: High]
This is also a pre-existing issue, but looking at the interrupt handler:

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

    return IRQ_HANDLED;
}

Since devm_request_irq() is called with IRQF_SHARED, is it safe to
unconditionally return IRQ_HANDLED without checking hardware registers to
confirm the interrupt actually originated from the BMC?

If another device on the shared IRQ line experiences an unhandled interrupt
storm, this handler will falsely claim it. This prevents the kernel's
spurious interrupt detector from disabling the stuck line, which can lead
to a hard system lockup.

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