Re: [PATCH] leds: is31fl319x: modernize registration

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The newly added devres cleanup action `is31_free_fwnode` unconditionally dereferences `is31->cdef` before it is initialized, causing a fatal NULL pointer dereference on early probe failures.
- [Low] Typo in the error message for invalid maximum current validation: 'corrunt' instead of 'current'.
--

commit f41ac0185576615af09273cdd8749b261df7d08e
Author: Andreas Kemnade <[email protected]>

leds: is31fl319x: modernize registration

This patch modifies the is31fl319x driver to use the extended LED registration
API to parse properties automatically. It also delays LED registration until
after all firmware nodes are parsed to correctly handle the maximum current
mechanics.

> diff --git a/drivers/leds/leds-is31fl319x.c b/drivers/leds/leds-is31fl319x.c
> index 80f38dba0fba4..e5cca596fe7c0 100644
> --- a/drivers/leds/leds-is31fl319x.c
> +++ b/drivers/leds/leds-is31fl319x.c
[ ... ]
> @@ -427,14 +400,20 @@ static int is31fl319x_parse_fw(struct device *dev, struct is31fl319x_chip *is31)
>  
>  		led = &is31->leds[reg - 1];
>  
> -		if (led->configured)
> +		if (led->fwnode)
>  			return dev_err_probe(dev, -EINVAL, "led %u is already configured\n", reg);
>  
> -		ret = is31fl319x_parse_child_fw(dev, child, led, is31);
> -		if (ret)
> -			return dev_err_probe(dev, ret, "led %u DT parsing failed\n", reg);
> +		led->max_microamp = is31->cdef->current_default;
> +		ret = fwnode_property_read_u32(child, "led-max-microamp", &led->max_microamp);
> +		if (!ret) {
> +			if (led->max_microamp < is31->cdef->current_min)
> +				return dev_err_probe(dev, -EINVAL, "invalid maximum corrunt\n");

[Severity: Low]
This isn't a bug, but there appears to be a typo here ("corrunt" instead
of "current").

> +
> +			led->max_microamp = min(led->max_microamp,
> +						is31->cdef->current_max);
> +		}
>  
> -		led->configured = true;
> +		led->fwnode = fwnode_handle_get(child);
>  	}
>  
>  	is31->audio_gain_db = 0;
[ ... ]
> @@ -483,6 +462,19 @@ static inline int is31fl3196_db_to_gain(u32 dezibel)
>  	return dezibel / IS31FL3196_AUDIO_GAIN_DB_STEP;
>  }
>  
> +static void is31_free_fwnode(void *data)
> +{
> +	struct is31fl319x_chip *is31 = data;
> +	int i;
> +
> +	for (i = 0; i < is31->cdef->num_leds; i++) {
> +		if (is31->leds[i].fwnode)
> +			fwnode_handle_put(is31->leds[i].fwnode);
> +
> +		is31->leds[i].fwnode = NULL;
> +	}
> +}
> +
>  static int is31fl319x_probe(struct i2c_client *client)
>  {
>  	struct is31fl319x_chip *is31;
> @@ -498,6 +490,10 @@ static int is31fl319x_probe(struct i2c_client *client)
>  	if (!is31)
>  		return -ENOMEM;
>  
> +	err = devm_add_action_or_reset(dev, is31_free_fwnode, is31);

[Severity: High]
Could this sequence cause a NULL pointer dereference on the error path?

The cleanup action is registered here before is31->cdef is populated.
If an initialization step fails early before is31->cdef is assigned, the
devres core will call is31_free_fwnode() which then dereferences the
uninitialized is31->cdef in its loop condition.

For example, if devm_gpiod_get_optional() defers probe in is31fl319x_parse_fw():

is31fl319x_parse_fw() {
	is31->shutdown_gpio = devm_gpiod_get_optional(...);
	if (IS_ERR(is31->shutdown_gpio))
		return dev_err_probe(...);

	...
	is31->cdef = device_get_match_data(dev);
}

The early return triggers the cleanup, which evaluates
is31->cdef->num_leds, leading to a panic.

> +	if (err)
> +		return err;
> +
>  	err = devm_mutex_init(dev, &is31->lock);
>  	if (err)
>  		return err;

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