Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

HID: fix a lockup regression when using force feedback on a PID device

Commit 8006479c9b75fb6594a7b746af3d7f1fbb68f18f introduced a spinlock in
input_dev->event_lock, which is locked when handling input events.
However, the hid-pidff driver sleeps when handling events as it waits for
reports being sent to the device before changing the report contents
again.
This causes a system lockup when trying to use force feedback with a PID
device, a regression introduced in 2.6.24 and 2.6.23.15.

Fix it by extracting the raw report data from struct hid_report
immediately when hid_submit_report() is called, therefore allowing
drivers to change the contents of struct hid_report immediately without
affecting the already-queued transfer.

In hid-pidff, re-add the removed usbhid_wait_io() to
pidff_erase_effect() instead, to prevent a full report queue from causing
the submission to fail, thus not freeing up device memory.
pidff_erase_effect() is not called while dev->event_lock is held.

Signed-off-by: Anssi Hannula <anssi.hannula@gmail.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>

authored by

Anssi Hannula and committed by
Jiri Kosina
f129ea6d dded364b

+37 -7
+27 -4
drivers/hid/usbhid/hid-core.c
··· 232 232 static int hid_submit_out(struct hid_device *hid) 233 233 { 234 234 struct hid_report *report; 235 + char *raw_report; 235 236 struct usbhid_device *usbhid = hid->driver_data; 236 237 237 - report = usbhid->out[usbhid->outtail]; 238 + report = usbhid->out[usbhid->outtail].report; 239 + raw_report = usbhid->out[usbhid->outtail].raw_report; 238 240 239 - hid_output_report(report, usbhid->outbuf); 240 241 usbhid->urbout->transfer_buffer_length = ((report->size - 1) >> 3) + 1 + (report->id > 0); 241 242 usbhid->urbout->dev = hid_to_usb_dev(hid); 243 + memcpy(usbhid->outbuf, raw_report, usbhid->urbout->transfer_buffer_length); 244 + kfree(raw_report); 242 245 243 246 dbg_hid("submitting out urb\n"); 244 247 ··· 257 254 { 258 255 struct hid_report *report; 259 256 unsigned char dir; 257 + char *raw_report; 260 258 int len; 261 259 struct usbhid_device *usbhid = hid->driver_data; 262 260 263 261 report = usbhid->ctrl[usbhid->ctrltail].report; 262 + raw_report = usbhid->ctrl[usbhid->ctrltail].raw_report; 264 263 dir = usbhid->ctrl[usbhid->ctrltail].dir; 265 264 266 265 len = ((report->size - 1) >> 3) + 1 + (report->id > 0); 267 266 if (dir == USB_DIR_OUT) { 268 - hid_output_report(report, usbhid->ctrlbuf); 269 267 usbhid->urbctrl->pipe = usb_sndctrlpipe(hid_to_usb_dev(hid), 0); 270 268 usbhid->urbctrl->transfer_buffer_length = len; 269 + memcpy(usbhid->ctrlbuf, raw_report, len); 270 + kfree(raw_report); 271 271 } else { 272 272 int maxpacket, padlen; 273 273 ··· 407 401 int head; 408 402 unsigned long flags; 409 403 struct usbhid_device *usbhid = hid->driver_data; 404 + int len = ((report->size - 1) >> 3) + 1 + (report->id > 0); 410 405 411 406 if ((hid->quirks & HID_QUIRK_NOGET) && dir == USB_DIR_IN) 412 407 return; ··· 422 415 return; 423 416 } 424 417 425 - usbhid->out[usbhid->outhead] = report; 418 + usbhid->out[usbhid->outhead].raw_report = kmalloc(len, GFP_ATOMIC); 419 + if (!usbhid->out[usbhid->outhead].raw_report) { 420 + spin_unlock_irqrestore(&usbhid->outlock, flags); 421 + warn("output queueing failed"); 422 + return; 423 + } 424 + hid_output_report(report, usbhid->out[usbhid->outhead].raw_report); 425 + usbhid->out[usbhid->outhead].report = report; 426 426 usbhid->outhead = head; 427 427 428 428 if (!test_and_set_bit(HID_OUT_RUNNING, &usbhid->iofl)) ··· 448 434 return; 449 435 } 450 436 437 + if (dir == USB_DIR_OUT) { 438 + usbhid->ctrl[usbhid->ctrlhead].raw_report = kmalloc(len, GFP_ATOMIC); 439 + if (!usbhid->ctrl[usbhid->ctrlhead].raw_report) { 440 + spin_unlock_irqrestore(&usbhid->ctrllock, flags); 441 + warn("control queueing failed"); 442 + return; 443 + } 444 + hid_output_report(report, usbhid->ctrl[usbhid->ctrlhead].raw_report); 445 + } 451 446 usbhid->ctrl[usbhid->ctrlhead].report = report; 452 447 usbhid->ctrl[usbhid->ctrlhead].dir = dir; 453 448 usbhid->ctrlhead = head;
+3 -2
drivers/hid/usbhid/hid-pidff.c
··· 397 397 effect->u.condition[i].left_saturation); 398 398 pidff_set(&pidff->set_condition[PID_DEAD_BAND], 399 399 effect->u.condition[i].deadband); 400 - usbhid_wait_io(pidff->hid); 401 400 usbhid_submit_report(pidff->hid, pidff->reports[PID_SET_CONDITION], 402 401 USB_DIR_OUT); 403 402 } ··· 511 512 pidff->effect_operation[PID_LOOP_COUNT].value[0] = n; 512 513 } 513 514 514 - usbhid_wait_io(pidff->hid); 515 515 usbhid_submit_report(pidff->hid, pidff->reports[PID_EFFECT_OPERATION], 516 516 USB_DIR_OUT); 517 517 } ··· 546 548 int pid_id = pidff->pid_id[effect_id]; 547 549 548 550 debug("starting to erase %d/%d", effect_id, pidff->pid_id[effect_id]); 551 + /* Wait for the queue to clear. We do not want a full fifo to 552 + prevent the effect removal. */ 553 + usbhid_wait_io(pidff->hid); 549 554 pidff_playback_pid(pidff, pid_id, 0); 550 555 pidff_erase_pid(pidff, pid_id); 551 556
+1 -1
drivers/hid/usbhid/usbhid.h
··· 67 67 spinlock_t ctrllock; /* Control fifo spinlock */ 68 68 69 69 struct urb *urbout; /* Output URB */ 70 - struct hid_report *out[HID_CONTROL_FIFO_SIZE]; /* Output pipe fifo */ 70 + struct hid_output_fifo out[HID_CONTROL_FIFO_SIZE]; /* Output pipe fifo */ 71 71 unsigned char outhead, outtail; /* Output pipe fifo head & tail */ 72 72 char *outbuf; /* Output buffer */ 73 73 dma_addr_t outbuf_dma; /* Output buffer dma */
+6
include/linux/hid.h
··· 388 388 struct hid_control_fifo { 389 389 unsigned char dir; 390 390 struct hid_report *report; 391 + char *raw_report; 392 + }; 393 + 394 + struct hid_output_fifo { 395 + struct hid_report *report; 396 + char *raw_report; 391 397 }; 392 398 393 399 #define HID_CLAIMED_INPUT 1