Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/bridge: ps8622: clobbered brightness in ps8622_backlight_update() --
commit a81389f64f9ebf64ed6660717e83f2f04182a30d Author: Mario Limonciello <[email protected]> drm/bridge: add PS8622 connector luminance support This commit exposes PS8622 brightness control through bridge luminance operations when DRM_LUMINANCE is enabled, while continuing to register the backlight class device otherwise. > diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c > b/drivers/gpu/drm/bridge/parade-ps8622.c > index 3a2ec482037df..c88bd7a97d4a4 100644 > --- a/drivers/gpu/drm/bridge/parade-ps8622.c > +++ b/drivers/gpu/drm/bridge/parade-ps8622.c [ ... ] > @@ -268,7 +270,7 @@ static int ps8622_send_config(struct ps8622_bridge > *ps8622) > goto error; > > > - if (ps8622->bl) { > + if (ps8622->internal_pwm) { > /* DPCD720, internal PWM */ > err = ps8622_set(cl, 0x01, 0xa5, 0xa0); > if (err) > @@ -276,7 +278,7 @@ static int ps8622_send_config(struct ps8622_bridge > *ps8622) > > /* FFh for 100% brightness, 0h for 0% brightness */ > err = ps8622_set(cl, 0x01, 0xa7, > - ps8622->bl->props.brightness); > + ps8622->brightness); > if (err) > goto error; > } else { > @@ -324,6 +326,8 @@ static int ps8622_backlight_update(struct > backlight_device *bl) > struct ps8622_bridge *ps8622 = dev_get_drvdata(&bl->dev); > int ret, brightness = backlight_get_brightness(bl); > > + ps8622->brightness = brightness; > + > if (!ps8622->enabled) > return -EINVAL; [Severity: Medium] Can updating ps8622->brightness from backlight_get_brightness(bl) before checking ps8622->enabled clobber the cached brightness with 0 when the backlight is blanked while the bridge is disabled? Previously, ps8622_send_config() programmed ps8622->bl->props.brightness, which preserves the requested brightness level across blanking. Now it programs ps8622->brightness, which is updated in ps8622_backlight_update() from backlight_get_brightness(bl): static inline int backlight_get_brightness(const struct backlight_device *bd) { if (backlight_is_blank(bd)) return 0; else return bd->props.brightness; } If userspace writes BACKLIGHT_POWER_OFF to bl_power while ps8622->enabled is false, bl_power_store() in drivers/video/backlight/backlight.c temporarily sets bd->props.power and calls backlight_update_status(bd) -> ps8622_backlight_update(bl), which overwrites ps8622->brightness with 0 and returns -EINVAL: if (bd->props.power != power) { old_power = bd->props.power; bd->props.power = power; rc = backlight_update_status(bd); if (rc) bd->props.power = old_power; Because ps8622_backlight_update() returns -EINVAL, bl_power_store() rolls bd->props.power back to BACKLIGHT_POWER_ON, leaving the backlight device unblanked at its non-zero bd->props.brightness while ps8622->brightness has been overwritten with 0. Similarly, during fbdev blanking or backlight_suspend(), if the bridge is disabled first (setting ps8622->enabled = false in ps8622_post_disable()) and the backlight notifier or suspend callback subsequently invokes ps8622_backlight_update(bl), ps8622->brightness is overwritten with 0 while disabled. On the next ps8622_pre_enable() -> ps8622_send_config(), the panel backlight will be programmed to 0 instead of the configured brightness. Would it make sense to avoid overwriting ps8622->brightness when returning -EINVAL, or to keep using bl->props.brightness when restoring the initial backlight level in ps8622_send_config()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20261103-superm1-backlight-property-v11-0-00cce7b2b...@kernel.org?part=11
