Hi guys,
These two patches enhance a little bit the HIDp driver:
- the first one re-implement in a safer way the callback hidinput_input_event,
allowing keyboards leds to be set over bluetooth
- the second one disable the send_report commands called during init, which
are not part of the spec and may fail with some devices (the Wiimote is in
this case, but David guarded it against this problem).
Cheers,
Benjamin
Benjamin Tissoires (2):
Bluetooth: hidp: implement hidinput_input_event callback
Bluetooth: hidp: remove wrong send_report at init
net/bluetooth/hidp/core.c | 40 ++++++++++++++++++++++++++--------------
1 file changed, 26 insertions(+), 14 deletions(-)
--
1.8.3.1
On 07/12/2013 12:09 PM, Gustavo Padovan wrote:
> Hi Benjamin,
>
> * Benjamin Tissoires <[email protected]> [2013-07-11 15:41:30 +0200]:
>
>> The USB hid implementation does retrieve the reports during the start.
>> However, this implementation does not call the HID command GET_REPORT
>> (which would fetch the current status of each report), but use the
>> DATA command, which is an Output Report (so transmitting data from the
>> host to the device).
>> The Wiimote controller is already guarded against this problem in the
>> protocol, but it is not conformant to the specification to set all the
>> reports to 0 on start.
>>
>> Signed-off-by: Benjamin Tissoires <[email protected]>
>> ---
>> net/bluetooth/hidp/core.c | 14 --------------
>> 1 file changed, 14 deletions(-)
>
> Both patches have been applied to bluetooth-next. Thanks.
>
> Gustavo
>
Thanks Gustavo!
Cheers,
Benjamin
Hi Benjamin,
* Benjamin Tissoires <[email protected]> [2013-07-11 15:41:30 +0200]:
> The USB hid implementation does retrieve the reports during the start.
> However, this implementation does not call the HID command GET_REPORT
> (which would fetch the current status of each report), but use the
> DATA command, which is an Output Report (so transmitting data from the
> host to the device).
> The Wiimote controller is already guarded against this problem in the
> protocol, but it is not conformant to the specification to set all the
> reports to 0 on start.
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> net/bluetooth/hidp/core.c | 14 --------------
> 1 file changed, 14 deletions(-)
Both patches have been applied to bluetooth-next. Thanks.
Gustavo
On Thu, 11 Jul 2013, Benjamin Tissoires wrote:
> The USB hid implementation does retrieve the reports during the start.
> However, this implementation does not call the HID command GET_REPORT
> (which would fetch the current status of each report), but use the
> DATA command, which is an Output Report (so transmitting data from the
> host to the device).
> The Wiimote controller is already guarded against this problem in the
> protocol, but it is not conformant to the specification to set all the
> reports to 0 on start.
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
Acked-by: Jiri Kosina <[email protected]>
Gustavo, could you please take it through your tree?
Thanks Benjamin, thanks David.
> ---
> net/bluetooth/hidp/core.c | 14 --------------
> 1 file changed, 14 deletions(-)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index 9c8b50d..59d132a 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -703,20 +703,6 @@ static int hidp_parse(struct hid_device *hid)
>
> static int hidp_start(struct hid_device *hid)
> {
> - struct hidp_session *session = hid->driver_data;
> - struct hid_report *report;
> -
> - if (hid->quirks & HID_QUIRK_NO_INIT_REPORTS)
> - return 0;
> -
> - list_for_each_entry(report, &hid->report_enum[HID_INPUT_REPORT].
> - report_list, list)
> - hidp_send_report(session, report);
> -
> - list_for_each_entry(report, &hid->report_enum[HID_FEATURE_REPORT].
> - report_list, list)
> - hidp_send_report(session, report);
> -
> return 0;
> }
>
> --
> 1.8.3.1
>
--
Jiri Kosina
SUSE Labs
On Thu, 11 Jul 2013, Benjamin Tissoires wrote:
> We can re-enable hidinput_input_event to allow the leds of bluetooth
> keyboards to be set.
> Now the callbacks uses hid core to retrieve the right HID report to
> send, so this version is safer.
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
Acked-by: Jiri Kosina <[email protected]>
> ---
> net/bluetooth/hidp/core.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index f13a8da..9c8b50d 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -238,6 +238,31 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep
> return hidp_send_intr_message(session, hdr, buf, rsize);
> }
>
> +static int hidp_hidinput_event(struct input_dev *dev, unsigned int type,
> + unsigned int code, int value)
> +{
> + struct hid_device *hid = input_get_drvdata(dev);
> + struct hidp_session *session = hid->driver_data;
> + struct hid_field *field;
> + int offset;
> +
> + BT_DBG("session %p type %d code %d value %d",
> + session, type, code, value);
> +
> + if (type != EV_LED)
> + return -1;
> +
> + offset = hidinput_find_field(hid, type, code, &field);
> + if (offset == -1) {
> + hid_warn(dev, "event field not found\n");
> + return -1;
> + }
> +
> + hid_set_field(field, offset, value);
> +
> + return hidp_send_report(session, field->report);
> +}
> +
> static int hidp_get_raw_report(struct hid_device *hid,
> unsigned char report_number,
> unsigned char *data, size_t count,
> @@ -711,6 +736,7 @@ static struct hid_ll_driver hidp_hid_driver = {
> .stop = hidp_stop,
> .open = hidp_open,
> .close = hidp_close,
> + .hidinput_input_event = hidp_hidinput_event,
> };
>
> /* This function sets up the hid device. It does not add it
> --
> 1.8.3.1
>
--
Jiri Kosina
SUSE Labs
Hi
On Thu, Jul 11, 2013 at 4:10 PM, Benjamin Tissoires
<[email protected]> wrote:
> Hi David,
>
> On Thu, Jul 11, 2013 at 4:02 PM, David Herrmann <[email protected]> wrote:
>> Hi
>>
>>> +static int hidp_hidinput_event(struct input_dev *dev, unsigned int type,
>>> + unsigned int code, int value)
>>> +{
>>> + struct hid_device *hid = input_get_drvdata(dev);
>>
>> I dislike that we have to deal with input_get_drvdata() in a transport
>> driver. It is not obvious from the transport driver that this gives us
>> an hid_device. But ok, that's how we do it in usbhid and hid-input so
>> it's another issue..
>
> right. So we should definitively change the callback signature here.
>
>>
>>> + struct hidp_session *session = hid->driver_data;
>>> + struct hid_field *field;
>>> + int offset;
>>> +
>>> + BT_DBG("session %p type %d code %d value %d",
>>> + session, type, code, value);
>>> +
>>> + if (type != EV_LED)
>>> + return -1;
>>> +
>>> + offset = hidinput_find_field(hid, type, code, &field);
>>> + if (offset == -1) {
>>> + hid_warn(dev, "event field not found\n");
>>> + return -1;
>>> + }
>>> +
>>> + hid_set_field(field, offset, value);
>>> +
>>> + return hidp_send_report(session, field->report);
>>> +}
>>> +
>>
>> We had this discussion before (regarding uhid and hidpinput_event), it
>> would be nice to have a helper in hid-input.c which does this. We copy
>> the code into every transport driver, which bugs me. But no-one
>> stepped up to do this, so I am fine.
>
> oh, yes, you are right. I'll add it to my todo list (now that I am
> responsible of 2/4 code duplication :-S )
Feel free to push this series forward unchanged, if it is urgent. I
have an experimental series here which implements a generic
hidinput_input_event() handler (including the scheduled worker) that I
will send later today or tomorrow.
Cheers
David
>>
>> Reviewed-by: David Herrmann <[email protected]>
>
> Thanks for the review!
>
> Cheers,
> Benjamin
>
>>
>> Thanks!
>> David
>>
>>> static int hidp_get_raw_report(struct hid_device *hid,
>>> unsigned char report_number,
>>> unsigned char *data, size_t count,
>>> @@ -711,6 +736,7 @@ static struct hid_ll_driver hidp_hid_driver = {
>>> .stop = hidp_stop,
>>> .open = hidp_open,
>>> .close = hidp_close,
>>> + .hidinput_input_event = hidp_hidinput_event,
>>> };
>>>
>>> /* This function sets up the hid device. It does not add it
>>> --
>>> 1.8.3.1
>>>
Hi David,
On Thu, Jul 11, 2013 at 4:02 PM, David Herrmann <[email protected]> wrote:
> Hi
>
>> +static int hidp_hidinput_event(struct input_dev *dev, unsigned int type,
>> + unsigned int code, int value)
>> +{
>> + struct hid_device *hid = input_get_drvdata(dev);
>
> I dislike that we have to deal with input_get_drvdata() in a transport
> driver. It is not obvious from the transport driver that this gives us
> an hid_device. But ok, that's how we do it in usbhid and hid-input so
> it's another issue..
right. So we should definitively change the callback signature here.
>
>> + struct hidp_session *session = hid->driver_data;
>> + struct hid_field *field;
>> + int offset;
>> +
>> + BT_DBG("session %p type %d code %d value %d",
>> + session, type, code, value);
>> +
>> + if (type != EV_LED)
>> + return -1;
>> +
>> + offset = hidinput_find_field(hid, type, code, &field);
>> + if (offset == -1) {
>> + hid_warn(dev, "event field not found\n");
>> + return -1;
>> + }
>> +
>> + hid_set_field(field, offset, value);
>> +
>> + return hidp_send_report(session, field->report);
>> +}
>> +
>
> We had this discussion before (regarding uhid and hidpinput_event), it
> would be nice to have a helper in hid-input.c which does this. We copy
> the code into every transport driver, which bugs me. But no-one
> stepped up to do this, so I am fine.
oh, yes, you are right. I'll add it to my todo list (now that I am
responsible of 2/4 code duplication :-S )
>
> Reviewed-by: David Herrmann <[email protected]>
Thanks for the review!
Cheers,
Benjamin
>
> Thanks!
> David
>
>> static int hidp_get_raw_report(struct hid_device *hid,
>> unsigned char report_number,
>> unsigned char *data, size_t count,
>> @@ -711,6 +736,7 @@ static struct hid_ll_driver hidp_hid_driver = {
>> .stop = hidp_stop,
>> .open = hidp_open,
>> .close = hidp_close,
>> + .hidinput_input_event = hidp_hidinput_event,
>> };
>>
>> /* This function sets up the hid device. It does not add it
>> --
>> 1.8.3.1
>>
Hi
On Thu, Jul 11, 2013 at 3:41 PM, Benjamin Tissoires
<[email protected]> wrote:
> We can re-enable hidinput_input_event to allow the leds of bluetooth
> keyboards to be set.
> Now the callbacks uses hid core to retrieve the right HID report to
> send, so this version is safer.
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> net/bluetooth/hidp/core.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index f13a8da..9c8b50d 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -238,6 +238,31 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep
> return hidp_send_intr_message(session, hdr, buf, rsize);
> }
>
> +static int hidp_hidinput_event(struct input_dev *dev, unsigned int type,
> + unsigned int code, int value)
> +{
> + struct hid_device *hid = input_get_drvdata(dev);
I dislike that we have to deal with input_get_drvdata() in a transport
driver. It is not obvious from the transport driver that this gives us
an hid_device. But ok, that's how we do it in usbhid and hid-input so
it's another issue..
> + struct hidp_session *session = hid->driver_data;
> + struct hid_field *field;
> + int offset;
> +
> + BT_DBG("session %p type %d code %d value %d",
> + session, type, code, value);
> +
> + if (type != EV_LED)
> + return -1;
> +
> + offset = hidinput_find_field(hid, type, code, &field);
> + if (offset == -1) {
> + hid_warn(dev, "event field not found\n");
> + return -1;
> + }
> +
> + hid_set_field(field, offset, value);
> +
> + return hidp_send_report(session, field->report);
> +}
> +
We had this discussion before (regarding uhid and hidpinput_event), it
would be nice to have a helper in hid-input.c which does this. We copy
the code into every transport driver, which bugs me. But no-one
stepped up to do this, so I am fine.
Reviewed-by: David Herrmann <[email protected]>
Thanks!
David
> static int hidp_get_raw_report(struct hid_device *hid,
> unsigned char report_number,
> unsigned char *data, size_t count,
> @@ -711,6 +736,7 @@ static struct hid_ll_driver hidp_hid_driver = {
> .stop = hidp_stop,
> .open = hidp_open,
> .close = hidp_close,
> + .hidinput_input_event = hidp_hidinput_event,
> };
>
> /* This function sets up the hid device. It does not add it
> --
> 1.8.3.1
>
Hi
On Thu, Jul 11, 2013 at 3:41 PM, Benjamin Tissoires
<[email protected]> wrote:
> The USB hid implementation does retrieve the reports during the start.
> However, this implementation does not call the HID command GET_REPORT
> (which would fetch the current status of each report), but use the
> DATA command, which is an Output Report (so transmitting data from the
> host to the device).
> The Wiimote controller is already guarded against this problem in the
> protocol, but it is not conformant to the specification to set all the
> reports to 0 on start.
I always wondered whether report-setup is really needed for BT-HIDP.
The BT profile doesn't mention it but I thought it was part of the
USBHID core specification.
During hid-wiimote development I added support for
HID_QUIRK_NO_INIT_REPORTS to HIDP to silence the wiimote errors. But
if you say that it's specific to USBHID, I am fine with this. I never
read the USBHID specs, though, I rely on your comment here.
Anyway, code looks good:
Reviewed-by: David Herrmann <[email protected]>
Thanks!
David
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> net/bluetooth/hidp/core.c | 14 --------------
> 1 file changed, 14 deletions(-)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index 9c8b50d..59d132a 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -703,20 +703,6 @@ static int hidp_parse(struct hid_device *hid)
>
> static int hidp_start(struct hid_device *hid)
> {
> - struct hidp_session *session = hid->driver_data;
> - struct hid_report *report;
> -
> - if (hid->quirks & HID_QUIRK_NO_INIT_REPORTS)
> - return 0;
> -
> - list_for_each_entry(report, &hid->report_enum[HID_INPUT_REPORT].
> - report_list, list)
> - hidp_send_report(session, report);
> -
> - list_for_each_entry(report, &hid->report_enum[HID_FEATURE_REPORT].
> - report_list, list)
> - hidp_send_report(session, report);
> -
> return 0;
> }
>
> --
> 1.8.3.1
>
The USB hid implementation does retrieve the reports during the start.
However, this implementation does not call the HID command GET_REPORT
(which would fetch the current status of each report), but use the
DATA command, which is an Output Report (so transmitting data from the
host to the device).
The Wiimote controller is already guarded against this problem in the
protocol, but it is not conformant to the specification to set all the
reports to 0 on start.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
net/bluetooth/hidp/core.c | 14 --------------
1 file changed, 14 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 9c8b50d..59d132a 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -703,20 +703,6 @@ static int hidp_parse(struct hid_device *hid)
static int hidp_start(struct hid_device *hid)
{
- struct hidp_session *session = hid->driver_data;
- struct hid_report *report;
-
- if (hid->quirks & HID_QUIRK_NO_INIT_REPORTS)
- return 0;
-
- list_for_each_entry(report, &hid->report_enum[HID_INPUT_REPORT].
- report_list, list)
- hidp_send_report(session, report);
-
- list_for_each_entry(report, &hid->report_enum[HID_FEATURE_REPORT].
- report_list, list)
- hidp_send_report(session, report);
-
return 0;
}
--
1.8.3.1
We can re-enable hidinput_input_event to allow the leds of bluetooth
keyboards to be set.
Now the callbacks uses hid core to retrieve the right HID report to
send, so this version is safer.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
net/bluetooth/hidp/core.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index f13a8da..9c8b50d 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -238,6 +238,31 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep
return hidp_send_intr_message(session, hdr, buf, rsize);
}
+static int hidp_hidinput_event(struct input_dev *dev, unsigned int type,
+ unsigned int code, int value)
+{
+ struct hid_device *hid = input_get_drvdata(dev);
+ struct hidp_session *session = hid->driver_data;
+ struct hid_field *field;
+ int offset;
+
+ BT_DBG("session %p type %d code %d value %d",
+ session, type, code, value);
+
+ if (type != EV_LED)
+ return -1;
+
+ offset = hidinput_find_field(hid, type, code, &field);
+ if (offset == -1) {
+ hid_warn(dev, "event field not found\n");
+ return -1;
+ }
+
+ hid_set_field(field, offset, value);
+
+ return hidp_send_report(session, field->report);
+}
+
static int hidp_get_raw_report(struct hid_device *hid,
unsigned char report_number,
unsigned char *data, size_t count,
@@ -711,6 +736,7 @@ static struct hid_ll_driver hidp_hid_driver = {
.stop = hidp_stop,
.open = hidp_open,
.close = hidp_close,
+ .hidinput_input_event = hidp_hidinput_event,
};
/* This function sets up the hid device. It does not add it
--
1.8.3.1