| Message ID | 1369307425-5993-1-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
This patch alone didn't work: http://privatepaste.com/a3fe0dff0a Should I also apply the one that returns ENODATA? From what I can tell it wouldn't be needed as this power_supply registration would be delayed tho I see nothing in this patch that does that. What am I missing? Thanks, 2013/5/23 David Herrmann <dh.herrmann@gmail.com>: > While l2cap_user callbacks are running, the whole hci_dev is locked. Even > if we would add more fine-grained locking to HCI core, it would still be > called from the non-reentrant rx work-queue and thus block the event > processing. > > However, if we want to perform synchronous I/O during HID device > registration (eg., to perform device-detection), we need the HCI core > to be able to dispatch incoming data. > > Therefore, we now move device-registration to a separate worker. The HCI > core can continue running and we add devices asynchronously in another > kernel thread. Device removal is synchronized and waits for the worker > to exit before calling the usual device removal functions. > > If l2cap_user->remove is called before the thread registered the devices, > we set "terminate" to true and the thread will skip it. If > l2cap_user->remove is called after it, we notice this as the device > is no longer in HIDP_SESSION_PREPARING state and simply unregister the > device as we did before. > There is no new deadlock as we now call hidp_session_add_dev() with > one lock less held (the HCI lock) and it cannot itself call back into > HCI as it was called with the HCI-lock held before. > > One might wonder whether this can block during device unregistration. > But we set "terminate" to true and wake the HIDP thread up _before_ > unregistering the HID/input devices. Therefore, all pending HID I/O > operations are canceled. All further I/O attempts will fail with ENODEV > or EIO. So all latency we can get are few context-switches, but no > timeouts or blocking I/O waits! > > This change also prepares for a long standing HID bug. All HID devices > that register power_supply devices need to be able to handle callbacks > during registration (a power_supply oddity that cannot easily be fixed). > So with this patch available, we can allow HID I/O during registration > by calling the recently introduced hid_device_io_start/stop helpers, > which currently are a no-op for bluetooth due to this locking. > > Note that we cannot do the same for input devices. input-core doesn't > allow us to call input_event() asynchronously to input_register_device(), > which HID-core kindly allows (for good reasons). > Fixing input-core to allow this isn't as easy as it sounds and is, > beside simplifying HIDP, not really an improvement. Hence, we still > register input devices synchronously as we did before. Only HID devices > are registered asynchronously. > > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > --- > Hi > > @Daniel, this should fix the battery issues for Bluetooth devices. As far > as I can see, it should have always worked for USB devices already as it uses > the synchronous HID data-callbacks. > > I tested this on my machine and it works quite well. However, I cannot tell > whether it fixes the battery issues as I have no device to test. It would be > nice to get a "tested-by" if it works for you. > > If it doesn't fix the issues, I will have to look closer again, but I didn't > find any other issues. > > Please note that this _must_ be applied on linux-next or linux-3.10-rc1 or > later. It does not work with linux-3.9 or older. > > Cheers > David > > net/bluetooth/hidp/core.c | 56 +++++++++++++++++++++++++++++++++++++++-------- > net/bluetooth/hidp/hidp.h | 2 ++ > 2 files changed, 49 insertions(+), 9 deletions(-) > > diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c > index c756b06..e3c14de 100644 > --- a/net/bluetooth/hidp/core.c > +++ b/net/bluetooth/hidp/core.c > @@ -848,6 +848,29 @@ static void hidp_session_dev_del(struct hidp_session *session) > } > > /* > + * Asynchronous device registration > + * HID device drivers might want to perform I/O during initialization to > + * detect device types. Therefore, call device registration in a separate > + * worker so the HIDP thread can schedule I/O operations. > + * Note that this must be called after the worker thread was initialized > + * successfully. This will then add the devices and increase session state > + * on success, otherwise it will terminate the session thread. > + */ > +static void hidp_session_dev_work(struct work_struct *work) > +{ > + struct hidp_session *session = container_of(work, > + struct hidp_session, > + dev_init); > + int ret; > + > + ret = hidp_session_dev_add(session); > + if (!ret) > + atomic_inc(&session->state); > + else > + hidp_session_terminate(session); > +} > + > +/* > * Create new session object > * Allocate session object, initialize static fields, copy input data into the > * object and take a reference to all sub-objects. > @@ -894,6 +917,7 @@ static int hidp_session_new(struct hidp_session **out, const bdaddr_t *bdaddr, > session->idle_to = req->idle_to; > > /* device management */ > + INIT_WORK(&session->dev_init, hidp_session_dev_work); > setup_timer(&session->timer, hidp_idle_timeout, > (unsigned long)session); > > @@ -1032,8 +1056,8 @@ static void hidp_session_terminate(struct hidp_session *session) > * Probe HIDP session > * This is called from the l2cap_conn core when our l2cap_user object is bound > * to the hci-connection. We get the session via the \user object and can now > - * start the session thread, register the HID/input devices and link it into > - * the global session list. > + * start the session thread, link it into the global session list and > + * schedule HID/input device registration. > * The global session-list owns its own reference to the session object so you > * can drop your own reference after registering the l2cap_user object. > */ > @@ -1055,21 +1079,30 @@ static int hidp_session_probe(struct l2cap_conn *conn, > goto out_unlock; > } > > + if (session->input) { > + ret = hidp_session_dev_add(session); > + if (ret) > + goto out_unlock; > + } > + > ret = hidp_session_start_sync(session); > if (ret) > - goto out_unlock; > + goto out_del; > > - ret = hidp_session_dev_add(session); > - if (ret) > - goto out_stop; > + /* HID device registration is async to allow I/O during probe */ > + if (session->input) > + atomic_inc(&session->state); > + else > + schedule_work(&session->dev_init); > > hidp_session_get(session); > list_add(&session->list, &hidp_session_list); > ret = 0; > goto out_unlock; > > -out_stop: > - hidp_session_terminate(session); > +out_del: > + if (session->input) > + hidp_session_dev_del(session); > out_unlock: > up_write(&hidp_session_sem); > return ret; > @@ -1099,7 +1132,12 @@ static void hidp_session_remove(struct l2cap_conn *conn, > down_write(&hidp_session_sem); > > hidp_session_terminate(session); > - hidp_session_dev_del(session); > + > + cancel_work_sync(&session->dev_init); > + if (session->input || > + atomic_read(&session->state) > HIDP_SESSION_PREPARING) > + hidp_session_dev_del(session); > + > list_del(&session->list); > > up_write(&hidp_session_sem); > diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h > index 6162ce8..9e6cc35 100644 > --- a/net/bluetooth/hidp/hidp.h > +++ b/net/bluetooth/hidp/hidp.h > @@ -128,6 +128,7 @@ int hidp_get_conninfo(struct hidp_conninfo *ci); > > enum hidp_session_state { > HIDP_SESSION_IDLING, > + HIDP_SESSION_PREPARING, > HIDP_SESSION_RUNNING, > }; > > @@ -156,6 +157,7 @@ struct hidp_session { > unsigned long idle_to; > > /* device management */ > + struct work_struct dev_init; > struct input_dev *input; > struct hid_device *hid; > struct timer_list timer; > -- > 1.8.2.3 >
Ok, just tested with my two Apple mouses (Trackpad & MagicMouse), and with the ENODATA patch it now works perfectly, the other mouse doesn't freeze when connecting and the udev signals are also properly emitted, is there some formal way of adding a tested-by? tested-by: Daniel Nicoletti <dantti12@gmail.com> Thank you! 2013/5/23 David Herrmann <dh.herrmann@gmail.com>: > While l2cap_user callbacks are running, the whole hci_dev is locked. Even > if we would add more fine-grained locking to HCI core, it would still be > called from the non-reentrant rx work-queue and thus block the event > processing. > > However, if we want to perform synchronous I/O during HID device > registration (eg., to perform device-detection), we need the HCI core > to be able to dispatch incoming data. > > Therefore, we now move device-registration to a separate worker. The HCI > core can continue running and we add devices asynchronously in another > kernel thread. Device removal is synchronized and waits for the worker > to exit before calling the usual device removal functions. > > If l2cap_user->remove is called before the thread registered the devices, > we set "terminate" to true and the thread will skip it. If > l2cap_user->remove is called after it, we notice this as the device > is no longer in HIDP_SESSION_PREPARING state and simply unregister the > device as we did before. > There is no new deadlock as we now call hidp_session_add_dev() with > one lock less held (the HCI lock) and it cannot itself call back into > HCI as it was called with the HCI-lock held before. > > One might wonder whether this can block during device unregistration. > But we set "terminate" to true and wake the HIDP thread up _before_ > unregistering the HID/input devices. Therefore, all pending HID I/O > operations are canceled. All further I/O attempts will fail with ENODEV > or EIO. So all latency we can get are few context-switches, but no > timeouts or blocking I/O waits! > > This change also prepares for a long standing HID bug. All HID devices > that register power_supply devices need to be able to handle callbacks > during registration (a power_supply oddity that cannot easily be fixed). > So with this patch available, we can allow HID I/O during registration > by calling the recently introduced hid_device_io_start/stop helpers, > which currently are a no-op for bluetooth due to this locking. > > Note that we cannot do the same for input devices. input-core doesn't > allow us to call input_event() asynchronously to input_register_device(), > which HID-core kindly allows (for good reasons). > Fixing input-core to allow this isn't as easy as it sounds and is, > beside simplifying HIDP, not really an improvement. Hence, we still > register input devices synchronously as we did before. Only HID devices > are registered asynchronously. > > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > --- > Hi > > @Daniel, this should fix the battery issues for Bluetooth devices. As far > as I can see, it should have always worked for USB devices already as it uses > the synchronous HID data-callbacks. > > I tested this on my machine and it works quite well. However, I cannot tell > whether it fixes the battery issues as I have no device to test. It would be > nice to get a "tested-by" if it works for you. > > If it doesn't fix the issues, I will have to look closer again, but I didn't > find any other issues. > > Please note that this _must_ be applied on linux-next or linux-3.10-rc1 or > later. It does not work with linux-3.9 or older. > > Cheers > David > > net/bluetooth/hidp/core.c | 56 +++++++++++++++++++++++++++++++++++++++-------- > net/bluetooth/hidp/hidp.h | 2 ++ > 2 files changed, 49 insertions(+), 9 deletions(-) > > diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c > index c756b06..e3c14de 100644 > --- a/net/bluetooth/hidp/core.c > +++ b/net/bluetooth/hidp/core.c > @@ -848,6 +848,29 @@ static void hidp_session_dev_del(struct hidp_session *session) > } > > /* > + * Asynchronous device registration > + * HID device drivers might want to perform I/O during initialization to > + * detect device types. Therefore, call device registration in a separate > + * worker so the HIDP thread can schedule I/O operations. > + * Note that this must be called after the worker thread was initialized > + * successfully. This will then add the devices and increase session state > + * on success, otherwise it will terminate the session thread. > + */ > +static void hidp_session_dev_work(struct work_struct *work) > +{ > + struct hidp_session *session = container_of(work, > + struct hidp_session, > + dev_init); > + int ret; > + > + ret = hidp_session_dev_add(session); > + if (!ret) > + atomic_inc(&session->state); > + else > + hidp_session_terminate(session); > +} > + > +/* > * Create new session object > * Allocate session object, initialize static fields, copy input data into the > * object and take a reference to all sub-objects. > @@ -894,6 +917,7 @@ static int hidp_session_new(struct hidp_session **out, const bdaddr_t *bdaddr, > session->idle_to = req->idle_to; > > /* device management */ > + INIT_WORK(&session->dev_init, hidp_session_dev_work); > setup_timer(&session->timer, hidp_idle_timeout, > (unsigned long)session); > > @@ -1032,8 +1056,8 @@ static void hidp_session_terminate(struct hidp_session *session) > * Probe HIDP session > * This is called from the l2cap_conn core when our l2cap_user object is bound > * to the hci-connection. We get the session via the \user object and can now > - * start the session thread, register the HID/input devices and link it into > - * the global session list. > + * start the session thread, link it into the global session list and > + * schedule HID/input device registration. > * The global session-list owns its own reference to the session object so you > * can drop your own reference after registering the l2cap_user object. > */ > @@ -1055,21 +1079,30 @@ static int hidp_session_probe(struct l2cap_conn *conn, > goto out_unlock; > } > > + if (session->input) { > + ret = hidp_session_dev_add(session); > + if (ret) > + goto out_unlock; > + } > + > ret = hidp_session_start_sync(session); > if (ret) > - goto out_unlock; > + goto out_del; > > - ret = hidp_session_dev_add(session); > - if (ret) > - goto out_stop; > + /* HID device registration is async to allow I/O during probe */ > + if (session->input) > + atomic_inc(&session->state); > + else > + schedule_work(&session->dev_init); > > hidp_session_get(session); > list_add(&session->list, &hidp_session_list); > ret = 0; > goto out_unlock; > > -out_stop: > - hidp_session_terminate(session); > +out_del: > + if (session->input) > + hidp_session_dev_del(session); > out_unlock: > up_write(&hidp_session_sem); > return ret; > @@ -1099,7 +1132,12 @@ static void hidp_session_remove(struct l2cap_conn *conn, > down_write(&hidp_session_sem); > > hidp_session_terminate(session); > - hidp_session_dev_del(session); > + > + cancel_work_sync(&session->dev_init); > + if (session->input || > + atomic_read(&session->state) > HIDP_SESSION_PREPARING) > + hidp_session_dev_del(session); > + > list_del(&session->list); > > up_write(&hidp_session_sem); > diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h > index 6162ce8..9e6cc35 100644 > --- a/net/bluetooth/hidp/hidp.h > +++ b/net/bluetooth/hidp/hidp.h > @@ -128,6 +128,7 @@ int hidp_get_conninfo(struct hidp_conninfo *ci); > > enum hidp_session_state { > HIDP_SESSION_IDLING, > + HIDP_SESSION_PREPARING, > HIDP_SESSION_RUNNING, > }; > > @@ -156,6 +157,7 @@ struct hidp_session { > unsigned long idle_to; > > /* device management */ > + struct work_struct dev_init; > struct input_dev *input; > struct hid_device *hid; > struct timer_list timer; > -- > 1.8.2.3 >
Hi On Fri, May 24, 2013 at 12:46 AM, Daniel Nicoletti <dantti12@gmail.com> wrote: > Ok, just tested with my two Apple mouses (Trackpad & MagicMouse), > and with the ENODATA patch it now works perfectly, the other mouse > doesn't freeze when connecting and the udev signals are also properly > emitted, is there some formal way of adding a tested-by? > > tested-by: Daniel Nicoletti <dantti12@gmail.com> Just this line is enough. Thanks! It's weird, though, that the other patch is required. With this patch here, hid_get_raw_report() should work just fine on bluetooth _and_ usb devices. And the fact that you no longer get a delay means that I/O is working properly. However, the GET_REPORT request seems to fail after device-initialization for some reason. My guess is that we send REPORT_INIT requests in hid_start() and Bluetooth devices seem to not work well with that. Hence, they send an HANDSHAKE error for each report once. This seems to fall into the same time-span where we send our first battery-request during registration and hence HIDP-core thinks the battery-request failed. @Jiri, @Marcel are you sure report-initialization is needed for BT-HIDP devices? BT-HID-profile doesn't mention it, but it relies heavily on USB-HID (which I haven't read..).. Anyway, would be nice to see both patches applied. Cheers David -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 23 May 2013, David Herrmann wrote: > While l2cap_user callbacks are running, the whole hci_dev is locked. Even > if we would add more fine-grained locking to HCI core, it would still be > called from the non-reentrant rx work-queue and thus block the event > processing. > > However, if we want to perform synchronous I/O during HID device > registration (eg., to perform device-detection), we need the HCI core > to be able to dispatch incoming data. > > Therefore, we now move device-registration to a separate worker. The HCI > core can continue running and we add devices asynchronously in another > kernel thread. Device removal is synchronized and waits for the worker > to exit before calling the usual device removal functions. > > If l2cap_user->remove is called before the thread registered the devices, > we set "terminate" to true and the thread will skip it. If > l2cap_user->remove is called after it, we notice this as the device > is no longer in HIDP_SESSION_PREPARING state and simply unregister the > device as we did before. > There is no new deadlock as we now call hidp_session_add_dev() with > one lock less held (the HCI lock) and it cannot itself call back into > HCI as it was called with the HCI-lock held before. > > One might wonder whether this can block during device unregistration. > But we set "terminate" to true and wake the HIDP thread up _before_ > unregistering the HID/input devices. Therefore, all pending HID I/O > operations are canceled. All further I/O attempts will fail with ENODEV > or EIO. So all latency we can get are few context-switches, but no > timeouts or blocking I/O waits! > > This change also prepares for a long standing HID bug. All HID devices > that register power_supply devices need to be able to handle callbacks > during registration (a power_supply oddity that cannot easily be fixed). > So with this patch available, we can allow HID I/O during registration > by calling the recently introduced hid_device_io_start/stop helpers, > which currently are a no-op for bluetooth due to this locking. > > Note that we cannot do the same for input devices. input-core doesn't > allow us to call input_event() asynchronously to input_register_device(), > which HID-core kindly allows (for good reasons). > Fixing input-core to allow this isn't as easy as it sounds and is, > beside simplifying HIDP, not really an improvement. Hence, we still > register input devices synchronously as we did before. Only HID devices > are registered asynchronously. > > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> Acked-by: Jiri Kosina <jkosina@suse.cz> Gustavo, I think I'd like to take this patch together with the ENODATA change for hid-input, as they, in some sense, stick together. If you are OK with that, could you please provide me with your Acked-by, and I'll take it through my tree? Thanks.
Hi Jiri, * Jiri Kosina <jkosina@suse.cz> [2013-05-28 10:45:26 +0200]: > On Thu, 23 May 2013, David Herrmann wrote: > > > While l2cap_user callbacks are running, the whole hci_dev is locked. Even > > if we would add more fine-grained locking to HCI core, it would still be > > called from the non-reentrant rx work-queue and thus block the event > > processing. > > > > However, if we want to perform synchronous I/O during HID device > > registration (eg., to perform device-detection), we need the HCI core > > to be able to dispatch incoming data. > > > > Therefore, we now move device-registration to a separate worker. The HCI > > core can continue running and we add devices asynchronously in another > > kernel thread. Device removal is synchronized and waits for the worker > > to exit before calling the usual device removal functions. > > > > If l2cap_user->remove is called before the thread registered the devices, > > we set "terminate" to true and the thread will skip it. If > > l2cap_user->remove is called after it, we notice this as the device > > is no longer in HIDP_SESSION_PREPARING state and simply unregister the > > device as we did before. > > There is no new deadlock as we now call hidp_session_add_dev() with > > one lock less held (the HCI lock) and it cannot itself call back into > > HCI as it was called with the HCI-lock held before. > > > > One might wonder whether this can block during device unregistration. > > But we set "terminate" to true and wake the HIDP thread up _before_ > > unregistering the HID/input devices. Therefore, all pending HID I/O > > operations are canceled. All further I/O attempts will fail with ENODEV > > or EIO. So all latency we can get are few context-switches, but no > > timeouts or blocking I/O waits! > > > > This change also prepares for a long standing HID bug. All HID devices > > that register power_supply devices need to be able to handle callbacks > > during registration (a power_supply oddity that cannot easily be fixed). > > So with this patch available, we can allow HID I/O during registration > > by calling the recently introduced hid_device_io_start/stop helpers, > > which currently are a no-op for bluetooth due to this locking. > > > > Note that we cannot do the same for input devices. input-core doesn't > > allow us to call input_event() asynchronously to input_register_device(), > > which HID-core kindly allows (for good reasons). > > Fixing input-core to allow this isn't as easy as it sounds and is, > > beside simplifying HIDP, not really an improvement. Hence, we still > > register input devices synchronously as we did before. Only HID devices > > are registered asynchronously. > > > > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > > Acked-by: Jiri Kosina <jkosina@suse.cz> > > Gustavo, I think I'd like to take this patch together with the ENODATA > change for hid-input, as they, in some sense, stick together. > > If you are OK with that, could you please provide me with your Acked-by, > and I'll take it through my tree? This looks good to me. Acked-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> Gustavo -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 28 May 2013, Gustavo Padovan wrote: > > > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > > > > Acked-by: Jiri Kosina <jkosina@suse.cz> > > > > Gustavo, I think I'd like to take this patch together with the ENODATA > > change for hid-input, as they, in some sense, stick together. > > > > If you are OK with that, could you please provide me with your Acked-by, > > and I'll take it through my tree? > > This looks good to me. > > Acked-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> Perfect, I am taking it through my tree. Thanks,
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c index c756b06..e3c14de 100644 --- a/net/bluetooth/hidp/core.c +++ b/net/bluetooth/hidp/core.c @@ -848,6 +848,29 @@ static void hidp_session_dev_del(struct hidp_session *session) } /* + * Asynchronous device registration + * HID device drivers might want to perform I/O during initialization to + * detect device types. Therefore, call device registration in a separate + * worker so the HIDP thread can schedule I/O operations. + * Note that this must be called after the worker thread was initialized + * successfully. This will then add the devices and increase session state + * on success, otherwise it will terminate the session thread. + */ +static void hidp_session_dev_work(struct work_struct *work) +{ + struct hidp_session *session = container_of(work, + struct hidp_session, + dev_init); + int ret; + + ret = hidp_session_dev_add(session); + if (!ret) + atomic_inc(&session->state); + else + hidp_session_terminate(session); +} + +/* * Create new session object * Allocate session object, initialize static fields, copy input data into the * object and take a reference to all sub-objects. @@ -894,6 +917,7 @@ static int hidp_session_new(struct hidp_session **out, const bdaddr_t *bdaddr, session->idle_to = req->idle_to; /* device management */ + INIT_WORK(&session->dev_init, hidp_session_dev_work); setup_timer(&session->timer, hidp_idle_timeout, (unsigned long)session); @@ -1032,8 +1056,8 @@ static void hidp_session_terminate(struct hidp_session *session) * Probe HIDP session * This is called from the l2cap_conn core when our l2cap_user object is bound * to the hci-connection. We get the session via the \user object and can now - * start the session thread, register the HID/input devices and link it into - * the global session list. + * start the session thread, link it into the global session list and + * schedule HID/input device registration. * The global session-list owns its own reference to the session object so you * can drop your own reference after registering the l2cap_user object. */ @@ -1055,21 +1079,30 @@ static int hidp_session_probe(struct l2cap_conn *conn, goto out_unlock; } + if (session->input) { + ret = hidp_session_dev_add(session); + if (ret) + goto out_unlock; + } + ret = hidp_session_start_sync(session); if (ret) - goto out_unlock; + goto out_del; - ret = hidp_session_dev_add(session); - if (ret) - goto out_stop; + /* HID device registration is async to allow I/O during probe */ + if (session->input) + atomic_inc(&session->state); + else + schedule_work(&session->dev_init); hidp_session_get(session); list_add(&session->list, &hidp_session_list); ret = 0; goto out_unlock; -out_stop: - hidp_session_terminate(session); +out_del: + if (session->input) + hidp_session_dev_del(session); out_unlock: up_write(&hidp_session_sem); return ret; @@ -1099,7 +1132,12 @@ static void hidp_session_remove(struct l2cap_conn *conn, down_write(&hidp_session_sem); hidp_session_terminate(session); - hidp_session_dev_del(session); + + cancel_work_sync(&session->dev_init); + if (session->input || + atomic_read(&session->state) > HIDP_SESSION_PREPARING) + hidp_session_dev_del(session); + list_del(&session->list); up_write(&hidp_session_sem); diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h index 6162ce8..9e6cc35 100644 --- a/net/bluetooth/hidp/hidp.h +++ b/net/bluetooth/hidp/hidp.h @@ -128,6 +128,7 @@ int hidp_get_conninfo(struct hidp_conninfo *ci); enum hidp_session_state { HIDP_SESSION_IDLING, + HIDP_SESSION_PREPARING, HIDP_SESSION_RUNNING, }; @@ -156,6 +157,7 @@ struct hidp_session { unsigned long idle_to; /* device management */ + struct work_struct dev_init; struct input_dev *input; struct hid_device *hid; struct timer_list timer;
While l2cap_user callbacks are running, the whole hci_dev is locked. Even if we would add more fine-grained locking to HCI core, it would still be called from the non-reentrant rx work-queue and thus block the event processing. However, if we want to perform synchronous I/O during HID device registration (eg., to perform device-detection), we need the HCI core to be able to dispatch incoming data. Therefore, we now move device-registration to a separate worker. The HCI core can continue running and we add devices asynchronously in another kernel thread. Device removal is synchronized and waits for the worker to exit before calling the usual device removal functions. If l2cap_user->remove is called before the thread registered the devices, we set "terminate" to true and the thread will skip it. If l2cap_user->remove is called after it, we notice this as the device is no longer in HIDP_SESSION_PREPARING state and simply unregister the device as we did before. There is no new deadlock as we now call hidp_session_add_dev() with one lock less held (the HCI lock) and it cannot itself call back into HCI as it was called with the HCI-lock held before. One might wonder whether this can block during device unregistration. But we set "terminate" to true and wake the HIDP thread up _before_ unregistering the HID/input devices. Therefore, all pending HID I/O operations are canceled. All further I/O attempts will fail with ENODEV or EIO. So all latency we can get are few context-switches, but no timeouts or blocking I/O waits! This change also prepares for a long standing HID bug. All HID devices that register power_supply devices need to be able to handle callbacks during registration (a power_supply oddity that cannot easily be fixed). So with this patch available, we can allow HID I/O during registration by calling the recently introduced hid_device_io_start/stop helpers, which currently are a no-op for bluetooth due to this locking. Note that we cannot do the same for input devices. input-core doesn't allow us to call input_event() asynchronously to input_register_device(), which HID-core kindly allows (for good reasons). Fixing input-core to allow this isn't as easy as it sounds and is, beside simplifying HIDP, not really an improvement. Hence, we still register input devices synchronously as we did before. Only HID devices are registered asynchronously. Signed-off-by: David Herrmann <dh.herrmann@gmail.com> --- Hi @Daniel, this should fix the battery issues for Bluetooth devices. As far as I can see, it should have always worked for USB devices already as it uses the synchronous HID data-callbacks. I tested this on my machine and it works quite well. However, I cannot tell whether it fixes the battery issues as I have no device to test. It would be nice to get a "tested-by" if it works for you. If it doesn't fix the issues, I will have to look closer again, but I didn't find any other issues. Please note that this _must_ be applied on linux-next or linux-3.10-rc1 or later. It does not work with linux-3.9 or older. Cheers David net/bluetooth/hidp/core.c | 56 +++++++++++++++++++++++++++++++++++++++-------- net/bluetooth/hidp/hidp.h | 2 ++ 2 files changed, 49 insertions(+), 9 deletions(-)