From 51623455c8136f40f47ad98a0532ea70dd1c99e4 Mon Sep 17 00:00:00 2001 From: Kimplul Date: Thu, 24 Mar 2022 13:34:49 +0200 Subject: fix work handler being run after destruction --- hid-tmff2.c | 47 +++++++++++++++++++++++++++++++++++------------ hid-tmff2.h | 6 ++++-- hid-tmt248.c | 22 +++++++++++++--------- hid-tmt300rs.c | 12 +++++------- 4 files changed, 57 insertions(+), 30 deletions(-) diff --git a/hid-tmff2.c b/hid-tmff2.c index d632b09..fe4950d 100644 --- a/hid-tmff2.c +++ b/hid-tmff2.c @@ -43,10 +43,10 @@ static struct tmff2_device_entry *tmff2_from_hdev(struct hid_device *hdev) { struct tmff2_device_entry *tmff2; spin_lock_irqsave(&lock, lock_flags); - if (!(tmff2 = hid_get_drvdata(hdev))) { + + if (!(tmff2 = hid_get_drvdata(hdev))) dev_err(&hdev->dev, "hdev private data not found\n"); - return NULL; - } + spin_unlock_irqrestore(&lock, lock_flags); return tmff2; @@ -56,10 +56,10 @@ static struct tmff2_device_entry *tmff2_from_input(struct input_dev *input_dev) { struct hid_device *hdev; spin_lock_irqsave(&lock, lock_flags); - if (!(hdev = input_get_drvdata(input_dev))) { + + if (!(hdev = input_get_drvdata(input_dev))) dev_err(&input_dev->dev, "input_dev private data not found\n"); - return NULL; - } + spin_unlock_irqrestore(&lock, lock_flags); return tmff2_from_hdev(hdev); @@ -90,6 +90,7 @@ static ssize_t spring_level_store(struct device *dev, static ssize_t spring_level_show(struct device *dev, struct device_attribute *attr, char *buf) { + return scnprintf(buf, PAGE_SIZE, "%u\n", spring_level); } static DEVICE_ATTR_RW(spring_level); @@ -100,6 +101,7 @@ static ssize_t damper_level_store(struct device *dev, unsigned int value; int ret; + ret = kstrtouint(buf, 0, &value); if (ret) { dev_err(dev, "kstrtouint failed at damper_level_store: %i", ret); @@ -119,6 +121,7 @@ static ssize_t damper_level_store(struct device *dev, static ssize_t damper_level_show(struct device *dev, struct device_attribute *attr, char *buf) { + return scnprintf(buf, PAGE_SIZE, "%u\n", damper_level); } static DEVICE_ATTR_RW(damper_level); @@ -129,6 +132,7 @@ static ssize_t friction_level_store(struct device *dev, unsigned int value; int ret; + ret = kstrtouint(buf, 0, &value); if (ret) { dev_err(dev, "kstrtouint failed at friction_level_store: %i", ret); @@ -150,6 +154,7 @@ static ssize_t friction_level_show(struct device *dev, { size_t count; + count = scnprintf(buf, PAGE_SIZE, "%u\n", friction_level); return count; @@ -163,6 +168,7 @@ static ssize_t range_store(struct device *dev, unsigned int value; int ret; + if (!tmff2) return -ENODEV; @@ -182,6 +188,7 @@ static ssize_t range_store(struct device *dev, static ssize_t range_show(struct device *dev, struct device_attribute *attr, char *buf) { + return scnprintf(buf, PAGE_SIZE, "%u\n", range); } static DEVICE_ATTR_RW(range); @@ -193,6 +200,7 @@ static ssize_t alt_mode_store(struct device *dev, unsigned int value; int ret; + if (!tmff2) return -ENODEV; @@ -215,6 +223,7 @@ static ssize_t alt_mode_show(struct device *dev, /* TODO: could be cool to add in something like a small menu that gives * names and corresponding index to modes, or maybe parsing modes * directly? */ + return scnprintf(buf, PAGE_SIZE, "%i\n", alt_mode); } static DEVICE_ATTR_RW(alt_mode); @@ -222,6 +231,7 @@ static DEVICE_ATTR_RW(alt_mode); static void tmff2_set_gain(struct input_dev *dev, uint16_t gain) { struct tmff2_device_entry *tmff2 = tmff2_from_input(dev); + if (!tmff2) return; @@ -237,6 +247,7 @@ static void tmff2_set_gain(struct input_dev *dev, uint16_t gain) static void tmff2_set_autocenter(struct input_dev *dev, uint16_t autocenter) { struct tmff2_device_entry *tmff2 = tmff2_from_input(dev); + if (!tmff2) return; @@ -258,6 +269,7 @@ static void tmff2_work_handler(struct work_struct *w) unsigned long time_now; __u16 effect_length; + if (!tmff2) return; @@ -324,7 +336,7 @@ static void tmff2_work_handler(struct work_struct *w) spin_unlock(&tmff2->lock); } - if (max_count) + if (max_count && tmff2->allow_scheduling) schedule_delayed_work(&tmff2->work, msecs_to_jiffies(timer_msecs)); } @@ -333,6 +345,7 @@ static int tmff2_upload(struct input_dev *dev, { struct tmff2_effect_state *state; struct tmff2_device_entry *tmff2 = tmff2_from_input(dev); + if (!tmff2) return -ENODEV; @@ -362,6 +375,7 @@ static int tmff2_play(struct input_dev *dev, int effect_id, int value) { struct tmff2_effect_state *state; struct tmff2_device_entry *tmff2 = tmff2_from_input(dev); + if (!tmff2) return -ENODEV; @@ -382,7 +396,7 @@ static int tmff2_play(struct input_dev *dev, int effect_id, int value) spin_unlock(&tmff2->lock); - if (!delayed_work_pending(&tmff2->work)) + if (!delayed_work_pending(&tmff2->work) && tmff2->allow_scheduling) schedule_delayed_work(&tmff2->work, 0); return 0; @@ -391,6 +405,7 @@ static int tmff2_play(struct input_dev *dev, int effect_id, int value) static int tmff2_open(struct input_dev *dev) { struct tmff2_device_entry *tmff2 = tmff2_from_input(dev); + if (!tmff2) return -ENODEV; @@ -404,11 +419,15 @@ static int tmff2_open(struct input_dev *dev) static void tmff2_close(struct input_dev *dev) { struct tmff2_device_entry *tmff2 = tmff2_from_input(dev); + if (!tmff2) return; + /* since we're closing the device, no need to continue feeding it new data */ + cancel_delayed_work_sync(&tmff2->work); + if (tmff2->close) { - tmff2->close(tmff2->data, tmff2->hdev != 0); + tmff2->close(tmff2->data); return; } @@ -420,6 +439,7 @@ static int tmff2_create_files(struct tmff2_device_entry *tmff2) struct device *dev = &tmff2->hdev->dev; int ret; + /* could use short circuiting but this is more explicit */ if (tmff2->params & PARAM_ALT_MODE) { if ((ret = device_create_file(dev, &dev_attr_alt_mode))) { @@ -474,6 +494,7 @@ static int tmff2_wheel_init(struct tmff2_device_entry *tmff2) { int ret, i; struct ff_device *ff; + spin_lock_init(&lock); spin_lock_init(&tmff2->lock); INIT_DELAYED_WORK(&tmff2->work, tmff2_work_handler); @@ -522,6 +543,7 @@ static int tmff2_wheel_init(struct tmff2_device_entry *tmff2) if ((ret = tmff2_create_files(tmff2))) goto err; + tmff2->allow_scheduling = 1; return 0; input_ff_destroy(tmff2->input_dev); @@ -536,6 +558,7 @@ static int tmff2_probe(struct hid_device *hdev, const struct hid_device_id *id) int ret; + if (!tmff2) { ret = -ENOMEM; goto oom_err; @@ -596,6 +619,7 @@ static __u8 *tmff2_report_fixup(struct hid_device *hdev, __u8 *rdesc, unsigned int *rsize) { struct tmff2_device_entry *tmff2 = tmff2_from_hdev(hdev); + if (!tmff2) /* not entirely sure what the best course of action would be here */ return rdesc; @@ -609,9 +633,11 @@ static void tmff2_remove(struct hid_device *hdev) { struct tmff2_device_entry *tmff2 = tmff2_from_hdev(hdev); struct device *dev; + if (!tmff2) return; + tmff2->allow_scheduling = 0; cancel_delayed_work_sync(&tmff2->work); dev = &tmff2->hdev->dev; @@ -627,9 +653,6 @@ static void tmff2_remove(struct hid_device *hdev) if (tmff2->params & PARAM_ALT_MODE) device_remove_file(dev, &dev_attr_alt_mode); - /* indicate that the underlying usb device should not be assumed to be - * accessible */ - tmff2->hdev = 0; hid_hw_stop(hdev); tmff2->wheel_destroy(tmff2->data); diff --git a/hid-tmff2.h b/hid-tmff2.h index 4058cf7..390bd94 100644 --- a/hid-tmff2.h +++ b/hid-tmff2.h @@ -62,6 +62,8 @@ struct tmff2_device_entry { spinlock_t lock; + int allow_scheduling; + /* fields relevant to each actual device (T300, T150...) */ void *data; unsigned long params; @@ -79,7 +81,7 @@ struct tmff2_device_entry { /* optional callbacks */ int (*open)(void *data); - int (*close)(void *data, int dev_accessible); + int (*close)(void *data); int (*set_gain)(void *data, uint16_t gain); int (*set_range)(void *data, uint16_t range); int (*switch_mode)(void *data, uint16_t mode); @@ -123,7 +125,7 @@ int t300rs_update_effect(void *, struct tmff2_effect_state *); int t300rs_stop_effect(void *, struct tmff2_effect_state *); int t300rs_open(void *); -int t300rs_close(void *, int); +int t300rs_close(void *); int t300rs_set_gain(void *, uint16_t); int t300rs_set_range(void *, uint16_t); int t300rs_set_autocenter(void *, uint16_t); diff --git a/hid-tmt248.c b/hid-tmt248.c index 839f670..aef2944 100644 --- a/hid-tmt248.c +++ b/hid-tmt248.c @@ -133,6 +133,7 @@ static int t248_interrupts(struct t300rs_device_entry *t248) struct usb_interface *usbif = to_usb_interface(t248->hdev->dev.parent); struct usb_host_endpoint *ep; int ret, trans, b_ep, i; + if (!send_buf) { hid_err(t248->hdev, "failed allocating send buffer\n"); return -ENOMEM; @@ -167,6 +168,7 @@ int t248_wheel_init(struct tmff2_device_entry *tmff2) struct list_head *report_list; int ret; + if (!t248) { ret = -ENOMEM; goto t248_err; @@ -213,6 +215,7 @@ t248_err: int t248_wheel_destroy(void *data) { struct t300rs_device_entry *t300rs = data; + if (!t300rs) return -ENODEV; @@ -224,6 +227,7 @@ int t248_wheel_destroy(void *data) int t248_set_range(void *data, uint16_t value) { struct t300rs_device_entry *t248 = data; + if (value < 140) { hid_info(t248->hdev, "value %i too small, clamping to 140\n", value); value = 140; @@ -240,6 +244,7 @@ int t248_set_range(void *data, uint16_t value) static int t248_open(void *data) { struct t300rs_device_entry *t248 = data; + if (!t248) return -ENODEV; @@ -254,21 +259,20 @@ static int t248_open(void *data) return t248->open(t248->input_dev); } -static int t248_close(void *data, int dev_accessible) +static int t248_close(void *data) { struct t300rs_device_entry *t248 = data; + if (!t248) return -ENODEV; - if (dev_accessible) { - t248->send_buffer[0] = 0x01; - t248->send_buffer[1] = 0x05; - t300rs_send_int(t248); + t248->send_buffer[0] = 0x01; + t248->send_buffer[1] = 0x05; + t300rs_send_int(t248); - t248->send_buffer[0] = 0x01; - t248->send_buffer[1] = 0x00; - t300rs_send_int(t248); - } + t248->send_buffer[0] = 0x01; + t248->send_buffer[1] = 0x00; + t300rs_send_int(t248); t248->close(t248->input_dev); return 0; diff --git a/hid-tmt300rs.c b/hid-tmt300rs.c index c2a6c64..167262c 100644 --- a/hid-tmt300rs.c +++ b/hid-tmt300rs.c @@ -1303,7 +1303,7 @@ int t300rs_open(void *data) return t300rs->open(t300rs->input_dev); } -int t300rs_close(void *data, int dev_accessible) +int t300rs_close(void *data) { struct t300rs_device_entry *t300rs = data; struct t300rs_packet_close { @@ -1314,13 +1314,11 @@ int t300rs_close(void *data, int dev_accessible) if (!t300rs) return -ENODEV; - if (dev_accessible) { - close_packet = (struct t300rs_packet_close *)t300rs->send_buffer; - close_packet->header.cmd = 0x01; + close_packet = (struct t300rs_packet_close *)t300rs->send_buffer; + close_packet->header.cmd = 0x01; - if ((ret = t300rs_send_int(t300rs))) - hid_warn(t300rs->hdev, "failed sending close command\n"); - } + if ((ret = t300rs_send_int(t300rs))) + hid_warn(t300rs->hdev, "failed sending close command\n"); t300rs->close(t300rs->input_dev); return ret; -- cgit v1.3