Thanks Jiri! Best Regards, Even Xu -----Original Message----- From: Jiri Kosina <jikos@xxxxxxxxxx> Sent: Thursday, August 25, 2022 5:36 PM To: Xu, Even <even.xu@xxxxxxxxx> Cc: srinivas.pandruvada@xxxxxxxxxxxxxxx; benjamin.tissoires@xxxxxxxxxx; linux-input@xxxxxxxxxxxxxxx Subject: Re: [PATCH] hid: intel-ish-hid: ishtp: Fix ishtp client sending disordered message On Thu, 4 Aug 2022, Even Xu wrote: > There is a timing issue captured during ishtp client sending stress tests. > It was observed during stress tests that ISH firmware is getting out > of ordered messages. This is a rare scenario as the current set of ISH > client drivers don't send much data to firmware. But this may not be > the case going forward. > > When message size is bigger than IPC MTU, ishtp splits the message > into fragments and uses serialized async method to send message fragments. > The call stack: > ishtp_cl_send_msg_ipc->ipc_tx_callback(first fregment)-> > ishtp_send_msg(with callback)->write_ipc_to_queue-> > write_ipc_from_queue->callback->ipc_tx_callback(next fregment)...... > > When an ipc write complete interrupt is received, driver also calls > write_ipc_from_queue->ipc_tx_callback in ISR to start sending of next fragment. > > Through ipc_tx_callback uses spin_lock to protect message splitting, > as the serialized sending method will call back to ipc_tx_callback > again, so it doesn't put sending under spin_lock, it causes driver > cannot guarantee all fragments be sent in order. > > Considering this scenario: > ipc_tx_callback just finished a fragment splitting, and not call > ishtp_send_msg yet, there is a write complete interrupt happens, then > ISR->write_ipc_from_queue > ->ipc_tx_callback->ishtp_send_msg->write_ipc_to_queue...... > > Because ISR has higher exec priority than normal thread, this causes > the new fragment be sent out before previous fragment. This disordered > message causes invalid message to firmware. I can imagine that this must have been nightmare to debug :) > The solution is, to send fragments synchronously: Use > ishtp_write_message writing fragments into tx queue directly one by > one, instead of ishtp_send_msg only writing one fragment with > completion callback. As no completion callback be used, so change > ipc_tx_callback to ipc_tx_send. Applied, thank you. -- Jiri Kosina SUSE Labs
