Re: [PATCH] leds: flash: sgm3140: fix child node reference leak

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Non-capitalized description in commit message.

Pre-existing issues:
- [High] Use-After-Free of driver private data (priv) due to missing or incorrectly ordered del_timer_sync() for powerdown_timer.
- [High] The powerdown timer callback calls regulator_disable(), which can sleep, from atomic context.
- [High] Unbalanced regulator_disable() in timer handler due to missing timer cancellation in brightness_set(LED_OFF).
--

commit ee14e99da842d6c5f84775b78b062eac6483dedf
Author: Guangshuo Li <lgs201920130244@gmail.com>

leds: flash: sgm3140: fix child node reference leak

This commit fixes a missing fwnode_handle_put() call in the successful
probe path to avoid leaking a reference to the LED child node.

> leds: flash: sgm3140: fix child node reference leak

[Severity: Low]
This isn't a bug introduced by this patch, but the subsystem guidelines
mandate capitalizing the description after the subsystem prefix. Could
"fix" be capitalized to "Fix"?

> diff --git a/drivers/leds/flash/leds-sgm3140.c b/drivers/leds/flash/leds-sgm3140.c
> index d3a30ad94ac46..ab649e2c8f20a 100644
> --- a/drivers/leds/flash/leds-sgm3140.c
> +++ b/drivers/leds/flash/leds-sgm3140.c

[ ... ]

>  	/* Create V4L2 Flash subdev */
>  	priv->v4l2_flash = v4l2_flash_init(&pdev->dev,
>  					   child_node,
>  					   fled_cdev, NULL,
>  					   &v4l2_sd_cfg);
>  	if (IS_ERR(priv->v4l2_flash)) {
>  		ret = PTR_ERR(priv->v4l2_flash);
>  		goto err;
>  	}

[Severity: High]
This is a pre-existing issue, but I noticed the error path unwinds devm
allocations without canceling the powerdown timer. 

Since devm_led_classdev_flash_register_ext() is called before this step, 
userspace could theoretically trigger a flash strobe and arm the timer. If 
v4l2_flash_init() fails and we take this error path, devres callbacks will 
free the driver data. Could the timer expire later and access freed memory?

Also, looking at sgm3140_remove(), del_timer_sync() is called manually
before the devm-managed LED is unregistered. Could this create a race
window where the timer is re-armed before the driver fully unloads?

>  
> -	return ret;
> +	fwnode_handle_put(child_node);
> +
> +	return 0;
>  
>  err:
>  	fwnode_handle_put(child_node);

While reviewing the timer code, I also noticed a couple of other
pre-existing issues in the driver:

[Severity: High]
This is a pre-existing issue, but sgm3140_powerdown_timer() executes in
atomic context and calls regulator_disable(), which can sleep:

sgm3140_powerdown_timer()
   ...
   regulator_disable(priv->vin_regulator);

Can this lead to a scheduling while atomic panic when the timer expires?

[Severity: High]
This is a pre-existing issue, but sgm3140_brightness_set() disables the 
regulator when setting LED_OFF without canceling the powerdown timer:

sgm3140_brightness_set()
   ...
   } else {
       gpiod_set_value_cansleep(priv->enable_gpio, 0);
       ret = regulator_disable(priv->vin_regulator);

If the timer was armed by a previous strobe, it will expire later and 
unconditionally call regulator_disable() again:

sgm3140_powerdown_timer()
   ...
   regulator_disable(priv->vin_regulator);
   
Could this unbalanced regulator disable cause regulator core warnings or
unintended shutdown of shared regulators?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914135723.1741327-1-lgs201920130244@gmail.com?part=1