Re: [PATCH v3 3/3] drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] Encoder atomic enable and disable callbacks invoke sleepable kernel functions from atomic context.
- [High] Incorrect end address calculation for column and row ranges during partial display updates.
- [High] Screen clear operations fail to reset the hardware column and row address ranges to the full display, leaving previous partial updates uncleared.
- [High] Missing format conversion state preallocation in `ssd133x_primary_plane_atomic_check` leads to forbidden `GFP_KERNEL` allocations in atomic context.
- [High] `ERR_PTR` dereference in `ssd130x_power_off` if `pwm_get` fails during display enable.
--

commit 28391e97ee3411b1213256c23b42e73a4550eba0
Author: Amit Barzilai <amit.barzilai22@gmail.com>
Subject: drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support

This commit introduces support for the Solomon SSD1351 RGB color OLED
controller. It adds a new SSD135X_FAMILY which reuses existing SSD133X
helpers while providing a custom initialization sequence and an explicit
Write RAM command path.

> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 3f09977d227b0..f7f0a74a297e2 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c

[ ... ]

> @@ -826,7 +927,7 @@ static int ssd133x_update_rect(struct ssd130x_device *ssd130x,
>  		return ret;
>  
>  	/* Write out update in one go since horizontal addressing mode is used */
> -	ret = ssd130x_write_data(ssd130x, data_array, pitch * rows);
> +	ret = ssd133x_write_pixels(ssd130x, data_array, pitch * rows);
>  
>  	return ret;

[Severity: High]
This is a pre-existing issue, but does ssd133x_update_rect() correctly
calculate the end address for the column and row ranges?

When calculating the hardware active window boundaries, the driver currently
sets the range width and height directly rather than adding them to the start
coordinates:

drivers/gpu/drm/solomon/ssd130x.c:ssd133x_update_rect() {
    ...
    ret = ssd130x_write_cmd(ssd130x, 3, SSD133X_SET_COL_RANGE, x, columns - 1);
    if (ret < 0)
        return ret;

    ret = ssd130x_write_cmd(ssd130x, 3, SSD133X_SET_ROW_RANGE, y, rows - 1);
    ...
}

For a partial update starting at offset x and y, would this program an
incorrect end address relative to 0 instead of the start coordinate, resulting
in corrupted partial updates? (The same appears to be true in
ssd132x_update_rect()).

[ ... ]

> @@ -896,7 +997,7 @@ static void ssd133x_clear_screen(struct ssd130x_device *ssd130x, u8 *data_array)
>  	memset(data_array, 0, pitch * ssd130x->height);
>  
>  	/* Write out update in one go since horizontal addressing mode is used */
> -	ssd130x_write_data(ssd130x, data_array, pitch * ssd130x->height);
> +	ssd133x_write_pixels(ssd130x, data_array, pitch * ssd130x->height);

[Severity: High]
This isn't a bug introduced by this patch, but does ssd133x_clear_screen()
fail to reset the address window before clearing the display?

When writing a full frame of zeros, if the previous operation was a partial
update, the controller's address window might remain restricted.

Could pushing a full screen of data simply overwrite that small rectangle
repeatedly while leaving the rest of the display uncleared?

>  }

[ ... ]

> @@ -1356,7 +1457,13 @@ static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs[]
>  		.atomic_check = ssd133x_primary_plane_atomic_check,
>  		.atomic_update = ssd133x_primary_plane_atomic_update,
>  		.atomic_disable = ssd133x_primary_plane_atomic_disable,
> -	}
> +	},
> +	[SSD135X_FAMILY] = {
> +		DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> +		.atomic_check = ssd133x_primary_plane_atomic_check,

[Severity: High]
This isn't a bug introduced by this patch, but is the format conversion state
preallocated during the atomic check phase?

Unlike the other plane check routines, ssd133x_primary_plane_atomic_check()
returns early without calling drm_format_conv_state_reserve().

During the non-blocking commit phase, ssd133x_primary_plane_atomic_update()
calls drm_fb_xrgb8888_to_rgb565be(), which then falls back to dynamically
allocating memory via krealloc with GFP_KERNEL since the state size is 0.

Can this trigger forbidden sleeping allocations in the atomic update path?

> +		.atomic_update = ssd133x_primary_plane_atomic_update,
> +		.atomic_disable = ssd133x_primary_plane_atomic_disable,
> +	},
>  };

[ ... ]

> @@ -1597,6 +1708,31 @@ static void ssd133x_encoder_atomic_enable(struct drm_encoder *encoder,
>  	ssd130x_power_off(ssd130x);
>  }
>  
> +static void ssd135x_encoder_atomic_enable(struct drm_encoder *encoder,
> +					  struct drm_atomic_commit *state)
> +{
> +	struct drm_device *drm = encoder->dev;
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> +	int ret;
> +
> +	ret = ssd130x_power_on(ssd130x);

[Severity: High]
This is a pre-existing issue in the driver, but does this newly added callback
also execute sleepable kernel functions from an atomic context?

The DRM subsystem rules state that encoder atomic enable and disable callbacks
run in an atomic context where sleeping is forbidden. This newly added
callback calls ssd130x_power_on(), which in turn invokes regulator_enable()
and pwm_apply_might_sleep() (via ssd130x_pwm_enable()). It also calls
backlight_enable() later in this function, which acquires a mutex.

Could these sleeping functions cause kernel warnings or deadlocks when called
from the atomic commit path?

> +	if (ret)
> +		return;
> +
> +	ret = ssd135x_init(ssd130x);
> +	if (ret)
> +		goto power_off;
> +
> +	ssd130x_write_cmd(ssd130x, 1, SSD13XX_DISPLAY_ON);
> +
> +	backlight_enable(ssd130x->bl_dev);
> +
> +	return;
> +
> +power_off:
> +	ssd130x_power_off(ssd130x);

[Severity: High]
This is a pre-existing issue, but what happens during the teardown sequence if
ssd130x_pwm_enable() previously failed?

If pwm_get() fails in ssd130x_pwm_enable(), it returns an error pointer which
is stored in ssd130x->pwm. The atomic enable aborts, but because it has a void
return type, the DRM core treats the encoder as successfully enabled.

When the display is later disabled or the power_off label is reached,
ssd130x_power_off() unconditionally calls pwm_disable(ssd130x->pwm).

Since pwm_disable() does not handle error pointers, could this dereference the
invalid error pointer and cause a kernel panic?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260704080925.75113-1-amit.barzilai22@gmail.com?part=3