plat-k3: Add locks for TI-SCI protocol #7089
Conversation
manorit2001
force-pushed
the
k3/tisci-locks
branch
2 times, most recently
from
9649f2f
to
36bd6bf
Compare
Current TI-SCI calls are not protected by any locks. OP-TEE running on multiple threads can end up receiving different message response then the one they sent due to no queuing model. *I/TC: Message with sequence ID <> is not expected Add mutex lock to prevent such issues. Signed-off-by: Manorit Chawdhry <m-chawdhry@ti.com>
manorit2001
force-pushed
the
k3/tisci-locks
branch
2 times, most recently
from
2b2814a
to
4bdc5f7
Compare
To avoid potential race condition, set the message_sequence inside ti_sci_do_xfer itself as the send and receive paths are protected by a mutex and avoid race conditions on message_sequence. Signed-off-by: Manorit Chawdhry <m-chawdhry@ti.com>
manorit2001
force-pushed
the
k3/tisci-locks
branch
4 times, most recently
from
dd07e89
to
e88be30
Compare
Create ti_sci_send_request similar to ti_sci_get_response. Signed-off-by: Manorit Chawdhry <m-chawdhry@ti.com>
These could be good for debugging tracing of TI-SCI messages Signed-off-by: Manorit Chawdhry <m-chawdhry@ti.com>
manorit2001
force-pushed
the
k3/tisci-locks
branch
from
e88be30
to
58cf0c1
Compare
|
@glneo I haven't put the host id check that I had mentioned as started facing some issues while testing. Will debug that separately and open a new PR if required, have refactored it the way you wanted, please review. |
| return 0; | ||
| unlock: | ||
| mutex_unlock(&ti_sci_mutex_lock); | ||
| return ret; |
There was a problem hiding this comment.
By the way and out of the scope of this change: I see k3_sec_proxy_xxx() return TEE_Result values while caller use int. Likely something to be fixed at some point of time.
| @@ -8,6 +8,7 @@ | |||
| */ | |||
|
|
|||
| #include <assert.h> | |||
| #include <kernel/thread.h> | |||
There was a problem hiding this comment.
prefer kernel/mutex.h for mutex support.
| int ret = 0; | ||
|
|
||
| mutex_lock(&ti_sci_mutex_lock); | ||
|
|
||
| hdr = (struct ti_sci_msg_hdr *)msg->buf; | ||
| hdr->seq = ++message_sequence; |
There was a problem hiding this comment.
For consistency in OP-TEE OS implementation, prefer a 2 step implementation and let compiler optimize the thing:
message_sequence++;
hdr->seq = message_sequence;There was a problem hiding this comment.
Could you explain this btw, what does it help in?
| * | ||
| * Return: 0 if all goes well, else appropriate error message | ||
| */ | ||
| static inline int ti_sci_send_request(struct ti_sci_xfer *xfer) |
There was a problem hiding this comment.
Could drop the inline attribute. Not strictly needed. But since there are such inline directive for many of the functions in this source file, maybe my comment is out of the scope of this P-R.
| @@ -96,6 +96,8 @@ static inline int ti_sci_send_request(struct ti_sci_xfer *xfer) | |||
| hdr = (struct ti_sci_msg_hdr *)msg->buf; | |||
| hdr->seq = ++message_sequence; | |||
|
|
|||
| FMSG("Sending %x with seq %u host %u", msg->type, msg->seq, msg->host); | |||
There was a problem hiding this comment.
type is uint16_t, seq and host are uint8_t, hence use PRIu16 and PRIu8. Ditto at line 146.
FMSG("Sending %#"PRIx16" with seq %"PRIu8" host %"PRIu8,
msg->type, msg->seq, msg->host);
There was a problem hiding this comment.
The original line doesn't even compile.. Building with CFG_TEE_CORE_LOG_LEVEL=4 shows that. type, seq, and host are members of hdr, not msg.
Currently TI-SCI layer doesn't have any locks that can lead to race conditions, add locks in appropriate places to avoid race conditions.
Signed-off-by: Manorit Chawdhry m-chawdhry@ti.com