Bug 68621 - Sega Genesis Mega Drive MD Controller adapter: two arrow keys don't work since kernel 3.3. release
Summary: Sega Genesis Mega Drive MD Controller adapter: two arrow keys don't work sinc...
Status: NEW
Alias: None
Product: Drivers
Classification: Unclassified
Component: Input Devices (show other bugs)
Hardware: i386 Linux
: P1 normal
Assignee: drivers_input-devices
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-12 18:00 UTC by Arthur Gafarov
Modified: 2015-07-30 21:28 UTC (History)
5 users (show)

See Also:
Kernel Version: 3.12-6.1
Tree: Mainline
Regression: Yes


Attachments
[PATCH 3.19.0] HID: reject input outside logical range only if null state is set (442 bytes, patch)
2015-05-25 05:01 UTC, Valtteri "tsone" Heikkilä
Details | Diff

Description Arthur Gafarov 2014-01-12 18:00:25 UTC
Hi!
Recently i got this device
http://www.amazon.com/Genesis-PC-USB-Cable-RETROLINK-Sega/dp/B0080RAT42 

When i plug it in machine and test it with with jstest, i found out that 2 keys - up and left do not work.

Also, in Ubuntu 12.04.3 LTS with Kernel 3.2 it works, but not with version 3.3.

# jstest /dev/input/js2
Joystick (INNEX GENESIS/ATARI Controller USB) has 4 axes and 16 buttons. Driver version is 2.1.0.
Testing ... (interrupt to exit)
Axes:  0:     0  1:     0  2: 32767  3: 32767 Buttons:  0:off  1:off  2:off  3:off  4:off  5:off  6:off  7:off  8:off  9:off 10:off 11:off 12:off 13:off 14:off 15:off 

In jstest 2 and 3 axes got only positive values (down and right), negative values (-32768 for up and left) not showing up.

# lsusb
Bus 008 Device 005: ID 1292:4745 Innomedia
Comment 1 mantoxpub 2014-01-28 08:16:24 UTC
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
Comment 2 Arthur Gafarov 2014-01-28 16:58:25 UTC
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?
Comment 3 Arthur Gafarov 2014-03-12 14:30:40 UTC
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.
Comment 4 Valtteri "tsone" Heikkilä 2015-05-21 13:16:53 UTC
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
Comment 5 Valtteri "tsone" Heikkilä 2015-05-25 04:20:51 UTC
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...
Comment 6 Valtteri "tsone" Heikkilä 2015-05-25 05:01:20 UTC
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>
Comment 7 Arthur Gafarov 2015-07-30 04:03:09 UTC
Any chance to get this patch upstream?
Comment 8 Dmitry Torokhov 2015-07-30 21:28:48 UTC
The best way would be to post the patch to linux-input@vger.kernel.org and Cc Jiri Kosina <jkosina@suse.cz>

Note You need to log in before you can comment on or make changes to this bug.