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

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

New issues:
- [High] The patch introduces unintentional strict validation on the BMC mode string format, breaking backward compatibility for previously valid strings.

Pre-existing issues:
- [Critical] Massive busy-wait delays inside atomic stop_machine context.
- [High] The ACPI GSI mapping is destroyed immediately after being requested, breaking the interrupt and creating an unbalanced lifecycle.
- [High] The driver modifies a global `static struct mfd_cell` array during probe, violating MFD subsystem guidelines and creating a concurrency race condition.
- [High] Unconditional IRQ_HANDLED in shared interrupt handler breaks interrupt sharing.
--

--- Patch [1]: [PATCH RFC v4 1/7] mfd: ls2kbmc: Make a copy when parsing mode string ---
commit 08b5951096b7b6051b970b868f431e24b8983d1a
Author: Miao Wang <[email protected]>

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.

> 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)
>   */
>  static int ls2k_bmc_parse_mode(struct pci_dev *pdev, struct simplefb_platform_data *pd)
>  {
[ ... ]
>  	/* The resolution field starts with the flag "video=". */
>  	if (!strncmp(mode, "video=", 6))
>  		mode = mode + 6;
> +	else
> +		goto invalid_mode;

[Severity: High]
Will this make the "video=" prefix mandatory, causing valid strings that
previously worked without this prefix to be rejected?

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

[Severity: High]
Is it possible that this rejects strings missing an explicit refresh rate
(e.g., "@60")? 

Since strsep() sets mode to NULL when the delimiter is absent, these new
checks could force the string to have an "@", rejecting valid fbdev
configurations that worked previously.

[ ... ]

[Severity: High]
This is a pre-existing issue, but the ACPI GSI mapping appears to be destroyed
immediately after being requested in ls2k_bmc_init():

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

Can this unconditional unregistration break the hardware interrupt mapping just
acquired, preventing the interrupt from ever reaching the driver handler?


[Severity: High]
This isn't a bug introduced by this patch, but the driver modifies a shared
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);
    ...
}

Could writing a stack pointer (&pd) to a global variable violate MFD subsystem
guidelines? 

If multiple instances of this driver were probed concurrently, they might race
to overwrite the shared .platform_data pointer, causing a use-after-free
during platform device registration.


[Severity: High]
This is a pre-existing issue, but the shared interrupt handler seems to
unconditionally return IRQ_HANDLED:

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

Since the driver requests its interrupts with the IRQF_SHARED flag, does
returning IRQ_HANDLED without verifying interrupt ownership break interrupt
sharing and unconditionally claim all interrupts on the shared line?


[Severity: Critical]
This is a pre-existing issue, but there appear to be massive busy-wait delays
inside an atomic stop_machine context:

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() {
    ...
    /* Waiting for U-Boot and DDR ready */
    mdelay(LS2K_BMC_RESET_WAIT_TIME);
    ...
}

Stop_machine() freezes all CPUs and disables interrupts globally. 

Could spinning for 10 seconds inside ls2k_bmc_recover_pci_data() cause system
lockups and trigger NMI watchdogs?

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