[bug report] HID: hid-lg4ff: Protect concurrent access to output HID report

[bug report] HID: hid-lg4ff: Protect concurrent access to output HID report

[Dan Carpenter]

Hello HID developers,

The patch c918fe781fab: "HID: hid-lg4ff: Protect concurrent access to
output HID report" from Apr 8, 2015, leads to the following Smatch
static checker warning:

	drivers/hid/hid-core.c:2142 hid_hw_request()
	warn: sleeping in atomic context

drivers/hid/hid-core.c
    2132  * @hdev: hid device
    2133  * @report: report to send
    2134  * @reqtype: hid request type
    2135  */
    2136 void hid_hw_request(struct hid_device *hdev,
    2137                     struct hid_report *report, int reqtype)
    2138 {
    2139         if (hdev->ll_driver->request)
    2140                 return hdev->ll_driver->request(hdev, report, reqtype);
                                      ^^^^^^^^^^^^^^^^^^^
There are three functions which implement ll_driver->request() in my
allmodconfig build: ishtp_hid_request(), amdtp_hid_request() and
usbhid_request().  The first two can sleep but usbhid_request() does not.

The problem is that a number of callers call hid_hw_request() with a
spinlock held.  It only affect two drivers, picolcd and lg4ff.  It's
possible that for those drivers we know that ->request() is going to
be usbhid_request() but I don't know the code at all.

    2141 
--> 2142         __hid_request(hdev, report, reqtype);
                 ^^^^^^^^^^^^^
This function definitely sleeps.

    2143 }

picolcd_send_and_wait() <- disables preempt
picolcd_reset() <- disables preempt
picolcd_operation_mode_store() <- disables preempt
picolcd_fb_send_tile() <- disables preempt
picolcd_fb_update() <- disables preempt
-> picolcd_fb_reset() <- disables preempt
picolcd_set_brightness() <- disables preempt
picolcd_leds_set() <- disables preempt
picolcd_set_contrast() <- disables preempt

lg4ff_play() <- disables preempt
lg4ff_set_autocenter_default() <- disables preempt
lg4ff_set_autocenter_ffex() <- disables preempt
lg4ff_set_range_g25() <- disables preempt
lg4ff_set_range_dfp() <- disables preempt
lg4ff_switch_compatibility_mode() <- disables preempt

Here is a sample caller:

drivers/hid/hid-picolcd_core.c
    90          mutex_lock(&data->mutex);
    91          spin_lock_irqsave(&data->lock, flags);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Takes a lock.

    92          for (i = k = 0; i < report->maxfield; i++)
    93                  for (j = 0; j < report->field[i]->report_count; j++) {
    94                          hid_set_field(report->field[i], j, k < size ? raw_data[k] : 0);
    95                          k++;
    96                  }
    97          if (data->status & PICOLCD_FAILED) {
    98                  kfree(work);
    99                  work = NULL;
   100          } else {
   101                  data->pending = work;
   102                  hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
                        ^^^^^^^^^^^^^^^

   103                  spin_unlock_irqrestore(&data->lock, flags);
   104                  wait_for_completion_interruptible_timeout(&work->ready, HZ*2);
   105                  spin_lock_irqsave(&data->lock, flags);
   106                  data->pending = NULL;
   107          }
   108          spin_unlock_irqrestore(&data->lock, flags);
   109          mutex_unlock(&data->mutex);
   110          return work;
   111  }

regards,
dan carpenter