public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] accel/rocket: fix ignored return value in rocket_ioctl_submit
@ 2026-05-10 20:00 Kartik Nair
  2026-05-16  6:08 ` Claude review: " Claude Code Review Bot
  2026-05-16  6:08 ` Claude Code Review Bot
  0 siblings, 2 replies; 3+ messages in thread
From: Kartik Nair @ 2026-05-10 20:00 UTC (permalink / raw)
  To: tomeu, ogabbay; +Cc: dri-devel, linux-kernel, Kartik Nair

rocket_ioctl_submit() calls rocket_ioctl_submit_job() in a loop but
ignores its return value. If any job submission fails, the error is
silently dropped and the function returns 0 (success) to userspace,
leaving subsequent jobs potentially in an inconsistent state.

Fix this by propagating the error and breaking out of the loop on
the first failure.

Signed-off-by: Kartik Nair <contact.kartikn@gmail.com>
---
 drivers/accel/rocket/rocket_job.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index ac51bff39..abac26853 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -626,8 +626,11 @@ int rocket_ioctl_submit(struct drm_device *dev, void *data, struct drm_file *fil
 	}
 
 
-	for (i = 0; i < args->job_count; i++)
-		rocket_ioctl_submit_job(dev, file, &jobs[i]);
+	for (i = 0; i < args->job_count; i++) {
+		ret = rocket_ioctl_submit_job(dev, file, &jobs[i]);
+		if (ret)
+			goto exit;
+	}
 
 exit:
 	kvfree(jobs);
-- 
2.50.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Claude review: accel/rocket: fix ignored return value in rocket_ioctl_submit
  2026-05-10 20:00 [PATCH] accel/rocket: fix ignored return value in rocket_ioctl_submit Kartik Nair
  2026-05-16  6:08 ` Claude review: " Claude Code Review Bot
@ 2026-05-16  6:08 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-05-16  6:08 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: accel/rocket: fix ignored return value in rocket_ioctl_submit
Author: Kartik Nair <contact.kartikn@gmail.com>
Patches: 1
Reviewed: 2026-05-16T16:08:43.322827

---

This is a single-patch fix for the `accel/rocket` driver where `rocket_ioctl_submit()` was ignoring the return value of `rocket_ioctl_submit_job()`. The bug is real — without this fix, a failed job submission is silently swallowed and userspace receives a success return code despite the failure. The fix is correct, minimal, and follows the existing error-handling pattern already used in the same function (the `copy_from_user` loop above uses the identical `goto exit` pattern).

**Verdict: The patch looks good.**

One minor observation worth noting, but not necessarily a blocker: when a job fails partway through a multi-job submission, the jobs that were already successfully submitted (pushed to the scheduler) will still execute. This is arguably the correct behavior — `rocket_ioctl_submit_job()` fully owns each job's lifecycle (it cleans up on error, and pushes to the scheduler on success) — but the caller has no way to cancel previously submitted jobs. Whether this is a problem depends on the userspace API contract. The original code had the same issue (it submitted all jobs regardless of failures), so this patch is strictly an improvement.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Claude review: accel/rocket: fix ignored return value in rocket_ioctl_submit
  2026-05-10 20:00 [PATCH] accel/rocket: fix ignored return value in rocket_ioctl_submit Kartik Nair
@ 2026-05-16  6:08 ` Claude Code Review Bot
  2026-05-16  6:08 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-05-16  6:08 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Correctness: Good.** The fix correctly captures the return value of `rocket_ioctl_submit_job()` and breaks out of the loop on the first failure, propagating the error to userspace via the existing `exit` label.

The key code change:

```c
-	for (i = 0; i < args->job_count; i++)
-		rocket_ioctl_submit_job(dev, file, &jobs[i]);
+	for (i = 0; i < args->job_count; i++) {
+		ret = rocket_ioctl_submit_job(dev, file, &jobs[i]);
+		if (ret)
+			goto exit;
+	}
```

**No resource leak concerns.** Looking at `rocket_ioctl_submit_job()` (line 533–588), it is fully self-contained with respect to resource management: on error, it cleans up the scheduler job and puts the job reference before returning. On success, the job is pushed to the scheduler and also properly ref-counted. So skipping subsequent jobs after a failure doesn't leak anything — the `jobs` array is just userspace-copied data (`struct drm_rocket_job` values), freed at the `exit` label via `kvfree(jobs)`.

**Pattern consistency.** The fix mirrors the error-handling style of the `copy_from_user` loop immediately above (lines 617–625), which also uses `goto exit` on failure. This is clean and consistent.

**Commit message.** Clear and accurate. Describes the problem (ignored return value, silent error), the consequence (success returned to userspace, inconsistent state), and the fix (propagate error, break on failure).

**Reviewed-by worthy.** No issues found. This is a straightforward, correct bugfix.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-05-16  6:08 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-10 20:00 [PATCH] accel/rocket: fix ignored return value in rocket_ioctl_submit Kartik Nair
2026-05-16  6:08 ` Claude review: " Claude Code Review Bot
2026-05-16  6:08 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox