Message ID | 1659574759-30159-1-git-send-email-even.xu@intel.com (mailing list archive) |
---|---|
State | Mainlined |
Commit | e1fa076706209cc447d7a2abd0843a18277e5ef7 |
Delegated to: | Jiri Kosina |
Headers | show |
Series | hid: intel-ish-hid: ishtp: Fix ishtp client sending disordered message | expand |
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.
Thanks Jiri! Best Regards, Even Xu -----Original Message----- From: Jiri Kosina <jikos@kernel.org> Sent: Thursday, August 25, 2022 5:36 PM To: Xu, Even <even.xu@intel.com> Cc: srinivas.pandruvada@linux.intel.com; benjamin.tissoires@redhat.com; linux-input@vger.kernel.org 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
diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c b/drivers/hid/intel-ish-hid/ishtp/client.c index 405e0d5..df0a825 100644 --- a/drivers/hid/intel-ish-hid/ishtp/client.c +++ b/drivers/hid/intel-ish-hid/ishtp/client.c @@ -626,13 +626,14 @@ static void ishtp_cl_read_complete(struct ishtp_cl_rb *rb) } /** - * ipc_tx_callback() - IPC tx callback function + * ipc_tx_send() - IPC tx send function * @prm: Pointer to client device instance * - * Send message over IPC either first time or on callback on previous message - * completion + * Send message over IPC. Message will be split into fragments + * if message size is bigger than IPC FIFO size, and all + * fragments will be sent one by one. */ -static void ipc_tx_callback(void *prm) +static void ipc_tx_send(void *prm) { struct ishtp_cl *cl = prm; struct ishtp_cl_tx_ring *cl_msg; @@ -677,32 +678,41 @@ static void ipc_tx_callback(void *prm) list); rem = cl_msg->send_buf.size - cl->tx_offs; - ishtp_hdr.host_addr = cl->host_client_id; - ishtp_hdr.fw_addr = cl->fw_client_id; - ishtp_hdr.reserved = 0; - pmsg = cl_msg->send_buf.data + cl->tx_offs; + while (rem > 0) { + ishtp_hdr.host_addr = cl->host_client_id; + ishtp_hdr.fw_addr = cl->fw_client_id; + ishtp_hdr.reserved = 0; + pmsg = cl_msg->send_buf.data + cl->tx_offs; + + if (rem <= dev->mtu) { + /* Last fragment or only one packet */ + ishtp_hdr.length = rem; + ishtp_hdr.msg_complete = 1; + /* Submit to IPC queue with no callback */ + ishtp_write_message(dev, &ishtp_hdr, pmsg); + cl->tx_offs = 0; + cl->sending = 0; - if (rem <= dev->mtu) { - ishtp_hdr.length = rem; - ishtp_hdr.msg_complete = 1; - cl->sending = 0; - list_del_init(&cl_msg->list); /* Must be before write */ - spin_unlock_irqrestore(&cl->tx_list_spinlock, tx_flags); - /* Submit to IPC queue with no callback */ - ishtp_write_message(dev, &ishtp_hdr, pmsg); - spin_lock_irqsave(&cl->tx_free_list_spinlock, tx_free_flags); - list_add_tail(&cl_msg->list, &cl->tx_free_list.list); - ++cl->tx_ring_free_size; - spin_unlock_irqrestore(&cl->tx_free_list_spinlock, - tx_free_flags); - } else { - /* Send IPC fragment */ - spin_unlock_irqrestore(&cl->tx_list_spinlock, tx_flags); - cl->tx_offs += dev->mtu; - ishtp_hdr.length = dev->mtu; - ishtp_hdr.msg_complete = 0; - ishtp_send_msg(dev, &ishtp_hdr, pmsg, ipc_tx_callback, cl); + break; + } else { + /* Send ipc fragment */ + ishtp_hdr.length = dev->mtu; + ishtp_hdr.msg_complete = 0; + /* All fregments submitted to IPC queue with no callback */ + ishtp_write_message(dev, &ishtp_hdr, pmsg); + cl->tx_offs += dev->mtu; + rem = cl_msg->send_buf.size - cl->tx_offs; + } } + + list_del_init(&cl_msg->list); + spin_unlock_irqrestore(&cl->tx_list_spinlock, tx_flags); + + spin_lock_irqsave(&cl->tx_free_list_spinlock, tx_free_flags); + list_add_tail(&cl_msg->list, &cl->tx_free_list.list); + ++cl->tx_ring_free_size; + spin_unlock_irqrestore(&cl->tx_free_list_spinlock, + tx_free_flags); } /** @@ -720,7 +730,7 @@ static void ishtp_cl_send_msg_ipc(struct ishtp_device *dev, return; cl->tx_offs = 0; - ipc_tx_callback(cl); + ipc_tx_send(cl); ++cl->send_msg_cnt_ipc; }