[PATCH BlueZ] hog-lib: Increase maximum report map size

[PATCH BlueZ] hog-lib: Increase maximum report map size

[Vicki Pfau]

Though a 512 byte report map size seems plenty large, there exist some devices
(e.g. Brydge W-Touch) that send larger reports. There is no protocol-defined
maximum size so doubling the maximum size is safe, and should hopefully fix
most real-world failures.
---
 profiles/input/hog-lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
index 4a9c60185..9f3eb428c 100644
--- a/profiles/input/hog-lib.c
+++ b/profiles/input/hog-lib.c
@@ -64,7 +64,7 @@
 #define HOG_PROTO_MODE_BOOT    0
 #define HOG_PROTO_MODE_REPORT  1
 
-#define HOG_REPORT_MAP_MAX_SIZE        512
+#define HOG_REPORT_MAP_MAX_SIZE        1024
 #define HID_INFO_SIZE			4
 #define ATT_NOTIFICATION_HEADER_SIZE	3
 
-- 
2.37.1

Re: [PATCH BlueZ] hog-lib: Increase maximum report map size

[Luiz Augusto von Dentz]

Hi Vicki,

On Wed, Aug 3, 2022 at 4:07 PM Vicki Pfau <vi@endrift.com> wrote:
>
> Though a 512 byte report map size seems plenty large, there exist some devices
> (e.g. Brydge W-Touch) that send larger reports. There is no protocol-defined
> maximum size so doubling the maximum size is safe, and should hopefully fix
> most real-world failures.
> ---
>  profiles/input/hog-lib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
> index 4a9c60185..9f3eb428c 100644
> --- a/profiles/input/hog-lib.c
> +++ b/profiles/input/hog-lib.c
> @@ -64,7 +64,7 @@
>  #define HOG_PROTO_MODE_BOOT    0
>  #define HOG_PROTO_MODE_REPORT  1
>
> -#define HOG_REPORT_MAP_MAX_SIZE        512
> +#define HOG_REPORT_MAP_MAX_SIZE        1024
>  #define HID_INFO_SIZE                  4
>  #define ATT_NOTIFICATION_HEADER_SIZE   3

Afaik 512 is the maximum length an attribute can have even when using
read long procedure:

BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 3, Part F
page 1416:

The maximum length of an attribute value shall be 512 octets.

And

BLUETOOTH SPECIFICATION
HID Service Specification
Page 16 of 26

2.6.1 Report Map Characteristic Behavior
The GATT Read Characteristic Value or Read Long Characteristic Values sub-
procedures are used to read the Report Map characteristic value.
The length of the Report Map characteristic value is limited to 512 octets.

So I believe the device is not compliant and very likely needs to have
multiple instances of HID Service instead of combining everything in a
single instance.

> --
> 2.37.1
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ] hog-lib: Increase maximum report map size

[Vicki Pfau]

On 8/3/22 16:55, Luiz Augusto von Dentz wrote:
> Hi Vicki,
> 
> On Wed, Aug 3, 2022 at 4:07 PM Vicki Pfau <vi@endrift.com> wrote:
>>
>> Though a 512 byte report map size seems plenty large, there exist some devices
>> (e.g. Brydge W-Touch) that send larger reports. There is no protocol-defined
>> maximum size so doubling the maximum size is safe, and should hopefully fix
>> most real-world failures.
>> ---
>>  profiles/input/hog-lib.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
>> index 4a9c60185..9f3eb428c 100644
>> --- a/profiles/input/hog-lib.c
>> +++ b/profiles/input/hog-lib.c
>> @@ -64,7 +64,7 @@
>>  #define HOG_PROTO_MODE_BOOT    0
>>  #define HOG_PROTO_MODE_REPORT  1
>>
>> -#define HOG_REPORT_MAP_MAX_SIZE        512
>> +#define HOG_REPORT_MAP_MAX_SIZE        1024
>>  #define HID_INFO_SIZE                  4
>>  #define ATT_NOTIFICATION_HEADER_SIZE   3
> 
> Afaik 512 is the maximum length an attribute can have even when using
> read long procedure:
> 
> BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 3, Part F
> page 1416:
> 
> The maximum length of an attribute value shall be 512 octets.
> 
> And
> 
> BLUETOOTH SPECIFICATION
> HID Service Specification
> Page 16 of 26
> 
> 2.6.1 Report Map Characteristic Behavior
> The GATT Read Characteristic Value or Read Long Characteristic Values sub-
> procedures are used to read the Report Map characteristic value.
> The length of the Report Map characteristic value is limited to 512 octets.
> 
> So I believe the device is not compliant and very likely needs to have
> multiple instances of HID Service instead of combining everything in a
> single instance.
> 
>> --
>> 2.37.1
>>
> 
> 

Ah, that's strange. I looked through the spec but didn't see those. That said, while the device may be non-compliant, the device is on the market and I doubt I could get them to update the firmware as a random third party. It works on Windows, so clearly Windows doesn't have a problem with its noncompliance. So this raises the question, how should Linux handle non-compliant hardware, especially when it could easily be made to work just by bending the rules in this one instance? I can absolutely change the commit message since it's erroneous, but the question then comes down to how should it be handled at all.

Re: [PATCH BlueZ] hog-lib: Increase maximum report map size

[Luiz Augusto von Dentz]

Hi Vicki,

On Wed, Aug 3, 2022 at 5:05 PM Vicki Pfau <vi@endrift.com> wrote:
>
>
>
> On 8/3/22 16:55, Luiz Augusto von Dentz wrote:
> > Hi Vicki,
> >
> > On Wed, Aug 3, 2022 at 4:07 PM Vicki Pfau <vi@endrift.com> wrote:
> >>
> >> Though a 512 byte report map size seems plenty large, there exist some devices
> >> (e.g. Brydge W-Touch) that send larger reports. There is no protocol-defined
> >> maximum size so doubling the maximum size is safe, and should hopefully fix
> >> most real-world failures.
> >> ---
> >>  profiles/input/hog-lib.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
> >> index 4a9c60185..9f3eb428c 100644
> >> --- a/profiles/input/hog-lib.c
> >> +++ b/profiles/input/hog-lib.c
> >> @@ -64,7 +64,7 @@
> >>  #define HOG_PROTO_MODE_BOOT    0
> >>  #define HOG_PROTO_MODE_REPORT  1
> >>
> >> -#define HOG_REPORT_MAP_MAX_SIZE        512
> >> +#define HOG_REPORT_MAP_MAX_SIZE        1024
> >>  #define HID_INFO_SIZE                  4
> >>  #define ATT_NOTIFICATION_HEADER_SIZE   3
> >
> > Afaik 512 is the maximum length an attribute can have even when using
> > read long procedure:
> >
> > BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 3, Part F
> > page 1416:
> >
> > The maximum length of an attribute value shall be 512 octets.
> >
> > And
> >
> > BLUETOOTH SPECIFICATION
> > HID Service Specification
> > Page 16 of 26
> >
> > 2.6.1 Report Map Characteristic Behavior
> > The GATT Read Characteristic Value or Read Long Characteristic Values sub-
> > procedures are used to read the Report Map characteristic value.
> > The length of the Report Map characteristic value is limited to 512 octets.
> >
> > So I believe the device is not compliant and very likely needs to have
> > multiple instances of HID Service instead of combining everything in a
> > single instance.
> >
> >> --
> >> 2.37.1
> >>
> >
> >
>
> Ah, that's strange. I looked through the spec but didn't see those. That said, while the device may be non-compliant, the device is on the market and I doubt I could get them to update the firmware as a random third party. It works on Windows, so clearly Windows doesn't have a problem with its noncompliance. So this raises the question, how should Linux handle non-compliant hardware, especially when it could easily be made to work just by bending the rules in this one instance? I can absolutely change the commit message since it's erroneous, but the question then comes down to how should it be handled at all.

While I agree this could be worked around it is probably worth
checking with the manufacturer if it is aware of the problem because
even if we were to allow reading past 512 bytes offset in the future
there may be qualification tests enforcing not to do so, besides
versions up to BlueZ 5.65 would still not work anyway so I thing
letting the manufacturer know there is a problem with their
implementation is actually worth a shot here.

-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ] hog-lib: Increase maximum report map size

[Vicki Pfau]

On 8/3/22 17:16, Luiz Augusto von Dentz wrote:
> Hi Vicki,
> 
> On Wed, Aug 3, 2022 at 5:05 PM Vicki Pfau <vi@endrift.com> wrote:
>>
>>
>>
>> On 8/3/22 16:55, Luiz Augusto von Dentz wrote:
>>> Hi Vicki,
>>>
>>> On Wed, Aug 3, 2022 at 4:07 PM Vicki Pfau <vi@endrift.com> wrote:
>>>>
>>>> Though a 512 byte report map size seems plenty large, there exist some devices
>>>> (e.g. Brydge W-Touch) that send larger reports. There is no protocol-defined
>>>> maximum size so doubling the maximum size is safe, and should hopefully fix
>>>> most real-world failures.
>>>> ---
>>>>  profiles/input/hog-lib.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
>>>> index 4a9c60185..9f3eb428c 100644
>>>> --- a/profiles/input/hog-lib.c
>>>> +++ b/profiles/input/hog-lib.c
>>>> @@ -64,7 +64,7 @@
>>>>  #define HOG_PROTO_MODE_BOOT    0
>>>>  #define HOG_PROTO_MODE_REPORT  1
>>>>
>>>> -#define HOG_REPORT_MAP_MAX_SIZE        512
>>>> +#define HOG_REPORT_MAP_MAX_SIZE        1024
>>>>  #define HID_INFO_SIZE                  4
>>>>  #define ATT_NOTIFICATION_HEADER_SIZE   3
>>>
>>> Afaik 512 is the maximum length an attribute can have even when using
>>> read long procedure:
>>>
>>> BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 3, Part F
>>> page 1416:
>>>
>>> The maximum length of an attribute value shall be 512 octets.
>>>
>>> And
>>>
>>> BLUETOOTH SPECIFICATION
>>> HID Service Specification
>>> Page 16 of 26
>>>
>>> 2.6.1 Report Map Characteristic Behavior
>>> The GATT Read Characteristic Value or Read Long Characteristic Values sub-
>>> procedures are used to read the Report Map characteristic value.
>>> The length of the Report Map characteristic value is limited to 512 octets.
>>>
>>> So I believe the device is not compliant and very likely needs to have
>>> multiple instances of HID Service instead of combining everything in a
>>> single instance.
>>>
>>>> --
>>>> 2.37.1
>>>>
>>>
>>>
>>
>> Ah, that's strange. I looked through the spec but didn't see those. That said, while the device may be non-compliant, the device is on the market and I doubt I could get them to update the firmware as a random third party. It works on Windows, so clearly Windows doesn't have a problem with its noncompliance. So this raises the question, how should Linux handle non-compliant hardware, especially when it could easily be made to work just by bending the rules in this one instance? I can absolutely change the commit message since it's erroneous, but the question then comes down to how should it be handled at all.
> 
> While I agree this could be worked around it is probably worth
> checking with the manufacturer if it is aware of the problem because
> even if we were to allow reading past 512 bytes offset in the future
> there may be qualification tests enforcing not to do so, besides
> versions up to BlueZ 5.65 would still not work anyway so I thing
> letting the manufacturer know there is a problem with their
> implementation is actually worth a shot here.
> 

That's fair enough. I'll see if I can find an email address for them.

Re: [PATCH BlueZ] hog-lib: Increase maximum report map size

[Vicki Pfau]

On 8/3/22 17:16, Luiz Augusto von Dentz wrote:
> Hi Vicki,
> 
> On Wed, Aug 3, 2022 at 5:05 PM Vicki Pfau <vi@endrift.com> wrote:
>>
>>
>>
>> On 8/3/22 16:55, Luiz Augusto von Dentz wrote:
>>> Hi Vicki,
>>>
>>> On Wed, Aug 3, 2022 at 4:07 PM Vicki Pfau <vi@endrift.com> wrote:
>>>>
>>>> Though a 512 byte report map size seems plenty large, there exist some devices
>>>> (e.g. Brydge W-Touch) that send larger reports. There is no protocol-defined
>>>> maximum size so doubling the maximum size is safe, and should hopefully fix
>>>> most real-world failures.
>>>> ---
>>>>  profiles/input/hog-lib.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
>>>> index 4a9c60185..9f3eb428c 100644
>>>> --- a/profiles/input/hog-lib.c
>>>> +++ b/profiles/input/hog-lib.c
>>>> @@ -64,7 +64,7 @@
>>>>  #define HOG_PROTO_MODE_BOOT    0
>>>>  #define HOG_PROTO_MODE_REPORT  1
>>>>
>>>> -#define HOG_REPORT_MAP_MAX_SIZE        512
>>>> +#define HOG_REPORT_MAP_MAX_SIZE        1024
>>>>  #define HID_INFO_SIZE                  4
>>>>  #define ATT_NOTIFICATION_HEADER_SIZE   3
>>>
>>> Afaik 512 is the maximum length an attribute can have even when using
>>> read long procedure:
>>>
>>> BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 3, Part F
>>> page 1416:
>>>
>>> The maximum length of an attribute value shall be 512 octets.
>>>
>>> And
>>>
>>> BLUETOOTH SPECIFICATION
>>> HID Service Specification
>>> Page 16 of 26
>>>
>>> 2.6.1 Report Map Characteristic Behavior
>>> The GATT Read Characteristic Value or Read Long Characteristic Values sub-
>>> procedures are used to read the Report Map characteristic value.
>>> The length of the Report Map characteristic value is limited to 512 octets.
>>>
>>> So I believe the device is not compliant and very likely needs to have
>>> multiple instances of HID Service instead of combining everything in a
>>> single instance.
>>>
>>>> --
>>>> 2.37.1
>>>>
>>>
>>>
>>
>> Ah, that's strange. I looked through the spec but didn't see those. That said, while the device may be non-compliant, the device is on the market and I doubt I could get them to update the firmware as a random third party. It works on Windows, so clearly Windows doesn't have a problem with its noncompliance. So this raises the question, how should Linux handle non-compliant hardware, especially when it could easily be made to work just by bending the rules in this one instance? I can absolutely change the commit message since it's erroneous, but the question then comes down to how should it be handled at all.
> 
> While I agree this could be worked around it is probably worth
> checking with the manufacturer if it is aware of the problem because
> even if we were to allow reading past 512 bytes offset in the future
> there may be qualification tests enforcing not to do so, besides
> versions up to BlueZ 5.65 would still not work anyway so I thing
> letting the manufacturer know there is a problem with their
> implementation is actually worth a shot here.
> 
Brydge replied with the standard tech support "this is only supported on Windows, so there probably won't be a firmware update" reply, despite its noncompliance. And since I doubt Windows will add a change to limit it, well, that kind of limits our options here to either "enforce compliance and break non-compliant hardware" or "figure out a way to bend the rules". Given that BlueZ, upon expanding the maximum size, does successfully read the overly-long report map (it does use the read blob with offset message to get the last several bytes), it does work as intended if we ignore that specific rule. Though obviously that's up to the bluetooth maintainers to solve, so at this point I'm just tossing my two cents at it.

Re: [PATCH BlueZ] hog-lib: Increase maximum report map size

[Luiz Augusto von Dentz]

Hi Vicki,

On Tue, Aug 9, 2022 at 7:02 PM Vicki Pfau <vi@endrift.com> wrote:
>
>
>
> On 8/3/22 17:16, Luiz Augusto von Dentz wrote:
> > Hi Vicki,
> >
> > On Wed, Aug 3, 2022 at 5:05 PM Vicki Pfau <vi@endrift.com> wrote:
> >>
> >>
> >>
> >> On 8/3/22 16:55, Luiz Augusto von Dentz wrote:
> >>> Hi Vicki,
> >>>
> >>> On Wed, Aug 3, 2022 at 4:07 PM Vicki Pfau <vi@endrift.com> wrote:
> >>>>
> >>>> Though a 512 byte report map size seems plenty large, there exist some devices
> >>>> (e.g. Brydge W-Touch) that send larger reports. There is no protocol-defined
> >>>> maximum size so doubling the maximum size is safe, and should hopefully fix
> >>>> most real-world failures.
> >>>> ---
> >>>>  profiles/input/hog-lib.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
> >>>> index 4a9c60185..9f3eb428c 100644
> >>>> --- a/profiles/input/hog-lib.c
> >>>> +++ b/profiles/input/hog-lib.c
> >>>> @@ -64,7 +64,7 @@
> >>>>  #define HOG_PROTO_MODE_BOOT    0
> >>>>  #define HOG_PROTO_MODE_REPORT  1
> >>>>
> >>>> -#define HOG_REPORT_MAP_MAX_SIZE        512
> >>>> +#define HOG_REPORT_MAP_MAX_SIZE        1024
> >>>>  #define HID_INFO_SIZE                  4
> >>>>  #define ATT_NOTIFICATION_HEADER_SIZE   3
> >>>
> >>> Afaik 512 is the maximum length an attribute can have even when using
> >>> read long procedure:
> >>>
> >>> BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 3, Part F
> >>> page 1416:
> >>>
> >>> The maximum length of an attribute value shall be 512 octets.
> >>>
> >>> And
> >>>
> >>> BLUETOOTH SPECIFICATION
> >>> HID Service Specification
> >>> Page 16 of 26
> >>>
> >>> 2.6.1 Report Map Characteristic Behavior
> >>> The GATT Read Characteristic Value or Read Long Characteristic Values sub-
> >>> procedures are used to read the Report Map characteristic value.
> >>> The length of the Report Map characteristic value is limited to 512 octets.
> >>>
> >>> So I believe the device is not compliant and very likely needs to have
> >>> multiple instances of HID Service instead of combining everything in a
> >>> single instance.
> >>>
> >>>> --
> >>>> 2.37.1
> >>>>
> >>>
> >>>
> >>
> >> Ah, that's strange. I looked through the spec but didn't see those. That said, while the device may be non-compliant, the device is on the market and I doubt I could get them to update the firmware as a random third party. It works on Windows, so clearly Windows doesn't have a problem with its noncompliance. So this raises the question, how should Linux handle non-compliant hardware, especially when it could easily be made to work just by bending the rules in this one instance? I can absolutely change the commit message since it's erroneous, but the question then comes down to how should it be handled at all.
> >
> > While I agree this could be worked around it is probably worth
> > checking with the manufacturer if it is aware of the problem because
> > even if we were to allow reading past 512 bytes offset in the future
> > there may be qualification tests enforcing not to do so, besides
> > versions up to BlueZ 5.65 would still not work anyway so I thing
> > letting the manufacturer know there is a problem with their
> > implementation is actually worth a shot here.
> >
> Brydge replied with the standard tech support "this is only supported on Windows, so there probably won't be a firmware update" reply, despite its noncompliance. And since I doubt Windows will add a change to limit it, well, that kind of limits our options here to either "enforce compliance and break non-compliant hardware" or "figure out a way to bend the rules". Given that BlueZ, upon expanding the maximum size, does successfully read the overly-long report map (it does use the read blob with offset message to get the last several bytes), it does work as intended if we ignore that specific rule. Though obviously that's up to the bluetooth maintainers to solve, so at this point I'm just tossing my two cents at it.

Well we can do what Windows is doing but let's have it documented
then, we could as well scalate this with Bluetooth SIG since the
manufacturer seem to be intentionally creating interoperability
problems with a standard service, but that would likely take a lot
more time and most likely will need to be resolved by introducing a
test that enforces that HoG client don't attempt to read past 512
bytes offset.

Btw, if there is no limitation on how big the report map can get in
HID, I'd leave it up to the kernel to figure out if that is acceptable
or not, so I'd remove the 1024.

-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ] hog-lib: Increase maximum report map size

[Luiz Augusto von Dentz]

Hi Vicki,

On Wed, Aug 10, 2022 at 1:13 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Vicki,
>
> On Tue, Aug 9, 2022 at 7:02 PM Vicki Pfau <vi@endrift.com> wrote:
> >
> >
> >
> > On 8/3/22 17:16, Luiz Augusto von Dentz wrote:
> > > Hi Vicki,
> > >
> > > On Wed, Aug 3, 2022 at 5:05 PM Vicki Pfau <vi@endrift.com> wrote:
> > >>
> > >>
> > >>
> > >> On 8/3/22 16:55, Luiz Augusto von Dentz wrote:
> > >>> Hi Vicki,
> > >>>
> > >>> On Wed, Aug 3, 2022 at 4:07 PM Vicki Pfau <vi@endrift.com> wrote:
> > >>>>
> > >>>> Though a 512 byte report map size seems plenty large, there exist some devices
> > >>>> (e.g. Brydge W-Touch) that send larger reports. There is no protocol-defined
> > >>>> maximum size so doubling the maximum size is safe, and should hopefully fix
> > >>>> most real-world failures.
> > >>>> ---
> > >>>>  profiles/input/hog-lib.c | 2 +-
> > >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>>
> > >>>> diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
> > >>>> index 4a9c60185..9f3eb428c 100644
> > >>>> --- a/profiles/input/hog-lib.c
> > >>>> +++ b/profiles/input/hog-lib.c
> > >>>> @@ -64,7 +64,7 @@
> > >>>>  #define HOG_PROTO_MODE_BOOT    0
> > >>>>  #define HOG_PROTO_MODE_REPORT  1
> > >>>>
> > >>>> -#define HOG_REPORT_MAP_MAX_SIZE        512
> > >>>> +#define HOG_REPORT_MAP_MAX_SIZE        1024
> > >>>>  #define HID_INFO_SIZE                  4
> > >>>>  #define ATT_NOTIFICATION_HEADER_SIZE   3
> > >>>
> > >>> Afaik 512 is the maximum length an attribute can have even when using
> > >>> read long procedure:
> > >>>
> > >>> BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 3, Part F
> > >>> page 1416:
> > >>>
> > >>> The maximum length of an attribute value shall be 512 octets.
> > >>>
> > >>> And
> > >>>
> > >>> BLUETOOTH SPECIFICATION
> > >>> HID Service Specification
> > >>> Page 16 of 26
> > >>>
> > >>> 2.6.1 Report Map Characteristic Behavior
> > >>> The GATT Read Characteristic Value or Read Long Characteristic Values sub-
> > >>> procedures are used to read the Report Map characteristic value.
> > >>> The length of the Report Map characteristic value is limited to 512 octets.
> > >>>
> > >>> So I believe the device is not compliant and very likely needs to have
> > >>> multiple instances of HID Service instead of combining everything in a
> > >>> single instance.
> > >>>
> > >>>> --
> > >>>> 2.37.1
> > >>>>
> > >>>
> > >>>
> > >>
> > >> Ah, that's strange. I looked through the spec but didn't see those. That said, while the device may be non-compliant, the device is on the market and I doubt I could get them to update the firmware as a random third party. It works on Windows, so clearly Windows doesn't have a problem with its noncompliance. So this raises the question, how should Linux handle non-compliant hardware, especially when it could easily be made to work just by bending the rules in this one instance? I can absolutely change the commit message since it's erroneous, but the question then comes down to how should it be handled at all.
> > >
> > > While I agree this could be worked around it is probably worth
> > > checking with the manufacturer if it is aware of the problem because
> > > even if we were to allow reading past 512 bytes offset in the future
> > > there may be qualification tests enforcing not to do so, besides
> > > versions up to BlueZ 5.65 would still not work anyway so I thing
> > > letting the manufacturer know there is a problem with their
> > > implementation is actually worth a shot here.
> > >
> > Brydge replied with the standard tech support "this is only supported on Windows, so there probably won't be a firmware update" reply, despite its noncompliance. And since I doubt Windows will add a change to limit it, well, that kind of limits our options here to either "enforce compliance and break non-compliant hardware" or "figure out a way to bend the rules". Given that BlueZ, upon expanding the maximum size, does successfully read the overly-long report map (it does use the read blob with offset message to get the last several bytes), it does work as intended if we ignore that specific rule. Though obviously that's up to the bluetooth maintainers to solve, so at this point I'm just tossing my two cents at it.
>
> Well we can do what Windows is doing but let's have it documented
> then, we could as well scalate this with Bluetooth SIG since the
> manufacturer seem to be intentionally creating interoperability
> problems with a standard service, but that would likely take a lot
> more time and most likely will need to be resolved by introducing a
> test that enforces that HoG client don't attempt to read past 512
> bytes offset.
>
> Btw, if there is no limitation on how big the report map can get in
> HID, I'd leave it up to the kernel to figure out if that is acceptable
> or not, so I'd remove the 1024.

Are you still planning to update this change? Btw, how about Android,
does it handle such devices?

-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ] hog-lib: Increase maximum report map size

[Vicki Pfau]

Hi,

On 8/15/22 11:09, Luiz Augusto von Dentz wrote:
> Hi Vicki,
> 
> On Wed, Aug 10, 2022 at 1:13 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>>
>> Hi Vicki,
>>
>> On Tue, Aug 9, 2022 at 7:02 PM Vicki Pfau <vi@endrift.com> wrote:
>>>
>>>
>>>
>>> On 8/3/22 17:16, Luiz Augusto von Dentz wrote:
>>>> Hi Vicki,
>>>>
>>>> On Wed, Aug 3, 2022 at 5:05 PM Vicki Pfau <vi@endrift.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 8/3/22 16:55, Luiz Augusto von Dentz wrote:
>>>>>> Hi Vicki,
>>>>>>
>>>>>> On Wed, Aug 3, 2022 at 4:07 PM Vicki Pfau <vi@endrift.com> wrote:
>>>>>>>
>>>>>>> Though a 512 byte report map size seems plenty large, there exist some devices
>>>>>>> (e.g. Brydge W-Touch) that send larger reports. There is no protocol-defined
>>>>>>> maximum size so doubling the maximum size is safe, and should hopefully fix
>>>>>>> most real-world failures.
>>>>>>> ---
>>>>>>>  profiles/input/hog-lib.c | 2 +-
>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
>>>>>>> index 4a9c60185..9f3eb428c 100644
>>>>>>> --- a/profiles/input/hog-lib.c
>>>>>>> +++ b/profiles/input/hog-lib.c
>>>>>>> @@ -64,7 +64,7 @@
>>>>>>>  #define HOG_PROTO_MODE_BOOT    0
>>>>>>>  #define HOG_PROTO_MODE_REPORT  1
>>>>>>>
>>>>>>> -#define HOG_REPORT_MAP_MAX_SIZE        512
>>>>>>> +#define HOG_REPORT_MAP_MAX_SIZE        1024
>>>>>>>  #define HID_INFO_SIZE                  4
>>>>>>>  #define ATT_NOTIFICATION_HEADER_SIZE   3
>>>>>>
>>>>>> Afaik 512 is the maximum length an attribute can have even when using
>>>>>> read long procedure:
>>>>>>
>>>>>> BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 3, Part F
>>>>>> page 1416:
>>>>>>
>>>>>> The maximum length of an attribute value shall be 512 octets.
>>>>>>
>>>>>> And
>>>>>>
>>>>>> BLUETOOTH SPECIFICATION
>>>>>> HID Service Specification
>>>>>> Page 16 of 26
>>>>>>
>>>>>> 2.6.1 Report Map Characteristic Behavior
>>>>>> The GATT Read Characteristic Value or Read Long Characteristic Values sub-
>>>>>> procedures are used to read the Report Map characteristic value.
>>>>>> The length of the Report Map characteristic value is limited to 512 octets.
>>>>>>
>>>>>> So I believe the device is not compliant and very likely needs to have
>>>>>> multiple instances of HID Service instead of combining everything in a
>>>>>> single instance.
>>>>>>
>>>>>>> --
>>>>>>> 2.37.1
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> Ah, that's strange. I looked through the spec but didn't see those. That said, while the device may be non-compliant, the device is on the market and I doubt I could get them to update the firmware as a random third party. It works on Windows, so clearly Windows doesn't have a problem with its noncompliance. So this raises the question, how should Linux handle non-compliant hardware, especially when it could easily be made to work just by bending the rules in this one instance? I can absolutely change the commit message since it's erroneous, but the question then comes down to how should it be handled at all.
>>>>
>>>> While I agree this could be worked around it is probably worth
>>>> checking with the manufacturer if it is aware of the problem because
>>>> even if we were to allow reading past 512 bytes offset in the future
>>>> there may be qualification tests enforcing not to do so, besides
>>>> versions up to BlueZ 5.65 would still not work anyway so I thing
>>>> letting the manufacturer know there is a problem with their
>>>> implementation is actually worth a shot here.
>>>>
>>> Brydge replied with the standard tech support "this is only supported on Windows, so there probably won't be a firmware update" reply, despite its noncompliance. And since I doubt Windows will add a change to limit it, well, that kind of limits our options here to either "enforce compliance and break non-compliant hardware" or "figure out a way to bend the rules". Given that BlueZ, upon expanding the maximum size, does successfully read the overly-long report map (it does use the read blob with offset message to get the last several bytes), it does work as intended if we ignore that specific rule. Though obviously that's up to the bluetooth maintainers to solve, so at this point I'm just tossing my two cents at it.
>>
>> Well we can do what Windows is doing but let's have it documented
>> then, we could as well scalate this with Bluetooth SIG since the
>> manufacturer seem to be intentionally creating interoperability
>> problems with a standard service, but that would likely take a lot
>> more time and most likely will need to be resolved by introducing a
>> test that enforces that HoG client don't attempt to read past 512
>> bytes offset.

Is the client the device or the computer? Because the computer does successfully read past 512 bytes, and I believe it's needed for this device.

>>
>> Btw, if there is no limitation on how big the report map can get in
>> HID, I'd leave it up to the kernel to figure out if that is acceptable
>> or not, so I'd remove the 1024.

That makes sense. I can look into how to arbitrarily size that buffer.

> 
> Are you still planning to update this change? Btw, how about Android,
> does it handle such devices?
> 

Ah sorry, your reply was somewhat open ended and I didn't realize you wanted me to specifically look into this. As for Android, I have no idea. I can try pairing the trackpad with an Android device and seeing what happens, but I don't have anything with modern Android as it's not my day-to-day phone OS.

Re: [PATCH BlueZ] hog-lib: Increase maximum report map size

[Luiz Augusto von Dentz]

Hi Vicki,

On Mon, Aug 15, 2022 at 6:49 PM Vicki Pfau <vi@endrift.com> wrote:
>
> Hi,
>
> On 8/15/22 11:09, Luiz Augusto von Dentz wrote:
> > Hi Vicki,
> >
> > On Wed, Aug 10, 2022 at 1:13 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> >>
> >> Hi Vicki,
> >>
> >> On Tue, Aug 9, 2022 at 7:02 PM Vicki Pfau <vi@endrift.com> wrote:
> >>>
> >>>
> >>>
> >>> On 8/3/22 17:16, Luiz Augusto von Dentz wrote:
> >>>> Hi Vicki,
> >>>>
> >>>> On Wed, Aug 3, 2022 at 5:05 PM Vicki Pfau <vi@endrift.com> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 8/3/22 16:55, Luiz Augusto von Dentz wrote:
> >>>>>> Hi Vicki,
> >>>>>>
> >>>>>> On Wed, Aug 3, 2022 at 4:07 PM Vicki Pfau <vi@endrift.com> wrote:
> >>>>>>>
> >>>>>>> Though a 512 byte report map size seems plenty large, there exist some devices
> >>>>>>> (e.g. Brydge W-Touch) that send larger reports. There is no protocol-defined
> >>>>>>> maximum size so doubling the maximum size is safe, and should hopefully fix
> >>>>>>> most real-world failures.
> >>>>>>> ---
> >>>>>>>  profiles/input/hog-lib.c | 2 +-
> >>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
> >>>>>>> index 4a9c60185..9f3eb428c 100644
> >>>>>>> --- a/profiles/input/hog-lib.c
> >>>>>>> +++ b/profiles/input/hog-lib.c
> >>>>>>> @@ -64,7 +64,7 @@
> >>>>>>>  #define HOG_PROTO_MODE_BOOT    0
> >>>>>>>  #define HOG_PROTO_MODE_REPORT  1
> >>>>>>>
> >>>>>>> -#define HOG_REPORT_MAP_MAX_SIZE        512
> >>>>>>> +#define HOG_REPORT_MAP_MAX_SIZE        1024
> >>>>>>>  #define HID_INFO_SIZE                  4
> >>>>>>>  #define ATT_NOTIFICATION_HEADER_SIZE   3
> >>>>>>
> >>>>>> Afaik 512 is the maximum length an attribute can have even when using
> >>>>>> read long procedure:
> >>>>>>
> >>>>>> BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 3, Part F
> >>>>>> page 1416:
> >>>>>>
> >>>>>> The maximum length of an attribute value shall be 512 octets.
> >>>>>>
> >>>>>> And
> >>>>>>
> >>>>>> BLUETOOTH SPECIFICATION
> >>>>>> HID Service Specification
> >>>>>> Page 16 of 26
> >>>>>>
> >>>>>> 2.6.1 Report Map Characteristic Behavior
> >>>>>> The GATT Read Characteristic Value or Read Long Characteristic Values sub-
> >>>>>> procedures are used to read the Report Map characteristic value.
> >>>>>> The length of the Report Map characteristic value is limited to 512 octets.
> >>>>>>
> >>>>>> So I believe the device is not compliant and very likely needs to have
> >>>>>> multiple instances of HID Service instead of combining everything in a
> >>>>>> single instance.
> >>>>>>
> >>>>>>> --
> >>>>>>> 2.37.1
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>>> Ah, that's strange. I looked through the spec but didn't see those. That said, while the device may be non-compliant, the device is on the market and I doubt I could get them to update the firmware as a random third party. It works on Windows, so clearly Windows doesn't have a problem with its noncompliance. So this raises the question, how should Linux handle non-compliant hardware, especially when it could easily be made to work just by bending the rules in this one instance? I can absolutely change the commit message since it's erroneous, but the question then comes down to how should it be handled at all.
> >>>>
> >>>> While I agree this could be worked around it is probably worth
> >>>> checking with the manufacturer if it is aware of the problem because
> >>>> even if we were to allow reading past 512 bytes offset in the future
> >>>> there may be qualification tests enforcing not to do so, besides
> >>>> versions up to BlueZ 5.65 would still not work anyway so I thing
> >>>> letting the manufacturer know there is a problem with their
> >>>> implementation is actually worth a shot here.
> >>>>
> >>> Brydge replied with the standard tech support "this is only supported on Windows, so there probably won't be a firmware update" reply, despite its noncompliance. And since I doubt Windows will add a change to limit it, well, that kind of limits our options here to either "enforce compliance and break non-compliant hardware" or "figure out a way to bend the rules". Given that BlueZ, upon expanding the maximum size, does successfully read the overly-long report map (it does use the read blob with offset message to get the last several bytes), it does work as intended if we ignore that specific rule. Though obviously that's up to the bluetooth maintainers to solve, so at this point I'm just tossing my two cents at it.
> >>
> >> Well we can do what Windows is doing but let's have it documented
> >> then, we could as well scalate this with Bluetooth SIG since the
> >> manufacturer seem to be intentionally creating interoperability
> >> problems with a standard service, but that would likely take a lot
> >> more time and most likely will need to be resolved by introducing a
> >> test that enforces that HoG client don't attempt to read past 512
> >> bytes offset.
>
> Is the client the device or the computer? Because the computer does successfully read past 512 bytes, and I believe it's needed for this device.
>
> >>
> >> Btw, if there is no limitation on how big the report map can get in
> >> HID, I'd leave it up to the kernel to figure out if that is acceptable
> >> or not, so I'd remove the 1024.
>
> That makes sense. I can look into how to arbitrarily size that buffer.
>
> >
> > Are you still planning to update this change? Btw, how about Android,
> > does it handle such devices?
> >
>
> Ah sorry, your reply was somewhat open ended and I didn't realize you wanted me to specifically look into this. As for Android, I have no idea. I can try pairing the trackpad with an Android device and seeing what happens, but I don't have anything with modern Android as it's not my day-to-day phone OS.

Well iOS would do as well, what Im after is if that is in fact never
enforced by other major OS, I do see there have been some tickets
created about that already so Im trying to get some response, please
forward this to the manufactures:

https://www.bluetooth.org/errata/errata_view.cfm?errata_id=17495

In the meantime there is yet another device that appear to have similar problem:

https://github.com/bluez/bluez/pull/376

-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ] hog-lib: Increase maximum report map size

[Luiz Augusto von Dentz]

Hi Vicki,

On Tue, Aug 16, 2022 at 11:31 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Vicki,
>
> On Mon, Aug 15, 2022 at 6:49 PM Vicki Pfau <vi@endrift.com> wrote:
> >
> > Hi,
> >
> > On 8/15/22 11:09, Luiz Augusto von Dentz wrote:
> > > Hi Vicki,
> > >
> > > On Wed, Aug 10, 2022 at 1:13 PM Luiz Augusto von Dentz
> > > <luiz.dentz@gmail.com> wrote:
> > >>
> > >> Hi Vicki,
> > >>
> > >> On Tue, Aug 9, 2022 at 7:02 PM Vicki Pfau <vi@endrift.com> wrote:
> > >>>
> > >>>
> > >>>
> > >>> On 8/3/22 17:16, Luiz Augusto von Dentz wrote:
> > >>>> Hi Vicki,
> > >>>>
> > >>>> On Wed, Aug 3, 2022 at 5:05 PM Vicki Pfau <vi@endrift.com> wrote:
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> On 8/3/22 16:55, Luiz Augusto von Dentz wrote:
> > >>>>>> Hi Vicki,
> > >>>>>>
> > >>>>>> On Wed, Aug 3, 2022 at 4:07 PM Vicki Pfau <vi@endrift.com> wrote:
> > >>>>>>>
> > >>>>>>> Though a 512 byte report map size seems plenty large, there exist some devices
> > >>>>>>> (e.g. Brydge W-Touch) that send larger reports. There is no protocol-defined
> > >>>>>>> maximum size so doubling the maximum size is safe, and should hopefully fix
> > >>>>>>> most real-world failures.
> > >>>>>>> ---
> > >>>>>>>  profiles/input/hog-lib.c | 2 +-
> > >>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>>>>>
> > >>>>>>> diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
> > >>>>>>> index 4a9c60185..9f3eb428c 100644
> > >>>>>>> --- a/profiles/input/hog-lib.c
> > >>>>>>> +++ b/profiles/input/hog-lib.c
> > >>>>>>> @@ -64,7 +64,7 @@
> > >>>>>>>  #define HOG_PROTO_MODE_BOOT    0
> > >>>>>>>  #define HOG_PROTO_MODE_REPORT  1
> > >>>>>>>
> > >>>>>>> -#define HOG_REPORT_MAP_MAX_SIZE        512
> > >>>>>>> +#define HOG_REPORT_MAP_MAX_SIZE        1024
> > >>>>>>>  #define HID_INFO_SIZE                  4
> > >>>>>>>  #define ATT_NOTIFICATION_HEADER_SIZE   3
> > >>>>>>
> > >>>>>> Afaik 512 is the maximum length an attribute can have even when using
> > >>>>>> read long procedure:
> > >>>>>>
> > >>>>>> BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 3, Part F
> > >>>>>> page 1416:
> > >>>>>>
> > >>>>>> The maximum length of an attribute value shall be 512 octets.
> > >>>>>>
> > >>>>>> And
> > >>>>>>
> > >>>>>> BLUETOOTH SPECIFICATION
> > >>>>>> HID Service Specification
> > >>>>>> Page 16 of 26
> > >>>>>>
> > >>>>>> 2.6.1 Report Map Characteristic Behavior
> > >>>>>> The GATT Read Characteristic Value or Read Long Characteristic Values sub-
> > >>>>>> procedures are used to read the Report Map characteristic value.
> > >>>>>> The length of the Report Map characteristic value is limited to 512 octets.
> > >>>>>>
> > >>>>>> So I believe the device is not compliant and very likely needs to have
> > >>>>>> multiple instances of HID Service instead of combining everything in a
> > >>>>>> single instance.
> > >>>>>>
> > >>>>>>> --
> > >>>>>>> 2.37.1
> > >>>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>
> > >>>>> Ah, that's strange. I looked through the spec but didn't see those. That said, while the device may be non-compliant, the device is on the market and I doubt I could get them to update the firmware as a random third party. It works on Windows, so clearly Windows doesn't have a problem with its noncompliance. So this raises the question, how should Linux handle non-compliant hardware, especially when it could easily be made to work just by bending the rules in this one instance? I can absolutely change the commit message since it's erroneous, but the question then comes down to how should it be handled at all.
> > >>>>
> > >>>> While I agree this could be worked around it is probably worth
> > >>>> checking with the manufacturer if it is aware of the problem because
> > >>>> even if we were to allow reading past 512 bytes offset in the future
> > >>>> there may be qualification tests enforcing not to do so, besides
> > >>>> versions up to BlueZ 5.65 would still not work anyway so I thing
> > >>>> letting the manufacturer know there is a problem with their
> > >>>> implementation is actually worth a shot here.
> > >>>>
> > >>> Brydge replied with the standard tech support "this is only supported on Windows, so there probably won't be a firmware update" reply, despite its noncompliance. And since I doubt Windows will add a change to limit it, well, that kind of limits our options here to either "enforce compliance and break non-compliant hardware" or "figure out a way to bend the rules". Given that BlueZ, upon expanding the maximum size, does successfully read the overly-long report map (it does use the read blob with offset message to get the last several bytes), it does work as intended if we ignore that specific rule. Though obviously that's up to the bluetooth maintainers to solve, so at this point I'm just tossing my two cents at it.
> > >>
> > >> Well we can do what Windows is doing but let's have it documented
> > >> then, we could as well scalate this with Bluetooth SIG since the
> > >> manufacturer seem to be intentionally creating interoperability
> > >> problems with a standard service, but that would likely take a lot
> > >> more time and most likely will need to be resolved by introducing a
> > >> test that enforces that HoG client don't attempt to read past 512
> > >> bytes offset.
> >
> > Is the client the device or the computer? Because the computer does successfully read past 512 bytes, and I believe it's needed for this device.
> >
> > >>
> > >> Btw, if there is no limitation on how big the report map can get in
> > >> HID, I'd leave it up to the kernel to figure out if that is acceptable
> > >> or not, so I'd remove the 1024.
> >
> > That makes sense. I can look into how to arbitrarily size that buffer.
> >
> > >
> > > Are you still planning to update this change? Btw, how about Android,
> > > does it handle such devices?
> > >
> >
> > Ah sorry, your reply was somewhat open ended and I didn't realize you wanted me to specifically look into this. As for Android, I have no idea. I can try pairing the trackpad with an Android device and seeing what happens, but I don't have anything with modern Android as it's not my day-to-day phone OS.
>
> Well iOS would do as well, what Im after is if that is in fact never
> enforced by other major OS, I do see there have been some tickets
> created about that already so Im trying to get some response, please
> forward this to the manufactures:
>
> https://www.bluetooth.org/errata/errata_view.cfm?errata_id=17495
>
> In the meantime there is yet another device that appear to have similar problem:
>
> https://github.com/bluez/bluez/pull/376

I went ahead and sent a patch myself, could you please verify it does
work the your device:

https://patchwork.kernel.org/project/bluetooth/patch/20220816191144.1515498-1-luiz.dentz@gmail.com/


-- 
Luiz Augusto von Dentz

RE: [BlueZ] hog-lib: Increase maximum report map size

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2014 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=665202

---Test result---

Test Summary:
CheckPatch                    FAIL      1.59 seconds
GitLint                       PASS      0.71 seconds
Prep - Setup ELL              PASS      27.17 seconds
Build - Prep                  PASS      0.71 seconds
Build - Configure             PASS      8.26 seconds
Build - Make                  PASS      737.08 seconds
Make Check                    PASS      11.31 seconds
Make Check w/Valgrind         PASS      282.32 seconds
Make Distcheck                PASS      233.41 seconds
Build w/ext ELL - Configure   PASS      8.32 seconds
Build w/ext ELL - Make        PASS      80.48 seconds
Incremental Build w/ patches  PASS      0.00 seconds
Scan Build                    PASS      485.57 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script with rule in .checkpatch.conf
Output:
[BlueZ] hog-lib: Increase maximum report map size
WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#53: 
Though a 512 byte report map size seems plenty large, there exist some devices

/github/workspace/src/12935951.patch total: 0 errors, 1 warnings, 8 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/12935951.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.




---
Regards,
Linux Bluetooth