Re: [PATCH RFC v6 7/7] 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:
- [Low] Casting phys_addr_t directly to void * causes compiler warnings on 32-bit builds where phys_addr_t is 64-bit.
- [Low] Inconsistent GPIO polarity configuration using GPIO_ACTIVE_HIGH for an active-low reset signal.

Pre-existing issues:
- [High] The driver mutates a global static `mfd_cell` array in the probe function, violating MFD subsystem guidelines and causing a data race.
- [High] The shared interrupt handler `ls2k_bmc_interrupt` unconditionally returns `IRQ_HANDLED`, defeating spurious interrupt detection.
--

--- Patch [7]: [PATCH RFC v6 7/7] mfd: ls2kbmc: Capture the reset event of BMC through GPIO ---
commit 9252e63d105a3c07a23e797a66a7260c9eee7e44
Author: Miao Wang <[email protected]>

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

This patch updates the ls2kbmc driver to retrieve the BMC reset GPIO
through the ACPI node's _CRS resources. If no declaration is found, it
falls back to finding the correct GPIO controller and pin using a fixed
address.

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> --- a/drivers/mfd/ls2k-bmc-core.c
> +++ b/drivers/mfd/ls2k-bmc-core.c
[ ... ]
> @@ -375,6 +368,91 @@ static void ls2k_bmc_save_pci_data(struct pci_dev *pdev, struct ls2k_bmc_ddata *
> +static struct gpio_desc *ls2k_bmc_find_gpio(struct ls2k_bmc_ddata *ddata)
> +{
[ ... ]
> +	static const phys_addr_t LOONGSON_GPIO_REG_BASE = 0x1FE00500;
> +	static const unsigned int LS2K_BMC_RESET_GPIO = 14;
> +	int ret;
> +	struct gpio_device *gdev __free(gpio_device_put) = NULL;
> +	struct property_entry ls2k_bmc_swnode_properties[2] = { 0 };
> +
> +	dev_dbg(ddata->dev, "Searching for GPIO chip at address %pa\n", &LOONGSON_GPIO_REG_BASE);
> +
> +	gdev = gpio_device_find((void *)LOONGSON_GPIO_REG_BASE, ls2k_bmc_gpiochip_find);

[Severity: Low]
Does casting phys_addr_t directly to a void * cause a truncation warning on
32-bit builds where phys_addr_t is 64-bit?

> +
> +	if (!gdev) {
> +		dev_dbg(ddata->dev, "cannot find GPIO chip at address %pa, deferring\n",
> +			&LOONGSON_GPIO_REG_BASE);
> +		return ERR_PTR(-EPROBE_DEFER);
> +	}
> +
> +	ls2k_bmc_swnode_properties[0] = PROPERTY_ENTRY_GPIO("gpio",
> +		gpio_dev_get_fwnode(gdev), LS2K_BMC_RESET_GPIO, GPIO_ACTIVE_HIGH);

[Severity: Low]
Could the fallback software node configure the active polarity inconsistently
with the hardware?

The ACPI specification and the previous hardware implementation define the BMC
reset GPIO as active-low, but it is registered here as GPIO_ACTIVE_HIGH.

> +
> +	ret = device_create_managed_software_node(ddata->dev, ls2k_bmc_swnode_properties, NULL);
[ ... ]
> @@ -402,44 +479,36 @@ 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 unconditionally returning IRQ_HANDLED
in the shared interrupt handler defeat spurious interrupt detection?

The driver requests the PCI and GPIO interrupts with IRQF_SHARED. However,
ls2k_bmc_interrupt() does not verify that the interrupt was generated by the
BMC before claiming it:

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

If another device sharing the IRQ line triggers a spurious interrupt, could
this false claim lead to a system-halting interrupt storm?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does the driver mutate a
global static mfd_cell array in the probe function?

Looking at ls2k_bmc_probe(), it injects a stack pointer into ls2k_bmc_cells:

static struct mfd_cell ls2k_bmc_cells[] = {
        ...
};

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

This violates the MFD subsystem guidelines which require cell arrays to be
static const. If multiple devices are probed concurrently, could this cause a
data race and potential use-after-free?

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