Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Overloading an action-triggering debugfs file with a configuration state setter creates a severe usability trap. - [Medium] Missing initialization of `gpu_recovery_allowed` in emulation mode unintentionally disables GPU recovery. Pre-existing issues: - [High] Kernel stack memory leak in `gpu_recover_get` during debugfs file read. -- --- Patch [1]: [PATCH v2 1/5] drm/amdgpu: add gpu_recovery_allowed flag to amdgpu_device --- commit 387fc7f831c412d1422444dc6a5d877786192a5b Author: Pierre-Eric Pelloux-Prayer <[email protected]> drm/amdgpu: add gpu_recovery_allowed flag to amdgpu_device Add a per-device boolean to control whether GPU recovery is attempted on a hang, independently of the global amdgpu.gpu_recovery module parameter. It defaults to true and is exposed as a write to the existing amdgpu_gpu_recover debugfs file. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index 520fd59036d57..eba75b41b50a7 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -4080,6 +4080,8 @@ int amdgpu_device_init(struct amdgpu_device *adev, > } > } > > + adev->gpu_recovery_allowed = true; > + > fence_driver_init: [Severity: Medium] Does this skip initializing the flag in emulation mode? If amdgpu_emu_mode == 1, the code earlier in this function jumps directly to fence_driver_init: amdgpu_device_init() { if (amdgpu_emu_mode == 1) { emu_soc_asic_init(adev); goto fence_driver_init; } ... } Since the device structure is zero-allocated, jumping over the assignment will leave gpu_recovery_allowed as false, which inadvertently disables GPU recovery for this device in emulation mode. Should the initialization be moved before the goto or handled at the beginning of the function? [ ... ] > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c > index 3043ad041bb42..707e69d8bb115 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c [ ... ] > static int gpu_recover_get(void *data, u64 *val) > { [Severity: High] This is a pre-existing issue, but does this function leak kernel stack memory? In simple_attr_read(), an uninitialized u64 val is allocated on the stack and passed to the attribute's get callback. gpu_recover_get() returns 0 without writing to val. The uninitialized value is then formatted into a string and copied to userspace. Because this patch converts the file into a configuration knob, users and tooling are much more likely to read it to check its state, making this leak more exposed. [ ... ] > @@ -1001,8 +1005,17 @@ static int gpu_recover_get(void *data, u64 *val) > return 0; > } > > +static int gpu_recover_set(void *data, u64 val) > +{ > + struct amdgpu_device *adev = (struct amdgpu_device *)data; > + > + adev->gpu_recovery_allowed = !!val; > + > + return 0; > +} > + > DEFINE_SHOW_ATTRIBUTE(amdgpu_debugfs_fence_info); > -DEFINE_DEBUGFS_ATTRIBUTE(amdgpu_debugfs_gpu_recover_fops, gpu_recover_get, > NULL, > +DEFINE_DEBUGFS_ATTRIBUTE(amdgpu_debugfs_gpu_recover_fops, gpu_recover_get, > gpu_recover_set, > "%lld\n"); [Severity: High] Does overloading this debugfs file create a usability trap? The patch adds a set callback to configure gpu_recovery_allowed, changing amdgpu_gpu_recover into a dual-purpose configuration file. However, gpu_recover_get unconditionally triggers a full GPU reset when read, rather than returning the current configuration value: gpu_recover_get() { ... if (amdgpu_reset_domain_schedule(adev->reset_domain, &adev->reset_work)) flush_work(&adev->reset_work); return 0; } If a user attempts to read amdgpu_gpu_recover to check if recovery is enabled, they will unintentionally reset the GPU. Would it be safer to use a separate debugfs file for the configuration flag? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
