* HID: intel_ish-hid: various cleanups
@ 2017-05-18 20:21 Arnd Bergmann
2017-05-18 20:21 ` [PATCH v2 1/5] HID: intel_ish-hid: fix potential uninitialized data usage Arnd Bergmann
` (6 more replies)
0 siblings, 7 replies; 19+ messages in thread
From: Arnd Bergmann @ 2017-05-18 20:21 UTC (permalink / raw)
To: Srinivas Pandruvada, Jiri Kosina; +Cc: linux-input, linux-kernel, arnd
I ran into a warning message during randconfig testing and spent way
too much time figuring out how to best address it. One thing led to
another and I ended up with a 5 patch series.
Unforunately I screwed up the first version of the series and
had to replace the first patch, but the second version should
be much better.
Please have a look at the first patch separately, it might fix an
important bug and need backporting to stable kernels, or it might
only address a harmless warning.
For the rest of the patches, please merge for 4.13 unless you
see something wrong.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 1/5] HID: intel_ish-hid: fix potential uninitialized data usage
2017-05-18 20:21 HID: intel_ish-hid: various cleanups Arnd Bergmann
@ 2017-05-18 20:21 ` Arnd Bergmann
2017-05-22 19:17 ` Srinivas Pandruvada
` (2 more replies)
2017-05-18 20:21 ` [PATCH v2 2/5] HID: intel_ish-hid: clarify locking in client code Arnd Bergmann
` (5 subsequent siblings)
6 siblings, 3 replies; 19+ messages in thread
From: Arnd Bergmann @ 2017-05-18 20:21 UTC (permalink / raw)
To: Srinivas Pandruvada, Jiri Kosina; +Cc: linux-input, linux-kernel, arnd
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>
---
drivers/hid/intel-ish-hid/ishtp/client.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c b/drivers/hid/intel-ish-hid/ishtp/client.c
index aad61328f282..78d393e616a4 100644
--- a/drivers/hid/intel-ish-hid/ishtp/client.c
+++ b/drivers/hid/intel-ish-hid/ishtp/client.c
@@ -925,6 +925,7 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
}
if (complete_rb) {
+ cl = complete_rb->cl;
getnstimeofday(&cl->ts_rx);
++cl->recv_msg_cnt_ipc;
ishtp_cl_read_complete(complete_rb);
@@ -1045,6 +1046,7 @@ void recv_ishtp_cl_msg_dma(struct ishtp_device *dev, void *msg,
}
if (complete_rb) {
+ cl = complete_rb->cl;
getnstimeofday(&cl->ts_rx);
++cl->recv_msg_cnt_dma;
ishtp_cl_read_complete(complete_rb);
--
2.9.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 2/5] HID: intel_ish-hid: clarify locking in client code
2017-05-18 20:21 HID: intel_ish-hid: various cleanups Arnd Bergmann
2017-05-18 20:21 ` [PATCH v2 1/5] HID: intel_ish-hid: fix potential uninitialized data usage Arnd Bergmann
@ 2017-05-18 20:21 ` Arnd Bergmann
2017-05-26 20:58 ` Srinivas Pandruvada
2017-05-18 20:21 ` [PATCH v2 3/5] HID: intel_ish-hid: convert timespec to ktime_t Arnd Bergmann
` (4 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Arnd Bergmann @ 2017-05-18 20:21 UTC (permalink / raw)
To: Srinivas Pandruvada, Jiri Kosina; +Cc: linux-input, linux-kernel, arnd
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>
---
drivers/hid/intel-ish-hid/ishtp/client.c | 43 +++++++++++++-------------------
1 file changed, 17 insertions(+), 26 deletions(-)
diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c b/drivers/hid/intel-ish-hid/ishtp/client.c
index 78d393e616a4..f54689ee67e1 100644
--- a/drivers/hid/intel-ish-hid/ishtp/client.c
+++ b/drivers/hid/intel-ish-hid/ishtp/client.c
@@ -803,7 +803,7 @@ void ishtp_cl_send_msg(struct ishtp_device *dev, struct ishtp_cl *cl)
* @ishtp_hdr: Pointer to message header
*
* Receive and dispatch ISHTP client messages. This function executes in ISR
- * context
+ * or work queue context
*/
void recv_ishtp_cl_msg(struct ishtp_device *dev,
struct ishtp_msg_hdr *ishtp_hdr)
@@ -813,7 +813,6 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
struct ishtp_cl_rb *new_rb;
unsigned char *buffer = NULL;
struct ishtp_cl_rb *complete_rb = NULL;
- unsigned long dev_flags;
unsigned long flags;
int rb_count;
@@ -828,7 +827,7 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
goto eoi;
}
- spin_lock_irqsave(&dev->read_list_spinlock, dev_flags);
+ spin_lock_irqsave(&dev->read_list_spinlock, flags);
rb_count = -1;
list_for_each_entry(rb, &dev->read_list.list, list) {
++rb_count;
@@ -840,8 +839,7 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
/* If no Rx buffer is allocated, disband the rb */
if (rb->buffer.size == 0 || rb->buffer.data == NULL) {
- spin_unlock_irqrestore(&dev->read_list_spinlock,
- dev_flags);
+ spin_unlock_irqrestore(&dev->read_list_spinlock, flags);
dev_err(&cl->device->dev,
"Rx buffer is not allocated.\n");
list_del(&rb->list);
@@ -857,8 +855,7 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
* back FC, so communication will be stuck anyway)
*/
if (rb->buffer.size < ishtp_hdr->length + rb->buf_idx) {
- spin_unlock_irqrestore(&dev->read_list_spinlock,
- dev_flags);
+ spin_unlock_irqrestore(&dev->read_list_spinlock, flags);
dev_err(&cl->device->dev,
"message overflow. size %d len %d idx %ld\n",
rb->buffer.size, ishtp_hdr->length,
@@ -884,14 +881,13 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
* the whole msg arrived, send a new FC, and add a new
* rb buffer for the next coming msg
*/
- spin_lock_irqsave(&cl->free_list_spinlock, flags);
+ spin_lock(&cl->free_list_spinlock);
if (!list_empty(&cl->free_rb_list.list)) {
new_rb = list_entry(cl->free_rb_list.list.next,
struct ishtp_cl_rb, list);
list_del_init(&new_rb->list);
- spin_unlock_irqrestore(&cl->free_list_spinlock,
- flags);
+ spin_unlock(&cl->free_list_spinlock);
new_rb->cl = cl;
new_rb->buf_idx = 0;
INIT_LIST_HEAD(&new_rb->list);
@@ -900,8 +896,7 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
ishtp_hbm_cl_flow_control_req(dev, cl);
} else {
- spin_unlock_irqrestore(&cl->free_list_spinlock,
- flags);
+ spin_unlock(&cl->free_list_spinlock);
}
}
/* One more fragment in message (even if this was last) */
@@ -914,7 +909,7 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
break;
}
- spin_unlock_irqrestore(&dev->read_list_spinlock, dev_flags);
+ spin_unlock_irqrestore(&dev->read_list_spinlock, flags);
/* If it's nobody's message, just read and discard it */
if (!buffer) {
uint8_t rd_msg_buf[ISHTP_RD_MSG_BUF_SIZE];
@@ -941,7 +936,7 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
* @hbm: hbm buffer
*
* Receive and dispatch ISHTP client messages using DMA. This function executes
- * in ISR context
+ * in ISR or work queue context
*/
void recv_ishtp_cl_msg_dma(struct ishtp_device *dev, void *msg,
struct dma_xfer_hbm *hbm)
@@ -951,10 +946,10 @@ void recv_ishtp_cl_msg_dma(struct ishtp_device *dev, void *msg,
struct ishtp_cl_rb *new_rb;
unsigned char *buffer = NULL;
struct ishtp_cl_rb *complete_rb = NULL;
- unsigned long dev_flags;
unsigned long flags;
- spin_lock_irqsave(&dev->read_list_spinlock, dev_flags);
+ spin_lock_irqsave(&dev->read_list_spinlock, flags);
+
list_for_each_entry(rb, &dev->read_list.list, list) {
cl = rb->cl;
if (!cl || !(cl->host_client_id == hbm->host_client_id &&
@@ -966,8 +961,7 @@ void recv_ishtp_cl_msg_dma(struct ishtp_device *dev, void *msg,
* If no Rx buffer is allocated, disband the rb
*/
if (rb->buffer.size == 0 || rb->buffer.data == NULL) {
- spin_unlock_irqrestore(&dev->read_list_spinlock,
- dev_flags);
+ spin_unlock_irqrestore(&dev->read_list_spinlock, flags);
dev_err(&cl->device->dev,
"response buffer is not allocated.\n");
list_del(&rb->list);
@@ -983,8 +977,7 @@ void recv_ishtp_cl_msg_dma(struct ishtp_device *dev, void *msg,
* back FC, so communication will be stuck anyway)
*/
if (rb->buffer.size < hbm->msg_length) {
- spin_unlock_irqrestore(&dev->read_list_spinlock,
- dev_flags);
+ spin_unlock_irqrestore(&dev->read_list_spinlock, flags);
dev_err(&cl->device->dev,
"message overflow. size %d len %d idx %ld\n",
rb->buffer.size, hbm->msg_length, rb->buf_idx);
@@ -1008,14 +1001,13 @@ void recv_ishtp_cl_msg_dma(struct ishtp_device *dev, void *msg,
* the whole msg arrived, send a new FC, and add a new
* rb buffer for the next coming msg
*/
- spin_lock_irqsave(&cl->free_list_spinlock, flags);
+ spin_lock(&cl->free_list_spinlock);
if (!list_empty(&cl->free_rb_list.list)) {
new_rb = list_entry(cl->free_rb_list.list.next,
struct ishtp_cl_rb, list);
list_del_init(&new_rb->list);
- spin_unlock_irqrestore(&cl->free_list_spinlock,
- flags);
+ spin_unlock(&cl->free_list_spinlock);
new_rb->cl = cl;
new_rb->buf_idx = 0;
INIT_LIST_HEAD(&new_rb->list);
@@ -1024,8 +1016,7 @@ void recv_ishtp_cl_msg_dma(struct ishtp_device *dev, void *msg,
ishtp_hbm_cl_flow_control_req(dev, cl);
} else {
- spin_unlock_irqrestore(&cl->free_list_spinlock,
- flags);
+ spin_unlock(&cl->free_list_spinlock);
}
/* One more fragment in message (this is always last) */
@@ -1038,7 +1029,7 @@ void recv_ishtp_cl_msg_dma(struct ishtp_device *dev, void *msg,
break;
}
- spin_unlock_irqrestore(&dev->read_list_spinlock, dev_flags);
+ spin_unlock_irqrestore(&dev->read_list_spinlock, flags);
/* If it's nobody's message, just read and discard it */
if (!buffer) {
dev_err(dev->devc, "Dropped Rx (DMA) msg - no request\n");
--
2.9.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 3/5] HID: intel_ish-hid: convert timespec to ktime_t
2017-05-18 20:21 HID: intel_ish-hid: various cleanups Arnd Bergmann
2017-05-18 20:21 ` [PATCH v2 1/5] HID: intel_ish-hid: fix potential uninitialized data usage Arnd Bergmann
2017-05-18 20:21 ` [PATCH v2 2/5] HID: intel_ish-hid: clarify locking in client code Arnd Bergmann
@ 2017-05-18 20:21 ` Arnd Bergmann
2017-05-26 20:58 ` Srinivas Pandruvada
2017-05-18 20:21 ` [PATCH v2 4/5] HID: intel_ish-hid: fix format string for size_t Arnd Bergmann
` (3 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Arnd Bergmann @ 2017-05-18 20:21 UTC (permalink / raw)
To: Srinivas Pandruvada, Jiri Kosina; +Cc: linux-input, linux-kernel, arnd
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>
---
drivers/hid/intel-ish-hid/ipc/ipc.c | 15 ++++-----------
drivers/hid/intel-ish-hid/ishtp/client.c | 4 ++--
drivers/hid/intel-ish-hid/ishtp/client.h | 6 +++---
drivers/hid/intel-ish-hid/ishtp/hbm.c | 11 ++++-------
4 files changed, 13 insertions(+), 23 deletions(-)
diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index 842d8416a7a6..9a60ec13cb10 100644
--- a/drivers/hid/intel-ish-hid/ipc/ipc.c
+++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
@@ -296,17 +296,12 @@ static int write_ipc_from_queue(struct ishtp_device *dev)
/* If sending MNG_SYNC_FW_CLOCK, update clock again */
if (IPC_HEADER_GET_PROTOCOL(doorbell_val) == IPC_PROTOCOL_MNG &&
IPC_HEADER_GET_MNG_CMD(doorbell_val) == MNG_SYNC_FW_CLOCK) {
- struct timespec ts_system;
- struct timeval tv_utc;
- uint64_t usec_system, usec_utc;
+ uint64_t usec_system, usec_utc;
struct ipc_time_update_msg time_update;
struct time_sync_format ts_format;
- get_monotonic_boottime(&ts_system);
- do_gettimeofday(&tv_utc);
- usec_system = (timespec_to_ns(&ts_system)) / NSEC_PER_USEC;
- usec_utc = (uint64_t)tv_utc.tv_sec * 1000000 +
- ((uint32_t)tv_utc.tv_usec);
+ usec_system = ktime_to_us(ktime_get_boottime());
+ usec_utc = ktime_to_us(ktime_get_real());
ts_format.ts1_source = HOST_SYSTEM_TIME_USEC;
ts_format.ts2_source = HOST_UTC_TIME_USEC;
ts_format.reserved = 0;
@@ -575,15 +570,13 @@ static void fw_reset_work_fn(struct work_struct *unused)
static void _ish_sync_fw_clock(struct ishtp_device *dev)
{
static unsigned long prev_sync;
- struct timespec ts;
uint64_t usec;
if (prev_sync && jiffies - prev_sync < 20 * HZ)
return;
prev_sync = jiffies;
- get_monotonic_boottime(&ts);
- usec = (timespec_to_ns(&ts)) / NSEC_PER_USEC;
+ usec = ktime_to_us(ktime_get_boottime());
ipc_send_mng_msg(dev, MNG_SYNC_FW_CLOCK, &usec, sizeof(uint64_t));
}
diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c b/drivers/hid/intel-ish-hid/ishtp/client.c
index f54689ee67e1..007443ef5fca 100644
--- a/drivers/hid/intel-ish-hid/ishtp/client.c
+++ b/drivers/hid/intel-ish-hid/ishtp/client.c
@@ -921,7 +921,7 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
if (complete_rb) {
cl = complete_rb->cl;
- getnstimeofday(&cl->ts_rx);
+ cl->ts_rx = ktime_get();
++cl->recv_msg_cnt_ipc;
ishtp_cl_read_complete(complete_rb);
}
@@ -1038,7 +1038,7 @@ void recv_ishtp_cl_msg_dma(struct ishtp_device *dev, void *msg,
if (complete_rb) {
cl = complete_rb->cl;
- getnstimeofday(&cl->ts_rx);
+ cl->ts_rx = ktime_get();
++cl->recv_msg_cnt_dma;
ishtp_cl_read_complete(complete_rb);
}
diff --git a/drivers/hid/intel-ish-hid/ishtp/client.h b/drivers/hid/intel-ish-hid/ishtp/client.h
index 444d069c2ed4..79eade547f5d 100644
--- a/drivers/hid/intel-ish-hid/ishtp/client.h
+++ b/drivers/hid/intel-ish-hid/ishtp/client.h
@@ -118,9 +118,9 @@ struct ishtp_cl {
unsigned int out_flow_ctrl_cnt;
/* Rx msg ... out FC timing */
- struct timespec ts_rx;
- struct timespec ts_out_fc;
- struct timespec ts_max_fc_delay;
+ ktime_t ts_rx;
+ ktime_t ts_out_fc;
+ ktime_t ts_max_fc_delay;
void *client_data;
};
diff --git a/drivers/hid/intel-ish-hid/ishtp/hbm.c b/drivers/hid/intel-ish-hid/ishtp/hbm.c
index b7213608ce43..ae4a69f7f2f4 100644
--- a/drivers/hid/intel-ish-hid/ishtp/hbm.c
+++ b/drivers/hid/intel-ish-hid/ishtp/hbm.c
@@ -321,13 +321,10 @@ int ishtp_hbm_cl_flow_control_req(struct ishtp_device *dev,
if (!rv) {
++cl->out_flow_ctrl_creds;
++cl->out_flow_ctrl_cnt;
- getnstimeofday(&cl->ts_out_fc);
- if (cl->ts_rx.tv_sec && cl->ts_rx.tv_nsec) {
- struct timespec ts_diff;
-
- ts_diff = timespec_sub(cl->ts_out_fc, cl->ts_rx);
- if (timespec_compare(&ts_diff, &cl->ts_max_fc_delay)
- > 0)
+ cl->ts_out_fc = ktime_get();
+ if (cl->ts_rx) {
+ ktime_t ts_diff = ktime_sub(cl->ts_out_fc, cl->ts_rx);
+ if (ktime_after(ts_diff, cl->ts_max_fc_delay))
cl->ts_max_fc_delay = ts_diff;
}
} else {
--
2.9.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 4/5] HID: intel_ish-hid: fix format string for size_t
2017-05-18 20:21 HID: intel_ish-hid: various cleanups Arnd Bergmann
` (2 preceding siblings ...)
2017-05-18 20:21 ` [PATCH v2 3/5] HID: intel_ish-hid: convert timespec to ktime_t Arnd Bergmann
@ 2017-05-18 20:21 ` Arnd Bergmann
2017-05-22 23:46 ` Srinivas Pandruvada
2017-05-26 20:58 ` Srinivas Pandruvada
2017-05-18 20:21 ` [PATCH v2 5/5] HID: intel_ish-hid: enable compile testing Arnd Bergmann
` (2 subsequent siblings)
6 siblings, 2 replies; 19+ messages in thread
From: Arnd Bergmann @ 2017-05-18 20:21 UTC (permalink / raw)
To: Srinivas Pandruvada, Jiri Kosina; +Cc: linux-input, linux-kernel, arnd
When building for 32-bit architectures, we get a harmless warning:
intel-ish-hid/ishtp-hid-client.c: In function 'process_recv':
intel-ish-hid/ishtp-hid-client.c:139:7: error: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'unsigned int' [-Werror=format=]
This changes the format string to print size_t variables using %zu
instead.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/hid/intel-ish-hid/ishtp-hid-client.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
index 5c643d7a07b2..157b44aacdff 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
+++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
@@ -136,10 +136,9 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
if (1 + sizeof(struct device_info) * i >=
payload_len) {
dev_err(&client_data->cl_device->dev,
- "[hid-ish]: [ENUM_DEVICES]: content size %lu is bigger than payload_len %u\n",
+ "[hid-ish]: [ENUM_DEVICES]: content size %zu is bigger than payload_len %zu\n",
1 + sizeof(struct device_info)
- * i,
- (unsigned int)payload_len);
+ * i, payload_len);
}
if (1 + sizeof(struct device_info) * i >=
--
2.9.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 5/5] HID: intel_ish-hid: enable compile testing
2017-05-18 20:21 HID: intel_ish-hid: various cleanups Arnd Bergmann
` (3 preceding siblings ...)
2017-05-18 20:21 ` [PATCH v2 4/5] HID: intel_ish-hid: fix format string for size_t Arnd Bergmann
@ 2017-05-18 20:21 ` Arnd Bergmann
2017-05-26 20:59 ` Srinivas Pandruvada
2017-05-26 20:57 ` HID: intel_ish-hid: various cleanups Srinivas Pandruvada
2017-05-30 12:13 ` Jiri Kosina
6 siblings, 1 reply; 19+ messages in thread
From: Arnd Bergmann @ 2017-05-18 20:21 UTC (permalink / raw)
To: Srinivas Pandruvada, Jiri Kosina; +Cc: linux-input, linux-kernel, arnd
To increase build coverage, drivers should generally be allowed to
build on other architectures even if they are only used on one
of them.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/hid/intel-ish-hid/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/intel-ish-hid/Kconfig b/drivers/hid/intel-ish-hid/Kconfig
index ea065b3684a2..519e4c8b53c4 100644
--- a/drivers/hid/intel-ish-hid/Kconfig
+++ b/drivers/hid/intel-ish-hid/Kconfig
@@ -1,5 +1,5 @@
menu "Intel ISH HID support"
- depends on X86_64 && PCI
+ depends on (X86_64 || COMPILE_TEST) && PCI
config INTEL_ISH_HID
tristate "Intel Integrated Sensor Hub"
--
2.9.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/5] HID: intel_ish-hid: fix potential uninitialized data usage
2017-05-18 20:21 ` [PATCH v2 1/5] HID: intel_ish-hid: fix potential uninitialized data usage Arnd Bergmann
@ 2017-05-22 19:17 ` Srinivas Pandruvada
2017-05-22 21:33 ` Arnd Bergmann
2017-05-23 22:24 ` Srinivas Pandruvada
2017-05-26 20:58 ` Srinivas Pandruvada
2 siblings, 1 reply; 19+ messages in thread
From: Srinivas Pandruvada @ 2017-05-22 19:17 UTC (permalink / raw)
To: Arnd Bergmann, Jiri Kosina; +Cc: linux-input, linux-kernel
Hi Arnd,
On Thu, 2017-05-18 at 22:21 +0200, Arnd Bergmann wrote:
> 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.
>
Did you get chance to test these changes on a platform with ISH?
Thanks,
Srinivas
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/hid/intel-ish-hid/ishtp/client.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c
> b/drivers/hid/intel-ish-hid/ishtp/client.c
> index aad61328f282..78d393e616a4 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/client.c
> +++ b/drivers/hid/intel-ish-hid/ishtp/client.c
> @@ -925,6 +925,7 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
> }
>
> if (complete_rb) {
> + cl = complete_rb->cl;
> getnstimeofday(&cl->ts_rx);
> ++cl->recv_msg_cnt_ipc;
> ishtp_cl_read_complete(complete_rb);
> @@ -1045,6 +1046,7 @@ void recv_ishtp_cl_msg_dma(struct ishtp_device
> *dev, void *msg,
> }
>
> if (complete_rb) {
> + cl = complete_rb->cl;
> getnstimeofday(&cl->ts_rx);
> ++cl->recv_msg_cnt_dma;
> ishtp_cl_read_complete(complete_rb);
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/5] HID: intel_ish-hid: fix potential uninitialized data usage
2017-05-22 19:17 ` Srinivas Pandruvada
@ 2017-05-22 21:33 ` Arnd Bergmann
0 siblings, 0 replies; 19+ messages in thread
From: Arnd Bergmann @ 2017-05-22 21:33 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: Jiri Kosina, linux-input, Linux Kernel Mailing List
On Mon, May 22, 2017 at 9:17 PM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> Hi Arnd,
> On Thu, 2017-05-18 at 22:21 +0200, Arnd Bergmann wrote:
>> 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.
>>
> Did you get chance to test these changes on a platform with ISH?
No, I only build-tested it and though about the fix carefully.
Arnd
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/5] HID: intel_ish-hid: fix format string for size_t
2017-05-18 20:21 ` [PATCH v2 4/5] HID: intel_ish-hid: fix format string for size_t Arnd Bergmann
@ 2017-05-22 23:46 ` Srinivas Pandruvada
2017-05-23 8:35 ` Arnd Bergmann
2017-05-26 20:58 ` Srinivas Pandruvada
1 sibling, 1 reply; 19+ messages in thread
From: Srinivas Pandruvada @ 2017-05-22 23:46 UTC (permalink / raw)
To: Arnd Bergmann, Jiri Kosina; +Cc: linux-input, linux-kernel
On Thu, 2017-05-18 at 22:21 +0200, Arnd Bergmann wrote:
> When building for 32-bit architectures, we get a harmless warning:
>
> intel-ish-hid/ishtp-hid-client.c: In function 'process_recv':
> intel-ish-hid/ishtp-hid-client.c:139:7: error: format '%lu' expects
> argument of type 'long unsigned int', but argument 3 has type
> 'unsigned int' [-Werror=format=]
>
> This changes the format string to print size_t variables using %zu
> instead.
Is the ordering of patch correct?
ISH config depends on X86_64, so it would not be enabled for 32 bit
build.
So your patch 5/5 will adding "|| COMPILE_TEST", hence it is building.
Thanks,
Srinivas
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/hid/intel-ish-hid/ishtp-hid-client.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> index 5c643d7a07b2..157b44aacdff 100644
> --- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> +++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> @@ -136,10 +136,9 @@ static void process_recv(struct ishtp_cl
> *hid_ishtp_cl, void *recv_buf,
> if (1 + sizeof(struct device_info) *
> i >=
> payload_len) {
> dev_err(&client_data-
> >cl_device->dev,
> - "[hid-ish]:
> [ENUM_DEVICES]: content size %lu is bigger than payload_len %u\n",
> + "[hid-ish]:
> [ENUM_DEVICES]: content size %zu is bigger than payload_len %zu\n",
> 1 + sizeof(struct
> device_info)
> - * i,
> - (unsigned
> int)payload_len);
> + * i, payload_len);
> }
>
> if (1 + sizeof(struct device_info) *
> i >=
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/5] HID: intel_ish-hid: fix format string for size_t
2017-05-22 23:46 ` Srinivas Pandruvada
@ 2017-05-23 8:35 ` Arnd Bergmann
0 siblings, 0 replies; 19+ messages in thread
From: Arnd Bergmann @ 2017-05-23 8:35 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: Jiri Kosina, linux-input, Linux Kernel Mailing List
On Tue, May 23, 2017 at 1:46 AM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> On Thu, 2017-05-18 at 22:21 +0200, Arnd Bergmann wrote:
>> When building for 32-bit architectures, we get a harmless warning:
>>
>> intel-ish-hid/ishtp-hid-client.c: In function 'process_recv':
>> intel-ish-hid/ishtp-hid-client.c:139:7: error: format '%lu' expects
>> argument of type 'long unsigned int', but argument 3 has type
>> 'unsigned int' [-Werror=format=]
>>
>> This changes the format string to print size_t variables using %zu
>> instead.
> Is the ordering of patch correct?
> ISH config depends on X86_64, so it would not be enabled for 32 bit
> build.
> So your patch 5/5 will adding "|| COMPILE_TEST", hence it is building.
Right, that is intentional. Adding ||COMPILE_TEST first would be a
regression by introducing the warning on 32-bit allmodconfig builds.
Arnd
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/5] HID: intel_ish-hid: fix potential uninitialized data usage
2017-05-18 20:21 ` [PATCH v2 1/5] HID: intel_ish-hid: fix potential uninitialized data usage Arnd Bergmann
2017-05-22 19:17 ` Srinivas Pandruvada
@ 2017-05-23 22:24 ` Srinivas Pandruvada
2017-05-24 8:29 ` Arnd Bergmann
2017-05-26 20:58 ` Srinivas Pandruvada
2 siblings, 1 reply; 19+ messages in thread
From: Srinivas Pandruvada @ 2017-05-23 22:24 UTC (permalink / raw)
To: Arnd Bergmann, Jiri Kosina; +Cc: linux-input, linux-kernel
On Thu, 2017-05-18 at 22:21 +0200, Arnd Bergmann wrote:
> 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:
In that case complete_rb should be NULL and it should not go to
if (complete_rb) {
shouldn't enter and cl is not dereferenced.
So not sure why is this warning.
Thanks,
Srinivas
>
> 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>
> ---
> drivers/hid/intel-ish-hid/ishtp/client.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c
> b/drivers/hid/intel-ish-hid/ishtp/client.c
> index aad61328f282..78d393e616a4 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/client.c
> +++ b/drivers/hid/intel-ish-hid/ishtp/client.c
> @@ -925,6 +925,7 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
> }
>
> if (complete_rb) {
> + cl = complete_rb->cl;
> getnstimeofday(&cl->ts_rx);
> ++cl->recv_msg_cnt_ipc;
> ishtp_cl_read_complete(complete_rb);
> @@ -1045,6 +1046,7 @@ void recv_ishtp_cl_msg_dma(struct ishtp_device
> *dev, void *msg,
> }
>
> if (complete_rb) {
> + cl = complete_rb->cl;
> getnstimeofday(&cl->ts_rx);
> ++cl->recv_msg_cnt_dma;
> ishtp_cl_read_complete(complete_rb);
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/5] HID: intel_ish-hid: fix potential uninitialized data usage
2017-05-23 22:24 ` Srinivas Pandruvada
@ 2017-05-24 8:29 ` Arnd Bergmann
0 siblings, 0 replies; 19+ messages in thread
From: Arnd Bergmann @ 2017-05-24 8:29 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: Jiri Kosina, linux-input, Linux Kernel Mailing List
On Wed, May 24, 2017 at 12:24 AM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> On Thu, 2017-05-18 at 22:21 +0200, Arnd Bergmann wrote:
>> 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:
> In that case complete_rb should be NULL and it should not go to
>
> if (complete_rb) {
>
> shouldn't enter and cl is not dereferenced.
>
> So not sure why is this warning.
Correct. Unfortunately gcc cannot track this for complex functions, as
figuring this out is a not a solvable problem in general. I think in this
particular case, it gives up either because of the multiple 'goto' and
'continue' statements inside of the loop that get in the way of a full
analysis, or the 'spin_unlock_irqrestore' makes it forget the state.
I've seen both in the past.
However, just like gcc gets confused easily, a human reader trying
to understand the function will have the same issue, so my workaround
also helps there.
Arnd
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: HID: intel_ish-hid: various cleanups
2017-05-18 20:21 HID: intel_ish-hid: various cleanups Arnd Bergmann
` (4 preceding siblings ...)
2017-05-18 20:21 ` [PATCH v2 5/5] HID: intel_ish-hid: enable compile testing Arnd Bergmann
@ 2017-05-26 20:57 ` Srinivas Pandruvada
2017-05-30 12:13 ` Jiri Kosina
6 siblings, 0 replies; 19+ messages in thread
From: Srinivas Pandruvada @ 2017-05-26 20:57 UTC (permalink / raw)
To: Arnd Bergmann, Jiri Kosina; +Cc: linux-input, linux-kernel
On Thu, 2017-05-18 at 22:21 +0200, Arnd Bergmann wrote:
> I ran into a warning message during randconfig testing and spent way
> too much time figuring out how to best address it. One thing led to
> another and I ended up with a 5 patch series.
>
> Unforunately I screwed up the first version of the series and
> had to replace the first patch, but the second version should
> be much better.
>
> Please have a look at the first patch separately, it might fix an
> important bug and need backporting to stable kernels, or it might
> only address a harmless warning.
This is just addressing some warning.
>
> For the rest of the patches, please merge for 4.13 unless you
> see something wrong.
I will ack individual patches as this patch is not 0/5 patch series.
Thanks,
Srinivas
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/5] HID: intel_ish-hid: fix potential uninitialized data usage
2017-05-18 20:21 ` [PATCH v2 1/5] HID: intel_ish-hid: fix potential uninitialized data usage Arnd Bergmann
2017-05-22 19:17 ` Srinivas Pandruvada
2017-05-23 22:24 ` Srinivas Pandruvada
@ 2017-05-26 20:58 ` Srinivas Pandruvada
2 siblings, 0 replies; 19+ messages in thread
From: Srinivas Pandruvada @ 2017-05-26 20:58 UTC (permalink / raw)
To: Arnd Bergmann, Jiri Kosina; +Cc: linux-input, linux-kernel
On Thu, 2017-05-18 at 22:21 +0200, Arnd Bergmann wrote:
> 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>
> ---
> drivers/hid/intel-ish-hid/ishtp/client.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c
> b/drivers/hid/intel-ish-hid/ishtp/client.c
> index aad61328f282..78d393e616a4 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/client.c
> +++ b/drivers/hid/intel-ish-hid/ishtp/client.c
> @@ -925,6 +925,7 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
> }
>
> if (complete_rb) {
> + cl = complete_rb->cl;
> getnstimeofday(&cl->ts_rx);
> ++cl->recv_msg_cnt_ipc;
> ishtp_cl_read_complete(complete_rb);
> @@ -1045,6 +1046,7 @@ void recv_ishtp_cl_msg_dma(struct ishtp_device
> *dev, void *msg,
> }
>
> if (complete_rb) {
> + cl = complete_rb->cl;
> getnstimeofday(&cl->ts_rx);
> ++cl->recv_msg_cnt_dma;
> ishtp_cl_read_complete(complete_rb);
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/5] HID: intel_ish-hid: clarify locking in client code
2017-05-18 20:21 ` [PATCH v2 2/5] HID: intel_ish-hid: clarify locking in client code Arnd Bergmann
@ 2017-05-26 20:58 ` Srinivas Pandruvada
0 siblings, 0 replies; 19+ messages in thread
From: Srinivas Pandruvada @ 2017-05-26 20:58 UTC (permalink / raw)
To: Arnd Bergmann, Jiri Kosina; +Cc: linux-input, linux-kernel
On Thu, 2017-05-18 at 22:21 +0200, Arnd Bergmann wrote:
> 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>
> ---
> drivers/hid/intel-ish-hid/ishtp/client.c | 43 +++++++++++++---------
> ----------
> 1 file changed, 17 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c
> b/drivers/hid/intel-ish-hid/ishtp/client.c
> index 78d393e616a4..f54689ee67e1 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/client.c
> +++ b/drivers/hid/intel-ish-hid/ishtp/client.c
> @@ -803,7 +803,7 @@ void ishtp_cl_send_msg(struct ishtp_device *dev,
> struct ishtp_cl *cl)
> * @ishtp_hdr: Pointer to message header
> *
> * Receive and dispatch ISHTP client messages. This function
> executes in ISR
> - * context
> + * or work queue context
> */
> void recv_ishtp_cl_msg(struct ishtp_device *dev,
> struct ishtp_msg_hdr *ishtp_hdr)
> @@ -813,7 +813,6 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
> struct ishtp_cl_rb *new_rb;
> unsigned char *buffer = NULL;
> struct ishtp_cl_rb *complete_rb = NULL;
> - unsigned long dev_flags;
> unsigned long flags;
> int rb_count;
>
> @@ -828,7 +827,7 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
> goto eoi;
> }
>
> - spin_lock_irqsave(&dev->read_list_spinlock, dev_flags);
> + spin_lock_irqsave(&dev->read_list_spinlock, flags);
> rb_count = -1;
> list_for_each_entry(rb, &dev->read_list.list, list) {
> ++rb_count;
> @@ -840,8 +839,7 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
>
> /* If no Rx buffer is allocated, disband the rb */
> if (rb->buffer.size == 0 || rb->buffer.data == NULL)
> {
> - spin_unlock_irqrestore(&dev-
> >read_list_spinlock,
> - dev_flags);
> + spin_unlock_irqrestore(&dev-
> >read_list_spinlock, flags);
> dev_err(&cl->device->dev,
> "Rx buffer is not allocated.\n");
> list_del(&rb->list);
> @@ -857,8 +855,7 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
> * back FC, so communication will be stuck anyway)
> */
> if (rb->buffer.size < ishtp_hdr->length + rb-
> >buf_idx) {
> - spin_unlock_irqrestore(&dev-
> >read_list_spinlock,
> - dev_flags);
> + spin_unlock_irqrestore(&dev-
> >read_list_spinlock, flags);
> dev_err(&cl->device->dev,
> "message overflow. size %d len %d
> idx %ld\n",
> rb->buffer.size, ishtp_hdr->length,
> @@ -884,14 +881,13 @@ void recv_ishtp_cl_msg(struct ishtp_device
> *dev,
> * the whole msg arrived, send a new FC, and
> add a new
> * rb buffer for the next coming msg
> */
> - spin_lock_irqsave(&cl->free_list_spinlock,
> flags);
> + spin_lock(&cl->free_list_spinlock);
>
> if (!list_empty(&cl->free_rb_list.list)) {
> new_rb = list_entry(cl-
> >free_rb_list.list.next,
> struct ishtp_cl_rb, list);
> list_del_init(&new_rb->list);
> - spin_unlock_irqrestore(&cl-
> >free_list_spinlock,
> - flags);
> + spin_unlock(&cl-
> >free_list_spinlock);
> new_rb->cl = cl;
> new_rb->buf_idx = 0;
> INIT_LIST_HEAD(&new_rb->list);
> @@ -900,8 +896,7 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
>
> ishtp_hbm_cl_flow_control_req(dev,
> cl);
> } else {
> - spin_unlock_irqrestore(&cl-
> >free_list_spinlock,
> - flags);
> + spin_unlock(&cl-
> >free_list_spinlock);
> }
> }
> /* One more fragment in message (even if this was
> last) */
> @@ -914,7 +909,7 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
> break;
> }
>
> - spin_unlock_irqrestore(&dev->read_list_spinlock, dev_flags);
> + spin_unlock_irqrestore(&dev->read_list_spinlock, flags);
> /* If it's nobody's message, just read and discard it */
> if (!buffer) {
> uint8_t rd_msg_buf[ISHTP_RD_MSG_BUF_SIZE];
> @@ -941,7 +936,7 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
> * @hbm: hbm buffer
> *
> * Receive and dispatch ISHTP client messages using DMA. This
> function executes
> - * in ISR context
> + * in ISR or work queue context
> */
> void recv_ishtp_cl_msg_dma(struct ishtp_device *dev, void *msg,
> struct dma_xfer_hbm *hbm)
> @@ -951,10 +946,10 @@ void recv_ishtp_cl_msg_dma(struct ishtp_device
> *dev, void *msg,
> struct ishtp_cl_rb *new_rb;
> unsigned char *buffer = NULL;
> struct ishtp_cl_rb *complete_rb = NULL;
> - unsigned long dev_flags;
> unsigned long flags;
>
> - spin_lock_irqsave(&dev->read_list_spinlock, dev_flags);
> + spin_lock_irqsave(&dev->read_list_spinlock, flags);
> +
> list_for_each_entry(rb, &dev->read_list.list, list) {
> cl = rb->cl;
> if (!cl || !(cl->host_client_id == hbm-
> >host_client_id &&
> @@ -966,8 +961,7 @@ void recv_ishtp_cl_msg_dma(struct ishtp_device
> *dev, void *msg,
> * If no Rx buffer is allocated, disband the rb
> */
> if (rb->buffer.size == 0 || rb->buffer.data == NULL)
> {
> - spin_unlock_irqrestore(&dev-
> >read_list_spinlock,
> - dev_flags);
> + spin_unlock_irqrestore(&dev-
> >read_list_spinlock, flags);
> dev_err(&cl->device->dev,
> "response buffer is not
> allocated.\n");
> list_del(&rb->list);
> @@ -983,8 +977,7 @@ void recv_ishtp_cl_msg_dma(struct ishtp_device
> *dev, void *msg,
> * back FC, so communication will be stuck anyway)
> */
> if (rb->buffer.size < hbm->msg_length) {
> - spin_unlock_irqrestore(&dev-
> >read_list_spinlock,
> - dev_flags);
> + spin_unlock_irqrestore(&dev-
> >read_list_spinlock, flags);
> dev_err(&cl->device->dev,
> "message overflow. size %d len %d
> idx %ld\n",
> rb->buffer.size, hbm->msg_length,
> rb->buf_idx);
> @@ -1008,14 +1001,13 @@ void recv_ishtp_cl_msg_dma(struct
> ishtp_device *dev, void *msg,
> * the whole msg arrived, send a new FC, and add a
> new
> * rb buffer for the next coming msg
> */
> - spin_lock_irqsave(&cl->free_list_spinlock, flags);
> + spin_lock(&cl->free_list_spinlock);
>
> if (!list_empty(&cl->free_rb_list.list)) {
> new_rb = list_entry(cl-
> >free_rb_list.list.next,
> struct ishtp_cl_rb, list);
> list_del_init(&new_rb->list);
> - spin_unlock_irqrestore(&cl-
> >free_list_spinlock,
> - flags);
> + spin_unlock(&cl->free_list_spinlock);
> new_rb->cl = cl;
> new_rb->buf_idx = 0;
> INIT_LIST_HEAD(&new_rb->list);
> @@ -1024,8 +1016,7 @@ void recv_ishtp_cl_msg_dma(struct ishtp_device
> *dev, void *msg,
>
> ishtp_hbm_cl_flow_control_req(dev, cl);
> } else {
> - spin_unlock_irqrestore(&cl-
> >free_list_spinlock,
> - flags);
> + spin_unlock(&cl->free_list_spinlock);
> }
>
> /* One more fragment in message (this is always
> last) */
> @@ -1038,7 +1029,7 @@ void recv_ishtp_cl_msg_dma(struct ishtp_device
> *dev, void *msg,
> break;
> }
>
> - spin_unlock_irqrestore(&dev->read_list_spinlock, dev_flags);
> + spin_unlock_irqrestore(&dev->read_list_spinlock, flags);
> /* If it's nobody's message, just read and discard it */
> if (!buffer) {
> dev_err(dev->devc, "Dropped Rx (DMA) msg - no
> request\n");
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/5] HID: intel_ish-hid: convert timespec to ktime_t
2017-05-18 20:21 ` [PATCH v2 3/5] HID: intel_ish-hid: convert timespec to ktime_t Arnd Bergmann
@ 2017-05-26 20:58 ` Srinivas Pandruvada
0 siblings, 0 replies; 19+ messages in thread
From: Srinivas Pandruvada @ 2017-05-26 20:58 UTC (permalink / raw)
To: Arnd Bergmann, Jiri Kosina; +Cc: linux-input, linux-kernel
On Thu, 2017-05-18 at 22:21 +0200, Arnd Bergmann wrote:
> 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>
> ---
> drivers/hid/intel-ish-hid/ipc/ipc.c | 15 ++++-----------
> drivers/hid/intel-ish-hid/ishtp/client.c | 4 ++--
> drivers/hid/intel-ish-hid/ishtp/client.h | 6 +++---
> drivers/hid/intel-ish-hid/ishtp/hbm.c | 11 ++++-------
> 4 files changed, 13 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-
> ish-hid/ipc/ipc.c
> index 842d8416a7a6..9a60ec13cb10 100644
> --- a/drivers/hid/intel-ish-hid/ipc/ipc.c
> +++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
> @@ -296,17 +296,12 @@ static int write_ipc_from_queue(struct
> ishtp_device *dev)
> /* If sending MNG_SYNC_FW_CLOCK, update clock again */
> if (IPC_HEADER_GET_PROTOCOL(doorbell_val) ==
> IPC_PROTOCOL_MNG &&
> IPC_HEADER_GET_MNG_CMD(doorbell_val) ==
> MNG_SYNC_FW_CLOCK) {
> - struct timespec ts_system;
> - struct timeval tv_utc;
> - uint64_t usec_system, usec_utc;
> + uint64_t usec_system, usec_utc;
> struct ipc_time_update_msg time_update;
> struct time_sync_format ts_format;
>
> - get_monotonic_boottime(&ts_system);
> - do_gettimeofday(&tv_utc);
> - usec_system = (timespec_to_ns(&ts_system)) /
> NSEC_PER_USEC;
> - usec_utc = (uint64_t)tv_utc.tv_sec * 1000000 +
> - ((uint32_t)tv_utc.tv
> _usec);
> + usec_system = ktime_to_us(ktime_get_boottime());
> + usec_utc = ktime_to_us(ktime_get_real());
> ts_format.ts1_source = HOST_SYSTEM_TIME_USEC;
> ts_format.ts2_source = HOST_UTC_TIME_USEC;
> ts_format.reserved = 0;
> @@ -575,15 +570,13 @@ static void fw_reset_work_fn(struct work_struct
> *unused)
> static void _ish_sync_fw_clock(struct ishtp_device *dev)
> {
> static unsigned long prev_sync;
> - struct timespec ts;
> uint64_t usec;
>
> if (prev_sync && jiffies - prev_sync < 20 * HZ)
> return;
>
> prev_sync = jiffies;
> - get_monotonic_boottime(&ts);
> - usec = (timespec_to_ns(&ts)) / NSEC_PER_USEC;
> + usec = ktime_to_us(ktime_get_boottime());
> ipc_send_mng_msg(dev, MNG_SYNC_FW_CLOCK, &usec,
> sizeof(uint64_t));
> }
>
> diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c
> b/drivers/hid/intel-ish-hid/ishtp/client.c
> index f54689ee67e1..007443ef5fca 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/client.c
> +++ b/drivers/hid/intel-ish-hid/ishtp/client.c
> @@ -921,7 +921,7 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
>
> if (complete_rb) {
> cl = complete_rb->cl;
> - getnstimeofday(&cl->ts_rx);
> + cl->ts_rx = ktime_get();
> ++cl->recv_msg_cnt_ipc;
> ishtp_cl_read_complete(complete_rb);
> }
> @@ -1038,7 +1038,7 @@ void recv_ishtp_cl_msg_dma(struct ishtp_device
> *dev, void *msg,
>
> if (complete_rb) {
> cl = complete_rb->cl;
> - getnstimeofday(&cl->ts_rx);
> + cl->ts_rx = ktime_get();
> ++cl->recv_msg_cnt_dma;
> ishtp_cl_read_complete(complete_rb);
> }
> diff --git a/drivers/hid/intel-ish-hid/ishtp/client.h
> b/drivers/hid/intel-ish-hid/ishtp/client.h
> index 444d069c2ed4..79eade547f5d 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/client.h
> +++ b/drivers/hid/intel-ish-hid/ishtp/client.h
> @@ -118,9 +118,9 @@ struct ishtp_cl {
> unsigned int out_flow_ctrl_cnt;
>
> /* Rx msg ... out FC timing */
> - struct timespec ts_rx;
> - struct timespec ts_out_fc;
> - struct timespec ts_max_fc_delay;
> + ktime_t ts_rx;
> + ktime_t ts_out_fc;
> + ktime_t ts_max_fc_delay;
> void *client_data;
> };
>
> diff --git a/drivers/hid/intel-ish-hid/ishtp/hbm.c
> b/drivers/hid/intel-ish-hid/ishtp/hbm.c
> index b7213608ce43..ae4a69f7f2f4 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/hbm.c
> +++ b/drivers/hid/intel-ish-hid/ishtp/hbm.c
> @@ -321,13 +321,10 @@ int ishtp_hbm_cl_flow_control_req(struct
> ishtp_device *dev,
> if (!rv) {
> ++cl->out_flow_ctrl_creds;
> ++cl->out_flow_ctrl_cnt;
> - getnstimeofday(&cl->ts_out_fc);
> - if (cl->ts_rx.tv_sec && cl->ts_rx.tv_nsec) {
> - struct timespec ts_diff;
> -
> - ts_diff = timespec_sub(cl->ts_out_fc, cl-
> >ts_rx);
> - if (timespec_compare(&ts_diff, &cl-
> >ts_max_fc_delay)
> - > 0)
> + cl->ts_out_fc = ktime_get();
> + if (cl->ts_rx) {
> + ktime_t ts_diff = ktime_sub(cl->ts_out_fc,
> cl->ts_rx);
> + if (ktime_after(ts_diff, cl-
> >ts_max_fc_delay))
> cl->ts_max_fc_delay = ts_diff;
> }
> } else {
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/5] HID: intel_ish-hid: fix format string for size_t
2017-05-18 20:21 ` [PATCH v2 4/5] HID: intel_ish-hid: fix format string for size_t Arnd Bergmann
2017-05-22 23:46 ` Srinivas Pandruvada
@ 2017-05-26 20:58 ` Srinivas Pandruvada
1 sibling, 0 replies; 19+ messages in thread
From: Srinivas Pandruvada @ 2017-05-26 20:58 UTC (permalink / raw)
To: Arnd Bergmann, Jiri Kosina; +Cc: linux-input, linux-kernel
On Thu, 2017-05-18 at 22:21 +0200, Arnd Bergmann wrote:
> When building for 32-bit architectures, we get a harmless warning:
>
> intel-ish-hid/ishtp-hid-client.c: In function 'process_recv':
> intel-ish-hid/ishtp-hid-client.c:139:7: error: format '%lu' expects
> argument of type 'long unsigned int', but argument 3 has type
> 'unsigned int' [-Werror=format=]
>
> This changes the format string to print size_t variables using %zu
> instead.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> drivers/hid/intel-ish-hid/ishtp-hid-client.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> index 5c643d7a07b2..157b44aacdff 100644
> --- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> +++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> @@ -136,10 +136,9 @@ static void process_recv(struct ishtp_cl
> *hid_ishtp_cl, void *recv_buf,
> if (1 + sizeof(struct device_info) *
> i >=
> payload_len) {
> dev_err(&client_data-
> >cl_device->dev,
> - "[hid-ish]:
> [ENUM_DEVICES]: content size %lu is bigger than payload_len %u\n",
> + "[hid-ish]:
> [ENUM_DEVICES]: content size %zu is bigger than payload_len %zu\n",
> 1 + sizeof(struct
> device_info)
> - * i,
> - (unsigned
> int)payload_len);
> + * i, payload_len);
> }
>
> if (1 + sizeof(struct device_info) *
> i >=
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 5/5] HID: intel_ish-hid: enable compile testing
2017-05-18 20:21 ` [PATCH v2 5/5] HID: intel_ish-hid: enable compile testing Arnd Bergmann
@ 2017-05-26 20:59 ` Srinivas Pandruvada
0 siblings, 0 replies; 19+ messages in thread
From: Srinivas Pandruvada @ 2017-05-26 20:59 UTC (permalink / raw)
To: Arnd Bergmann, Jiri Kosina; +Cc: linux-input, linux-kernel
On Thu, 2017-05-18 at 22:21 +0200, Arnd Bergmann wrote:
> To increase build coverage, drivers should generally be allowed to
> build on other architectures even if they are only used on one
> of them.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> drivers/hid/intel-ish-hid/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/intel-ish-hid/Kconfig b/drivers/hid/intel-
> ish-hid/Kconfig
> index ea065b3684a2..519e4c8b53c4 100644
> --- a/drivers/hid/intel-ish-hid/Kconfig
> +++ b/drivers/hid/intel-ish-hid/Kconfig
> @@ -1,5 +1,5 @@
> menu "Intel ISH HID support"
> - depends on X86_64 && PCI
> + depends on (X86_64 || COMPILE_TEST) && PCI
>
> config INTEL_ISH_HID
> tristate "Intel Integrated Sensor Hub"
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: HID: intel_ish-hid: various cleanups
2017-05-18 20:21 HID: intel_ish-hid: various cleanups Arnd Bergmann
` (5 preceding siblings ...)
2017-05-26 20:57 ` HID: intel_ish-hid: various cleanups Srinivas Pandruvada
@ 2017-05-30 12:13 ` Jiri Kosina
6 siblings, 0 replies; 19+ messages in thread
From: Jiri Kosina @ 2017-05-30 12:13 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Srinivas Pandruvada, linux-input, linux-kernel
On Thu, 18 May 2017, Arnd Bergmann wrote:
> I ran into a warning message during randconfig testing and spent way
> too much time figuring out how to best address it. One thing led to
> another and I ended up with a 5 patch series.
>
> Unforunately I screwed up the first version of the series and
> had to replace the first patch, but the second version should
> be much better.
>
> Please have a look at the first patch separately, it might fix an
> important bug and need backporting to stable kernels, or it might
> only address a harmless warning.
>
> For the rest of the patches, please merge for 4.13 unless you
> see something wrong.
All queued in for-4.13/ish. Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2017-05-30 12:13 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-18 20:21 HID: intel_ish-hid: various cleanups Arnd Bergmann
2017-05-18 20:21 ` [PATCH v2 1/5] HID: intel_ish-hid: fix potential uninitialized data usage Arnd Bergmann
2017-05-22 19:17 ` Srinivas Pandruvada
2017-05-22 21:33 ` Arnd Bergmann
2017-05-23 22:24 ` Srinivas Pandruvada
2017-05-24 8:29 ` Arnd Bergmann
2017-05-26 20:58 ` Srinivas Pandruvada
2017-05-18 20:21 ` [PATCH v2 2/5] HID: intel_ish-hid: clarify locking in client code Arnd Bergmann
2017-05-26 20:58 ` Srinivas Pandruvada
2017-05-18 20:21 ` [PATCH v2 3/5] HID: intel_ish-hid: convert timespec to ktime_t Arnd Bergmann
2017-05-26 20:58 ` Srinivas Pandruvada
2017-05-18 20:21 ` [PATCH v2 4/5] HID: intel_ish-hid: fix format string for size_t Arnd Bergmann
2017-05-22 23:46 ` Srinivas Pandruvada
2017-05-23 8:35 ` Arnd Bergmann
2017-05-26 20:58 ` Srinivas Pandruvada
2017-05-18 20:21 ` [PATCH v2 5/5] HID: intel_ish-hid: enable compile testing Arnd Bergmann
2017-05-26 20:59 ` Srinivas Pandruvada
2017-05-26 20:57 ` HID: intel_ish-hid: various cleanups Srinivas Pandruvada
2017-05-30 12:13 ` Jiri Kosina
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.