Skip to content

Commit 19689e7

Browse files
committed
hid: intel-ish-hid: ishtp: Fix ishtp client sending disordered message
``` Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2131760 Upstream status: v6.0-rc7 commit e1fa076 Author: Even Xu <even.xu@intel.com> Date: Thu Aug 4 08:59:19 2022 +0800 hid: intel-ish-hid: ishtp: Fix ishtp client sending disordered message 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> Signed-off-by: Jiri Kosina <jkosina@suse.cz> Signed-off-by: Tony Camuso <tcamuso@redhat.com>
1 parent fae3a59 commit 19689e7

File tree

1 file changed

+39
-29
lines changed

1 file changed

+39
-29
lines changed

drivers/hid/intel-ish-hid/ishtp/client.c

Lines changed: 39 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -626,13 +626,14 @@ static void ishtp_cl_read_complete(struct ishtp_cl_rb *rb)
626626
}
627627

628628
/**
629-
* ipc_tx_callback() - IPC tx callback function
629+
* ipc_tx_send() - IPC tx send function
630630
* @prm: Pointer to client device instance
631631
*
632-
* Send message over IPC either first time or on callback on previous message
633-
* completion
632+
* Send message over IPC. Message will be split into fragments
633+
* if message size is bigger than IPC FIFO size, and all
634+
* fragments will be sent one by one.
634635
*/
635-
static void ipc_tx_callback(void *prm)
636+
static void ipc_tx_send(void *prm)
636637
{
637638
struct ishtp_cl *cl = prm;
638639
struct ishtp_cl_tx_ring *cl_msg;
@@ -677,32 +678,41 @@ static void ipc_tx_callback(void *prm)
677678
list);
678679
rem = cl_msg->send_buf.size - cl->tx_offs;
679680

680-
ishtp_hdr.host_addr = cl->host_client_id;
681-
ishtp_hdr.fw_addr = cl->fw_client_id;
682-
ishtp_hdr.reserved = 0;
683-
pmsg = cl_msg->send_buf.data + cl->tx_offs;
681+
while (rem > 0) {
682+
ishtp_hdr.host_addr = cl->host_client_id;
683+
ishtp_hdr.fw_addr = cl->fw_client_id;
684+
ishtp_hdr.reserved = 0;
685+
pmsg = cl_msg->send_buf.data + cl->tx_offs;
686+
687+
if (rem <= dev->mtu) {
688+
/* Last fragment or only one packet */
689+
ishtp_hdr.length = rem;
690+
ishtp_hdr.msg_complete = 1;
691+
/* Submit to IPC queue with no callback */
692+
ishtp_write_message(dev, &ishtp_hdr, pmsg);
693+
cl->tx_offs = 0;
694+
cl->sending = 0;
684695

685-
if (rem <= dev->mtu) {
686-
ishtp_hdr.length = rem;
687-
ishtp_hdr.msg_complete = 1;
688-
cl->sending = 0;
689-
list_del_init(&cl_msg->list); /* Must be before write */
690-
spin_unlock_irqrestore(&cl->tx_list_spinlock, tx_flags);
691-
/* Submit to IPC queue with no callback */
692-
ishtp_write_message(dev, &ishtp_hdr, pmsg);
693-
spin_lock_irqsave(&cl->tx_free_list_spinlock, tx_free_flags);
694-
list_add_tail(&cl_msg->list, &cl->tx_free_list.list);
695-
++cl->tx_ring_free_size;
696-
spin_unlock_irqrestore(&cl->tx_free_list_spinlock,
697-
tx_free_flags);
698-
} else {
699-
/* Send IPC fragment */
700-
spin_unlock_irqrestore(&cl->tx_list_spinlock, tx_flags);
701-
cl->tx_offs += dev->mtu;
702-
ishtp_hdr.length = dev->mtu;
703-
ishtp_hdr.msg_complete = 0;
704-
ishtp_send_msg(dev, &ishtp_hdr, pmsg, ipc_tx_callback, cl);
696+
break;
697+
} else {
698+
/* Send ipc fragment */
699+
ishtp_hdr.length = dev->mtu;
700+
ishtp_hdr.msg_complete = 0;
701+
/* All fregments submitted to IPC queue with no callback */
702+
ishtp_write_message(dev, &ishtp_hdr, pmsg);
703+
cl->tx_offs += dev->mtu;
704+
rem = cl_msg->send_buf.size - cl->tx_offs;
705+
}
705706
}
707+
708+
list_del_init(&cl_msg->list);
709+
spin_unlock_irqrestore(&cl->tx_list_spinlock, tx_flags);
710+
711+
spin_lock_irqsave(&cl->tx_free_list_spinlock, tx_free_flags);
712+
list_add_tail(&cl_msg->list, &cl->tx_free_list.list);
713+
++cl->tx_ring_free_size;
714+
spin_unlock_irqrestore(&cl->tx_free_list_spinlock,
715+
tx_free_flags);
706716
}
707717

708718
/**
@@ -720,7 +730,7 @@ static void ishtp_cl_send_msg_ipc(struct ishtp_device *dev,
720730
return;
721731

722732
cl->tx_offs = 0;
723-
ipc_tx_callback(cl);
733+
ipc_tx_send(cl);
724734
++cl->send_msg_cnt_ipc;
725735
}
726736

0 commit comments

Comments
 (0)