Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Incomplete fix for NULL pointer dereference. The patch adds a local check for `enc_priv` (`dig`) in `atombios_set_encoder_crtc_source()`, but leaves the root cause unaddressed. In `radeon_add_atom_encoder()`, if the memory allocation for `enc_priv` fails (e.g., via `radeon_atombios_get_lvds_info()`, `radeon_atombios_set_dig_info()`, or `radeon_atombios_set_dac_info()`), the error is ignored and the encoder is registered with a NULL `enc_priv`. This broken encoder is then passed to numerous other functions (like `atombios_dig_encoder_setup2`, `atombios_dig_transmitter_setup2`, and `atombios_dac_setup`) which unconditionally dereference `enc_priv` (via `dig` or `dac_info`), leading to guaranteed kernel panics. The proper fix is to handle the allocation failure at the source and abort encoder registration. - [High] The driver automatically defaults the display scaling mode to RMX_FULL for LCD devices, which violates the DRM subsystem guideline on scaling modes. This stretches the image and distorts its aspect ratio (e.g., circles become ovals) instead of preserving it. -- commit 21561a0f42099aab9f6cd586dc86a1e791580aef Author: Alexey Velichayshiy <[email protected]> drm/radeon: Add NULL pointer check for enc_priv in atombios_set_encoder_crtc_source() This commit adds a NULL pointer check for enc_priv in atombios_set_encoder_crtc_source() to prevent a NULL pointer dereference. The issue was reported to be found with SVACE. > diff --git a/drivers/gpu/drm/radeon/atombios_encoders.c > b/drivers/gpu/drm/radeon/atombios_encoders.c > index 3d9f47bc807af..6efb6bd2d344c 100644 > --- a/drivers/gpu/drm/radeon/atombios_encoders.c > +++ b/drivers/gpu/drm/radeon/atombios_encoders.c [ ... ] > @@ -1931,6 +1931,8 @@ atombios_set_encoder_crtc_source(struct drm_encoder > *encoder) > case ENCODER_OBJECT_ID_INTERNAL_UNIPHY3: > case ENCODER_OBJECT_ID_INTERNAL_KLDSCP_LVTMA: > dig = radeon_encoder->enc_priv; > + if (!dig) > + return; [Severity: High] This isn't a bug introduced by this patch, but does this local check leave the root cause unaddressed? In drivers/gpu/drm/radeon/atombios_encoders.c:radeon_add_atom_encoder(), if the memory allocation for enc_priv fails (e.g. via radeon_atombios_set_dig_info()), the error is ignored and the encoder is registered with a NULL enc_priv: radeon_add_atom_encoder() { ... radeon_encoder->enc_priv = radeon_atombios_set_dig_info(radeon_encoder); ... } This broken encoder is then passed to numerous other functions which unconditionally dereference enc_priv, such as: atombios_dig_encoder_setup2() { ... if (dig->dig_encoder == -1) ... } atombios_dig_transmitter_setup2() { ... int dig_encoder = dig->dig_encoder; ... } atombios_dac_setup() { ... switch (dac_info->tv_std) { ... } Since other DRM callbacks will unconditionally dereference it, could we handle the allocation failure at the source and abort encoder registration instead? [Severity: High] This is a pre-existing issue, but does this driver incorrectly default to distorting the aspect ratio for LCD devices? Also in drivers/gpu/drm/radeon/atombios_encoders.c:radeon_add_atom_encoder(): radeon_add_atom_encoder() { ... radeon_encoder->rmx_type = RMX_FULL; ... } Setting the default scaling mode to RMX_FULL stretches the image and distorts its aspect ratio (e.g., circles become ovals). Should this default to RMX_ASPECT instead to preserve the aspect ratio? > switch (dig->dig_encoder) { > case 0: > args.v2.ucEncoderID = > ASIC_INT_DIG1_ENCODER_ID; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
