Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/amdgpu: missing error return in amdgpu_atomfirmware_get_clock_info() --
commit 26bf4f616a4482a2f511b593f7d62e14678802b1 Author: Alex Deucher <[email protected]> drm/amdgpu/atomfirmware: WARN() rather than BUG() This commit removes the BUG() call in amdgpu_atomfirmware_get_clock_info() when encountering an unhandled VBIOS gfx_info table revision on CHIP_NAVI10 and newer ASICs. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c > index c7d32fb216e4e..ce2a4a15d5ecb 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c > @@ -806,8 +806,6 @@ int amdgpu_atomfirmware_get_clock_info(struct > amdgpu_device *adev) > (crev != 6)) { > spll->reference_freq = > le32_to_cpu(gfx_info->v22.rlc_gpu_timer_refclk); > ret = 0; > - } else { > - BUG(); > } > } > } [Severity: Medium] The commit subject says "WARN() rather than BUG()" and the commit message states "Just return an error", but simply removing the else branch neither emits a warning nor returns an error. Earlier in amdgpu_atomfirmware_get_clock_info(), parsing any of the preceding VBIOS tables (firmwareinfo, smu_info, or umc_info) overwrites ret from -EINVAL to 0: drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c:amdgpu_atomfirmware_get_clock_info() { ... if (amdgpu_atom_parse_data_header(mode_info->atom_context, index, NULL, &frev, &crev, &data_offset)) { ... adev->pm.current_sclk = adev->clock.default_sclk; adev->pm.current_mclk = adev->clock.default_mclk; ret = 0; } ... } When amdgpu_device_init() -> amdgpu_atomfirmware_get_clock_info() runs on an ASIC with adev->asic_type >= CHIP_NAVI10 and the VBIOS gfx_info table has an (frev, crev) combination other than frev == 3 or frev == 2 with crev >= 2, won't this now silently return 0 to amdgpu_device_init() while leaving adev->clock.spll.reference_freq unupdated from gfx_info? Should the else branch be kept to call WARN() and set ret = -EINVAL (or return -EINVAL) so amdgpu_device_init() detects the failure? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
