HID: intel_ish-hid: various cleanups

HID: intel_ish-hid: various cleanups

[Arnd Bergmann]

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.

[PATCH v2 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.

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

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

[Srinivas Pandruvada]

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);

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

[Arnd Bergmann]

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

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

[Srinivas Pandruvada]

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);

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

[Arnd Bergmann]

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

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

[Srinivas Pandruvada]

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);

[PATCH v2 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 | 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

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

[Srinivas Pandruvada]

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");

[PATCH v2 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 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

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

[Srinivas Pandruvada]

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 {

[PATCH v2 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

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

[Srinivas Pandruvada]

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 >=

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

[Arnd Bergmann]

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

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

[Srinivas Pandruvada]

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 >=

[PATCH v2 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 v2 5/5] HID: intel_ish-hid: enable compile testing

[Srinivas Pandruvada]

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"

Re: HID: intel_ish-hid: various cleanups

[Srinivas Pandruvada]

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

Re: HID: intel_ish-hid: various cleanups

[Jiri Kosina]

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