linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hid: intel-ish-hid: ishtp: Fix ishtp client sending disordered message
@ 2022-08-04  0:59 Even Xu
  2022-08-25  9:36 ` Jiri Kosina
  0 siblings, 1 reply; 3+ messages in thread
From: Even Xu @ 2022-08-04  0:59 UTC (permalink / raw)
  To: srinivas.pandruvada, jikos, benjamin.tissoires; +Cc: linux-input, Even Xu

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.

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.

Signed-off-by: Even Xu <even.xu@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@intel.com>
---
 drivers/hid/intel-ish-hid/ishtp/client.c | 68 ++++++++++++++++++--------------
 1 file changed, 39 insertions(+), 29 deletions(-)

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;
 }
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] hid: intel-ish-hid: ishtp: Fix ishtp client sending disordered message
  2022-08-04  0:59 [PATCH] hid: intel-ish-hid: ishtp: Fix ishtp client sending disordered message Even Xu
@ 2022-08-25  9:36 ` Jiri Kosina
  2022-08-26  0:39   ` Xu, Even
  0 siblings, 1 reply; 3+ messages in thread
From: Jiri Kosina @ 2022-08-25  9:36 UTC (permalink / raw)
  To: Even Xu; +Cc: srinivas.pandruvada, benjamin.tissoires, linux-input

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* RE: [PATCH] hid: intel-ish-hid: ishtp: Fix ishtp client sending disordered message
  2022-08-25  9:36 ` Jiri Kosina
@ 2022-08-26  0:39   ` Xu, Even
  0 siblings, 0 replies; 3+ messages in thread
From: Xu, Even @ 2022-08-26  0:39 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: srinivas.pandruvada@linux.intel.com,
	benjamin.tissoires@redhat.com, linux-input@vger.kernel.org

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-08-26  0:39 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-04  0:59 [PATCH] hid: intel-ish-hid: ishtp: Fix ishtp client sending disordered message Even Xu
2022-08-25  9:36 ` Jiri Kosina
2022-08-26  0:39   ` Xu, Even

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).