Re: [PATCH 3/5] winebus.sys: Process device reports for linux event devices

[Sebastian Lackner]

On 27.02.2017 16:09, Aric Stewart wrote:
> Signed-off-by: Aric Stewart <aric(a)codeweavers.com>
> ---
>  dlls/winebus.sys/bus_udev.c | 163 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 162 insertions(+), 1 deletion(-)
>
> 0003-winebus.sys-Process-device-reports-for-linux-event-dev.txt
>
> diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
> index e2a475ccd8..2f198c7d4f 100644
> --- a/dlls/winebus.sys/bus_udev.c
> +++ b/dlls/winebus.sys/bus_udev.c
> @@ -242,6 +242,9 @@ struct wine_input_absinfo {
>  struct wine_input_private {
>      struct platform_private base;
>
> +    int buffer_length;
> +    BYTE *report_buffer;
> +
>      int report_descriptor_size;
>      BYTE *report_descriptor;
>
> @@ -340,6 +343,87 @@ static const BYTE* what_am_I(struct udev_device *dev)
>      return Unknown;
>  }
>
> +static BOOL set_button_value(struct wine_input_private *ext, int code, int value)
> +{
> +    int index = ext->button_map[code];
> +    int bindex = index / 8;
> +    int b = index % 8;
> +    BYTE mask;
> +
> +    mask = 1<<b;
> +    if (value)
> +        ext->report_buffer[bindex] = ext->report_buffer[bindex] | mask;
> +    else
> +    {
> +        mask = ~mask;
> +        ext->report_buffer[bindex] = ext->report_buffer[bindex] & mask;
> +    }
> +    return TRUE;
> +}
> +
> +static BOOL set_abs_axis_value(struct wine_input_private *ext, int code, int value)
> +{
> +    int index;
> +    /* check for hatswitches */
> +    if (code <= ABS_HAT3Y && code >= ABS_HAT0X)
> +    {
> +        index = code - ABS_HAT0X;
> +        ext->hat_values[index] = value;
> +        if ((code - ABS_HAT0X) % 2)
> +            index--;
> +        if (ext->hat_values[index] == 0)
> +        {
> +            if (ext->hat_values[index+1] == 0)
> +                value = 8;
> +            else if (ext->hat_values[index+1] < 0)
> +                value = 0;
> +            else
> +                value = 4;
> +        }
> +        else if (ext->hat_values[index] > 0)
> +        {
> +            if (ext->hat_values[index+1] == 0)
> +                value = 2;
> +            else if (ext->hat_values[index+1] < 0)
> +                value = 1;
> +            else
> +                value = 3;
> +        }
> +        else
> +        {
> +            if (ext->hat_values[index+1] == 0)
> +                value = 6;
> +            else if (ext->hat_values[index+1] < 0)
> +                value = 7;
> +            else
> +                value = 5;
> +        }
> +        ext->report_buffer[ext->hat_map[index]] = value;
> +        return TRUE;
> +    }
> +    else if (code < HID_ABS_MAX && ABS_to_HID_map[code][0] != 0)
> +    {
> +        index = ext->abs_map[code].report_index;
> +        *((WORD*)&ext->report_buffer[index]) = MulDiv(value, 0xff, ext->abs_map[code].info.maximum - ext->abs_map[code].info.minimum);
> +        return TRUE;
> +    }
> +    return FALSE;
> +}
> +
> +static BOOL set_rel_axis_value(struct wine_input_private *ext, int code, int value)
> +{
> +    int index;
> +    if (code < HID_REL_MAX && REL_to_HID_map[code][0] != 0)
> +    {
> +        index = ext->rel_map[code];
> +        if (value > 127) value = 127;
> +        if (value < -127) value = -127;
> +        ext->report_buffer[index] = value;
> +        return TRUE;
> +    }
> +    return FALSE;
> +}
> +
>  static VOID build_report_descriptor(struct wine_input_private *ext, struct udev_device *dev)
>  {
>      int abs_pages[TOP_ABS_PAGE][HID_ABS_MAX+1];
> @@ -510,6 +594,37 @@ static VOID build_report_descriptor(struct wine_input_private *ext, struct udev_
>      memcpy(report_ptr, report_tail, sizeof(report_tail));
>
> ext->report_descriptor_size = descript_size;
> +    ext->buffer_length = report_size;
> +    ext->report_buffer = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, report_size);
> +
> +    /* Initialize axis in the report */
> +    for (i = 0; i < HID_ABS_MAX; i++)
> +        if (test_bit(absbits, i))
> +            set_abs_axis_value(ext, i, ext->abs_map[i].info.value);
> +}
> +
> +static BOOL set_report_from_event(struct wine_input_private *ext, struct input_event *ie)
> +{
> +    switch(ie->type)
> +    {
> +#ifdef EV_SYN
> +        case EV_SYN:
> +            return FALSE;
> +#endif
> +#ifdef EV_MSC
> +        case EV_MSC:
> +            return FALSE;
> +#endif
> +        case EV_KEY:
> +            return set_button_value(ext, ie->code, ie->value);
> +        case EV_ABS:
> +            return set_abs_axis_value(ext, ie->code, ie->value);
> +        case EV_REL:
> +            return set_rel_axis_value(ext, ie->code, ie->value);
> +        default:
> +            ERR("TODO: Process Report (%i, %i)\n",ie->type, ie->code);
> +            return FALSE;
> +    }
>  }
>  #endif
>
> @@ -846,9 +961,52 @@ static NTSTATUS lnxev_get_string(DEVICE_OBJECT *device, DWORD index, WCHAR *buff
>      return STATUS_NOT_IMPLEMENTED;
>  }
>
> +static DWORD CALLBACK lnxev_device_report_thread(void *args)
> +{
> +    DEVICE_OBJECT *device = (DEVICE_OBJECT*)args;
> +    struct wine_input_private *private = input_impl_from_DEVICE_OBJECT(device);
> +    struct pollfd plfds[2];
> +
> +    plfds[0].fd = private->base.device_fd;
> +    plfds[0].events = POLLIN;
> +    plfds[0].revents = 0;
> +    plfds[1].fd = private->base.control_pipe[0];
> +    plfds[1].events = POLLIN;
> +    plfds[1].revents = 0;
> +
> +    while (1)
> +    {
> +        int size;
> +        struct input_event ie;
> +
> +        if (poll(plfds, 2, -1) <= 0) continue;
> +        if (plfds[1].revents || !private->report_buffer || private->buffer_length == 0)

It would be better to catch memory allocation failures earlier than while processing device reports.
> +            break;
> +        size = read(plfds[0].fd, &ie, sizeof(ie));
> +        if (size == -1)
> +            TRACE_(hid_report)("Read failed. Likely an unplugged device\n");
> +        else if (size == 0)
> +            TRACE_(hid_report)("Failed to read report\n");
> +        else if (set_report_from_event(private, &ie))
> +            process_hid_report(device, private->report_buffer, private->buffer_length);
> +    }
> +    return 0;
> +}
> +
>  static NTSTATUS lnxev_begin_report_processing(DEVICE_OBJECT *device)
>  {
> -    return STATUS_NOT_IMPLEMENTED;
> +    struct wine_input_private *private = input_impl_from_DEVICE_OBJECT(device);
> +

You have to check that the thread was not started yet.
> +    pipe(private->base.control_pipe);

Some error handling wouldn't hurt.
> +    private->base.report_thread = CreateThread(NULL, 0, lnxev_device_report_thread, device, 0, NULL);
> +    if (!private->base.report_thread)
> +    {
> +        ERR("Unable to create device report thread\n");
> +        close(private->base.control_pipe[0]);
> +        close(private->base.control_pipe[1]);
> +        return STATUS_UNSUCCESSFUL;
> +    }
> +    return STATUS_SUCCESS;
>  }
>
> static NTSTATUS lnxev_set_output_report(DEVICE_OBJECT *device, UCHAR id, BYTE *report, DWORD length, ULONG_PTR *written)
> @@ -990,6 +1148,9 @@ static void try_remove_device(struct udev_device *dev)
>          close(private->control_pipe[0]);
>          close(private->control_pipe[1]);
>          CloseHandle(private->report_thread);
> +#ifdef HAS_PROPER_INPUT_HEADER
> +        HeapFree(GetProcessHeap(), 0, ((struct wine_input_private*)private)->report_buffer);

It looks like this will attempt to release an invalid pointer for hidraw devices.
> +#endif
>      }
>
> dev = private->udev_device;