Bug 68621

i can confirm the same issue with the INNEX NES Controller USB adapter.

A fix has already been discovered:
https://github.com/robmcmullen/hid-atari-retrobit
https://github.com/raspberrypi/linux/issues/432

Sadly, the method described in first link didn't work for me.

Any chance to merge the code in the second link to upstream? Or just fix it somehow?

I've compiled the kernel with version 3.13-5 without the next code in hid-input.c
at line 1050

	/*
	 * Ignore out-of-range values as per HID specification,
	 * section 5.10 and 6.2.25.
	 *
	 * The logical_minimum < logical_maximum check is done so that we
	 * don't unintentionally discard values sent by devices which
	 * don't specify logical min and max.
	 */
	if ((field->flags & HID_MAIN_ITEM_VARIABLE) &&
	    (field->logical_minimum < field->logical_maximum) &&
	    (value < field->logical_minimum ||
	     value > field->logical_maximum)) {
		dbg_hid("Ignoring out-of-range value %x\n", value);
		return;
	}

and the up and left buttons again working without problems.

I also have this issue with INNEX NES Controller USB.

After analyzing the hid driver code and the USB HID spec [1], I think there is a flaw in the above out-of-range check in hid-input.c. The check should only apply if the control has the null state flag enabled. So, this issue could be resolved by introducing an additional condition:

    (field->flags & HID_MAIN_ITEM_NULL_STATE)

However, I'm not an USB expert so can someone confirm this?

The USB HID spec [1] says the following concerning the null state flag:

"Indicates whether the control has a state in which it is not sending meaningful data. ... When in a null state, the control will report a value outside of the specified Logical Minimum and Logical Maximum (the most negative value, such as -128 for an 8-bit value)."

So, if the control has no null state then all values are be reported. Right?

Anyway, the INNEX NES Controller USB doesn't seem to have a null state. Here is the device description and report from lsusb:

root@tsonepad:~# lsusb -vd 1292:4643

Bus 001 Device 012: ID 1292:4643 Innomedia 
Device Descriptor:
  bLength                18
  bDescriptorType         1
  bcdUSB               1.10
  bDeviceClass            0 (Defined at Interface level)
  bDeviceSubClass         0 
  bDeviceProtocol         0 
  bMaxPacketSize0         8
  idVendor           0x1292 Innomedia
  idProduct          0x4643 
  bcdDevice            1.00
  iManufacturer           1 INNEX
  iProduct                2 NES Controller USB
  iSerial                 0 
  bNumConfigurations      1
  Configuration Descriptor:
    bLength                 9
    bDescriptorType         2
    wTotalLength           34
    bNumInterfaces          1
    bConfigurationValue     1
    iConfiguration          4 Cfg1
    bmAttributes         0x80
      (Bus Powered)
    MaxPower              100mA
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       0
      bNumEndpoints           1
      bInterfaceClass         3 Human Interface Device
      bInterfaceSubClass      0 No Subclass
      bInterfaceProtocol      0 None
      iInterface              5 (error)
        HID Device Descriptor:
          bLength                 9
          bDescriptorType        33
          bcdHID               1.10
          bCountryCode           33 US
          bNumDescriptors         1
          bDescriptorType        34 Report
          wDescriptorLength      58
          Report Descriptor: (length is 58)
            Item(Global): Usage Page, data= [ 0x01 ] 1
                            Generic Desktop Controls
            Item(Local ): Usage, data= [ 0x05 ] 5
                            Gamepad
            Item(Main  ): Collection, data= [ 0x01 ] 1
                            Application
            Item(Local ): Usage, data= [ 0x01 ] 1
                            Pointer
            Item(Main  ): Collection, data= [ 0x00 ] 0
                            Physical
            Item(Local ): Usage, data= [ 0x30 ] 48
                            Direction-X
            Item(Local ): Usage, data= [ 0x31 ] 49
                            Direction-Y
            Item(Global): Logical Minimum, data= [ 0xff ] 255
            Item(Global): Logical Maximum, data= [ 0x01 ] 1
            Item(Global): Report Size, data= [ 0x02 ] 2
            Item(Global): Report Count, data= [ 0x02 ] 2
            Item(Main  ): Input, data= [ 0x02 ] 2
                            Data Variable Absolute No_Wrap Linear
                            Preferred_State No_Null_Position Non_Volatile Bitfield
            Item(Main  ): End Collection, data=none
            Item(Global): Report Count, data= [ 0x04 ] 4
            Item(Global): Report Size, data= [ 0x01 ] 1
            Item(Main  ): Input, data= [ 0x01 ] 1
                            Constant Array Absolute No_Wrap Linear
                            Preferred_State No_Null_Position Non_Volatile Bitfield
            Item(Global): Usage Page, data= [ 0x09 ] 9
                            Buttons
            Item(Local ): Usage Minimum, data= [ 0x01 ] 1
                            Button 1 (Primary)
            Item(Local ): Usage Maximum, data= [ 0x04 ] 4
                            Button 4
            Item(Global): Logical Minimum, data= [ 0x00 ] 0
            Item(Global): Logical Maximum, data= [ 0x01 ] 1
            Item(Global): Report Count, data= [ 0x04 ] 4
            Item(Global): Report Size, data= [ 0x01 ] 1
            Item(Main  ): Input, data= [ 0x02 ] 2
                            Data Variable Absolute No_Wrap Linear
                            Preferred_State No_Null_Position Non_Volatile Bitfield
            Item(Global): Report Count, data= [ 0x04 ] 4
            Item(Main  ): Input, data= [ 0x01 ] 1
                            Constant Array Absolute No_Wrap Linear
                            Preferred_State No_Null_Position Non_Volatile Bitfield
            Item(Global): Report Size, data= [ 0x08 ] 8
            Item(Global): Report Count, data= [ 0x06 ] 6
            Item(Main  ): Input, data= [ 0x01 ] 1
                            Constant Array Absolute No_Wrap Linear
                            Preferred_State No_Null_Position Non_Volatile Bitfield
            Item(Main  ): End Collection, data=none
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0008  1x 8 bytes
        bInterval              10
Device Status:     0x0002
  (Bus Powered)
  Remote Wakeup Enabled

I guess this 'No_Null_Position' is synonymous to no null state, right? If I'm correct, this could be a single line fix.


References:
[1] http://www.usb.org/developers/hidpage/HID1_11.pdf

According to lsusb source 'No_Null_Position' indeed equals no null state. Please see for example:
http://bazaar.launchpad.net/~ubuntu-branches/ubuntu/wily/usbutils/wily/view/head:/lsusb.c#L2903

To further support my fix claims, at least Cypress's 'USB HID Basics with PSoC 3 and PSoC 5LP' document (http://www.cypress.com/?docID=47050, p.5) mentions:

"No Null Position versus Null State: No Null Position means that all data sent by the control is meaningful. Null State means that the control can send meaningless data that is represented by a value outside the set logical minimum and maximum range."

Meaning, null state flag should be verified before rejecting input outside the logical range...

I will attach a patch shortly. However, I don't know enough on this topic to estimate the impact of this patch. There are likely devices that do not function within the USB HID spec. To resolve future issues, new quirk HID_QUIRK_MISSING_NULL_STATE could be created.

It'd be interesting to know if there's more devices properly reporting the null state in comparison to ones that don't...

Created attachment 177751 [details]
[PATCH 3.19.0] HID: reject input outside logical range only if null state is set


This patch fixes an issue in drivers/hid/hid-input.c where USB HID control null state flag is not checked upon rejecting inputs outside logical minumum-maximum range. The check should be made according to USB HID specification 1.11, section 6.2.2.5, p.31. The fix will resolve issues with some game controllers, such as: https://bugzilla.kernel.org/show_bug.cgi?id=68621 


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

Signed-Off-by: Valtteri "tsone" Heikkilä <rnd@nic.fi>

Any chance to get this patch upstream?

The best way would be to post the patch to linux-input@vger.kernel.org and Cc Jiri Kosina <jkosina@suse.cz>