Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] amdxdna_cmd_set_error() writes the error state to the wrong chain command BO by hardcoding index 0. -- commit 5dc0906b3cc41016d562034c7287f924681a97d7 Author: Taimuraz Kaitmazov <[email protected]> accel/amdxdna: check the command chain payload before using it This commit updates amdxdna_cmd_get_payload() to make the size parameter mandatory and introduces amdxdna_cmd_get_chain() to ensure the command count fits within the BO before returning the chain payload. > diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c > b/drivers/accel/amdxdna/amdxdna_ctx.c > index 888e857ec5582..caf7295a2b5b9 100644 > --- a/drivers/accel/amdxdna/amdxdna_ctx.c > +++ b/drivers/accel/amdxdna/amdxdna_ctx.c [ ... ] > @@ -177,8 +199,13 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo, > cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state); > > if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) { > - cc = amdxdna_cmd_get_payload(abo, NULL); > - cc->error_index = (cmd_idx < cc->command_count) ? cmd_idx : 0; > + u32 ccnt; > + > + cc = amdxdna_cmd_get_chain(abo, &ccnt); > + if (!cc || !ccnt) > + return -EINVAL; > + > + cc->error_index = (cmd_idx < ccnt) ? cmd_idx : 0; > abo = amdxdna_gem_get_obj(client, cc->data[0], > AMDXDNA_BO_SHARE); [Severity: High] This isn't a bug introduced by this patch, but does this logic write the error state to the wrong command BO? In amdxdna_cmd_set_error(), the code correctly assigns cc->error_index based on cmd_idx, but then it hardcodes cc->data[0] when looking up the BO. If a chained command fails at an index greater than 0, won't this clobber the first command in the chain with error data and completely ignore the actual failing command? > if (!abo) > return -EINVAL; > cmd = amdxdna_gem_vmap(abo); > if (!cmd) { -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
