[PATCH 0/2] Bluetooth: hidp: cleanup HID init and re-enable hidinput_input_event callback

[PATCH 0/2] Bluetooth: hidp: cleanup HID init and re-enable hidinput_input_event callback

[Benjamin Tissoires]

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

[PATCH 1/2] Bluetooth: hidp: implement hidinput_input_event callback

[Benjamin Tissoires]

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 <benjamin.tissoires-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 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

Re: [PATCH 1/2] Bluetooth: hidp: implement hidinput_input_event callback

[David Herrmann]

Hi

On Thu, Jul 11, 2013 at 3:41 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> 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 <benjamin.tissoires@redhat.com>
> ---
>  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 <dh.herrmann@gmail.com>

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
>

Re: [PATCH 1/2] Bluetooth: hidp: implement hidinput_input_event callback

[Benjamin Tissoires]

Hi David,

On Thu, Jul 11, 2013 at 4:02 PM, David Herrmann <dh.herrmann@gmail.com> 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 <dh.herrmann@gmail.com>

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
>>

Re: [PATCH 1/2] Bluetooth: hidp: implement hidinput_input_event callback

[David Herrmann]

Hi

On Thu, Jul 11, 2013 at 4:10 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> Hi David,
>
> On Thu, Jul 11, 2013 at 4:02 PM, David Herrmann <dh.herrmann@gmail.com> 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 <dh.herrmann@gmail.com>
>
> 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
>>>

Re: [PATCH 1/2] Bluetooth: hidp: implement hidinput_input_event callback

[Jiri Kosina]

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 <benjamin.tissoires-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Acked-by: Jiri Kosina <jkosina-AlSwsSmVLrQ@public.gmane.org>

> ---
>  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

[PATCH 2/2] Bluetooth: hidp: remove wrong send_report at init

[Benjamin Tissoires]

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 <benjamin.tissoires@redhat.com>
---
 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

Re: [PATCH 2/2] Bluetooth: hidp: remove wrong send_report at init

[David Herrmann]

Hi

On Thu, Jul 11, 2013 at 3:41 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> 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 <dh.herrmann@gmail.com>

Thanks!
David

> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  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
>

Re: [PATCH 2/2] Bluetooth: hidp: remove wrong send_report at init

[Jiri Kosina]

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 <benjamin.tissoires@redhat.com>

Acked-by: Jiri Kosina <jkosina@suse.cz>

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

Re: [PATCH 2/2] Bluetooth: hidp: remove wrong send_report at init

[Gustavo Padovan]

Hi Benjamin,

* Benjamin Tissoires <benjamin.tissoires-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> [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 <benjamin.tissoires-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  net/bluetooth/hidp/core.c | 14 --------------
>  1 file changed, 14 deletions(-)

Both patches have been applied to bluetooth-next. Thanks.

	Gustavo

Re: [PATCH 2/2] Bluetooth: hidp: remove wrong send_report at init

[Benjamin Tissoires]

On 07/12/2013 12:09 PM, Gustavo Padovan wrote:
> Hi Benjamin,
> 
> * Benjamin Tissoires <benjamin.tissoires-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> [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 <benjamin.tissoires-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  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