On 14.11.2016 15:38, Aric Stewart wrote:
On 11/14/16 8:17 AM, Sebastian Lackner wrote:
On 14.11.2016 14:35, Aric Stewart wrote:
This is a little confusing because reports to and from the device itself only have a leading reportId if the reportid is not 0. Otherwise there is no leading reportId. So we need to add a 0 reportId to reports coming from such a device and strip the leading 0 from reports written to the device.
Signed-off-by: Aric Stewart aric@codeweavers.com
dlls/hidclass.sys/descriptor.c | 31 ++++----------- dlls/hidclass.sys/device.c | 90 ++++++++++++++++++++++++++++-------------- 2 files changed, 67 insertions(+), 54 deletions(-)
0001-hidclass-All-reports-read-or-written-to-user-space-lea.txt
diff --git a/dlls/hidclass.sys/descriptor.c b/dlls/hidclass.sys/descriptor.c index 9a26f0f4..8bcda43 100644 --- a/dlls/hidclass.sys/descriptor.c +++ b/dlls/hidclass.sys/descriptor.c @@ -887,10 +887,8 @@ static WINE_HIDP_PREPARSED_DATA* build_PreparseData( new_report(wine_report, input_features[0]); data->dwInputReportCount++;
if (input_features[0]->caps.ReportID != 0)bitOffset = 8;elsebitOffset = 0;
/* Room for the reportID */bitOffset = 8; for (i = 0; i < i_count; i++) {@@ -901,10 +899,7 @@ static WINE_HIDP_PREPARSED_DATA* build_PreparseData( new_report(wine_report, input_features[i]); data->dwInputReportCount++; bitLength = max(bitOffset, bitLength);
if (input_features[i]->caps.ReportID != 0)bitOffset = 8;elsebitOffset = 0;
bitOffset = 8; } build_elements(wine_report, input_features[i], &bitOffset); count_elements(input_features[i], &data->caps.NumberInputButtonCaps,@@ -922,10 +917,7 @@ static WINE_HIDP_PREPARSED_DATA* build_PreparseData( data->dwOutputReportOffset = (BYTE*)wine_report - (BYTE*)data->InputReports; new_report(wine_report, output_features[0]); data->dwOutputReportCount++;
if (output_features[0]->caps.ReportID != 0)bitOffset = 8;elsebitOffset = 0;
bitOffset = 8; for (i = 0; i < o_count; i++) {@@ -936,10 +928,7 @@ static WINE_HIDP_PREPARSED_DATA* build_PreparseData( new_report(wine_report, output_features[i]); data->dwOutputReportCount++; bitLength = max(bitOffset, bitLength);
if (output_features[0]->caps.ReportID != 0)bitOffset = 8;elsebitOffset = 0;
bitOffset = 8; } build_elements(wine_report, output_features[i], &bitOffset); count_elements(output_features[i], &data->caps.NumberOutputButtonCaps,@@ -957,10 +946,7 @@ static WINE_HIDP_PREPARSED_DATA* build_PreparseData( data->dwFeatureReportOffset = (BYTE*)wine_report - (BYTE*)data->InputReports; new_report(wine_report, feature_features[0]); data->dwFeatureReportCount++;
if (feature_features[0]->caps.ReportID != 0)bitOffset = 8;elsebitOffset = 0;
bitOffset = 8; for (i = 0; i < f_count; i++) {@@ -971,10 +957,7 @@ static WINE_HIDP_PREPARSED_DATA* build_PreparseData( new_report(wine_report, feature_features[i]); data->dwFeatureReportCount++; bitLength = max(bitOffset, bitLength);
if (feature_features[0]->caps.ReportID != 0)bitOffset = 8;elsebitOffset = 0;
bitOffset = 8; } build_elements(wine_report, feature_features[i], &bitOffset); count_elements(feature_features[i], &data->caps.NumberFeatureButtonCaps,diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c index 5dd5ebb..6454b67 100644 --- a/dlls/hidclass.sys/device.c +++ b/dlls/hidclass.sys/device.c @@ -197,6 +197,28 @@ void HID_DeleteDevice(HID_MINIDRIVER_REGISTRATION *driver, DEVICE_OBJECT *device IoDeleteDevice(device); }
+static NTSTATUS copy_packet_into_buffer(HID_XFER_PACKET *packet, BYTE* buffer, ULONG buffer_length, ULONG *out_length) +{
- *out_length = 0;
- if (buffer_length > packet->reportBufferLen)
- {
if (packet->reportId != 0){memcpy(buffer, packet->reportBuffer, packet->reportBufferLen);*out_length = packet->reportBufferLen;The length check is a bit too strict in this case. You are returning STATUS_BUFFER_OVERFLOW even when the buffer is sufficient (buffer_length == packet->reportBufferLen).
}else{buffer[0] = 0;memcpy(&buffer[1], packet->reportBuffer, packet->reportBufferLen);*out_length = packet->reportBufferLen + 1;}return STATUS_SUCCESS;- }
- else
return STATUS_BUFFER_OVERFLOW;+}
static void HID_Device_processQueue(DEVICE_OBJECT *device) { LIST_ENTRY *entry; @@ -217,20 +239,14 @@ static void HID_Device_processQueue(DEVICE_OBJECT *device) RingBuffer_Read(ext->ring_buffer, ptr, packet, &buffer_size); if (buffer_size) {
NTSTATUS hr;Maybe its just me, but I think "hr" is really a very bad name for NTSTATUS codes. ;)
ULONG out_length; IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation(irp); packet->reportBuffer = (BYTE *)packet + sizeof(*packet); TRACE_(hid_report)("Processing Request (%i)\n",ptr);
if (irpsp->Parameters.Read.Length >= packet->reportBufferLen){memcpy(irp->AssociatedIrp.SystemBuffer, packet->reportBuffer, packet->reportBufferLen);irp->IoStatus.Information = packet->reportBufferLen;irp->IoStatus.u.Status = STATUS_SUCCESS;}else{irp->IoStatus.Information = 0;irp->IoStatus.u.Status = STATUS_BUFFER_OVERFLOW;}
hr = copy_packet_into_buffer(packet, irp->AssociatedIrp.SystemBuffer, irpsp->Parameters.Read.Length, &out_length);irp->IoStatus.u.Status = hr;irp->IoStatus.Information = out_length; } else {@@ -331,7 +347,10 @@ static DWORD CALLBACK hid_device_thread(void *args) if (!exit_now && irp->IoStatus.u.Status == STATUS_SUCCESS) { packet->reportBufferLen = irp->IoStatus.Information;
packet->reportId = packet->reportBuffer[0];
if (ext->preparseData->InputReports[0].reportID)packet->reportId = packet->reportBuffer[0];elsepacket->reportId = 0; RingBuffer_Write(ext->ring_buffer, packet); HID_Device_processQueue(device); }@@ -461,9 +480,17 @@ static NTSTATUS HID_set_to_device(DEVICE_OBJECT *device, IRP *irp) NTSTATUS rc;
TRACE_(hid_report)("Device %p Buffer length %i Buffer %p\n", device, irpsp->Parameters.DeviceIoControl.InputBufferLength, irp->AssociatedIrp.SystemBuffer);
- packet.reportBuffer = irp->AssociatedIrp.SystemBuffer;
- packet.reportId = ((char*)irp->AssociatedIrp.SystemBuffer)[0];
- packet.reportBufferLen = irpsp->Parameters.DeviceIoControl.InputBufferLength;
packet.reportId = ((BYTE*)irp->AssociatedIrp.SystemBuffer)[0];
if (packet.reportId == 0)
{
packet.reportBuffer = &((BYTE*)irp->AssociatedIrp.SystemBuffer)[1];packet.reportBufferLen = irpsp->Parameters.DeviceIoControl.InputBufferLength - 1;}
else
{
packet.reportBuffer = irp->AssociatedIrp.SystemBuffer;packet.reportBufferLen = irpsp->Parameters.DeviceIoControl.InputBufferLength;} TRACE_(hid_report)("(id %i, len %i buffer %p)\n", packet.reportId, packet.reportBufferLen, packet.reportBuffer);
rc = call_minidriver(irpsp->Parameters.DeviceIoControl.IoControlCode,
@@ -636,20 +663,15 @@ NTSTATUS WINAPI HID_Device_read(DEVICE_OBJECT *device, IRP *irp) if (buffer_size) { IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation( irp );
NTSTATUS hr;Same here.
ULONG out_length; packet->reportBuffer = (BYTE *)packet + sizeof(*packet); TRACE_(hid_report)("Got Packet %p %i\n", packet->reportBuffer, packet->reportBufferLen);
if (irpsp->Parameters.Read.Length >= packet->reportBufferLen){memcpy(irp->AssociatedIrp.SystemBuffer, packet->reportBuffer, packet->reportBufferLen);irp->IoStatus.Information = packet->reportBufferLen;irp->IoStatus.u.Status = STATUS_SUCCESS;}else{irp->IoStatus.Information = 0;irp->IoStatus.u.Status = STATUS_BUFFER_OVERFLOW;}IoCompleteRequest( irp, IO_NO_INCREMENT );
hr = copy_packet_into_buffer(packet, irp->AssociatedIrp.SystemBuffer, irpsp->Parameters.Read.Length, &out_length);irp->IoStatus.Information = out_length;irp->IoStatus.u.Status = hr;IoCompleteRequest(irp, IO_NO_INCREMENT);@@ -671,9 +693,17 @@ NTSTATUS WINAPI HID_Device_write(DEVICE_OBJECT *device, IRP *irp) irp->IoStatus.Information = 0;
TRACE_(hid_report)("Device %p Buffer length %i Buffer %p\n", device, irpsp->Parameters.Write.Length, irp->AssociatedIrp.SystemBuffer);
- packet.reportBuffer = irp->AssociatedIrp.SystemBuffer;
- packet.reportId = ((char*)irp->AssociatedIrp.SystemBuffer)[0];
- packet.reportBufferLen = irpsp->Parameters.Write.Length;
- packet.reportId = ((BYTE*)irp->AssociatedIrp.SystemBuffer)[0];
- if (packet.reportId == 0)
- {
packet.reportBuffer = &((BYTE*)irp->AssociatedIrp.SystemBuffer)[1];packet.reportBufferLen = irpsp->Parameters.Write.Length - 1;Besides the fact that the code looks suspicious (couldn't a 0 byte also have a different meaning?)
What part do you mean about a 0 byte having a different meaning. When coming in from the write (user space) the first byte will be the report id, so a 0 is a 0 reportId.
Unfortunately its a bit difficult to find documentation, so I am not sure if the 0 should always be present, or if it depends on the device (like the hidraw read() command). If you are sure it is present this should be fine (well, except that you would need a length check to avoid crashes caused by empty messages).
this will probably break the udev backend. As documented in https://www.kernel.org/doc/Documentation/hid/hidraw.txt, the write() command expects that the first byte is 0 if the device does not use numbered reports.
Yeah, thinking about this I will fix this on the hidraw backend. I like having this level be consistent where the packet buffer only leads with the reportid if the reportid is non-zero. So adding that zero in on the hidraw platform backend is not hard.
Is that also how it is done on Windows? We have to keep in mind that Hidclass itself is not Wine specific and also exists on Windows.
-aric
}
else
{
packet.reportBuffer = irp->AssociatedIrp.SystemBuffer;packet.reportBufferLen = irpsp->Parameters.Write.Length;} TRACE_(hid_report)("(id %i, len %i buffer %p)\n", packet.reportId, packet.reportBufferLen, packet.reportBuffer);
rc = call_minidriver(IOCTL_HID_WRITE_REPORT, device, NULL, 0, &packet, sizeof(packet));