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

On Thu, 09 Jul 2026, [email protected] 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 <[email protected]>
> 
> 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/[email protected]?part=7

-- 
Lee Jones