lkml.org 
[lkml]   [2017]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH v2 2/5] HID: intel_ish-hid: clarify locking in client code
    Date
    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
    \
     
     \ /
      Last update: 2017-05-18 22:23    [W:2.200 / U:0.128 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site