Re: [PATCH v11 11/12] drm/bridge: add PS8622 connector luminance support

[sashiko-bot@kernel.org]

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 <mario.limonciello@amd.com>

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-00cce7b2ba22@kernel.org?part=11