Re: [PATCH v2] leds: flash: leds-qcom-flash: Release all V4L2 flash devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] leds: qcom-flash: mutex_destroy() called before devm LED unregister
--

commit a8ffc86d81428f44e3211062ba0e1f32e17c9078
Author: George Neacsu <geocausa@gmail.com>

leds: flash: leds-qcom-flash: Release all V4L2 flash devices

This commit fixes the V4L2 flash release loop in both the probe error path
and qcom_flash_led_remove() so that all registered entries from
leds_count - 1 down to 0 are properly released. It also moves mutex_init()
after the early probe checks, destroys the mutex on the failure path, and
routes a torch-clamp register write failure through the release label.

Link: https://sashiko.dev/#/patchset/20261016-qcom-flash-v4l2-release-v1-1-0e92aa7c9327@gmail.com?part=1

> diff --git a/drivers/leds/flash/leds-qcom-flash.c b/drivers/leds/flash/leds-qcom-flash.c
> index b03a6833e3e3a..35646c4b839dd 100644
> --- a/drivers/leds/flash/leds-qcom-flash.c
> +++ b/drivers/leds/flash/leds-qcom-flash.c
> @@ -923,7 +923,6 @@ static int qcom_flash_led_probe(struct platform_device *pdev)
>  	devm_kfree(dev, regs); /* devm_regmap_field_bulk_alloc() makes copies */
>  
>  	platform_set_drvdata(pdev, flash_data);
> -	mutex_init(&flash_data->lock);
>  
>  	count = device_get_child_node_count(dev);
>  	if (count == 0 || count > flash_data->max_channels) {
> @@ -936,6 +935,8 @@ static int qcom_flash_led_probe(struct platform_device *pdev)
>  	if (!flash_data->v4l2_flash)
>  		return -ENOMEM;
>  
> +	mutex_init(&flash_data->lock);
> +
>  	device_for_each_child_node_scoped(dev, child) {
>  		led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
>  		if (!led) {
> @@ -951,10 +952,16 @@ static int qcom_flash_led_probe(struct platform_device *pdev)
>  		flash_data->leds_count++;
>  	}
>  
> -	return regmap_field_write(flash_data->r_fields[REG_TORCH_CLAMP], flash_data->torch_clamp);
> +	rc = regmap_field_write(flash_data->r_fields[REG_TORCH_CLAMP],
> +				flash_data->torch_clamp);
> +	if (rc)
> +		goto release;
> +
> +	return 0;
>  release:
> -	while (flash_data->v4l2_flash[flash_data->leds_count] && flash_data->leds_count)
> -		v4l2_flash_release(flash_data->v4l2_flash[flash_data->leds_count--]);
> +	while (flash_data->leds_count)
> +		v4l2_flash_release(flash_data->v4l2_flash[--flash_data->leds_count]);
> +	mutex_destroy(&flash_data->lock);
>  	return rc;
>  }
>  
> @@ -962,8 +969,8 @@ static void qcom_flash_led_remove(struct platform_device *pdev)
>  {
>  	struct qcom_flash_data *flash_data = platform_get_drvdata(pdev);
>  
> -	while (flash_data->v4l2_flash[flash_data->leds_count] && flash_data->leds_count)
> -		v4l2_flash_release(flash_data->v4l2_flash[flash_data->leds_count--]);
> +	while (flash_data->leds_count)
> +		v4l2_flash_release(flash_data->v4l2_flash[--flash_data->leds_count]);
>  
>  	mutex_destroy(&flash_data->lock);
>  }

[Severity: High]
Does calling mutex_destroy(&flash_data->lock) in the qcom_flash_led_probe()
error path and in qcom_flash_led_remove() destroy the mutex while managed
LED class devices can still acquire it?

In qcom_flash_register_led_device(), each LED registers a managed flash
classdev before qcom_flash_v4l2_init() is called:

    flash->led_cdev.brightness_set_blocking = qcom_flash_led_brightness_set;
    ...
    rc = devm_led_classdev_flash_register_ext(dev, flash, &init_data);

Because devm_led_classdev_flash_register_ext() is devres-managed, its
cleanup runs during devres_release_all(), which happens after
qcom_flash_led_probe() returns an error or after qcom_flash_led_remove()
completes.

When devres_release_all() unregisters the LED via
devm_led_classdev_flash_release() -> led_classdev_flash_unregister() ->
led_classdev_unregister(), it turns off the LED and flushes
set_brightness_work:

    if (!(led_cdev->flags & LED_RETAIN_AT_SHUTDOWN))
        led_set_brightness(led_cdev, LED_OFF);

    flush_work(&led_cdev->set_brightness_work);

Flushing set_brightness_work invokes qcom_flash_led_brightness_set() ->
set_flash_module_en() and update_allowed_flash_current(), both of which
call mutex_lock(&flash_data->lock) and mutex_unlock(&flash_data->lock) on
the mutex that was already destroyed.

Would using devm_mutex_init(dev, &flash_data->lock) before the child node
registration loop (and dropping the manual mutex_destroy() calls) ensure
the mutex is destroyed by devres only after all managed LED devices have
been unregistered?

титься

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018-qcom-flash-v4l2-release-v2-1-8e2297844d9f@gmail.com?part=1