amdxdna_cmd::header packs the state, the opcode, the CU mask count and the payload count into one u32 that lives in a BO user space keeps mapped. The driver reads it with plain accesses, and reads it more than once per function: amdxdna_cmd_get_payload() takes num_masks and count from it separately, so the compiler is free to load it twice and the two fields can come from different values.
amdxdna_cmd_set_error() and amdxdna_cmd_set_state() also read, modify and write it in place, which lets the compiler split the store. Read it once into a local with READ_ONCE() and write it back with WRITE_ONCE(). Signed-off-by: Taimuraz Kaitmazov <[email protected]> --- v3: - Dropped 1/2. You are right, EXTRA_CU_MASK is 2 bits, so the payload starts at most 16 bytes in and both reads land well inside a page granular BO. - This patch never depended on it, so it goes standalone. drivers/accel/amdxdna/amdxdna_ctx.c | 14 +++++++++----- drivers/accel/amdxdna/amdxdna_ctx.h | 11 +++++++---- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c index 888e857ec558..02027522210d 100644 --- a/drivers/accel/amdxdna/amdxdna_ctx.c +++ b/drivers/accel/amdxdna/amdxdna_ctx.c @@ -123,10 +123,11 @@ void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size) if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) num_masks = 0; else - num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, cmd->header); + num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, + READ_ONCE(cmd->header)); if (size) { - count = FIELD_GET(AMDXDNA_CMD_COUNT, cmd->header); + count = FIELD_GET(AMDXDNA_CMD_COUNT, READ_ONCE(cmd->header)); if (unlikely(count <= num_masks || count * sizeof(u32) + offsetof(struct amdxdna_cmd, data[0]) > @@ -151,7 +152,7 @@ u32 amdxdna_cmd_get_cu_idx(struct amdxdna_gem_obj *abo) if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) return INVALID_CU_IDX; - num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, cmd->header); + num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, READ_ONCE(cmd->header)); cu_mask = cmd->data; for (i = 0; i < num_masks; i++) { if (cu_mask[i]) @@ -169,12 +170,15 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo, struct amdxdna_client *client = job->hwctx->client; struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo); struct amdxdna_cmd_chain *cc = NULL; + u32 header; if (!cmd) return -ENOMEM; - cmd->header &= ~AMDXDNA_CMD_STATE; - cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state); + header = READ_ONCE(cmd->header); + header &= ~AMDXDNA_CMD_STATE; + header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state); + WRITE_ONCE(cmd->header, header); if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) { cc = amdxdna_cmd_get_payload(abo, NULL); diff --git a/drivers/accel/amdxdna/amdxdna_ctx.h b/drivers/accel/amdxdna/amdxdna_ctx.h index 6e78bab8a02c..fb440a0e2c54 100644 --- a/drivers/accel/amdxdna/amdxdna_ctx.h +++ b/drivers/accel/amdxdna/amdxdna_ctx.h @@ -169,19 +169,22 @@ amdxdna_cmd_get_op(struct amdxdna_gem_obj *abo) if (!cmd) return ERT_INVALID_CMD; - return FIELD_GET(AMDXDNA_CMD_OPCODE, cmd->header); + return FIELD_GET(AMDXDNA_CMD_OPCODE, READ_ONCE(cmd->header)); } static inline void amdxdna_cmd_set_state(struct amdxdna_gem_obj *abo, enum ert_cmd_state s) { struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo); + u32 header; if (!cmd) return; - cmd->header &= ~AMDXDNA_CMD_STATE; - cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, s); + header = READ_ONCE(cmd->header); + header &= ~AMDXDNA_CMD_STATE; + header |= FIELD_PREP(AMDXDNA_CMD_STATE, s); + WRITE_ONCE(cmd->header, header); } static inline enum ert_cmd_state @@ -192,7 +195,7 @@ amdxdna_cmd_get_state(struct amdxdna_gem_obj *abo) if (!cmd) return ERT_CMD_STATE_INVALID; - return FIELD_GET(AMDXDNA_CMD_STATE, cmd->header); + return FIELD_GET(AMDXDNA_CMD_STATE, READ_ONCE(cmd->header)); } void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size); -- 2.55.0
