* [PATCH] HID: sony: disable descriptor fixup for FutureMax Dance Mat @ 2016-07-17 17:25 Mikko Perttunen 2016-07-18 14:28 ` Benjamin Tissoires 0 siblings, 1 reply; 8+ messages in thread From: Mikko Perttunen @ 2016-07-17 17:25 UTC (permalink / raw) To: jikos, benjamin.tissoires; +Cc: linux-input, linux-kernel, Mikko Perttunen From: Mikko Perttunen <mperttunen@nvidia.com> The FutureMax Dance Mat claims to be a SixAxis controller but breaks if descriptor fixups are applied. Detect the device using its USB product string and disable fixups when it is detected. Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> --- drivers/hid/hid-sony.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c index 310436a..4c976f7 100644 --- a/drivers/hid/hid-sony.c +++ b/drivers/hid/hid-sony.c @@ -36,6 +36,9 @@ #include <linux/list.h> #include <linux/idr.h> #include <linux/input/mt.h> +#include <linux/usb.h> + +#include "usbhid/usbhid.h" #include "hid-ids.h" @@ -51,6 +54,7 @@ #define NAVIGATION_CONTROLLER_USB BIT(9) #define NAVIGATION_CONTROLLER_BT BIT(10) #define SINO_LITE_CONTROLLER BIT(11) +#define FUTUREMAX_DANCE_MAT BIT(12) #define SIXAXIS_CONTROLLER (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT) #define MOTION_CONTROLLER (MOTION_CONTROLLER_USB | MOTION_CONTROLLER_BT) @@ -1125,7 +1129,7 @@ static u8 *sony_report_fixup(struct hid_device *hdev, u8 *rdesc, { struct sony_sc *sc = hid_get_drvdata(hdev); - if (sc->quirks & SINO_LITE_CONTROLLER) + if (sc->quirks & (SINO_LITE_CONTROLLER | FUTUREMAX_DANCE_MAT)) return rdesc; /* @@ -2288,6 +2292,15 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) unsigned long quirks = id->driver_data; struct sony_sc *sc; unsigned int connect_mask = HID_CONNECT_DEFAULT; + struct usb_device *usb_dev; + + if (quirks & SIXAXIS_CONTROLLER_USB) { + usb_dev = hid_to_usb_dev(hdev); + if (usb_dev && usb_dev->product && + !strcmp(usb_dev->product, "FutureMax Dance Mat")) { + quirks |= FUTUREMAX_DANCE_MAT; + } + } sc = devm_kzalloc(&hdev->dev, sizeof(*sc), GFP_KERNEL); if (sc == NULL) { -- 2.8.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: sony: disable descriptor fixup for FutureMax Dance Mat 2016-07-17 17:25 [PATCH] HID: sony: disable descriptor fixup for FutureMax Dance Mat Mikko Perttunen @ 2016-07-18 14:28 ` Benjamin Tissoires 2016-07-19 6:08 ` Mikko Perttunen 0 siblings, 1 reply; 8+ messages in thread From: Benjamin Tissoires @ 2016-07-18 14:28 UTC (permalink / raw) To: Mikko Perttunen; +Cc: jikos, linux-input, linux-kernel, Mikko Perttunen On Jul 17 2016 or thereabouts, Mikko Perttunen wrote: > From: Mikko Perttunen <mperttunen@nvidia.com> > > The FutureMax Dance Mat claims to be a SixAxis controller > but breaks if descriptor fixups are applied. Detect the > device using its USB product string and disable fixups > when it is detected. > > Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> > --- > drivers/hid/hid-sony.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c > index 310436a..4c976f7 100644 > --- a/drivers/hid/hid-sony.c > +++ b/drivers/hid/hid-sony.c > @@ -36,6 +36,9 @@ > #include <linux/list.h> > #include <linux/idr.h> > #include <linux/input/mt.h> > +#include <linux/usb.h> > + > +#include "usbhid/usbhid.h" I spent a lot of effort 2 years ago to remove the usb dependency, I'd prefer not adding a strong deps on it again. > > #include "hid-ids.h" > > @@ -51,6 +54,7 @@ > #define NAVIGATION_CONTROLLER_USB BIT(9) > #define NAVIGATION_CONTROLLER_BT BIT(10) > #define SINO_LITE_CONTROLLER BIT(11) > +#define FUTUREMAX_DANCE_MAT BIT(12) > > #define SIXAXIS_CONTROLLER (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT) > #define MOTION_CONTROLLER (MOTION_CONTROLLER_USB | MOTION_CONTROLLER_BT) > @@ -1125,7 +1129,7 @@ static u8 *sony_report_fixup(struct hid_device *hdev, u8 *rdesc, > { > struct sony_sc *sc = hid_get_drvdata(hdev); > > - if (sc->quirks & SINO_LITE_CONTROLLER) > + if (sc->quirks & (SINO_LITE_CONTROLLER | FUTUREMAX_DANCE_MAT)) > return rdesc; > > /* > @@ -2288,6 +2292,15 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) > unsigned long quirks = id->driver_data; > struct sony_sc *sc; > unsigned int connect_mask = HID_CONNECT_DEFAULT; > + struct usb_device *usb_dev; > + > + if (quirks & SIXAXIS_CONTROLLER_USB) { > + usb_dev = hid_to_usb_dev(hdev); > + if (usb_dev && usb_dev->product && > + !strcmp(usb_dev->product, "FutureMax Dance Mat")) {i If they decided to reuse the same PID than one existing, and you have no other choice but to rely on the name, you can simply have a match on hdev->name instead of adding the USB dependency. Also, I think it would be better to have a check on the existing report descriptor instead of blindly fixing it. Other drivers make sure they are fixing the correct region before overriding it, but I think using a md5sum might be just easier (though that's not required for your patch I think). Cheers, Benjamin > + quirks |= FUTUREMAX_DANCE_MAT; > + } > + } > > sc = devm_kzalloc(&hdev->dev, sizeof(*sc), GFP_KERNEL); > if (sc == NULL) { > -- > 2.8.2 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: sony: disable descriptor fixup for FutureMax Dance Mat 2016-07-18 14:28 ` Benjamin Tissoires @ 2016-07-19 6:08 ` Mikko Perttunen 2016-07-21 16:34 ` Antonio Ospite 0 siblings, 1 reply; 8+ messages in thread From: Mikko Perttunen @ 2016-07-19 6:08 UTC (permalink / raw) To: Benjamin Tissoires; +Cc: jikos, linux-input, linux-kernel, Mikko Perttunen On 07/18/16 17:28, Benjamin Tissoires wrote: > On Jul 17 2016 or thereabouts, Mikko Perttunen wrote: >> From: Mikko Perttunen <mperttunen@nvidia.com> >> ... >> #include <linux/list.h> >> #include <linux/idr.h> >> #include <linux/input/mt.h> >> +#include <linux/usb.h> >> + >> +#include "usbhid/usbhid.h" > > I spent a lot of effort 2 years ago to remove the usb dependency, I'd > prefer not adding a strong deps on it again. > I see, I'll remove it. >> >> #include "hid-ids.h" >> >> @@ -51,6 +54,7 @@ >> #define NAVIGATION_CONTROLLER_USB BIT(9) >> #define NAVIGATION_CONTROLLER_BT BIT(10) >> #define SINO_LITE_CONTROLLER BIT(11) >> +#define FUTUREMAX_DANCE_MAT BIT(12) >> >> #define SIXAXIS_CONTROLLER (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT) >> #define MOTION_CONTROLLER (MOTION_CONTROLLER_USB | MOTION_CONTROLLER_BT) >> @@ -1125,7 +1129,7 @@ static u8 *sony_report_fixup(struct hid_device *hdev, u8 *rdesc, >> { >> struct sony_sc *sc = hid_get_drvdata(hdev); >> >> - if (sc->quirks & SINO_LITE_CONTROLLER) >> + if (sc->quirks & (SINO_LITE_CONTROLLER | FUTUREMAX_DANCE_MAT)) >> return rdesc; >> >> /* >> @@ -2288,6 +2292,15 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) >> unsigned long quirks = id->driver_data; >> struct sony_sc *sc; >> unsigned int connect_mask = HID_CONNECT_DEFAULT; >> + struct usb_device *usb_dev; >> + >> + if (quirks & SIXAXIS_CONTROLLER_USB) { >> + usb_dev = hid_to_usb_dev(hdev); >> + if (usb_dev && usb_dev->product && >> + !strcmp(usb_dev->product, "FutureMax Dance Mat")) {i > > If they decided to reuse the same PID than one existing, and you have no > other choice but to rely on the name, you can simply have a match on > hdev->name instead of adding the USB dependency. Ah, didn't notice this! > > Also, I think it would be better to have a check on the existing report > descriptor instead of blindly fixing it. Other drivers make sure they > are fixing the correct region before overriding it, but I think using a > md5sum might be just easier (though that's not required for your patch I > think). I agree. In fact, the mat used to work back in 3.15 when the driver did some rudimentary checks before overwriting the descriptor. In any case, it would be difficult for me to add the checks since the mat is the only piece of hardware I own that is handled by this driver. > > Cheers, > Benjamin > >> ... Cheers, and thanks for the review! I'll post a v2 once I get the chance. Mikko ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: sony: disable descriptor fixup for FutureMax Dance Mat 2016-07-19 6:08 ` Mikko Perttunen @ 2016-07-21 16:34 ` Antonio Ospite 2016-07-25 9:14 ` Benjamin Tissoires 0 siblings, 1 reply; 8+ messages in thread From: Antonio Ospite @ 2016-07-21 16:34 UTC (permalink / raw) To: Mikko Perttunen Cc: Benjamin Tissoires, jikos, linux-input, linux-kernel, Mikko Perttunen On Tue, 19 Jul 2016 09:08:27 +0300 Mikko Perttunen <mikko.perttunen@kapsi.fi> wrote: > On 07/18/16 17:28, Benjamin Tissoires wrote: [...] > > > > Also, I think it would be better to have a check on the existing report > > descriptor instead of blindly fixing it. Other drivers make sure they > > are fixing the correct region before overriding it, but I think using a > > md5sum might be just easier (though that's not required for your patch I > > think). > > I agree. In fact, the mat used to work back in 3.15 when the driver did > some rudimentary checks before overwriting the descriptor. In any case, > it would be difficult for me to add the checks since the mat is the only > piece of hardware I own that is handled by this driver. > The fact that the report gets fixed up unconditionally is from commit c607fb8 (HID: sony: Always override the Sixaxis descriptor). The rationale behind this change was that compatible devices are supposed to send exactly the same _raw_ data as the original ones, so overriding the _whole_ descriptor unconditionally should keep them working. I would be interesting to understand why some (supposedly) compatible devices break, maybe they rely on the fact that the PS3 would use the original report? BTW I'd slightly prefer the current way of having a straight path for the common case (original and actually compatible devices) and handle the exceptions explicitly using quirks, but I am open to changes and I can test patches with some original controllers if needed. Ciao ciao, Antonio -- Antonio Ospite http://ao2.it A: Because it messes up the order in which people normally read text. See http://en.wikipedia.org/wiki/Posting_style Q: Why is top-posting such a bad thing? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: sony: disable descriptor fixup for FutureMax Dance Mat 2016-07-21 16:34 ` Antonio Ospite @ 2016-07-25 9:14 ` Benjamin Tissoires 2016-07-25 11:05 ` Antonio Ospite 0 siblings, 1 reply; 8+ messages in thread From: Benjamin Tissoires @ 2016-07-25 9:14 UTC (permalink / raw) To: Antonio Ospite Cc: Mikko Perttunen, jikos, linux-input, linux-kernel, Mikko Perttunen On Jul 21 2016 or thereabouts, Antonio Ospite wrote: > On Tue, 19 Jul 2016 09:08:27 +0300 > Mikko Perttunen <mikko.perttunen@kapsi.fi> wrote: > > > On 07/18/16 17:28, Benjamin Tissoires wrote: > [...] > > > > > > Also, I think it would be better to have a check on the existing report > > > descriptor instead of blindly fixing it. Other drivers make sure they > > > are fixing the correct region before overriding it, but I think using a > > > md5sum might be just easier (though that's not required for your patch I > > > think). > > > > I agree. In fact, the mat used to work back in 3.15 when the driver did > > some rudimentary checks before overwriting the descriptor. In any case, > > it would be difficult for me to add the checks since the mat is the only > > piece of hardware I own that is handled by this driver. > > > > The fact that the report gets fixed up unconditionally is from > commit c607fb8 (HID: sony: Always override the Sixaxis descriptor). > > The rationale behind this change was that compatible devices are > supposed to send exactly the same _raw_ data as the original ones, so > overriding the _whole_ descriptor unconditionally should keep them > working. > > I would be interesting to understand why some (supposedly) compatible > devices break, maybe they rely on the fact that the PS3 would use the > original report? [just speculating] I would be *really* surprised if the Sony software were to overwrite the report descriptor of their own sony controllers. My guess is that their HID stack or their Playstation driver is compatible with the original report descriptors and so they can handle the differences between original and weirdo controllers. > > BTW I'd slightly prefer the current way of having a straight path for > the common case (original and actually compatible devices) and handle > the exceptions explicitly using quirks, but I am open to changes and I > can test patches with some original controllers if needed. Would the v2 of Mikko's patch be OK with you? Cheers, Benjamin > > Ciao ciao, > Antonio > > -- > Antonio Ospite > http://ao2.it > > A: Because it messes up the order in which people normally read text. > See http://en.wikipedia.org/wiki/Posting_style > Q: Why is top-posting such a bad thing? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: sony: disable descriptor fixup for FutureMax Dance Mat 2016-07-25 9:14 ` Benjamin Tissoires @ 2016-07-25 11:05 ` Antonio Ospite 2016-07-25 15:23 ` Mikko Perttunen 0 siblings, 1 reply; 8+ messages in thread From: Antonio Ospite @ 2016-07-25 11:05 UTC (permalink / raw) To: Benjamin Tissoires Cc: Mikko Perttunen, jikos, linux-input, linux-kernel, Mikko Perttunen On Mon, 25 Jul 2016 11:14:04 +0200 Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > On Jul 21 2016 or thereabouts, Antonio Ospite wrote: [...] > > It would be interesting to understand why some (supposedly) compatible > > devices break, maybe they rely on the fact that the PS3 would use the > > original report? > > [just speculating] > I would be *really* surprised if the Sony software were to overwrite the > report descriptor of their own sony controllers. My guess is that their > HID stack or their Playstation driver is compatible with the original > report descriptors and so they can handle the differences between > original and weirdo controllers. > That's basically what I was thinking too, in my last sentence I meant to write report _descriptor_ :) Mikko, if you have some time, could you post the original report descriptor for your device? $ hexdump /sys/bus/hid/devices/XXXX\:YYYY\:ZZZZ.0001/report_descriptor or $ hidrd-convert -i natv -o code /sys/bus/hid/devices/XXXX\:YYYY\:ZZZZ.0001/report_descriptor You can also find out the length of the raw output report with trial and error, start with a line like this: $ sudo hexdump -v -e '49/1 "%02x " "\n"' /dev/hidraw0 and increase/decrease the value (49 works with original controllers) until you see a regular pattern when comparing lines. Anyways this is just for curiosity, we are not going to spend too much time on that... or are we? :) Thanks, Antonio -- Antonio Ospite http://ao2.it A: Because it messes up the order in which people normally read text. See http://en.wikipedia.org/wiki/Posting_style Q: Why is top-posting such a bad thing? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: sony: disable descriptor fixup for FutureMax Dance Mat 2016-07-25 11:05 ` Antonio Ospite @ 2016-07-25 15:23 ` Mikko Perttunen 2016-07-25 15:25 ` Mikko Perttunen 0 siblings, 1 reply; 8+ messages in thread From: Mikko Perttunen @ 2016-07-25 15:23 UTC (permalink / raw) To: Antonio Ospite, Benjamin Tissoires Cc: jikos, linux-input, linux-kernel, Mikko Perttunen On 07/25/16 14:05, Antonio Ospite wrote: > On Mon, 25 Jul 2016 11:14:04 +0200 > Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > >> On Jul 21 2016 or thereabouts, Antonio Ospite wrote: > [...] >>> It would be interesting to understand why some (supposedly) compatible >>> devices break, maybe they rely on the fact that the PS3 would use the >>> original report? >> >> [just speculating] >> I would be *really* surprised if the Sony software were to overwrite the >> report descriptor of their own sony controllers. My guess is that their >> HID stack or their Playstation driver is compatible with the original >> report descriptors and so they can handle the differences between >> original and weirdo controllers. >> > > That's basically what I was thinking too, in my last sentence I meant > to write report _descriptor_ :) > > Mikko, if you have some time, could you post the original report > descriptor for your device? Sure, here's the output from hidrd-convert: 0x05, 0x01, /* Usage Page (Desktop), */ 0x09, 0x04, /* Usage (Joystick), */ 0xA1, 0x01, /* Collection (Application), */ 0x09, 0x01, /* Usage (Pointer), */ 0xA1, 0x00, /* Collection (Physical), */ 0x05, 0x09, /* Usage Page (Button), */ 0x19, 0x01, /* Usage Minimum (01h), */ 0x29, 0x0A, /* Usage Maximum (0Ah), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x25, 0x01, /* Logical Maximum (1), */ 0x75, 0x01, /* Report Size (1), */ 0x95, 0x0A, /* Report Count (10), */ 0x81, 0x02, /* Input (Variable), */ 0x95, 0x06, /* Report Count (6), */ 0x81, 0x03, /* Input (Constant, Variable), */ 0xC0, /* End Collection, */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x15, 0x00, /* Logical Minimum (0), */ 0x25, 0x01, /* Logical Maximum (1), */ 0x25, 0x01, /* Logical Maximum (1), */ 0x25, 0x01, /* Logical Maximum (1), */ 0xC0 /* End Collection */ > > $ hexdump /sys/bus/hid/devices/XXXX\:YYYY\:ZZZZ.0001/report_descriptor > > or > > $ hidrd-convert -i natv -o code /sys/bus/hid/devices/XXXX\:YYYY\:ZZZZ.0001/report_descriptor > > You can also find out the length of the raw output report with trial and > error, start with a line like this: > > $ sudo hexdump -v -e '49/1 "%02x " "\n"' /dev/hidraw0 > > and increase/decrease the value (49 works with original controllers) > until you see a regular pattern when comparing lines. > /dev/hidraw0 seems to be all zeroes straight from the beginning > Anyways this is just for curiosity, we are not going to spend too much > time on that... or are we? :) At least it's some reference information.. :) > > Thanks, > Antonio > Cheers, Mikko. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: sony: disable descriptor fixup for FutureMax Dance Mat 2016-07-25 15:23 ` Mikko Perttunen @ 2016-07-25 15:25 ` Mikko Perttunen 0 siblings, 0 replies; 8+ messages in thread From: Mikko Perttunen @ 2016-07-25 15:25 UTC (permalink / raw) To: Antonio Ospite, Benjamin Tissoires Cc: jikos, linux-input, linux-kernel, Mikko Perttunen On 07/25/16 18:23, Mikko Perttunen wrote: > On 07/25/16 14:05, Antonio Ospite wrote: >> You can also find out the length of the raw output report with trial and >> error, start with a line like this: >> >> $ sudo hexdump -v -e '49/1 "%02x " "\n"' /dev/hidraw0 >> >> and increase/decrease the value (49 works with original controllers) >> until you see a regular pattern when comparing lines. >> > > /dev/hidraw0 seems to be all zeroes straight from the beginning Actually, pressing some buttons on the pad, it seems that the report length is two bytes > >> Anyways this is just for curiosity, we are not going to spend too much >> time on that... or are we? :) > > At least it's some reference information.. :) > >> >> Thanks, >> Antonio >> > > Cheers, > Mikko. > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-07-25 15:25 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-17 17:25 [PATCH] HID: sony: disable descriptor fixup for FutureMax Dance Mat Mikko Perttunen 2016-07-18 14:28 ` Benjamin Tissoires 2016-07-19 6:08 ` Mikko Perttunen 2016-07-21 16:34 ` Antonio Ospite 2016-07-25 9:14 ` Benjamin Tissoires 2016-07-25 11:05 ` Antonio Ospite 2016-07-25 15:23 ` Mikko Perttunen 2016-07-25 15:25 ` Mikko Perttunen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.