[PATCH 0/5] HID: intel_ish-hid: various cleanups

[PATCH 0/5] HID: intel_ish-hid: various cleanups

[Arnd Bergmann]

Hi,

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.

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.

      Arnd

 [PATCH 1/5] HID: intel_ish-hid: fix potential uninitialized data
 [PATCH 2/5] HID: intel_ish-hid: clarify locking in client code
 [PATCH 3/5] HID: intel_ish-hid: convert timespec to ktime_t
 [PATCH 4/5] HID: intel_ish-hid: fix format string for size_t
 [PATCH 5/5] HID: intel_ish-hid: enable compile testing

[PATCH 1/5] HID: intel_ish-hid: fix potential uninitialized data usage

[Arnd Bergmann]

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.

I have not been able to figure out whether this case can happen in
practice, but it's better to be defensive here and handle the case
explicitly by returning from the function.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/hid/intel-ish-hid/ishtp/client.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c b/drivers/hid/intel-ish-hid/ishtp/client.c
index aad61328f282..69c9d43612ec 100644
--- a/drivers/hid/intel-ish-hid/ishtp/client.c
+++ b/drivers/hid/intel-ish-hid/ishtp/client.c
@@ -829,6 +829,11 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
 	}
 
 	spin_lock_irqsave(&dev->read_list_spinlock, dev_flags);
+	if (list_empty(&dev->read_list.list)) {
+		spin_unlock_irqrestore(&dev->read_list_spinlock, dev_flags);
+		goto eoi;
+	}
+
 	rb_count = -1;
 	list_for_each_entry(rb, &dev->read_list.list, list) {
 		++rb_count;
@@ -954,6 +959,11 @@ void recv_ishtp_cl_msg_dma(struct ishtp_device *dev, void *msg,
 	unsigned long	flags;
 
 	spin_lock_irqsave(&dev->read_list_spinlock, dev_flags);
+	if (list_empty(&dev->read_list.list)) {
+		spin_unlock_irqrestore(&dev->read_list_spinlock, dev_flags);
+		goto eoi;
+	}
+
 	list_for_each_entry(rb, &dev->read_list.list, list) {
 		cl = rb->cl;
 		if (!cl || !(cl->host_client_id == hbm->host_client_id &&
-- 
2.9.0

[PATCH 2/5] HID: intel_ish-hid: clarify locking in client code

[Arnd Bergmann]

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 | 46 +++++++++++++-------------------
 1 file changed, 18 insertions(+), 28 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c b/drivers/hid/intel-ish-hid/ishtp/client.c
index 69c9d43612ec..a4511ac8a87f 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,9 +827,9 @@ 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);
 	if (list_empty(&dev->read_list.list)) {
-		spin_unlock_irqrestore(&dev->read_list_spinlock, dev_flags);
+		spin_unlock_irqrestore(&dev->read_list_spinlock, flags);
 		goto eoi;
 	}
 
@@ -845,8 +844,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);
@@ -862,8 +860,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,
@@ -889,14 +886,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);
@@ -905,8 +901,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) */
@@ -919,7 +914,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];
@@ -945,7 +940,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)
@@ -955,12 +950,11 @@ 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);
 	if (list_empty(&dev->read_list.list)) {
-		spin_unlock_irqrestore(&dev->read_list_spinlock, dev_flags);
+		spin_unlock_irqrestore(&dev->read_list_spinlock, flags);
 		goto eoi;
 	}
 
@@ -975,8 +969,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);
@@ -992,8 +985,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);
@@ -1017,14 +1009,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);
@@ -1033,8 +1024,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) */
@@ -1047,7 +1037,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

[PATCH 3/5] HID: intel_ish-hid: convert timespec to ktime_t

[Arnd Bergmann]

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 a4511ac8a87f..6cb78c29d111 100644
--- a/drivers/hid/intel-ish-hid/ishtp/client.c
+++ b/drivers/hid/intel-ish-hid/ishtp/client.c
@@ -925,7 +925,7 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,
 	}
 
 	if (complete_rb) {
-		getnstimeofday(&cl->ts_rx);
+		cl->ts_rx = ktime_get();
 		++cl->recv_msg_cnt_ipc;
 		ishtp_cl_read_complete(complete_rb);
 	}
@@ -1045,7 +1045,7 @@ void recv_ishtp_cl_msg_dma(struct ishtp_device *dev, void *msg,
 	}
 
 	if (complete_rb) {
-		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

[PATCH 4/5] HID: intel_ish-hid: fix format string for size_t

[Arnd Bergmann]

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

[PATCH 5/5] HID: intel_ish-hid: enable compile testing

[Arnd Bergmann]

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

Re: [PATCH 1/5] HID: intel_ish-hid: fix potential uninitialized data usage

[Arnd Bergmann]

On Tue, May 16, 2017 at 1:09 PM, Arnd Bergmann <arnd@arndb.de> 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.
>
> I have not been able to figure out whether this case can happen in
> practice, but it's better to be defensive here and handle the case
> explicitly by returning from the function.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Embarrassingly, this did not fix the warning in all cases after all, I ran
into the same warning with the patch applied now. The other patches
in the series are probably still good, but they might not apply without
the first, so I'll have to come up with a better fix here and resend the
series.

      Arnd