Return-Path: Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 7.2 \(1874\)) Subject: Re: [PATCH v2] input: uHIDP: HIDP in userspace From: Marcel Holtmann In-Reply-To: Date: Thu, 1 May 2014 10:38:32 -0700 Cc: linux-bluetooth Message-Id: <6CFDB7FC-673C-468F-A901-157746778F43@holtmann.org> References: <20140408011009.55000101498@puck.mtv.corp.google.com> <83DDEFEF-702B-4A41-BBC2-A9CD8F37ECCD@holtmann.org> To: Petri Gynther Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Petri, >>> Enable HID protocol handling in userspace when uHIDP=true in input.conf. >>> --- >>> Makefile.plugins | 3 +- >>> profiles/input/device.c | 677 +++++++++++++++++++++++++++++++++++++++++++-- >>> profiles/input/device.h | 1 + >>> profiles/input/hidp_defs.h | 77 ++++++ >>> profiles/input/input.conf | 4 + >>> profiles/input/manager.c | 9 + >>> 6 files changed, 754 insertions(+), 17 deletions(-) >>> create mode 100644 profiles/input/hidp_defs.h >>> >>> diff --git a/Makefile.plugins b/Makefile.plugins >>> index 6a1ddbf..f0eada9 100644 >>> --- a/Makefile.plugins >>> +++ b/Makefile.plugins >>> @@ -55,7 +55,8 @@ builtin_sources += profiles/network/manager.c \ >>> builtin_modules += input >>> builtin_sources += profiles/input/manager.c \ >>> profiles/input/server.h profiles/input/server.c \ >>> - profiles/input/device.h profiles/input/device.c >>> + profiles/input/device.h profiles/input/device.c \ >>> + profiles/input/uhid_copy.h profiles/input/hidp_defs.h >>> >>> builtin_modules += hog >>> builtin_sources += profiles/input/hog.c profiles/input/uhid_copy.h \ >>> diff --git a/profiles/input/device.c b/profiles/input/device.c >>> index f1fa716..01891f4 100644 >>> --- a/profiles/input/device.c >>> +++ b/profiles/input/device.c >>> @@ -53,9 +53,13 @@ >>> #include "src/sdp-client.h" >>> >>> #include "device.h" >>> +#include "hidp_defs.h" >>> +#include "uhid_copy.h" >>> >>> #define INPUT_INTERFACE "org.bluez.Input1" >>> >>> +#define UHID_DEVICE_FILE "/dev/uhid" >>> + >>> enum reconnect_mode_t { >>> RECONNECT_NONE = 0, >>> RECONNECT_DEVICE, >>> @@ -81,16 +85,30 @@ struct input_device { >>> enum reconnect_mode_t reconnect_mode; >>> guint reconnect_timer; >>> uint32_t reconnect_attempt; >>> + bool uhidp_enabled; >>> + int uhid_fd; >>> + guint uhid_watch; >>> + bool uhid_created; >>> + uint8_t report_req_pending; >>> + guint report_req_timer; >>> + uint32_t report_rsp_id; >>> }; >>> >>> static int idle_timeout = 0; >>> +static bool uhidp_enabled = false; >> >> I am not really in favor of naming this uhidp. I realize how you get to this, but I am not sure this is a good naming. Maybe we should just stick with hidp for this. > > I picked this name following typical kernel approach to naming > something that happens in userspace -- uhid, uinput, udev, etc. > I don't know how to name this better. Do you feel "hidp" is better? > input.conf would then have an item "hidp=" as well. internally we might just call this uhid_enabled. Since that is what we are doing here. We are using uHID. For HoG it just happens to be the only possible option. For the input.conf file, I would propose using UserspaceDriver or maybe UserspaceHID. I am open for better names. I have not come up with a really good one yet. >> And I am just thinking out load, but maybe creating a src/shared/hidp.[ch] as generalized implementation of the HID protocol over L2CAP might be something to think about it a bit. No requirement to do it right away, but something that should be considered. >> > > Sounds good. Action item after initial commit. > >>> >>> void input_set_idle_timeout(int timeout) >>> { >>> idle_timeout = timeout; >>> } >>> >>> +void input_enable_uhidp(bool state) >>> +{ >>> + uhidp_enabled = state; >>> +} >>> + >>> static void input_device_enter_reconnect_mode(struct input_device *idev); >>> +static int connection_disconnect(struct input_device *idev, uint32_t flags); >>> >>> static void input_device_free(struct input_device *idev) >>> { >>> @@ -124,14 +142,189 @@ static void input_device_free(struct input_device *idev) >>> if (idev->reconnect_timer > 0) >>> g_source_remove(idev->reconnect_timer); >>> >>> + if (idev->report_req_timer > 0) >>> + g_source_remove(idev->report_req_timer); >>> + >>> g_free(idev); >>> } >>> >>> +static bool hidp_send_message(GIOChannel *chan, uint8_t hdr, >>> + const uint8_t *data, size_t size) >>> +{ >>> + int fd; >>> + ssize_t len; >>> + uint8_t msg[size + 1]; >>> + >>> + if (data == NULL) >>> + size = 0; >>> + >>> + msg[0] = hdr; >>> + if (size > 0) >>> + memcpy(&msg[1], data, size); >>> + ++size; >>> + >>> + fd = g_io_channel_unix_get_fd(chan); >>> + >>> + len = write(fd, msg, size); >>> + if (len < 0) { >>> + error("BT socket write error: %s (%d)", strerror(errno), errno); >>> + return false; >>> + } >>> + >>> + if (len < size) { >>> + error("BT socket write error: partial write (%zd of %zu bytes)", >>> + len, size); >>> + return false; >>> + } >>> + >>> + return true; >>> +} >>> + >>> +static bool hidp_send_ctrl_message(struct input_device *idev, uint8_t hdr, >>> + const uint8_t *data, size_t size) >>> +{ >>> + return hidp_send_message(idev->ctrl_io, hdr, data, size); >>> +} >>> + >>> +static bool hidp_send_intr_message(struct input_device *idev, uint8_t hdr, >>> + const uint8_t *data, size_t size) >>> +{ >>> + return hidp_send_message(idev->intr_io, hdr, data, size); >>> +} >>> + >>> +static bool uhid_send_feature_answer(struct input_device *idev, >>> + const uint8_t *data, size_t size, >>> + uint32_t id, uint16_t err) >>> +{ >>> + struct uhid_event ev; >>> + ssize_t len; >>> + >>> + if (data == NULL) >>> + size = 0; >>> + >>> + if (size > sizeof(ev.u.feature_answer.data)) >>> + size = sizeof(ev.u.feature_answer.data); >>> + >>> + if (!idev->uhid_created) { >>> + DBG("HID report (%zu bytes) dropped", size); >>> + return false; >>> + } >>> + >>> + memset(&ev, 0, sizeof(ev)); >>> + ev.type = UHID_FEATURE_ANSWER; >>> + ev.u.feature_answer.id = id; >>> + ev.u.feature_answer.err = err; >>> + ev.u.feature_answer.size = size; >>> + >>> + if (size > 0) >>> + memcpy(ev.u.feature_answer.data, data, size); >>> + >>> + len = write(idev->uhid_fd, &ev, sizeof(ev)); >> >> Can we just use writev here and avoid the memcpy? > > I would prefer not to, initially. It gets a bit ugly calculating from > field offsets how much to send from ev and how much from data. I > modeled this based on report_value_cb() in hog.c. Fair point. No problem. We can fix that later. Just keep it in mind. > Also, this function is not getting called that often. It would be more > beneficial for uhid_send_input_report() which is called for input > events. But, it is even uglier there: > > struct uhid_input_req { > __u8 data[UHID_DATA_MAX]; > __u16 size; > } __attribute__((__packed__)); > > struct uhid_event { > __u32 type; > > union { > struct uhid_create_req create; > struct uhid_input_req input; > struct uhid_output_req output; > struct uhid_output_ev_req output_ev; > struct uhid_feature_req feature; > struct uhid_feature_answer_req feature_answer; > } u; > } __attribute__((__packed__)); > > Because the size field is last in uhid_input_req, writev would need 4 > different vectors: type + actual data + rest of data filled with 0s + > size. > > I have actually got this fixed in mainline kernel with new UHID_INPUT2 > + UHID_CREATE2 message types: > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4522643aa9630be17238edf1b4c0b690c5dd7f5d > > Can we optimize these writes later? If we can easily detect UHID_CREATE2 is available in the kernel or not, I am more than happy to go with what you have right now and then move on to a more optimized version. Just make sure it is easy enough to detect the support so we can have a graceful fallback. >> And strictly speaking we should use an asynchronous non-blocking write handling here. Similar to what src/shared/io.[ch] is giving us the correct helpers for. Similar to how we implement it in src/shard/hci.[ch] or src/shared/mgmt.[ch]. >> > > I'm not familiar with those. I just modeled this based on how > report_value_cb() does it in hog.c. We can fix this later on as well. However have a look at how we do a POLLOUT before writing anything to a file descriptor. Long term goal is to turn into using fully asynchronous non-blocking IO for writing. Regards Marcel