Re: [RFC PATCH 06/14] leds: add support for macsmc kbl

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] leds: macsmc: sleeping SMC write registered as non-blocking brightness_set
- [Medium] leds: macsmc: non-standard LED name breaks kbd_backlight discovery
- [Medium] leds: macsmc: missing suspend/resume handling loses backlight state
- [Low] leds: macsmc: commit prefix, drv naming, and Makefile sort order violations
--

commit 62f13fe6413b6947c13026e13a39c022cd83dde2
Author: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

leds: add support for macsmc kbl

This commit adds a LED class driver for the Apple SMC keyboard backlight on
Intel Macs and registers the macsmc-led MFD cell when the LKSB key is present,
intended to replace the keyboard backlight support in applesmc.

>     leds: add support for macsmc kbl
>     
>     Add a LED class driver to control the keyboard backlight.
>     
>     This replaces the keyboard backlight support in applesmc.
>     
>     Signed-off-by: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

[Severity: Low]
This isn't a bug, but should the commit subject follow the LED subsystem
convention of including the driver prefix and capitalizing the description,
such as "leds: macsmc: Add support for keyboard backlight"?

[ ... ]

> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 4d4b089156e24..7c38dbef42390 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_LEDS_KUNIT_TEST)		+= led-test.o
>  obj-$(CONFIG_LEDS_88PM860X)		+= leds-88pm860x.o
>  obj-$(CONFIG_LEDS_ACER_A500)		+= leds-acer-a500.o
>  obj-$(CONFIG_LEDS_ADP5520)		+= leds-adp5520.o
> +obj-$(CONFIG_LEDS_MACSMC)		+= leds-macsmc.o
>  obj-$(CONFIG_LEDS_AN30259A)		+= leds-an30259a.o

[Severity: Low]
This isn't a bug, but the platform drivers section in drivers/leds/Makefile
is kept sorted alphabetically. Should leds-macsmc.o be placed in alphabetical
order right before leds-max5970.o?

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

[ ... ]

> +static enum led_brightness macsmc_led_get_brightness(struct led_classdev *cdev)
> +{
> +	struct macsmc_led *drv = container_of(cdev, struct macsmc_led, cdev);
> +
> +	return drv->brightness;
> +}
> +
> +static void macsmc_led_set_brightness(struct led_classdev *cdev, enum led_brightness value)
> +{
> +	struct macsmc_led *drv = container_of(cdev, struct macsmc_led, cdev);
> +
> +	drv->brightness = (u8) value;
> +
> +	apple_smc_write_u16(drv->smc, SMC_KEY(LKSB), drv->brightness);
> +}
> +
> +static int macsmc_led_probe(struct platform_device *pdev)
> +{
> +	struct macsmc_led *drv;

[Severity: Low]
This isn't a bug, but could the private data pointer be named ddata instead
of drv in macsmc_led_get_brightness(), macsmc_led_set_brightness(), and
macsmc_led_probe() to match the LED subsystem naming convention?

> +
> +	drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
> +	if (!drv)
> +		return -ENOMEM;
> +
> +	drv->smc = dev_get_drvdata(pdev->dev.parent);
> +
> +	drv->cdev.name = "macsmc-led";

[Severity: Medium]
Will setting drv->cdev.name to "macsmc-led" in macsmc_led_probe() cause a
regression for userspace tools discovering the keyboard backlight?

In drivers/hwmon/applesmc.c, which this driver replaces, the LED class device
is registered with the name "smc::kbd_backlight":

    static struct led_classdev applesmc_backlight = {
        .name            = "smc::kbd_backlight",
        .default_trigger = "nand-disk",
        .brightness_set  = applesmc_brightness_set,
    };

Documentation/leds/leds-class.rst also specifies that keyboard backlight LED
devices should use kbd_backlight as the function suffix so that userspace
consumers (such as upower and systemd-backlight) can identify them under
/sys/class/leds/. Should this keep "smc::kbd_backlight" to preserve UAPI
compatibility?

> +	drv->cdev.max_brightness = 255;
> +	drv->cdev.brightness_get = macsmc_led_get_brightness;
> +	drv->cdev.brightness_set = macsmc_led_set_brightness;

[Severity: High]
Can registering macsmc_led_set_brightness() on drv->cdev.brightness_set in
macsmc_led_probe() sleep in atomic context when an atomic LED trigger or
software blink timer fires?

When software blinking or an atomic trigger updates the LED, the call chain
led_timer_function() or led_trigger_event() -> led_set_brightness_nosleep() ->
led_set_brightness_nopm() -> __led_set_brightness() invokes the
brightness_set callback synchronously without deferring to
set_brightness_work:

    /* Use brightness_set op if available, it is guaranteed not to sleep */
    if (!__led_set_brightness(led_cdev, value))
        return;

However, macsmc_led_set_brightness() calls apple_smc_write_u16() ->
apple_smc_write():

    int apple_smc_write(struct apple_smc *smc, smc_key key, const void *buf, size_t size)
    {
        guard(mutex)(&smc->mutex);
        return smc->be->write(smc->be_cookie, key, buf, size);
    }

This acquires a sleeping mutex and calls apple_smc_acpi_write(), which sleeps
via wait_for_completion_timeout() and fsleep(). Unlike applesmc, which queued
a work item on a workqueue in applesmc_brightness_set(), this sleeps directly
in the caller's context and also discards any error returned by
apple_smc_write_u16().

Should macsmc_led_set_brightness() be registered via
drv->cdev.brightness_set_blocking instead, and return the result of
apple_smc_write_u16()?

> +	drv->cdev.dev = &pdev->dev;
> +
> +	return devm_led_classdev_register(&pdev->dev, &drv->cdev);
> +}

[Severity: Medium]
Does omitting suspend and resume handling here cause the keyboard backlight
state to be lost across system suspend or hibernation?

In drivers/hwmon/applesmc.c, applesmc_pm_resume() and applesmc_pm_restore()
restore the saved brightness to the hardware key on resume:

    static int applesmc_pm_resume(struct device *dev)
    {
        if (smcreg.has_key_backlight)
            applesmc_write_key(BACKLIGHT_KEY, backlight_state, 2);
        return 0;
    }

Because macsmc_led_probe() does not set LED_CORE_SUSPENDRESUME in
drv->cdev.flags (and macsmc_led_driver does not define PM callbacks), the
keyboard backlight is not turned off on suspend or restored on resume. Note
 also that if LED_CORE_SUSPENDRESUME is enabled, the custom brightness_get
callback returning drv->brightness would return 0 during resume after
led_classdev_suspend() sets the brightness to 0, unless the LED core's
cached brightness is used instead of a custom brightness_get callback.

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