Re: [PATCH resend 5/7] winebus.sys: reports for iohid

[Sebastian Lackner]

On 04.11.2016 13:46, Aric Stewart wrote:
> Signed-off-by: Aric Stewart
>
> dlls/winebus.sys/bus_iohid.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> v2-0005-winebus.sys-reports-for-iohid.txt
>
> diff --git a/dlls/winebus.sys/bus_iohid.c b/dlls/winebus.sys/bus_iohid.c
> index e1a97ef..7208117 100644
> --- a/dlls/winebus.sys/bus_iohid.c
> +++ b/dlls/winebus.sys/bus_iohid.c
> @@ -119,6 +119,13 @@ static DWORD CFNumberToDWORD(CFNumberRef num)
>      return dwNum;
>  }
>
> +static void handle_IOHIDDeviceIOHIDReportCallback(void * inContext,
> -
>    IOReturn inResult, void * inSender, IOHIDReportType inType,
> -
>    uint32_t inReportID, uint8_t * inReport, CFIndex inReportLength)

I would suggest to use parameter names without "in" prefix. Also, there usually
shouldn't be any space between "*" and the following variable name.
> +{

It might be better to store the DEVICE_OBJECT in a local variable before passing
it to the next function, to ensure the types are checked correctly. Otherwise
it remains unnoticed when the signature of process_hid_report changes in the future.
> - process_hid_report(inContext, inReport, inReportLength);
>
> +}
> -
>
> static int compare_platform_device(DEVICE_OBJECT *device, void *platform_dev)
>  {
>      ULONG_PTR dev1 = (ULONG_PTR)*(IOHIDDeviceRef*)get_platform_private(device);
> @@ -179,7 +186,17 @@ static NTSTATUS get_string(DEVICE_OBJECT *device, DWORD index, WCHAR *buffer, DW
>
> static NTSTATUS begin_report_processing(DEVICE_OBJECT *device)
>  {
> - return STATUS_NOT_IMPLEMENTED;
> - DWORD length;
> - uint8_t *buffer;
> - IOHIDDeviceRef dev = *(IOHIDDeviceRef*)get_platform_private(device);
> - CFNumberRef num;

To be compatible with the rest of the code, this should only start processing
once. You might have to store the state of report processing in the platform
struct.
> -
> - num = IOHIDDeviceGetProperty(dev, CFSTR(kIOHIDMaxInputReportSizeKey));
> - length = CFNumberToDWORD(num);
> - buffer = HeapAlloc(GetProcessHeap(), 0, length);

You might want to do some error checking here.
> -
> - IOHIDDeviceRegisterInputReportCallback(dev, buffer, length, handle_IOHIDDeviceIOHIDReportCallback, device);
> - return STATUS_SUCCESS;
>
> }
>
> static NTSTATUS set_output_report(DEVICE_OBJECT *device, UCHAR id, BYTE *report, DWORD length, ULONG_PTR *written)
> @@ -244,6 +261,9 @@ static void handle_RemovalCallback(void *inContext, IOReturn inResult, void *inS
>  {
>      DEVICE_OBJECT *device;
>      TRACE("OS/X IOHID Device Removed %p\n", inIOHIDDeviceRef);
> - IOHIDDeviceRegisterInputReportCallback(inIOHIDDeviceRef, NULL, 0, NULL, NULL);
> - /* Note: Yes, we leak the buffer. But according to research there is no
> -
>         safe way to deallocate that buffer. */

Is it a problem when IOHIDDeviceRegisterInputReportCallback is called, although
report processing was never started? And why isn't it safe to deallocate the buffer
here? I don't think you will get additional reports when the device has been
removed.
>  device = bus_find_hid_device(&iohid_vtbl, inIOHIDDeviceRef);
>  if (device)
>  {

[PATCH resend 5/7] winebus.sys: reports for iohid

[Aric Stewart]

On 11/7/16 10:21 AM, Sebastian Lackner wrote:
> On 04.11.2016 13:46, Aric Stewart wrote:
> > Signed-off-by: Aric Stewart
> >
> > dlls/winebus.sys/bus_iohid.c | 22 +++++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > v2-0005-winebus.sys-reports-for-iohid.txt
> >
> > diff --git a/dlls/winebus.sys/bus_iohid.c b/dlls/winebus.sys/bus_iohid.c
> > index e1a97ef..7208117 100644
> > --- a/dlls/winebus.sys/bus_iohid.c
> > +++ b/dlls/winebus.sys/bus_iohid.c
> > @@ -119,6 +119,13 @@ static DWORD CFNumberToDWORD(CFNumberRef num)
> >      return dwNum;
> >  }
> >
> > +static void handle_IOHIDDeviceIOHIDReportCallback(void * inContext,
> > -
> >    IOReturn inResult, void * inSender, IOHIDReportType inType,
> > -
> >    uint32_t inReportID, uint8_t * inReport, CFIndex inReportLength)
>
> I would suggest to use parameter names without "in" prefix. Also, there usually
> shouldn't be any space between "*" and the following variable name.
> > +{
>
> It might be better to store the DEVICE_OBJECT in a local variable before passing
> it to the next function, to ensure the types are checked correctly. Otherwise
> it remains unnoticed when the signature of process_hid_report changes in the future.
> > - process_hid_report(inContext, inReport, inReportLength);
> >
> > +}
> > -
> >
> > static int compare_platform_device(DEVICE_OBJECT *device, void *platform_dev)
> >  {
> >      ULONG_PTR dev1 = (ULONG_PTR)*(IOHIDDeviceRef*)get_platform_private(device);
> > @@ -179,7 +186,17 @@ static NTSTATUS get_string(DEVICE_OBJECT *device, DWORD index, WCHAR *buffer, DW
> >
> > static NTSTATUS begin_report_processing(DEVICE_OBJECT *device)
> >  {
> > - return STATUS_NOT_IMPLEMENTED;
> > - DWORD length;
> > - uint8_t *buffer;
> > - IOHIDDeviceRef dev = *(IOHIDDeviceRef*)get_platform_private(device);
> > - CFNumberRef num;
>
> To be compatible with the rest of the code, this should only start processing
> once. You might have to store the state of report processing in the platform
> struct.
> > -
> > - num = IOHIDDeviceGetProperty(dev, CFSTR(kIOHIDMaxInputReportSizeKey));
> > - length = CFNumberToDWORD(num);
> > - buffer = HeapAlloc(GetProcessHeap(), 0, length);
>
> You might want to do some error checking here.
> > -
> > - IOHIDDeviceRegisterInputReportCallback(dev, buffer, length, handle_IOHIDDeviceIOHIDReportCallback, device);
> > - return STATUS_SUCCESS;
> >
> > }
> >
> > static NTSTATUS set_output_report(DEVICE_OBJECT *device, UCHAR id, BYTE *report, DWORD length, ULONG_PTR *written)
> > @@ -244,6 +261,9 @@ static void handle_RemovalCallback(void *inContext, IOReturn inResult, void *inS
> >  {
> >      DEVICE_OBJECT *device;
> >      TRACE("OS/X IOHID Device Removed %p\n", inIOHIDDeviceRef);
> > - IOHIDDeviceRegisterInputReportCallback(inIOHIDDeviceRef, NULL, 0, NULL, NULL);
> > - /* Note: Yes, we leak the buffer. But according to research there is no
> > -
> >         safe way to deallocate that buffer. */
>
> Is it a problem when IOHIDDeviceRegisterInputReportCallback is called, although
> report processing was never started? And why isn't it safe to deallocate the buffer
> here? I don't think you will get additional reports when the device has been
> removed.

I was seeing random crashes sometimes when a device was unplugged or on shutdown and according to a few forum posts I found during searching tracking down a crash on exit there is evidently no way to flush a report queue so even if you have removed the callback there is a chance that the system may still use the previous buffer internally for an unknown number of times and if you free that buffer there is crash internally in the Apple framework.

My crash seemed to match that and went away if I did not free the buffer, which was the recommendation of the developers

-aric
> >  device = bus_find_hid_device(&iohid_vtbl, inIOHIDDeviceRef);
> >  if (device)
> >  {