Hi all,
So I just found out the hard way that this commit has a small bug, which when
using one of the new supported receiver types may result in a locked
machine, see inline comment below.
On 30-03-19 12:24, Hans de Goede wrote:
dj/HID++ receivers are really a single logical entity, but for BIOS/Windows
compatibility they have multiple USB interfaces. For the upcoming
non-unifying receiver support, we need to listen for events from / bind to
all USB-interfaces of the receiver.
This commit add support to the logitech-dj code for creating a single
dj_receiver_dev struct for all interfaces belonging to a single
USB-device / receiver, in preparation for adding non-unifying receiver
support.
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
drivers/hid/hid-logitech-dj.c | 175 ++++++++++++++++++++++++++++------
1 file changed, 148 insertions(+), 27 deletions(-)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 9e08b44c76b2..337389c2d6c7 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -119,11 +119,16 @@ struct hidpp_event {
} __packed;
struct dj_receiver_dev {
+ struct hid_device *mouse;
+ struct hid_device *keyboard;
struct hid_device *hidpp;
struct dj_device *paired_dj_devices[DJ_MAX_PAIRED_DEVICES +
DJ_DEVICE_INDEX_MIN];
+ struct list_head list;
+ struct kref kref;
struct work_struct work;
struct kfifo notif_fifo;
+ bool ready;
spinlock_t lock;
};
@@ -363,6 +368,110 @@ static const u8 hid_reportid_size_map[NUMBER_OF_HID_REPORTS] = {
static struct hid_ll_driver logi_dj_ll_driver;
static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev);
+static void delayedwork_callback(struct work_struct *work);
+
+static LIST_HEAD(dj_hdev_list);
+static DEFINE_MUTEX(dj_hdev_list_lock);
+
+/*
+ * dj/HID++ receivers are really a single logical entity, but for BIOS/Windows
+ * compatibility they have multiple USB interfaces. On HID++ receivers we need
+ * to listen for input reports on both interfaces. The functions below are used
+ * to create a single struct dj_receiver_dev for all interfaces belonging to
+ * a single USB-device / receiver.
+ */
+static struct dj_receiver_dev *dj_find_receiver_dev(struct hid_device *hdev)
+{
+ struct dj_receiver_dev *djrcv_dev;
+
+ /* Try to find an already-probed interface from the same device */
+ list_for_each_entry(djrcv_dev, &dj_hdev_list, list) {
+ if (djrcv_dev->mouse &&
+ hid_compare_device_paths(hdev, djrcv_dev->mouse, '/')) {
+ kref_get(&djrcv_dev->kref);
+ return djrcv_dev;
+ }
+ if (djrcv_dev->keyboard &&
+ hid_compare_device_paths(hdev, djrcv_dev->keyboard, '/')) {
+ kref_get(&djrcv_dev->kref);
+ return djrcv_dev;
+ }
+ if (djrcv_dev->hidpp &&
+ hid_compare_device_paths(hdev, djrcv_dev->hidpp, '/')) {
+ kref_get(&djrcv_dev->kref);
+ return djrcv_dev;
+ }
+ }
+
+ return NULL;
+}
+
+static void dj_release_receiver_dev(struct kref *kref)
+{
+ struct dj_receiver_dev *djrcv_dev = container_of(kref, struct dj_receiver_dev, kref);
+
+ list_del(&djrcv_dev->list);
+ kfifo_free(&djrcv_dev->notif_fifo);
+ kfree(djrcv_dev);
+}
+
+static void dj_put_receiver_dev(struct hid_device *hdev)
+{
+ struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev);
+
+ mutex_lock(&dj_hdev_list_lock);
+
+ if (djrcv_dev->mouse == hdev)
+ djrcv_dev->mouse = NULL;
+ if (djrcv_dev->keyboard == hdev)
+ djrcv_dev->keyboard = NULL;
+ if (djrcv_dev->hidpp == hdev)
+ djrcv_dev->hidpp = NULL;
+
+ kref_put(&djrcv_dev->kref, dj_release_receiver_dev);
+
+ mutex_unlock(&dj_hdev_list_lock);
+}
+
+static struct dj_receiver_dev *dj_get_receiver_dev(struct hid_device *hdev,
+ unsigned int application,
+ bool is_hidpp)
+{
+ struct dj_receiver_dev *djrcv_dev;
+
+ mutex_lock(&dj_hdev_list_lock);
+
+ djrcv_dev = dj_find_receiver_dev(hdev);
+ if (!djrcv_dev) {
+ djrcv_dev = kzalloc(sizeof(*djrcv_dev), GFP_KERNEL);
+ if (!djrcv_dev)
+ goto out;
+
+ INIT_WORK(&djrcv_dev->work, delayedwork_callback);
+ spin_lock_init(&djrcv_dev->lock);
+ if (kfifo_alloc(&djrcv_dev->notif_fifo,
+ DJ_MAX_NUMBER_NOTIFS * sizeof(struct dj_workitem),
+ GFP_KERNEL)) {
+ kfree(djrcv_dev);
+ djrcv_dev = NULL;
+ goto out;
+ }
+ kref_init(&djrcv_dev->kref);
+ list_add_tail(&djrcv_dev->list, &dj_hdev_list);
+ }
+
+ if (application == HID_GD_KEYBOARD)
+ djrcv_dev->keyboard = hdev;
+ if (application == HID_GD_MOUSE)
+ djrcv_dev->mouse = hdev;
+ if (is_hidpp)
+ djrcv_dev->hidpp = hdev;
+
+ hid_set_drvdata(hdev, djrcv_dev);
+out:
+ mutex_unlock(&dj_hdev_list_lock);
+ return djrcv_dev;
+}
static void logi_dj_recv_destroy_djhid_device(struct dj_receiver_dev *djrcv_dev,
struct dj_workitem *workitem)
@@ -480,6 +589,16 @@ static void delayedwork_callback(struct work_struct *work)
spin_lock_irqsave(&djrcv_dev->lock, flags);
+ /*
+ * Since we attach to multiple interfaces, we may get scheduled before
+ * we are bound to the HID++ interface, catch this.
+ */
+ if (!djrcv_dev->ready) {
+ hid_err(djrcv_dev->hidpp, "delayedwork queued before hidpp interface was enumerated\n");
So the whole purpose of this is to catch the work being queued while
djrcv_dev->hidpp may be NULL, so printing the error using djrcv_dev->hidpp
here is not the best of ideas.
While trying to add support for the C-UV35 Bluetooth Mini-Receiver in
HID proxy mode I manually unbound the driver from the mouse/hidpp interface
only and pressed a key causing the work to be queued with a WORKITEM_TYPE_UNKNOWN
workitem, then hit this path, resulting in a NULL ptr deref with the spinlock held
and after that the whole machine becomes unusable.
I've replaced this with:
pr_err("%s: delayedwork queued before hidpp interface was enumerated\n");
__func__);
For v2 of this patch set, fixing this.
Regards,
Hans
+ spin_unlock_irqrestore(&djrcv_dev->lock, flags);
+ return;
+ }
+
count = kfifo_out(&djrcv_dev->notif_fifo, &workitem, sizeof(workitem));
if (count != sizeof(workitem)) {
@@ -1041,6 +1160,7 @@ static int logi_dj_probe(struct hid_device *hdev,
struct hid_report *rep;
struct dj_receiver_dev *djrcv_dev;
bool has_hidpp = false;
+ unsigned long flags;
int retval;
/* Call to usbhid to fetch the HID descriptors of the current
@@ -1070,27 +1190,15 @@ static int logi_dj_probe(struct hid_device *hdev,
if (!has_hidpp)
return -ENODEV;
- /* Treat DJ/HID++ interface */
-
- djrcv_dev = kzalloc(sizeof(struct dj_receiver_dev), GFP_KERNEL);
+ /* get the current application attached to the node */
+ rep = list_first_entry(&rep_enum->report_list, struct hid_report, list);
+ djrcv_dev = dj_get_receiver_dev(hdev,
+ rep->application, has_hidpp);
if (!djrcv_dev) {
dev_err(&hdev->dev,
"%s:failed allocating dj_receiver_dev\n", __func__);
return -ENOMEM;
}
- djrcv_dev->hidpp = hdev;
- INIT_WORK(&djrcv_dev->work, delayedwork_callback);
- spin_lock_init(&djrcv_dev->lock);
- if (kfifo_alloc(&djrcv_dev->notif_fifo,
- DJ_MAX_NUMBER_NOTIFS * sizeof(struct dj_workitem),
- GFP_KERNEL)) {
- dev_err(&hdev->dev,
- "%s:failed allocating notif_fifo\n", __func__);
- kfree(djrcv_dev);
- return -ENOMEM;
- }
- hid_set_drvdata(hdev, djrcv_dev);
-
/* Starts the usb device and connects to upper interfaces hiddev and
* hidraw */
@@ -1120,6 +1228,9 @@ static int logi_dj_probe(struct hid_device *hdev,
/* Allow incoming packets to arrive: */
hid_device_io_start(hdev);
+ spin_lock_irqsave(&djrcv_dev->lock, flags);
+ djrcv_dev->ready = true;
+ spin_unlock_irqrestore(&djrcv_dev->lock, flags);
retval = logi_dj_recv_query_paired_devices(djrcv_dev);
if (retval < 0) {
dev_err(&hdev->dev, "%s:logi_dj_recv_query_paired_devices "
@@ -1137,10 +1248,8 @@ static int logi_dj_probe(struct hid_device *hdev,
hid_hw_stop(hdev);
hid_hw_start_fail:
- kfifo_free(&djrcv_dev->notif_fifo);
- kfree(djrcv_dev);
+ dj_put_receiver_dev(hdev);
return retval;
-
}
#ifdef CONFIG_PM
@@ -1164,31 +1273,43 @@ static void logi_dj_remove(struct hid_device *hdev)
{
struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev);
struct dj_device *dj_dev;
+ unsigned long flags;
int i;
dbg_hid("%s\n", __func__);
+ /*
+ * This ensures that if the work gets requeued from another
+ * interface of the same receiver it will be a no-op.
+ */
+ spin_lock_irqsave(&djrcv_dev->lock, flags);
+ djrcv_dev->ready = false;
+ spin_unlock_irqrestore(&djrcv_dev->lock, flags);
+
cancel_work_sync(&djrcv_dev->work);
hid_hw_close(hdev);
hid_hw_stop(hdev);
- /* I suppose that at this point the only context that can access
- * the djrecv_data is this thread as the work item is guaranteed to
- * have finished and no more raw_event callbacks should arrive after
- * the remove callback was triggered so no locks are put around the
- * code below */
+ /*
+ * For proper operation we need access to all interfaces, so we destroy
+ * the paired devices when we're unbound from any interface.
+ *
+ * Note we may still be bound to other interfaces, sharing the same
+ * djrcv_dev, so we need locking here.
+ */
for (i = 0; i < (DJ_MAX_PAIRED_DEVICES + DJ_DEVICE_INDEX_MIN); i++) {
+ spin_lock_irqsave(&djrcv_dev->lock, flags);
dj_dev = djrcv_dev->paired_dj_devices[i];
+ djrcv_dev->paired_dj_devices[i] = NULL;
+ spin_unlock_irqrestore(&djrcv_dev->lock, flags);
if (dj_dev != NULL) {
hid_destroy_device(dj_dev->hdev);
kfree(dj_dev);
- djrcv_dev->paired_dj_devices[i] = NULL;
}
}
- kfifo_free(&djrcv_dev->notif_fifo);
- kfree(djrcv_dev);
+ dj_put_receiver_dev(hdev);
}
static const struct hid_device_id logi_dj_receivers[] = {
