From: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
To: Arnd Bergmann <arnd@arndb.de>, Jiri Kosina <jikos@kernel.org>
Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/5] HID: intel_ish-hid: clarify locking in client code
Date: Fri, 26 May 2017 13:58:19 -0700 [thread overview]
Message-ID: <1495832299.79527.3.camel@linux.intel.com> (raw)
In-Reply-To: <20170518202144.3482304-3-arnd@arndb.de>
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");
next prev parent reply other threads:[~2017-05-26 20:58 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1495832299.79527.3.camel@linux.intel.com \
--to=srinivas.pandruvada@linux.intel.com \
--cc=arnd@arndb.de \
--cc=jikos@kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).