From patchwork Tue May 16 11:09:04 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Arnd Bergmann X-Patchwork-Id: 99863 Delivered-To: patch@linaro.org Received: by 10.182.142.97 with SMTP id rv1csp1824957obb; Tue, 16 May 2017 04:10:12 -0700 (PDT) X-Received: by 10.99.123.70 with SMTP id k6mr10983642pgn.135.1494933012486; Tue, 16 May 2017 04:10:12 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1494933012; cv=none; d=google.com; s=arc-20160816; b=LRy8LJC+HjQnjKdoXVsj+wTmwxFd7JaSnfNXFv+G3B2OwfOUXla7oIOt+lsHceNyza 17Y2scJjcihqWDUEJGiHsDhChHnCb8EykMT1qhZP6aeXBk0OUquYFlMfX6NyFCXFYXLE 7iOakhWgtlLD+qq0KMZ5OECA5EG4AIOsTCf4U1yOha9ZBG0KpA3wWSrECi5GwHcUd+kk +J04yV31eQ/BZsXZ7VvJMxl3dtLoPsplsj/aBSxnHtpr1Rnxmng3Vtla0DxUMas9b85H mmiygOm8VRzFZeq+OHxvClHJk1S3odQwuLm/DqAgOwRNeIaZuWd4aqev9ufqR1ji+BPa C5nA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:references:in-reply-to:message-id:date :subject:cc:to:from:arc-authentication-results; bh=xfphMH6dY/aAi+K6FyU0NegSnBJZBe9c09cMf+oWkqk=; b=swbXLEv7RP39HtKkgQWRqvhpfSJo0rcskNbtf34AIdGUFMdQ8Tk6PDwxRc3903Wetz 4uAuYxIhsKKfGomNQ6fQp/dr8GgMQb4g2Rd8c65crJ65dYlA/1En6uHELd06dsN3NR24 At99sUbVxGsARbmaYqVVrJrI8XxlJJFqjrrxiUMNbe62OMeh/XAcrzW+gNoYURgerGTS oZj3V9dGvN+u1ztQDdpo6tuwezh0EACtkMB5TSdOUjGiRana1CML5PX3n/8Tfk7E9TAG zVY7j+jcj9qD7/PXYKTaSXe9M+WCv0VJdWdDFRk5VmNIdyL3OATBTMzP/33VY5CZg1DS YPeg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id o6si8799373pfk.340.2017.05.16.04.10.11; Tue, 16 May 2017 04:10:12 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752702AbdEPLKE (ORCPT + 25 others); Tue, 16 May 2017 07:10:04 -0400 Received: from mout.kundenserver.de ([212.227.126.134]:54805 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751640AbdEPLJS (ORCPT ); Tue, 16 May 2017 07:09:18 -0400 Received: from wuerfel.lan ([78.42.17.5]) by mrelayeu.kundenserver.de (mreue002 [212.227.15.129]) with ESMTPA (Nemesis) id 0MUhql-1dafvI2ev5-00RH8E; Tue, 16 May 2017 13:09:11 +0200 From: Arnd Bergmann To: Srinivas Pandruvada , Jiri Kosina Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, arnd@arndb.de Subject: [PATCH 2/5] HID: intel_ish-hid: clarify locking in client code Date: Tue, 16 May 2017 13:09:04 +0200 Message-Id: <20170516110907.3545799-3-arnd@arndb.de> X-Mailer: git-send-email 2.9.0 In-Reply-To: <20170516110907.3545799-1-arnd@arndb.de> References: <20170516110907.3545799-1-arnd@arndb.de> X-Provags-ID: V03:K0:MR1ANaxYuK82uXjYA8c6syjz+egSwpMKQiR5lmzAV3I2eD2kuKI UGk9emSByHziwXz0imaj9f39VfRCNOeBUDgOkIE5hBNmY0eLdqhi2seak9bddoo0f6SMYRN Oo+p5krVx51TlPvBY24VJXMIJWVIIJC9XJ258z2Y4Uzofz+giaWY3W9yj6aTyZZSdoo4KEL 6j2kl+CTzq3lsJijn1ipg== X-UI-Out-Filterresults: notjunk:1; V01:K0:TJTr3XxwE0o=:cmBhyZ0bHaOZ8LM1f7AjEz OcK0gKfYBPESP+tY66wCSoqHmBQen5UyYsGECW9uDHl6o5HW/6JQQjZeZop4NbLSxpNxKkH62 MQFnvA9FnoInulJqHD4SSorok9uFms3scPuPOIbZZlIN/KwMM4aiRz3oGIy2VmutvR7qf8G9Y hKOUCXch5YCozVxdS3xb1HglDzdAW7nRyS0gXm+LkHlD8eYhrUAV+5P6s6FTq04D6XafruJGu iw/pgmak6acIp5PeaWBR4UQ7M3AgGb1f4T+ItaWnQEbaxtKAVIWHGuban9HzyrKd8YD8r+WDK IKSoxbGRuhq0WtZ31FQ5oEksUwIJFGpDMhzE1gj0/4IJh9kcSklJh27YbTM/bN1FntRKTnkQ2 xviKEFLNUvvIbEdjTHUYkNjQfvPcRpPU2a1Qw+oe8AbVeQOP3i0usjV/6jojuNX5lwsmyQKIC iE66oOoHXXjhV0eiPiWVOEbrq6GEW4EUlmnePcsS75aWJwMZHmkfqp/84DY4CUaxeZ7ocYU2k jqbHRR51I4zm5fM+KqK1JbV7ck2AcWZSGVYzPMsMNQ7AKLixLnwlKuvTB2ZLanN7RJIsAKN3X HHM/A2T+5JYY+GKKpvL8pnXYnc3yV0QBuS09hY+c62Fqhhz0kffWlhKM9Sicmh174uIuTm+9a Wv7eyG4saFuXr/Pco4x0jv4vwYiaYnRA5tv33Zz1WmOcgHSexuvXibFZm4D+ZwJ45JEo= Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 --- drivers/hid/intel-ish-hid/ishtp/client.c | 46 +++++++++++++------------------- 1 file changed, 18 insertions(+), 28 deletions(-) -- 2.9.0 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");