Re: [PATCH v2] leds: Add driver for MAX8907 LED cell

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] leds: use-after-free on driver unbind due to mismatched devm lifecycles
- [Medium] leds: software enum LED_ON is used as a hardware register bitmask
- [Medium] leds: stale non-zero brightness returned when LED is disabled
- [Low] leds: hardcoded device name prevents standard DT property configuration
- [Low] leds: commit message prefix missing driver name
--

commit 18d3fac6772516707b8183897b99b767fe8a81f1
Author: Svyatoslav Ryhel <clamor95@gmail.com>

leds: Add driver for MAX8907 LED cell

Add a small driver to control the LED cell provided by the MAX8907 PMIC.
The LED does not require any specific configuration and is folded into the
PMIC's node.

> leds: Add driver for MAX8907 LED cell

[Severity: Low]
This isn't a bug, but does the commit message prefix follow the standard
subsystem guidelines? The LED subsystem conventions typically expect the
driver name in the prefix, such as "leds: max8907: Add driver ...".

> diff --git a/drivers/leds/leds-max8907.c b/drivers/leds/leds-max8907.c
> new file mode 100644
> index 0000000000000..358505a1c799f
> --- /dev/null
> +++ b/drivers/leds/leds-max8907.c

[ ... ]

> +static enum led_brightness max8907_led_get_brightness(struct led_classdev *cdev)
> +{
> +	struct max8907_led_data *ddata = container_of(cdev, struct max8907_led_data, cdev);
> +	unsigned int brightness_lvl;
> +	int ret;
> +
> +	ret = regmap_read(ddata->regmap, MAX8907_REG_WLED_CNTL, &brightness_lvl);
> +	if (ret)
> +		return LED_OFF;
> +
> +	return brightness_lvl;
> +}

[Severity: Medium]
Will this return a stale non-zero value if the LED was previously turned off?

If max8907_led_set_brightness() is called with LED_OFF, it clears the
enable bit in MAX8907_REG_WLED_MODE_CNTL but skips updating the
MAX8907_REG_WLED_CNTL register. Since max8907_led_get_brightness() does not
verify the mode control enable state, it seems it will incorrectly return the
old brightness value even when the LED is disabled.

[ ... ]

> +static int max8907_led_set_brightness(struct led_classdev *cdev,
> +				      enum led_brightness brightness)
> +{
> +	struct max8907_led_data *ddata = container_of(cdev, struct max8907_led_data, cdev);
> +	int brightness_lvl, ret;
> +
> +	brightness_lvl = clamp(brightness, LED_OFF, LED_FULL);
> +
> +	if (brightness_lvl) {
> +		ret = regmap_write(ddata->regmap, MAX8907_REG_WLED_CNTL, brightness_lvl);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return regmap_update_bits(ddata->regmap, MAX8907_REG_WLED_MODE_CNTL,
> +				  LED_ON, brightness_lvl ? LED_ON : LED_OFF);
> +}

[Severity: Medium]
Is the software enum LED_ON intentionally used as a hardware register bitmask
here?

LED_ON is defined in the LED subsystem as 1. Using it as the mask argument
for regmap_update_bits() on MAX8907_REG_WLED_MODE_CNTL mixes a software
state enum with hardware layout. If the PMIC's enable bit changes or differs
from BIT(0), this could fail to enable the LED and potentially corrupt other
configuration bits.

[ ... ]

> +static int max8907_led_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct max8907 *max8907 = dev_get_drvdata(dev->parent);
> +	struct max8907_led_data *ddata;
> +
> +	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> +	if (!ddata)
> +		return -ENOMEM;
> +
> +	ddata->max8907 = max8907;
> +	ddata->regmap = max8907->regmap_gen;
> +
> +	platform_set_drvdata(pdev, ddata);
> +
> +	ddata->cdev.brightness = LED_OFF;
> +	ddata->cdev.flags = LED_CORE_SUSPENDRESUME;
> +	ddata->cdev.max_brightness = LED_FULL;
> +	ddata->cdev.name = dev_name(dev);

[Severity: Low]
Does hardcoding the device name prevent standard Device Tree property
configuration?

By directly assigning dev_name(dev), it bypasses parsing standard DT
properties like color and function, which means board designers cannot
configure standard sysfs directory names for this LED.

> +
> +	ddata->cdev.brightness_get = max8907_led_get_brightness;
> +	ddata->cdev.brightness_set_blocking = max8907_led_set_brightness;
> +
> +	return devm_led_classdev_register(dev->parent, &ddata->cdev);
> +}

[Severity: High]
Can this lead to a use-after-free during driver unbind?

The private data ddata is allocated using devm_kzalloc(dev, ...) tying
its lifecycle to the child platform device. However, the LED classdev is
registered using devm_led_classdev_register(dev->parent, &ddata->cdev),
tying the unregistration action to the parent MFD core device.

If the child platform driver is unbound or removed, ddata will be freed,
but the LED classdev unregistration won't trigger because it's attached
to the parent device. Any subsequent access via sysfs or subsystem triggers
could dereference the freed memory.

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