Re: [PATCH] leds: ktd2692: Pass context to regulator cleanup

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Silent suppression of -EPROBE_DEFER from devm_regulator_get() and missing return on regulator_enable() failure.
- [High] Race condition exposing the LED device to userspace before hardware setup is complete.
- [High] Kernel panic due to division by zero if `flash-max-timeout-us` is configured too small.
--

commit 6025dd2c27de45e105ee950ad7b4f5c0824e9c17
Author: Myeonghun Pak <mhun512@gmail.com>

leds: ktd2692: Pass context to regulator cleanup

This commit fixes a NULL pointer dereference in the regulator disable action.
It passes the allocated driver context directly to devm_add_action_or_reset()
instead of relying on platform_get_drvdata() which is uninitialized at that
point.

> diff --git a/drivers/leds/flash/leds-ktd2692.c b/drivers/leds/flash/leds-ktd2692.c
> index 22fbfccd48735..43d4d55503ab5 100644
> --- a/drivers/leds/flash/leds-ktd2692.c
> +++ b/drivers/leds/flash/leds-ktd2692.c
[ ... ]
> @@ -233,7 +234,7 @@ static int ktd2692_parse_dt(struct ktd2692_context *led, struct device *dev,
>  			dev_err(dev, "Failed to enable supply: %d\n", ret);
>  		} else {
>  			ret = devm_add_action_or_reset(dev,
> -						regulator_disable_action, dev);
> +						regulator_disable_action, led);
>  			if (ret)
>  				return ret;
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but does ktd2692_parse_dt()
silently suppress probe deferral and missing regulator errors?

If devm_regulator_get() returns -EPROBE_DEFER, the error is swallowed:

	led->regulator = devm_regulator_get(dev, "vin");
	if (IS_ERR(led->regulator))
		led->regulator = NULL;

Additionally, if regulator_enable() fails, it prints an error but continues
probing without power instead of returning the error code:

	if (led->regulator) {
		ret = regulator_enable(led->regulator);
		if (ret) {
			dev_err(dev, "Failed to enable supply: %d\n", ret);
		} else {

Should the driver properly propagate -EPROBE_DEFER and return the failure
from regulator_enable()?

[ ... ]
> @@ -290,6 +291,7 @@ static int ktd2692_probe(struct platform_device *pdev)
>  	if (!led)
>  		return -ENOMEM;
>  
> +	led->dev = &pdev->dev;
>  	fled_cdev = &led->fled_cdev;
>  	led_cdev = &fled_cdev->led_cdev;
>  	led->props.timing = ktd2692_timing;

[Severity: High]
This is a pre-existing issue, but does ktd2692_probe() expose the LED device
to userspace before hardware setup is complete?

led_classdev_flash_register() exposes the device to userspace sysfs.
ktd2692_setup() is called afterward:

	ret = led_classdev_flash_register(&pdev->dev, fled_cdev);
	if (ret) {
		dev_err(&pdev->dev, "can't register LED %s\n", led_cdev->name);
		mutex_destroy(&led->lock);
		return ret;
	}

	ktd2692_setup(led);

If userspace writes to sysfs (e.g., brightness) in the race window, it could
trigger ktd2692_led_brightness_set() before or during ktd2692_setup().

Could this lead to concurrent access to uninitialized hardware?

[Severity: High]
This is a pre-existing issue, but can the driver trigger a kernel panic due
to a division by zero if flash-max-timeout-us is configured too small?

If flash-max-timeout-us in the Device Tree is less than 7, setting->step
evaluates to 0 in ktd2692_init_flash_timeout():

	setting->step = cfg->flash_max_timeout
			/ (KTD2692_FLASH_MODE_TIMEOUT_LEVELS - 1);

Later, when userspace triggers a flash strobe via sysfs,
ktd2692_led_flash_strobe_set() executes GET_TIMEOUT_OFFSET, which divides by
step (which is 0):

	flash_tm_reg = GET_TIMEOUT_OFFSET(timeout->val, timeout->step);

Could this cause a division by zero? Should the driver validate the minimum
value of flash-max-timeout-us during initialization?

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