Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] This problem wasn't introduced by this patch, but this patch provides an incomplete fix. It adds a check for `!adev->bios_scratch_reg_offset` in the backlight helper to prevent unmapped MMIO accesses, but fails to add this necessary safety check to several other functions that unconditionally access scratch registers using the identical offset. -- commit 23cd4f591b277fb308823be50c628e3526a8743e Author: Mario Limonciello (AMD) <[email protected]> drm/amdgpu: Check bios_scratch_reg_offset in backlight level helper Ensure adev->bios_scratch_reg_offset is non-zero before accessing scratch registers in amdgpu_atombios_scratch_regs_set_backlight_level() to avoid dereferencing unmapped MMIO offsets. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c > index 27c0dc8f6137a..2d8c2ba04e670 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c > @@ -1622,7 +1622,12 @@ void amdgpu_atombios_scratch_regs_engine_hung(struct > amdgpu_device *adev, > void amdgpu_atombios_scratch_regs_set_backlight_level(struct amdgpu_device > *adev, > u32 backlight_level) > { > - u32 tmp = RREG32(adev->bios_scratch_reg_offset + 2); > + u32 tmp; > + > + if (!adev->bios_scratch_reg_offset) > + return; > + [Severity: High] This is a pre-existing issue, but while this patch adds a safety check here, should we also add similar checks to other functions that access scratch registers using the identical offset? If the firmware info table is missing or fails to parse properly, leaving the offset at 0, these other paths are exposed to arbitrary MMIO writes: In amdgpu_atombios_scratch_regs_engine_hung() (drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c): u32 tmp = RREG32(adev->bios_scratch_reg_offset + 3); In amdgpu_atombios_scratch_need_asic_init() (drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c): u32 tmp = RREG32(adev->bios_scratch_reg_offset + 7); In smu_v11_0_baco_set_state() (drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c): WREG32(adev->bios_scratch_reg_offset + 6, 0); Could this missing coverage lead to corrupting hardware state or triggering machine check exceptions during GPU initialization, GPU reset, and BACO state transitions if the offset is 0? > + tmp = RREG32(adev->bios_scratch_reg_offset + 2); > > tmp &= ~ATOM_S2_CURRENT_BL_LEVEL_MASK; [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
