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

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

New issues:
- [High] The fallback path will likely fail at `gpiod_to_irq()` on older firmware, causing the driver probe to fail.
- [Low] Casting `phys_addr_t` to `void *` and back causes compiler warnings and potential truncation on 32-bit architectures with 64-bit physical addresses.

Pre-existing issues:
- [High] The probe function assigns a pointer to a local stack variable directly to the global `ls2k_bmc_cells` array, violating MFD subsystem guidelines and creating a concurrency vulnerability.
- [High] The interrupt handler unconditionally returns `IRQ_HANDLED` without checking hardware registers to verify if the interrupt originated from the device, despite being registered with `IRQF_SHARED`.
--

--- Patch [7]: [PATCH RFC v8 7/8] mfd: ls2kbmc: Capture the reset event of BMC through GPIO ---
commit 0a3e35583f83690aea838854a1f8bc9d3f258974
Author: Miao Wang <[email protected]>

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

This commit updates the ls2kbmc driver to retrieve the BMC reset GPIO
from the ACPI node instead of using a hardcoded address. If the ACPI
node lacks the GPIO declaration, it falls back to searching for the
GPIO controller using a fixed address.

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

[ ... ]

> @@ -375,6 +368,82 @@ static void ls2k_bmc_save_pci_data(struct pci_dev *pdev, struct ls2k_bmc_ddata *
[ ... ]
> +static int ls2k_bmc_gpiochip_find(struct gpio_chip *gc, const void *data)
> +{
[ ... ]
> +	phys_addr_t start_addr = (phys_addr_t) data;

[Severity: Low]
Could this cast from a void pointer back to phys_addr_t cause truncation or
compiler warnings on 32-bit architectures that use 64-bit physical
addresses?

[ ... ]

> +static struct gpio_desc *ls2k_bmc_find_gpio(struct ls2k_bmc_ddata *ddata)
> +{
[ ... ]
> +	struct gpio_device *gdev __free(gpio_device_put) =
> +		gpio_device_find((void *)LOONGSON_GPIO_REG_BASE, ls2k_bmc_gpiochip_find);

[Severity: Low]
This is the other side of the cast mentioned above. Can we avoid casting a
physical address integer directly to a void pointer?

[ ... ]

> @@ -402,44 +470,31 @@ static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
[ ... ]
> +	ddata->reset_gpio = devm_gpiod_get_index_optional(&pdev->dev, NULL, 0, GPIOD_IN);
> +	if (IS_ERR(ddata->reset_gpio))
> +		return dev_err_probe(ddata->dev, PTR_ERR(ddata->reset_gpio),
> +				     "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))
> +			return dev_err_probe(ddata->dev, PTR_ERR(ddata->reset_gpio),
> +					     "Failed to find GPIO pin for reset signal\n");
> +	}
>  
> -	iounmap(gpio_base);
> +	gpio_irq = gpiod_to_irq(ddata->reset_gpio);

[Severity: High]
Might this gpiod_to_irq() call fail on older firmware?

The previous implementation used acpi_register_gsi() directly because the
GPIO controller's ACPI node on older firmware lacked comprehensive IRQ
resource definitions.

If the fallback path triggers on this older firmware, gpiod_to_irq() will
eventually call platform_get_irq(), which relies on those ACPI IRQ
resources.

If they are missing, it might return -ENXIO and cause the entire driver
probe to abort.

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

[Severity: High]
This is a pre-existing issue, but while we are looking at the IRQ
registration, does ls2k_bmc_interrupt() need to check hardware registers to
verify if the interrupt actually originated from this device?

Since it is registered with IRQF_SHARED, but unconditionally returns
IRQ_HANDLED:

static irqreturn_t ls2k_bmc_interrupt(int irq, void *arg)
{
    ...
    return IRQ_HANDLED;
}

Could this bypass the kernel's spurious interrupt detection and lead to an
interrupt storm if another device sharing the line generates an interrupt?

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential concurrency issue in
ls2k_bmc_probe() regarding the global mfd_cell array:

static int ls2k_bmc_probe(struct platform_device *pdev)
{
    ...
    struct ls2k_bmc_pci_data pd;
    ...
    ls2k_bmc_cells[LS2K_BMC_DISPLAY].platform_data = &pd;
    ...
}

Is it safe to assign a pointer to a local stack variable to the global
ls2k_bmc_cells array?

In a multi-probe scenario, multiple threads could concurrently modify
ls2k_bmc_cells. If one thread overwrites the pointer with its own stack
address, could another thread dereference a dangling stack pointer?

The MFD subsystem guidelines typically require mfd_cell arrays to be static
const.

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