| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
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>
|
|
Different platforms have different DMA capability, on most of
platforms, DMA support cache snooping. But few platforms,
such as ElkhartLake (EHL), don't support cache snooping
which requires cache flush from driver.
So add a hardware level callback to let ishtp driver know if cache
flush is needed.
As most of platform support cache snooping, so driver will not
do cache flush by default, until platform implements this callback
and return true explicitly.
Acked-by: Pandruvada, Srinivas <srinivas.pandruvada@intel.com>
Signed-off-by: Even Xu <even.xu@intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
|
|
Fixes the following W=1 kernel build warning(s):
drivers/hid/intel-ish-hid/ishtp/client.c:121: warning: Function parameter or member 'cl_device' not described in 'ishtp_cl_allocate'
drivers/hid/intel-ish-hid/ishtp/client.c:121: warning: Excess function parameter 'dev' description in 'ishtp_cl_allocate'
Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: linux-input@vger.kernel.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
|
|
Fixes the following W=1 kernel build warning(s):
drivers/hid/intel-ish-hid/ishtp/client.c: In function ‘ishtp_cl_disconnect’:
drivers/hid/intel-ish-hid/ishtp/client.c:266:6: warning: variable ‘err’ set but not used [-Wunused-but-set-variable]
Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: linux-input@vger.kernel.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
|
|
Based on 1 normalized pattern(s):
this program is free software you can redistribute it and or modify
it under the terms and conditions of the gnu general public license
version 2 as published by the free software foundation this program
is distributed in the hope it will be useful but without any
warranty without even the implied warranty of merchantability or
fitness for a particular purpose see the gnu general public license
for more details
extracted by the scancode license scanner the SPDX license identifier
GPL-2.0-only
has been chosen to replace the boilerplate/reference in 263 file(s).
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Allison Randal <allison@lohutok.net>
Reviewed-by: Alexios Zavras <alexios.zavras@intel.com>
Cc: linux-spdx@vger.kernel.org
Link: https://lkml.kernel.org/r/20190529141901.208660670@linutronix.de
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
|
|
Instead of directly accessing members of struct ishtp_cl, create interface
functions to access them.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
|
|
All callers will only use ISHTP_HOST_CLIENT_ID_ANY, so get rid of
option to pass this additional id.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
|
|
ISH clients don't need to access any field of struct ishtp_cl_device. To
avoid this create an interface functions instead where it is required.
In the case of ishtp_cl_allocate(), modify the parameters so that the
clients don't have to dereference.
Clients can also use tracing, here a new interface is added to get the
common trace function pointer, instead of direct call.
The new interface functions defined in one external header file, named
intel-ish-client-if.h. This is the only header files all ISHTP clients
must include.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
|
|
Added two APIs:
- ishtp_cl_get_tx_free_buffer_size:
This returns total size available for a client to queue TX data.
- ishtp_cl_get_tx_free_rings:
This returns total number of TX rings available for a client.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
|
|
The internal accounting uses 'timespec' based time stamps, which is
slightly inefficient and also problematic once we get to the time_t
overflow in 2038.
When communicating to the firmware, we even get an open-coded 64-bit
division that prevents the code from being build-tested on 32-bit
architectures and is inefficient due to the double conversion from
64-bit nanoseconds to seconds+nanoseconds and then microseconds.
This changes the code to use ktime_t instead.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
|
|
I was trying to understand this code while working on a warning
fix and the locking made no sense: spin_lock_irqsave() is pointless
when run inside of an interrupt handler or nested inside of another
spin_lock_irq() or spin_lock_irqsave().
Here it turned out that the comment above the function is wrong,
as both recv_ishtp_cl_msg_dma() and recv_ishtp_cl_msg() can in fact
be called from a work queue rather than an ISR, so we do have to
use the irqsave() version once.
This fixes the comments accordingly, removes the misleading 'dev_flags'
variable and modifies the inner spinlock to not use 'irqsave'.
No functional change is intended, this is just for readability and
it slightly simplifies the object code.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
|
|
gcc points out an uninialized pointer dereference that could happen
if we ever get to recv_ishtp_cl_msg_dma() or recv_ishtp_cl_msg()
with an empty &dev->read_list:
drivers/hid/intel-ish-hid/ishtp/client.c: In function 'recv_ishtp_cl_msg_dma':
drivers/hid/intel-ish-hid/ishtp/client.c:1049:3: error: 'cl' may be used uninitialized in this function [-Werror=maybe-uninitialized]
The warning only appeared in very few randconfig builds, as the
spinlocks tend to prevent gcc from tracing the variables. I only
saw it in configurations that had neither SMP nor LOCKDEP enabled.
As we can see, we only enter the case if 'complete_rb' is non-NULL,
and then 'cl' is known to point to complete_rb->cl. Adding another
initialization to the same pointer is harmless here and makes it
clear to the compiler that the behavior is well-defined.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
|
|
The ISH transport layer (ishtp) is a bi-directional protocol implemented
on the top of PCI based inter processor communication layer. This layer
offers:
- Connection management
- Flow control with the firmware
- Multiple client sessions
- Client message transfer
- Client message reception
- DMA for RX and TX for fast data transfer
Refer to Documentation/hid/intel-ish-hid.txt for
overview of the functionality implemented in this layer.
Original-author: Daniel Drubin <daniel.drubin@intel.com>
Reviewed-and-tested-by: Ooi, Joyce <joyce.ooi@intel.com>
Tested-by: Grant Likely <grant.likely@secretlab.ca>
Tested-by: Rann Bar-On <rb6@duke.edu>
Tested-by: Atri Bhattacharya <badshah400@aim.com>
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
|
