[v2] hid-sony: Prevent crash when rumble effects are still loaded at USB disconnect

Commit Message

Manuel Reimer June 2, 2016, 5:53 p.m. UTC

On 05/30/2016 09:15 PM, Manuel Reimer wrote:
> It may now actually be possible that the memory has been freed again and
> some event in the queue kicked in after that. It may also be possible
> that the hid driver subsystem doesn't really like this case, that the
> device is stopped, but someone still sends to it.

It definitively seems to be a bad idea to continue to interact with the 
HID device as soon as hid_hw_stop was called.

"Somewhere" while hid_hw_stop is running, the cleanup of loaded force 
feedback events is happening. I don't know if schedule_work will run the 
work asynchronously. If so, then calls could be triggered even after 
hid_hw_stop is already finished.

One possible option would be to add a input_unregister_device into 
sony_remove right before the call to "sony_cancel_work_sync". A call to 
input_unregister_device triggers the cleanup, too and so allows to move 
this to a point earlier in the shutdown procedure. But I don't know if 
this is actually a good idea to do this here.

At the end of this mail is a second attempt for a fix. The idea of this 
is, that after "sony_cancel_work_sync" was called, the worker itself 
shouldn't be able to take any new work. To do this, I set 
worker_initialized back to zero and check for this in a newly introduced 
function sony_schedule_work. This seems to fix the problem. If I pull 
the plug while effects are loaded or playing, then I get one or more of 
my added error messages, but, so far, no more kernel panic.

So finally I am at a point where I need feedback to continue my work. 
Would be great if someone could offer some help.

Signed-off-by: Manuel Reimer <[e-mail]>

hid_device_id *id)
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

--- a/drivers/hid/hid-sony.c	2016-05-13 16:13:00.339346161 +0200
+++ b/drivers/hid/hid-sony.c	2016-06-02 19:33:59.621212336 +0200
@@ -1051,6 +1051,14 @@  struct sony_sc {
  	__u8 led_count;
  };

+static inline void sony_schedule_work(struct sony_sc *sc)
+{
+	if (sc->worker_initialized)
+		schedule_work(&sc->state_worker);
+	else
+		printk(KERN_ERR "hid-sony: Sending to uninitialized device failed!\n");
+}
+
  static __u8 *sixaxis_fixup(struct hid_device *hdev, __u8 *rdesc,
  			     unsigned int *rsize)
  {
@@ -1542,7 +1550,7 @@  static void buzz_set_leds(struct sony_sc
  static void sony_set_leds(struct sony_sc *sc)
  {
  	if (!(sc->quirks & BUZZ_CONTROLLER))
-		schedule_work(&sc->state_worker);
+		sony_schedule_work(sc);
  	else
  		buzz_set_leds(sc);
  }
@@ -1653,7 +1661,7 @@  static int sony_led_blink_set(struct led
  		new_off != drv_data->led_delay_off[n]) {
  		drv_data->led_delay_on[n] = new_on;
  		drv_data->led_delay_off[n] = new_off;
-		schedule_work(&drv_data->state_worker);
+		sony_schedule_work(drv_data);
  	}

  	return 0;
@@ -1963,7 +1971,7 @@  static int sony_play_effect(struct input
  	sc->left = effect->u.rumble.strong_magnitude / 256;
  	sc->right = effect->u.rumble.weak_magnitude / 256;

-	schedule_work(&sc->state_worker);
+	sony_schedule_work(sc);
  	return 0;
  }

@@ -2265,8 +2273,10 @@  static inline void sony_init_output_repo

  static inline void sony_cancel_work_sync(struct sony_sc *sc)
  {
-	if (sc->worker_initialized)
+	if (sc->worker_initialized) {
+		sc->worker_initialized = 0;
  		cancel_work_sync(&sc->state_worker);
+	}
  }

  static int sony_probe(struct hid_device *hdev, const struct