From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: accel/habanalabs: kzalloc + kcalloc to kzalloc_flex Date: Wed, 11 Mar 2026 12:54:51 +1000 Message-ID: In-Reply-To: <20260311004459.32567-1-rosenp@gmail.com> References: <20260311004459.32567-1-rosenp@gmail.com> <20260311004459.32567-1-rosenp@gmail.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Positive aspects:** - The conversion to a flexible array member is a valid cleanup. It eliminat= es a separate allocation, a separate free, and an error-handling path (`fre= e_cs_cmpl` label). - The `kzalloc_flex` API is used correctly =E2=80=94 the macro takes `(VAR_= OR_TYPE, FAM, COUNT, GFP_FLAGS)`. - The GFP_ATOMIC-then-GFP_KERNEL fallback pattern is preserved. - All users of `cs->jobs_in_queue_cnt` access it as an array, so the flexib= le array member works as a drop-in replacement. **Issues:** 1. **Missing `@jobs_in_queue_cnt` doc comment update.** The kernel-doc comm= ent for `struct hl_cs` at line 2007 describes `jobs_in_queue_cnt` as: ``` * @jobs_in_queue_cnt: per each queue, maintain counter of submitted jobs. ``` This remains valid, but since the member was moved from its original pos= ition (pointer at the top of the struct) to the end (as a FAM), the doc com= ment at `habanalabs.h:2007` should ideally be moved or noted as being at th= e end. Currently the doc comment ordering already has `jobs_in_queue_cnt` l= isted before `ctx` (line 2007 vs 2049), which matched the old layout but no= longer matches the new layout. This is a minor nit =E2=80=94 the comment c= ontent is still correct. 2. **Removed error path may hide a subtle change in behavior.** The origina= l code had a separate `free_cs_cmpl` label in the error path: ```c free_fence: spin_unlock(&ctx->cs_lock); kfree(cs->jobs_in_queue_cnt); free_cs_cmpl: kfree(cs_cmpl); free_cs: kfree(cs); ``` The patch removes the `free_cs_cmpl` label entirely, and the `free_fence= ` path now falls through directly to `kfree(cs_cmpl)`. This is correct beca= use the `jobs_in_queue_cnt` allocation that previously jumped to `free_cs_c= mpl` on failure has been eliminated. The remaining error paths (`free_fence= ` from the cs_lock section, and `free_cs` from cs_cmpl allocation failure) = are properly handled. No issue here. 3. **No `__counted_by` annotation on the FAM.** The flexible array member i= s declared as: ```c u16 jobs_in_queue_cnt[]; ``` Modern kernel style recommends annotating FAMs with `__counted_by(field)= ` where feasible, to enable compile-time and runtime bounds checking. There= doesn't appear to be a direct count field in the struct for this (the coun= t comes from `hdev->asic_prop.max_queues`), so adding `__counted_by` would = require adding a count field to the struct. This is not a blocker but could= be a follow-up improvement. 4. **Struct layout / padding consideration.** The FAM `u16 jobs_in_queue_cn= t[]` is placed after `u8 encaps_signals` at the end of the struct. Since th= e preceding field is `u8` and the FAM element type is `u16`, there will be = 1 byte of padding inserted before the array. This is handled correctly by t= he compiler and `struct_size()` (which `kzalloc_flex` uses internally), so = it's not a bug =E2=80=94 just a minor observation that there's a wasted byt= e. The old layout with the pointer (`u16 *jobs_in_queue_cnt`) at the top of= the struct actually occupied 8 bytes (pointer), so the overall savings are= still positive. **Verdict:** The patch is correct and a nice cleanup. It could use a `__cou= nted_by` annotation on the FAM (possibly as a follow-up if a counter field = is added to the struct), but is otherwise ready to go with minor doc commen= t ordering nit. --- Generated by Claude Code Patch Reviewer