* [PATCH] HID: sony: support for the ghlive ps4 dongles @ 2021-07-15 19:57 Daniel Nguyen 2021-07-20 11:36 ` Benjamin Tissoires 0 siblings, 1 reply; 8+ messages in thread From: Daniel Nguyen @ 2021-07-15 19:57 UTC (permalink / raw) To: Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, Daniel Nguyen, Pascal Giard This commit adds support for the Guitar Hero Live PS4 dongles. These dongles require a "magic" USB interrupt message to be sent every 8 seconds otherwise the dongle will not report events where the strumbar is hit while a fret is being held. Note that the GHL_GUITAR_POKE_INTERVAL is reduced to 8 seconds in order to support PS3, Wii U, and PS4 GHL dongles. Also note that the constant for vendor id 0x1430 has been renamed from Activision to RedOctane as self-declared by the device. Co-developed-by: Pascal Giard <pascal.giard@etsmtl.ca> Signed-off-by: Pascal Giard <pascal.giard@etsmtl.ca> Signed-off-by: Daniel Nguyen <daniel.nguyen.1@ens.etsmtl.ca> --- drivers/hid/Kconfig | 2 +- drivers/hid/hid-ids.h | 7 ++-- drivers/hid/hid-sony.c | 76 +++++++++++++++++++++++++++++++++++++----- 3 files changed, 72 insertions(+), 13 deletions(-) diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index 160554903ef9..b743e7f2587a 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -952,7 +952,7 @@ config HID_SONY * Buzz controllers * Sony PS3 Blue-ray Disk Remote Control (Bluetooth) * Logitech Harmony adapter for Sony Playstation 3 (Bluetooth) - * Guitar Hero Live PS3 and Wii U guitar dongles + * Guitar Hero Live PS3, Wii U, and PS4 guitar dongles * Guitar Hero PS3 and PC guitar dongles config SONY_FF diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index 8f1893e68112..55990018836a 100644 --- a/drivers/hid/hid-ids.h +++ b/drivers/hid/hid-ids.h @@ -41,9 +41,6 @@ #define USB_VENDOR_ID_ACTIONSTAR 0x2101 #define USB_DEVICE_ID_ACTIONSTAR_1011 0x1011 -#define USB_VENDOR_ID_ACTIVISION 0x1430 -#define USB_DEVICE_ID_ACTIVISION_GUITAR_DONGLE 0x474c - #define USB_VENDOR_ID_ADS_TECH 0x06e1 #define USB_DEVICE_ID_ADS_TECH_RADIO_SI470X 0xa155 @@ -1018,6 +1015,10 @@ #define USB_VENDOR_ID_REALTEK 0x0bda #define USB_DEVICE_ID_REALTEK_READER 0x0152 +#define USB_VENDOR_ID_REDOCTANE 0x1430 +#define USB_DEVICE_ID_REDOCTANE_GUITAR_DONGLE 0x474c +#define USB_DEVICE_ID_REDOCTANE_PS4_GHLIVE_DONGLE 0x07bb + #define USB_VENDOR_ID_RETROUSB 0xf000 #define USB_DEVICE_ID_RETROUSB_SNES_RETROPAD 0x0003 #define USB_DEVICE_ID_RETROUSB_SNES_RETROPORT 0x00f1 diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c index b3722c51ec78..a8ff6f767e5d 100644 --- a/drivers/hid/hid-sony.c +++ b/drivers/hid/hid-sony.c @@ -11,8 +11,9 @@ * Copyright (c) 2013 Colin Leitner <colin.leitner@gmail.com> * Copyright (c) 2014-2016 Frank Praznik <frank.praznik@gmail.com> * Copyright (c) 2018 Todd Kelner - * Copyright (c) 2020 Pascal Giard <pascal.giard@etsmtl.ca> + * Copyright (c) 2020-2021 Pascal Giard <pascal.giard@etsmtl.ca> * Copyright (c) 2020 Sanjay Govind <sanjay.govind9@gmail.com> + * Copyright (c) 2021 Daniel Nguyen <daniel.nguyen.1@ens.etsmtl.ca> */ /* @@ -62,6 +63,7 @@ #define SHANWAN_GAMEPAD BIT(16) #define GH_GUITAR_CONTROLLER BIT(17) #define GHL_GUITAR_PS3WIIU BIT(18) +#define GHL_GUITAR_PS4 BIT(19) #define SIXAXIS_CONTROLLER (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT) #define MOTION_CONTROLLER (MOTION_CONTROLLER_USB | MOTION_CONTROLLER_BT) @@ -85,7 +87,10 @@ #define NSG_MRXU_MAX_X 1667 #define NSG_MRXU_MAX_Y 1868 -#define GHL_GUITAR_POKE_INTERVAL 10 /* In seconds */ +/* The PS3/Wii U dongles require a poke every 10 seconds, but the PS4 + * requires one every 8 seconds. Using 8 seconds for all for simplicity. + */ +#define GHL_GUITAR_POKE_INTERVAL 8 /* In seconds */ #define GUITAR_TILT_USAGE 44 /* Magic value and data taken from GHLtarUtility: @@ -97,6 +102,13 @@ static const char ghl_ps3wiiu_magic_data[] = { 0x02, 0x08, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00 }; +/* Magic data for the PS4 dongles sniffed with a USB protocol + * analyzer. + */ +static const char ghl_ps4_magic_data[] = { + 0x30, 0x02, 0x08, 0x0A, 0x00, 0x00, 0x00, 0x00, 0x00 +}; + /* PS/3 Motion controller */ static u8 motion_rdesc[] = { 0x05, 0x01, /* Usage Page (Desktop), */ @@ -642,7 +654,7 @@ static void ghl_magic_poke(struct timer_list *t) hid_err(sc->hdev, "usb_submit_urb failed: %d", ret); } -static int ghl_init_urb(struct sony_sc *sc, struct usb_device *usbdev) +static int ghl_init_urb_ps3wiiu(struct sony_sc *sc, struct usb_device *usbdev) { struct usb_ctrlrequest *cr; u16 poke_size; @@ -674,6 +686,47 @@ static int ghl_init_urb(struct sony_sc *sc, struct usb_device *usbdev) return 0; } +static int ghl_init_urb_ps4(struct sony_sc *sc, struct usb_device *usbdev) +{ + int i; + struct usb_interface *intf; + struct usb_endpoint_descriptor *ep; + u16 poke_size; + u8 *databuf; + unsigned int pipe; + struct usb_endpoint_descriptor *ep_irq_out = NULL; + + intf = to_usb_interface(sc->hdev->dev.parent); + if (intf->cur_altsetting->desc.bNumEndpoints != 2) + return -ENODEV; + + for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) { + ep = &intf->cur_altsetting->endpoint[i].desc; + + if (usb_endpoint_xfer_int(ep)) { + if (usb_endpoint_dir_out(ep)) + ep_irq_out = ep; + } + } + + if (!ep_irq_out) + return -ENODEV; + + poke_size = ARRAY_SIZE(ghl_ps4_magic_data); + pipe = usb_sndintpipe(usbdev, ep_irq_out->bEndpointAddress); + + databuf = devm_kzalloc(&sc->hdev->dev, poke_size, GFP_ATOMIC); + if (databuf == NULL) + return -ENOMEM; + + memcpy(databuf, ghl_ps4_magic_data, poke_size); + usb_fill_int_urb( + sc->ghl_urb, usbdev, pipe, + databuf, poke_size, + ghl_magic_poke_cb, sc, ep_irq_out->bInterval); + return 0; +} + static int guitar_mapping(struct hid_device *hdev, struct hid_input *hi, struct hid_field *field, struct hid_usage *usage, unsigned long **bit, int *max) @@ -3030,21 +3083,23 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) return -ENODEV; } - if (sc->quirks & GHL_GUITAR_PS3WIIU) { + if (sc->quirks & (GHL_GUITAR_PS3WIIU | GHL_GUITAR_PS4)) { sc->ghl_urb = usb_alloc_urb(0, GFP_ATOMIC); if (!sc->ghl_urb) return -ENOMEM; - ret = ghl_init_urb(sc, usbdev); + + if (sc->quirks & GHL_GUITAR_PS3WIIU) + ret = ghl_init_urb_ps3wiiu(sc, usbdev); + else if (sc->quirks & GHL_GUITAR_PS4) + ret = ghl_init_urb_ps4(sc, usbdev); if (ret) { hid_err(hdev, "error preparing URB\n"); return ret; } - timer_setup(&sc->ghl_poke_timer, ghl_magic_poke, 0); mod_timer(&sc->ghl_poke_timer, jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); } - return ret; } @@ -3052,7 +3107,7 @@ static void sony_remove(struct hid_device *hdev) { struct sony_sc *sc = hid_get_drvdata(hdev); - if (sc->quirks & GHL_GUITAR_PS3WIIU) { + if (sc->quirks & (GHL_GUITAR_PS3WIIU | GHL_GUITAR_PS4)) { del_timer_sync(&sc->ghl_poke_timer); usb_free_urb(sc->ghl_urb); } @@ -3172,11 +3227,14 @@ static const struct hid_device_id sony_devices[] = { { HID_USB_DEVICE(USB_VENDOR_ID_SONY_RHYTHM, USB_DEVICE_ID_SONY_PS3WIIU_GHLIVE_DONGLE), .driver_data = GHL_GUITAR_PS3WIIU | GH_GUITAR_CONTROLLER }, /* Guitar Hero PC Guitar Dongle */ - { HID_USB_DEVICE(USB_VENDOR_ID_ACTIVISION, USB_DEVICE_ID_ACTIVISION_GUITAR_DONGLE), + { HID_USB_DEVICE(USB_VENDOR_ID_REDOCTANE, USB_DEVICE_ID_REDOCTANE_GUITAR_DONGLE), .driver_data = GH_GUITAR_CONTROLLER }, /* Guitar Hero PS3 World Tour Guitar Dongle */ { HID_USB_DEVICE(USB_VENDOR_ID_SONY_RHYTHM, USB_DEVICE_ID_SONY_PS3_GUITAR_DONGLE), .driver_data = GH_GUITAR_CONTROLLER }, + /* Guitar Hero Live PS4 guitar dongles */ + { HID_USB_DEVICE(USB_VENDOR_ID_REDOCTANE, USB_DEVICE_ID_REDOCTANE_PS4_GHLIVE_DONGLE), + .driver_data = GHL_GUITAR_PS4 | GH_GUITAR_CONTROLLER }, { } }; MODULE_DEVICE_TABLE(hid, sony_devices); -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: sony: support for the ghlive ps4 dongles 2021-07-15 19:57 [PATCH] HID: sony: support for the ghlive ps4 dongles Daniel Nguyen @ 2021-07-20 11:36 ` Benjamin Tissoires 2021-07-20 14:33 ` Pascal Giard 0 siblings, 1 reply; 8+ messages in thread From: Benjamin Tissoires @ 2021-07-20 11:36 UTC (permalink / raw) To: Daniel Nguyen, Colenbrander, Roelof Cc: Jiri Kosina, open list:HID CORE LAYER, Daniel Nguyen, Pascal Giard Hi Daniel (and Pascal), [adding Roderick in Cc who is dealing with the Sony driver from Sony itself :) ] On Thu, Jul 15, 2021 at 9:58 PM Daniel Nguyen <daniel.nguyen.1@ens.etsmtl.ca> wrote: > > This commit adds support for the Guitar Hero Live PS4 dongles. I was about to ask you to add some regression tests to https://gitlab.freedesktop.org/libevdev/hid-tools/-/blob/master/tests/test_sony.py This would likely have avoided the previous patch that was required and cc-ed to stable. But after looking slightly at the patch, I realized that the Guitar Hero code uses direct USB calls, which is not something we can emulate at the hid-tools level. However, after a second look at the code, I think that this part of the code just reimplements its own HID SET_OUTPUT code, and that is something we can easily emulate. Could you check if the following patch is still working properly on a PS3 dongle? and if so, add your PS4 support on top? --- commit 10e14f105553d2bd88bc7748e87154c5a8131e9e Author: Benjamin Tissoires <benjamin.tissoires@redhat.com> Date: Tue Jul 20 11:44:10 2021 +0200 HID: sony: GHL: do not use raw USB calls We can rely on HID to do the job for us. This simplifies the code and also allows to implement regression tests. Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c index b3722c51ec78..901f61d286e8 100644 --- a/drivers/hid/hid-sony.c +++ b/drivers/hid/hid-sony.c @@ -37,7 +37,6 @@ #include <linux/idr.h> #include <linux/input/mt.h> #include <linux/crc32.h> -#include <linux/usb.h> #include <linux/timer.h> #include <asm/unaligned.h> @@ -92,7 +91,7 @@ * https://github.com/ghlre/GHLtarUtility/blob/master/PS3Guitar.cs * Note: The Wii U and PS3 dongles happen to share the same! */ -static const u16 ghl_ps3wiiu_magic_value = 0x201; +static const u16 ghl_ps3wiiu_magic_report = 1; static const char ghl_ps3wiiu_magic_data[] = { 0x02, 0x08, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00 }; @@ -597,7 +596,6 @@ struct sony_sc { /* DS4 calibration data */ struct ds4_calibration_data ds4_calib_data[6]; /* GH Live */ - struct urb *ghl_urb; struct timer_list ghl_poke_timer; }; @@ -622,56 +620,29 @@ static inline void sony_schedule_work(struct sony_sc *sc, } } -static void ghl_magic_poke_cb(struct urb *urb) -{ - struct sony_sc *sc = urb->context; - - if (urb->status < 0) - hid_err(sc->hdev, "URB transfer failed : %d", urb->status); - - mod_timer(&sc->ghl_poke_timer, jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); -} - static void ghl_magic_poke(struct timer_list *t) { int ret; struct sony_sc *sc = from_timer(sc, t, ghl_poke_timer); + const int buf_size = ARRAY_SIZE(ghl_ps3wiiu_magic_data); + u8 *buf; - ret = usb_submit_urb(sc->ghl_urb, GFP_ATOMIC); - if (ret < 0) - hid_err(sc->hdev, "usb_submit_urb failed: %d", ret); -} - -static int ghl_init_urb(struct sony_sc *sc, struct usb_device *usbdev) -{ - struct usb_ctrlrequest *cr; - u16 poke_size; - u8 *databuf; - unsigned int pipe; - - poke_size = ARRAY_SIZE(ghl_ps3wiiu_magic_data); - pipe = usb_sndctrlpipe(usbdev, 0); + buf = kmemdup(ghl_ps3wiiu_magic_data, buf_size, GFP_KERNEL); + if (!buf) + return; - cr = devm_kzalloc(&sc->hdev->dev, sizeof(*cr), GFP_ATOMIC); - if (cr == NULL) - return -ENOMEM; + ret = hid_hw_raw_request(sc->hdev, ghl_ps3wiiu_magic_report, buf, + buf_size, + HID_OUTPUT_REPORT, HID_REQ_SET_REPORT); + if (ret < 0) { + hid_err(sc->hdev, "can't poke ghl magic\n"); + goto out; + } - databuf = devm_kzalloc(&sc->hdev->dev, poke_size, GFP_ATOMIC); - if (databuf == NULL) - return -ENOMEM; + mod_timer(&sc->ghl_poke_timer, jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); - cr->bRequestType = - USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_OUT; - cr->bRequest = USB_REQ_SET_CONFIGURATION; - cr->wValue = cpu_to_le16(ghl_ps3wiiu_magic_value); - cr->wIndex = 0; - cr->wLength = cpu_to_le16(poke_size); - memcpy(databuf, ghl_ps3wiiu_magic_data, poke_size); - usb_fill_control_urb( - sc->ghl_urb, usbdev, pipe, - (unsigned char *) cr, databuf, poke_size, - ghl_magic_poke_cb, sc); - return 0; +out: + kfree(buf); } static int guitar_mapping(struct hid_device *hdev, struct hid_input *hi, @@ -2968,7 +2939,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) int ret; unsigned long quirks = id->driver_data; struct sony_sc *sc; - struct usb_device *usbdev; unsigned int connect_mask = HID_CONNECT_DEFAULT; if (!strcmp(hdev->name, "FutureMax Dance Mat")) @@ -2988,7 +2958,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) sc->quirks = quirks; hid_set_drvdata(hdev, sc); sc->hdev = hdev; - usbdev = to_usb_device(sc->hdev->dev.parent->parent); ret = hid_parse(hdev); if (ret) { @@ -3031,15 +3000,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) } if (sc->quirks & GHL_GUITAR_PS3WIIU) { - sc->ghl_urb = usb_alloc_urb(0, GFP_ATOMIC); - if (!sc->ghl_urb) - return -ENOMEM; - ret = ghl_init_urb(sc, usbdev); - if (ret) { - hid_err(hdev, "error preparing URB\n"); - return ret; - } - timer_setup(&sc->ghl_poke_timer, ghl_magic_poke, 0); mod_timer(&sc->ghl_poke_timer, jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); @@ -3054,7 +3014,6 @@ static void sony_remove(struct hid_device *hdev) if (sc->quirks & GHL_GUITAR_PS3WIIU) { del_timer_sync(&sc->ghl_poke_timer); - usb_free_urb(sc->ghl_urb); } hid_hw_close(hdev); --- Cheers, Benjamin > > These dongles require a "magic" USB interrupt message to be sent > every 8 seconds otherwise the dongle will not report events where > the strumbar is hit while a fret is being held. > > Note that the GHL_GUITAR_POKE_INTERVAL is reduced to 8 seconds in order > to support PS3, Wii U, and PS4 GHL dongles. > > Also note that the constant for vendor id 0x1430 has been renamed from > Activision to RedOctane as self-declared by the device. > > Co-developed-by: Pascal Giard <pascal.giard@etsmtl.ca> > Signed-off-by: Pascal Giard <pascal.giard@etsmtl.ca> > Signed-off-by: Daniel Nguyen <daniel.nguyen.1@ens.etsmtl.ca> > --- > drivers/hid/Kconfig | 2 +- > drivers/hid/hid-ids.h | 7 ++-- > drivers/hid/hid-sony.c | 76 +++++++++++++++++++++++++++++++++++++----- > 3 files changed, 72 insertions(+), 13 deletions(-) > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > index 160554903ef9..b743e7f2587a 100644 > --- a/drivers/hid/Kconfig > +++ b/drivers/hid/Kconfig > @@ -952,7 +952,7 @@ config HID_SONY > * Buzz controllers > * Sony PS3 Blue-ray Disk Remote Control (Bluetooth) > * Logitech Harmony adapter for Sony Playstation 3 (Bluetooth) > - * Guitar Hero Live PS3 and Wii U guitar dongles > + * Guitar Hero Live PS3, Wii U, and PS4 guitar dongles > * Guitar Hero PS3 and PC guitar dongles > > config SONY_FF > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > index 8f1893e68112..55990018836a 100644 > --- a/drivers/hid/hid-ids.h > +++ b/drivers/hid/hid-ids.h > @@ -41,9 +41,6 @@ > #define USB_VENDOR_ID_ACTIONSTAR 0x2101 > #define USB_DEVICE_ID_ACTIONSTAR_1011 0x1011 > > -#define USB_VENDOR_ID_ACTIVISION 0x1430 > -#define USB_DEVICE_ID_ACTIVISION_GUITAR_DONGLE 0x474c > - > #define USB_VENDOR_ID_ADS_TECH 0x06e1 > #define USB_DEVICE_ID_ADS_TECH_RADIO_SI470X 0xa155 > > @@ -1018,6 +1015,10 @@ > #define USB_VENDOR_ID_REALTEK 0x0bda > #define USB_DEVICE_ID_REALTEK_READER 0x0152 > > +#define USB_VENDOR_ID_REDOCTANE 0x1430 > +#define USB_DEVICE_ID_REDOCTANE_GUITAR_DONGLE 0x474c > +#define USB_DEVICE_ID_REDOCTANE_PS4_GHLIVE_DONGLE 0x07bb > + > #define USB_VENDOR_ID_RETROUSB 0xf000 > #define USB_DEVICE_ID_RETROUSB_SNES_RETROPAD 0x0003 > #define USB_DEVICE_ID_RETROUSB_SNES_RETROPORT 0x00f1 > diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c > index b3722c51ec78..a8ff6f767e5d 100644 > --- a/drivers/hid/hid-sony.c > +++ b/drivers/hid/hid-sony.c > @@ -11,8 +11,9 @@ > * Copyright (c) 2013 Colin Leitner <colin.leitner@gmail.com> > * Copyright (c) 2014-2016 Frank Praznik <frank.praznik@gmail.com> > * Copyright (c) 2018 Todd Kelner > - * Copyright (c) 2020 Pascal Giard <pascal.giard@etsmtl.ca> > + * Copyright (c) 2020-2021 Pascal Giard <pascal.giard@etsmtl.ca> > * Copyright (c) 2020 Sanjay Govind <sanjay.govind9@gmail.com> > + * Copyright (c) 2021 Daniel Nguyen <daniel.nguyen.1@ens.etsmtl.ca> > */ > > /* > @@ -62,6 +63,7 @@ > #define SHANWAN_GAMEPAD BIT(16) > #define GH_GUITAR_CONTROLLER BIT(17) > #define GHL_GUITAR_PS3WIIU BIT(18) > +#define GHL_GUITAR_PS4 BIT(19) > > #define SIXAXIS_CONTROLLER (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT) > #define MOTION_CONTROLLER (MOTION_CONTROLLER_USB | MOTION_CONTROLLER_BT) > @@ -85,7 +87,10 @@ > #define NSG_MRXU_MAX_X 1667 > #define NSG_MRXU_MAX_Y 1868 > > -#define GHL_GUITAR_POKE_INTERVAL 10 /* In seconds */ > +/* The PS3/Wii U dongles require a poke every 10 seconds, but the PS4 > + * requires one every 8 seconds. Using 8 seconds for all for simplicity. > + */ > +#define GHL_GUITAR_POKE_INTERVAL 8 /* In seconds */ > #define GUITAR_TILT_USAGE 44 > > /* Magic value and data taken from GHLtarUtility: > @@ -97,6 +102,13 @@ static const char ghl_ps3wiiu_magic_data[] = { > 0x02, 0x08, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00 > }; > > +/* Magic data for the PS4 dongles sniffed with a USB protocol > + * analyzer. > + */ > +static const char ghl_ps4_magic_data[] = { > + 0x30, 0x02, 0x08, 0x0A, 0x00, 0x00, 0x00, 0x00, 0x00 > +}; > + > /* PS/3 Motion controller */ > static u8 motion_rdesc[] = { > 0x05, 0x01, /* Usage Page (Desktop), */ > @@ -642,7 +654,7 @@ static void ghl_magic_poke(struct timer_list *t) > hid_err(sc->hdev, "usb_submit_urb failed: %d", ret); > } > > -static int ghl_init_urb(struct sony_sc *sc, struct usb_device *usbdev) > +static int ghl_init_urb_ps3wiiu(struct sony_sc *sc, struct usb_device *usbdev) > { > struct usb_ctrlrequest *cr; > u16 poke_size; > @@ -674,6 +686,47 @@ static int ghl_init_urb(struct sony_sc *sc, struct usb_device *usbdev) > return 0; > } > > +static int ghl_init_urb_ps4(struct sony_sc *sc, struct usb_device *usbdev) > +{ > + int i; > + struct usb_interface *intf; > + struct usb_endpoint_descriptor *ep; > + u16 poke_size; > + u8 *databuf; > + unsigned int pipe; > + struct usb_endpoint_descriptor *ep_irq_out = NULL; > + > + intf = to_usb_interface(sc->hdev->dev.parent); > + if (intf->cur_altsetting->desc.bNumEndpoints != 2) > + return -ENODEV; > + > + for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) { > + ep = &intf->cur_altsetting->endpoint[i].desc; > + > + if (usb_endpoint_xfer_int(ep)) { > + if (usb_endpoint_dir_out(ep)) > + ep_irq_out = ep; > + } > + } > + > + if (!ep_irq_out) > + return -ENODEV; > + > + poke_size = ARRAY_SIZE(ghl_ps4_magic_data); > + pipe = usb_sndintpipe(usbdev, ep_irq_out->bEndpointAddress); > + > + databuf = devm_kzalloc(&sc->hdev->dev, poke_size, GFP_ATOMIC); > + if (databuf == NULL) > + return -ENOMEM; > + > + memcpy(databuf, ghl_ps4_magic_data, poke_size); > + usb_fill_int_urb( > + sc->ghl_urb, usbdev, pipe, > + databuf, poke_size, > + ghl_magic_poke_cb, sc, ep_irq_out->bInterval); > + return 0; > +} > + > static int guitar_mapping(struct hid_device *hdev, struct hid_input *hi, > struct hid_field *field, struct hid_usage *usage, > unsigned long **bit, int *max) > @@ -3030,21 +3083,23 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) > return -ENODEV; > } > > - if (sc->quirks & GHL_GUITAR_PS3WIIU) { > + if (sc->quirks & (GHL_GUITAR_PS3WIIU | GHL_GUITAR_PS4)) { > sc->ghl_urb = usb_alloc_urb(0, GFP_ATOMIC); > if (!sc->ghl_urb) > return -ENOMEM; > - ret = ghl_init_urb(sc, usbdev); > + > + if (sc->quirks & GHL_GUITAR_PS3WIIU) > + ret = ghl_init_urb_ps3wiiu(sc, usbdev); > + else if (sc->quirks & GHL_GUITAR_PS4) > + ret = ghl_init_urb_ps4(sc, usbdev); > if (ret) { > hid_err(hdev, "error preparing URB\n"); > return ret; > } > - > timer_setup(&sc->ghl_poke_timer, ghl_magic_poke, 0); > mod_timer(&sc->ghl_poke_timer, > jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); > } > - > return ret; > } > > @@ -3052,7 +3107,7 @@ static void sony_remove(struct hid_device *hdev) > { > struct sony_sc *sc = hid_get_drvdata(hdev); > > - if (sc->quirks & GHL_GUITAR_PS3WIIU) { > + if (sc->quirks & (GHL_GUITAR_PS3WIIU | GHL_GUITAR_PS4)) { > del_timer_sync(&sc->ghl_poke_timer); > usb_free_urb(sc->ghl_urb); > } > @@ -3172,11 +3227,14 @@ static const struct hid_device_id sony_devices[] = { > { HID_USB_DEVICE(USB_VENDOR_ID_SONY_RHYTHM, USB_DEVICE_ID_SONY_PS3WIIU_GHLIVE_DONGLE), > .driver_data = GHL_GUITAR_PS3WIIU | GH_GUITAR_CONTROLLER }, > /* Guitar Hero PC Guitar Dongle */ > - { HID_USB_DEVICE(USB_VENDOR_ID_ACTIVISION, USB_DEVICE_ID_ACTIVISION_GUITAR_DONGLE), > + { HID_USB_DEVICE(USB_VENDOR_ID_REDOCTANE, USB_DEVICE_ID_REDOCTANE_GUITAR_DONGLE), > .driver_data = GH_GUITAR_CONTROLLER }, > /* Guitar Hero PS3 World Tour Guitar Dongle */ > { HID_USB_DEVICE(USB_VENDOR_ID_SONY_RHYTHM, USB_DEVICE_ID_SONY_PS3_GUITAR_DONGLE), > .driver_data = GH_GUITAR_CONTROLLER }, > + /* Guitar Hero Live PS4 guitar dongles */ > + { HID_USB_DEVICE(USB_VENDOR_ID_REDOCTANE, USB_DEVICE_ID_REDOCTANE_PS4_GHLIVE_DONGLE), > + .driver_data = GHL_GUITAR_PS4 | GH_GUITAR_CONTROLLER }, > { } > }; > MODULE_DEVICE_TABLE(hid, sony_devices); > -- > 2.25.1 > ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: sony: support for the ghlive ps4 dongles 2021-07-20 11:36 ` Benjamin Tissoires @ 2021-07-20 14:33 ` Pascal Giard 2021-07-20 22:42 ` Nguyen, Daniel 2021-07-21 10:16 ` Benjamin Tissoires 0 siblings, 2 replies; 8+ messages in thread From: Pascal Giard @ 2021-07-20 14:33 UTC (permalink / raw) To: Benjamin Tissoires Cc: Daniel Nguyen, Colenbrander, Roelof, Jiri Kosina, open list:HID CORE LAYER Hi Benjamin, On Tue, Jul 20, 2021 at 7:39 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > Hi Daniel (and Pascal), > > [adding Roderick in Cc who is dealing with the Sony driver from Sony > itself :) ] > > > On Thu, Jul 15, 2021 at 9:58 PM Daniel Nguyen <daniel.nguyen.1@ens.etsmtl.ca> wrote: > > > > This commit adds support for the Guitar Hero Live PS4 dongles. > > I was about to ask you to add some regression tests to > https://gitlab.freedesktop.org/libevdev/hid-tools/-/blob/master/tests/test_sony.py > > This would likely have avoided the previous patch that was required and > cc-ed to stable. > > But after looking slightly at the patch, I realized that the Guitar Hero > code uses direct USB calls, which is not something we can emulate at the > hid-tools level. > > However, after a second look at the code, I think that this part of the > code just reimplements its own HID SET_OUTPUT code, and that is > something we can easily emulate. > > Could you check if the following patch is still working properly on a > PS3 dongle? and if so, add your PS4 support on top? > [...] I wasn't aware that this was possible. Of course we will check whether that works or not. > --- > commit 10e14f105553d2bd88bc7748e87154c5a8131e9e > Author: Benjamin Tissoires <benjamin.tissoires@redhat.com> > Date: Tue Jul 20 11:44:10 2021 +0200 > > HID: sony: GHL: do not use raw USB calls > > We can rely on HID to do the job for us. > This simplifies the code and also allows to implement regression tests. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c > index b3722c51ec78..901f61d286e8 100644 > --- a/drivers/hid/hid-sony.c > +++ b/drivers/hid/hid-sony.c > @@ -37,7 +37,6 @@ > #include <linux/idr.h> > #include <linux/input/mt.h> > #include <linux/crc32.h> > -#include <linux/usb.h> > #include <linux/timer.h> > #include <asm/unaligned.h> > > @@ -92,7 +91,7 @@ > * https://github.com/ghlre/GHLtarUtility/blob/master/PS3Guitar.cs > * Note: The Wii U and PS3 dongles happen to share the same! > */ > -static const u16 ghl_ps3wiiu_magic_value = 0x201; > +static const u16 ghl_ps3wiiu_magic_report = 1; > static const char ghl_ps3wiiu_magic_data[] = { > 0x02, 0x08, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00 > }; > @@ -597,7 +596,6 @@ struct sony_sc { > /* DS4 calibration data */ > struct ds4_calibration_data ds4_calib_data[6]; > /* GH Live */ > - struct urb *ghl_urb; > struct timer_list ghl_poke_timer; > }; > > @@ -622,56 +620,29 @@ static inline void sony_schedule_work(struct sony_sc *sc, > } > } > > -static void ghl_magic_poke_cb(struct urb *urb) > -{ > - struct sony_sc *sc = urb->context; > - > - if (urb->status < 0) > - hid_err(sc->hdev, "URB transfer failed : %d", urb->status); > - > - mod_timer(&sc->ghl_poke_timer, jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); > -} > - > static void ghl_magic_poke(struct timer_list *t) > { > int ret; > struct sony_sc *sc = from_timer(sc, t, ghl_poke_timer); > + const int buf_size = ARRAY_SIZE(ghl_ps3wiiu_magic_data); > + u8 *buf; > > - ret = usb_submit_urb(sc->ghl_urb, GFP_ATOMIC); > - if (ret < 0) > - hid_err(sc->hdev, "usb_submit_urb failed: %d", ret); > -} > - > -static int ghl_init_urb(struct sony_sc *sc, struct usb_device *usbdev) > -{ > - struct usb_ctrlrequest *cr; > - u16 poke_size; > - u8 *databuf; > - unsigned int pipe; > - > - poke_size = ARRAY_SIZE(ghl_ps3wiiu_magic_data); > - pipe = usb_sndctrlpipe(usbdev, 0); > + buf = kmemdup(ghl_ps3wiiu_magic_data, buf_size, GFP_KERNEL); > + if (!buf) > + return; > > - cr = devm_kzalloc(&sc->hdev->dev, sizeof(*cr), GFP_ATOMIC); > - if (cr == NULL) > - return -ENOMEM; > + ret = hid_hw_raw_request(sc->hdev, ghl_ps3wiiu_magic_report, buf, > + buf_size, > + HID_OUTPUT_REPORT, HID_REQ_SET_REPORT); > + if (ret < 0) { > + hid_err(sc->hdev, "can't poke ghl magic\n"); > + goto out; > + } > > - databuf = devm_kzalloc(&sc->hdev->dev, poke_size, GFP_ATOMIC); > - if (databuf == NULL) > - return -ENOMEM; > + mod_timer(&sc->ghl_poke_timer, jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); > > - cr->bRequestType = > - USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_OUT; > - cr->bRequest = USB_REQ_SET_CONFIGURATION; > - cr->wValue = cpu_to_le16(ghl_ps3wiiu_magic_value); > - cr->wIndex = 0; > - cr->wLength = cpu_to_le16(poke_size); > - memcpy(databuf, ghl_ps3wiiu_magic_data, poke_size); > - usb_fill_control_urb( > - sc->ghl_urb, usbdev, pipe, > - (unsigned char *) cr, databuf, poke_size, > - ghl_magic_poke_cb, sc); > - return 0; > +out: > + kfree(buf); > } > > static int guitar_mapping(struct hid_device *hdev, struct hid_input *hi, > @@ -2968,7 +2939,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) > int ret; > unsigned long quirks = id->driver_data; > struct sony_sc *sc; > - struct usb_device *usbdev; > unsigned int connect_mask = HID_CONNECT_DEFAULT; > > if (!strcmp(hdev->name, "FutureMax Dance Mat")) > @@ -2988,7 +2958,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) > sc->quirks = quirks; > hid_set_drvdata(hdev, sc); > sc->hdev = hdev; > - usbdev = to_usb_device(sc->hdev->dev.parent->parent); > > ret = hid_parse(hdev); > if (ret) { > @@ -3031,15 +3000,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) > } > > if (sc->quirks & GHL_GUITAR_PS3WIIU) { > - sc->ghl_urb = usb_alloc_urb(0, GFP_ATOMIC); > - if (!sc->ghl_urb) > - return -ENOMEM; > - ret = ghl_init_urb(sc, usbdev); > - if (ret) { > - hid_err(hdev, "error preparing URB\n"); > - return ret; > - } > - > timer_setup(&sc->ghl_poke_timer, ghl_magic_poke, 0); > mod_timer(&sc->ghl_poke_timer, > jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); > @@ -3054,7 +3014,6 @@ static void sony_remove(struct hid_device *hdev) > > if (sc->quirks & GHL_GUITAR_PS3WIIU) { > del_timer_sync(&sc->ghl_poke_timer); > - usb_free_urb(sc->ghl_urb); > } > > hid_hw_close(hdev); > --- Was your patch against the master branch of hid/hid.git ? I'm asking because it doesn't apply at all on my end, unless I use --ignore-whitespace in which case, 3 out of 8 hunks fail. I assume I may be doing something wrong. I tried both downloading the raw message from marc.info and from patchwork in case gmail would mangle spaces/tabs, same result. Any idea? Thank you for your patience, -Pascal ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: sony: support for the ghlive ps4 dongles 2021-07-20 14:33 ` Pascal Giard @ 2021-07-20 22:42 ` Nguyen, Daniel 2021-07-21 10:16 ` Benjamin Tissoires 1 sibling, 0 replies; 8+ messages in thread From: Nguyen, Daniel @ 2021-07-20 22:42 UTC (permalink / raw) To: Giard, Pascal, Benjamin Tissoires Cc: Colenbrander, Roelof, Jiri Kosina, open list:HID CORE LAYER Hi Benjamin, ________________________________________ From: Pascal Giard <pascal.giard@etsmtl.ca> Sent: Tuesday, July 20, 2021 10:33 AM To: Benjamin Tissoires Cc: Nguyen, Daniel; Colenbrander, Roelof; Jiri Kosina; open list:HID CORE LAYER Subject: Re: [PATCH] HID: sony: support for the ghlive ps4 dongles Hi Benjamin, On Tue, Jul 20, 2021 at 7:39 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > Hi Daniel (and Pascal), > > [adding Roderick in Cc who is dealing with the Sony driver from Sony > itself :) ] > > > On Thu, Jul 15, 2021 at 9:58 PM Daniel Nguyen <daniel.nguyen.1@ens.etsmtl.ca> wrote: > > > > This commit adds support for the Guitar Hero Live PS4 dongles. > > I was about to ask you to add some regression tests to > https://can01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Flibevdev%2Fhid-tools%2F-%2Fblob%2Fmaster%2Ftests%2Ftest_sony.py&data=04%7C01%7Cdaniel.nguyen.1%40ens.etsmtl.ca%7C88d07df3b0e748fe593208d94b8b4f1a%7C70aae3b79f3b484d8f9549e8fbb783c0%7C0%7C0%7C637623883961755185%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=%2BAvFyi%2FYcaVUXnk0a5Wz80N%2F1ZdLWhkExWQyVeBZmxo%3D&reserved=0 > > This would likely have avoided the previous patch that was required and > cc-ed to stable. > > But after looking slightly at the patch, I realized that the Guitar Hero > code uses direct USB calls, which is not something we can emulate at the > hid-tools level. > > However, after a second look at the code, I think that this part of the > code just reimplements its own HID SET_OUTPUT code, and that is > something we can easily emulate. > > Could you check if the following patch is still working properly on a > PS3 dongle? and if so, add your PS4 support on top? > [...] I wasn't aware that this was possible. Of course we will check whether that works or not. Forgive me if I perform this reply incorrectly. It is my first time replying using plain text and responding in-line. I applied the patch manually and tested it on a VM with a dongle while monitoring with Wireshark. I get a hard freeze if I leave the dongle plugged in. I ran a test where i pulled the dongle out before it freezes and i was able to observe the SET_REPORT that was created. The data seems to have corrupted because I see : 01 08 20 00 00 00 00 00, but the first byte should be 02. Regarding the hard freeze, dmesg gives me these bugs/bads/rips: BUG: scheduling while atomic: swapper /0/0/0x00000100 bad: scheduling from the idle thread! RIP: 0010:native_safe_halt+0xb/0x10 Moreover, Pascal applied the patch to his DKMS module and recovered different bugs/bads/rips through dmesg: BUG: scheduling while atomic: swapper/5/0/0x00000100 RIP: 0010:cpuidle_enter_state+0xc7/0x350 > --- > commit 10e14f105553d2bd88bc7748e87154c5a8131e9e > Author: Benjamin Tissoires <benjamin.tissoires@redhat.com> > Date: Tue Jul 20 11:44:10 2021 +0200 > > HID: sony: GHL: do not use raw USB calls > > We can rely on HID to do the job for us. > This simplifies the code and also allows to implement regression tests. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c > index b3722c51ec78..901f61d286e8 100644 > --- a/drivers/hid/hid-sony.c > +++ b/drivers/hid/hid-sony.c > @@ -37,7 +37,6 @@ > #include <linux/idr.h> > #include <linux/input/mt.h> > #include <linux/crc32.h> > -#include <linux/usb.h> > #include <linux/timer.h> > #include <asm/unaligned.h> > > @@ -92,7 +91,7 @@ > * https://can01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fghlre%2FGHLtarUtility%2Fblob%2Fmaster%2FPS3Guitar.cs&data=04%7C01%7Cdaniel.nguyen.1%40ens.etsmtl.ca%7C88d07df3b0e748fe593208d94b8b4f1a%7C70aae3b79f3b484d8f9549e8fbb783c0%7C0%7C0%7C637623883961760177%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=wDZUqmkAgtS2%2BTfcEkyWYbeXjK2xQRei%2Fb1N3MAMlQ4%3D&reserved=0 > * Note: The Wii U and PS3 dongles happen to share the same! > */ > -static const u16 ghl_ps3wiiu_magic_value = 0x201; > +static const u16 ghl_ps3wiiu_magic_report = 1; > static const char ghl_ps3wiiu_magic_data[] = { > 0x02, 0x08, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00 > }; > @@ -597,7 +596,6 @@ struct sony_sc { > /* DS4 calibration data */ > struct ds4_calibration_data ds4_calib_data[6]; > /* GH Live */ > - struct urb *ghl_urb; > struct timer_list ghl_poke_timer; > }; > > @@ -622,56 +620,29 @@ static inline void sony_schedule_work(struct sony_sc *sc, > } > } > > -static void ghl_magic_poke_cb(struct urb *urb) > -{ > - struct sony_sc *sc = urb->context; > - > - if (urb->status < 0) > - hid_err(sc->hdev, "URB transfer failed : %d", urb->status); > - > - mod_timer(&sc->ghl_poke_timer, jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); > -} > - > static void ghl_magic_poke(struct timer_list *t) > { > int ret; > struct sony_sc *sc = from_timer(sc, t, ghl_poke_timer); > + const int buf_size = ARRAY_SIZE(ghl_ps3wiiu_magic_data); > + u8 *buf; > > - ret = usb_submit_urb(sc->ghl_urb, GFP_ATOMIC); > - if (ret < 0) > - hid_err(sc->hdev, "usb_submit_urb failed: %d", ret); > -} > - > -static int ghl_init_urb(struct sony_sc *sc, struct usb_device *usbdev) > -{ > - struct usb_ctrlrequest *cr; > - u16 poke_size; > - u8 *databuf; > - unsigned int pipe; > - > - poke_size = ARRAY_SIZE(ghl_ps3wiiu_magic_data); > - pipe = usb_sndctrlpipe(usbdev, 0); > + buf = kmemdup(ghl_ps3wiiu_magic_data, buf_size, GFP_KERNEL); > + if (!buf) > + return; > > - cr = devm_kzalloc(&sc->hdev->dev, sizeof(*cr), GFP_ATOMIC); > - if (cr == NULL) > - return -ENOMEM; > + ret = hid_hw_raw_request(sc->hdev, ghl_ps3wiiu_magic_report, buf, > + buf_size, > + HID_OUTPUT_REPORT, HID_REQ_SET_REPORT); > + if (ret < 0) { > + hid_err(sc->hdev, "can't poke ghl magic\n"); > + goto out; > + } > > - databuf = devm_kzalloc(&sc->hdev->dev, poke_size, GFP_ATOMIC); > - if (databuf == NULL) > - return -ENOMEM; > + mod_timer(&sc->ghl_poke_timer, jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); > > - cr->bRequestType = > - USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_OUT; > - cr->bRequest = USB_REQ_SET_CONFIGURATION; > - cr->wValue = cpu_to_le16(ghl_ps3wiiu_magic_value); > - cr->wIndex = 0; > - cr->wLength = cpu_to_le16(poke_size); > - memcpy(databuf, ghl_ps3wiiu_magic_data, poke_size); > - usb_fill_control_urb( > - sc->ghl_urb, usbdev, pipe, > - (unsigned char *) cr, databuf, poke_size, > - ghl_magic_poke_cb, sc); > - return 0; > +out: > + kfree(buf); > } > > static int guitar_mapping(struct hid_device *hdev, struct hid_input *hi, > @@ -2968,7 +2939,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) > int ret; > unsigned long quirks = id->driver_data; > struct sony_sc *sc; > - struct usb_device *usbdev; > unsigned int connect_mask = HID_CONNECT_DEFAULT; > > if (!strcmp(hdev->name, "FutureMax Dance Mat")) > @@ -2988,7 +2958,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) > sc->quirks = quirks; > hid_set_drvdata(hdev, sc); > sc->hdev = hdev; > - usbdev = to_usb_device(sc->hdev->dev.parent->parent); > > ret = hid_parse(hdev); > if (ret) { > @@ -3031,15 +3000,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) > } > > if (sc->quirks & GHL_GUITAR_PS3WIIU) { > - sc->ghl_urb = usb_alloc_urb(0, GFP_ATOMIC); > - if (!sc->ghl_urb) > - return -ENOMEM; > - ret = ghl_init_urb(sc, usbdev); > - if (ret) { > - hid_err(hdev, "error preparing URB\n"); > - return ret; > - } > - > timer_setup(&sc->ghl_poke_timer, ghl_magic_poke, 0); > mod_timer(&sc->ghl_poke_timer, > jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); > @@ -3054,7 +3014,6 @@ static void sony_remove(struct hid_device *hdev) > > if (sc->quirks & GHL_GUITAR_PS3WIIU) { > del_timer_sync(&sc->ghl_poke_timer); > - usb_free_urb(sc->ghl_urb); > } > > hid_hw_close(hdev); > --- Was your patch against the master branch of hid/hid.git ? I'm asking because it doesn't apply at all on my end, unless I use --ignore-whitespace in which case, 3 out of 8 hunks fail. I assume I may be doing something wrong. I tried both downloading the raw message from marc.info and from patchwork in case gmail would mangle spaces/tabs, same result. Any idea? Thank you for your patience, -Pascal It appears as though the timer is causing the issues for hard freeze, but I can't figure out why the data is different. Any idea? -Daniel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: sony: support for the ghlive ps4 dongles 2021-07-20 14:33 ` Pascal Giard 2021-07-20 22:42 ` Nguyen, Daniel @ 2021-07-21 10:16 ` Benjamin Tissoires 2021-07-21 18:15 ` Daniel Nguyen 1 sibling, 1 reply; 8+ messages in thread From: Benjamin Tissoires @ 2021-07-21 10:16 UTC (permalink / raw) To: Pascal Giard Cc: Daniel Nguyen, Colenbrander, Roelof, Jiri Kosina, open list:HID CORE LAYER On Tue, Jul 20, 2021 at 4:33 PM Pascal Giard <pascal.giard@etsmtl.ca> wrote: > > Hi Benjamin, > > On Tue, Jul 20, 2021 at 7:39 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > Hi Daniel (and Pascal), > > > > [adding Roderick in Cc who is dealing with the Sony driver from Sony > > itself :) ] > > > > > > On Thu, Jul 15, 2021 at 9:58 PM Daniel Nguyen <daniel.nguyen.1@ens.etsmtl.ca> wrote: > > > > > > This commit adds support for the Guitar Hero Live PS4 dongles. > > > > I was about to ask you to add some regression tests to > > https://gitlab.freedesktop.org/libevdev/hid-tools/-/blob/master/tests/test_sony.py > > > > This would likely have avoided the previous patch that was required and > > cc-ed to stable. > > > > But after looking slightly at the patch, I realized that the Guitar Hero > > code uses direct USB calls, which is not something we can emulate at the > > hid-tools level. > > > > However, after a second look at the code, I think that this part of the > > code just reimplements its own HID SET_OUTPUT code, and that is > > something we can easily emulate. > > > > Could you check if the following patch is still working properly on a > > PS3 dongle? and if so, add your PS4 support on top? > > > [...] > > I wasn't aware that this was possible. Of course we will check whether > that works or not. > > > --- > > commit 10e14f105553d2bd88bc7748e87154c5a8131e9e > > Author: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > Date: Tue Jul 20 11:44:10 2021 +0200 > > > > HID: sony: GHL: do not use raw USB calls > > > > We can rely on HID to do the job for us. > > This simplifies the code and also allows to implement regression tests. > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c > > index b3722c51ec78..901f61d286e8 100644 > > --- a/drivers/hid/hid-sony.c > > +++ b/drivers/hid/hid-sony.c > > @@ -37,7 +37,6 @@ > > #include <linux/idr.h> > > #include <linux/input/mt.h> > > #include <linux/crc32.h> > > -#include <linux/usb.h> > > #include <linux/timer.h> > > #include <asm/unaligned.h> > > > > @@ -92,7 +91,7 @@ > > * https://github.com/ghlre/GHLtarUtility/blob/master/PS3Guitar.cs > > * Note: The Wii U and PS3 dongles happen to share the same! > > */ > > -static const u16 ghl_ps3wiiu_magic_value = 0x201; > > +static const u16 ghl_ps3wiiu_magic_report = 1; > > static const char ghl_ps3wiiu_magic_data[] = { > > 0x02, 0x08, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00 > > }; > > @@ -597,7 +596,6 @@ struct sony_sc { > > /* DS4 calibration data */ > > struct ds4_calibration_data ds4_calib_data[6]; > > /* GH Live */ > > - struct urb *ghl_urb; > > struct timer_list ghl_poke_timer; > > }; > > > > @@ -622,56 +620,29 @@ static inline void sony_schedule_work(struct sony_sc *sc, > > } > > } > > > > -static void ghl_magic_poke_cb(struct urb *urb) > > -{ > > - struct sony_sc *sc = urb->context; > > - > > - if (urb->status < 0) > > - hid_err(sc->hdev, "URB transfer failed : %d", urb->status); > > - > > - mod_timer(&sc->ghl_poke_timer, jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); > > -} > > - > > static void ghl_magic_poke(struct timer_list *t) > > { > > int ret; > > struct sony_sc *sc = from_timer(sc, t, ghl_poke_timer); > > + const int buf_size = ARRAY_SIZE(ghl_ps3wiiu_magic_data); > > + u8 *buf; > > > > - ret = usb_submit_urb(sc->ghl_urb, GFP_ATOMIC); > > - if (ret < 0) > > - hid_err(sc->hdev, "usb_submit_urb failed: %d", ret); > > -} > > - > > -static int ghl_init_urb(struct sony_sc *sc, struct usb_device *usbdev) > > -{ > > - struct usb_ctrlrequest *cr; > > - u16 poke_size; > > - u8 *databuf; > > - unsigned int pipe; > > - > > - poke_size = ARRAY_SIZE(ghl_ps3wiiu_magic_data); > > - pipe = usb_sndctrlpipe(usbdev, 0); > > + buf = kmemdup(ghl_ps3wiiu_magic_data, buf_size, GFP_KERNEL); > > + if (!buf) > > + return; > > > > - cr = devm_kzalloc(&sc->hdev->dev, sizeof(*cr), GFP_ATOMIC); > > - if (cr == NULL) > > - return -ENOMEM; > > + ret = hid_hw_raw_request(sc->hdev, ghl_ps3wiiu_magic_report, buf, > > + buf_size, > > + HID_OUTPUT_REPORT, HID_REQ_SET_REPORT); > > + if (ret < 0) { > > + hid_err(sc->hdev, "can't poke ghl magic\n"); > > + goto out; > > + } > > > > - databuf = devm_kzalloc(&sc->hdev->dev, poke_size, GFP_ATOMIC); > > - if (databuf == NULL) > > - return -ENOMEM; > > + mod_timer(&sc->ghl_poke_timer, jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); > > > > - cr->bRequestType = > > - USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_OUT; > > - cr->bRequest = USB_REQ_SET_CONFIGURATION; > > - cr->wValue = cpu_to_le16(ghl_ps3wiiu_magic_value); > > - cr->wIndex = 0; > > - cr->wLength = cpu_to_le16(poke_size); > > - memcpy(databuf, ghl_ps3wiiu_magic_data, poke_size); > > - usb_fill_control_urb( > > - sc->ghl_urb, usbdev, pipe, > > - (unsigned char *) cr, databuf, poke_size, > > - ghl_magic_poke_cb, sc); > > - return 0; > > +out: > > + kfree(buf); > > } > > > > static int guitar_mapping(struct hid_device *hdev, struct hid_input *hi, > > @@ -2968,7 +2939,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) > > int ret; > > unsigned long quirks = id->driver_data; > > struct sony_sc *sc; > > - struct usb_device *usbdev; > > unsigned int connect_mask = HID_CONNECT_DEFAULT; > > > > if (!strcmp(hdev->name, "FutureMax Dance Mat")) > > @@ -2988,7 +2958,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) > > sc->quirks = quirks; > > hid_set_drvdata(hdev, sc); > > sc->hdev = hdev; > > - usbdev = to_usb_device(sc->hdev->dev.parent->parent); > > > > ret = hid_parse(hdev); > > if (ret) { > > @@ -3031,15 +3000,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) > > } > > > > if (sc->quirks & GHL_GUITAR_PS3WIIU) { > > - sc->ghl_urb = usb_alloc_urb(0, GFP_ATOMIC); > > - if (!sc->ghl_urb) > > - return -ENOMEM; > > - ret = ghl_init_urb(sc, usbdev); > > - if (ret) { > > - hid_err(hdev, "error preparing URB\n"); > > - return ret; > > - } > > - > > timer_setup(&sc->ghl_poke_timer, ghl_magic_poke, 0); > > mod_timer(&sc->ghl_poke_timer, > > jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); > > @@ -3054,7 +3014,6 @@ static void sony_remove(struct hid_device *hdev) > > > > if (sc->quirks & GHL_GUITAR_PS3WIIU) { > > del_timer_sync(&sc->ghl_poke_timer); > > - usb_free_urb(sc->ghl_urb); > > } > > > > hid_hw_close(hdev); > > --- > > Was your patch against the master branch of hid/hid.git ? It was against the branch for-next of hid/hid.git, to account for the PS3 fix that was sent earlier. > I'm asking because it doesn't apply at all on my end, unless I use > --ignore-whitespace in which case, 3 out of 8 hunks fail. > > I assume I may be doing something wrong. I tried both downloading the > raw message from marc.info and from patchwork in case gmail would > mangle spaces/tabs, same result. > > Any idea? Usually opening the source of the email, and doing `git am -3`, hit enter, then paste the content of the patch, then Ctrl-D is the quickest you can do to apply such inlined patches (at least, that's what I do). But Daniel found out that the patch is buggy, so let's concentrate on the next iteration. Cheers, Benjamin > > Thank you for your patience, > > -Pascal > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: sony: support for the ghlive ps4 dongles 2021-07-21 10:16 ` Benjamin Tissoires @ 2021-07-21 18:15 ` Daniel Nguyen 2021-07-21 18:44 ` Benjamin Tissoires 0 siblings, 1 reply; 8+ messages in thread From: Daniel Nguyen @ 2021-07-21 18:15 UTC (permalink / raw) To: Benjamin Tissoires Cc: Pascal Giard, Colenbrander, Roelof, Jiri Kosina, open list:HID CORE LAYER Hello Benjamin, On Wed, Jul 21, 2021 at 6:16 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Tue, Jul 20, 2021 at 4:33 PM Pascal Giard <pascal.giard@etsmtl.ca> wrote: > > > > Hi Benjamin, > > > > On Tue, Jul 20, 2021 at 7:39 AM Benjamin Tissoires > > <benjamin.tissoires@redhat.com> wrote: > > > > > > Hi Daniel (and Pascal), > > > > > > [adding Roderick in Cc who is dealing with the Sony driver from Sony > > > itself :) ] > > > > > > > > > On Thu, Jul 15, 2021 at 9:58 PM Daniel Nguyen <daniel.nguyen.1@ens.etsmtl.ca> wrote: > > > > > > > > This commit adds support for the Guitar Hero Live PS4 dongles. > > > > > > I was about to ask you to add some regression tests to > > > https://gitlab.freedesktop.org/libevdev/hid-tools/-/blob/master/tests/test_sony.py > > > > > > This would likely have avoided the previous patch that was required and > > > cc-ed to stable. > > > > > > But after looking slightly at the patch, I realized that the Guitar Hero > > > code uses direct USB calls, which is not something we can emulate at the > > > hid-tools level. > > > > > > However, after a second look at the code, I think that this part of the > > > code just reimplements its own HID SET_OUTPUT code, and that is > > > something we can easily emulate. > > > > > > Could you check if the following patch is still working properly on a > > > PS3 dongle? and if so, add your PS4 support on top? > > > > > [...] > > > > I wasn't aware that this was possible. Of course we will check whether > > that works or not. > > > > > --- > > > commit 10e14f105553d2bd88bc7748e87154c5a8131e9e > > > Author: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > Date: Tue Jul 20 11:44:10 2021 +0200 > > > > > > HID: sony: GHL: do not use raw USB calls > > > > > > We can rely on HID to do the job for us. > > > This simplifies the code and also allows to implement regression tests. > > > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > > > diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c > > > index b3722c51ec78..901f61d286e8 100644 > > > --- a/drivers/hid/hid-sony.c > > > +++ b/drivers/hid/hid-sony.c > > > @@ -37,7 +37,6 @@ > > > #include <linux/idr.h> > > > #include <linux/input/mt.h> > > > #include <linux/crc32.h> > > > -#include <linux/usb.h> > > > #include <linux/timer.h> > > > #include <asm/unaligned.h> > > > > > > @@ -92,7 +91,7 @@ > > > * https://github.com/ghlre/GHLtarUtility/blob/master/PS3Guitar.cs > > > * Note: The Wii U and PS3 dongles happen to share the same! > > > */ > > > -static const u16 ghl_ps3wiiu_magic_value = 0x201; > > > +static const u16 ghl_ps3wiiu_magic_report = 1; > > > static const char ghl_ps3wiiu_magic_data[] = { > > > 0x02, 0x08, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00 > > > }; > > > @@ -597,7 +596,6 @@ struct sony_sc { > > > /* DS4 calibration data */ > > > struct ds4_calibration_data ds4_calib_data[6]; > > > /* GH Live */ > > > - struct urb *ghl_urb; > > > struct timer_list ghl_poke_timer; > > > }; > > > > > > @@ -622,56 +620,29 @@ static inline void sony_schedule_work(struct sony_sc *sc, > > > } > > > } > > > > > > -static void ghl_magic_poke_cb(struct urb *urb) > > > -{ > > > - struct sony_sc *sc = urb->context; > > > - > > > - if (urb->status < 0) > > > - hid_err(sc->hdev, "URB transfer failed : %d", urb->status); > > > - > > > - mod_timer(&sc->ghl_poke_timer, jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); > > > -} > > > - > > > static void ghl_magic_poke(struct timer_list *t) > > > { > > > int ret; > > > struct sony_sc *sc = from_timer(sc, t, ghl_poke_timer); > > > + const int buf_size = ARRAY_SIZE(ghl_ps3wiiu_magic_data); > > > + u8 *buf; > > > > > > - ret = usb_submit_urb(sc->ghl_urb, GFP_ATOMIC); > > > - if (ret < 0) > > > - hid_err(sc->hdev, "usb_submit_urb failed: %d", ret); > > > -} > > > - > > > -static int ghl_init_urb(struct sony_sc *sc, struct usb_device *usbdev) > > > -{ > > > - struct usb_ctrlrequest *cr; > > > - u16 poke_size; > > > - u8 *databuf; > > > - unsigned int pipe; > > > - > > > - poke_size = ARRAY_SIZE(ghl_ps3wiiu_magic_data); > > > - pipe = usb_sndctrlpipe(usbdev, 0); > > > + buf = kmemdup(ghl_ps3wiiu_magic_data, buf_size, GFP_KERNEL); > > > + if (!buf) > > > + return; > > > > > > - cr = devm_kzalloc(&sc->hdev->dev, sizeof(*cr), GFP_ATOMIC); > > > - if (cr == NULL) > > > - return -ENOMEM; > > > + ret = hid_hw_raw_request(sc->hdev, ghl_ps3wiiu_magic_report, buf, > > > + buf_size, > > > + HID_OUTPUT_REPORT, HID_REQ_SET_REPORT); > > > + if (ret < 0) { > > > + hid_err(sc->hdev, "can't poke ghl magic\n"); > > > + goto out; > > > + } > > > > > > - databuf = devm_kzalloc(&sc->hdev->dev, poke_size, GFP_ATOMIC); > > > - if (databuf == NULL) > > > - return -ENOMEM; > > > + mod_timer(&sc->ghl_poke_timer, jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); > > > > > > - cr->bRequestType = > > > - USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_OUT; > > > - cr->bRequest = USB_REQ_SET_CONFIGURATION; > > > - cr->wValue = cpu_to_le16(ghl_ps3wiiu_magic_value); > > > - cr->wIndex = 0; > > > - cr->wLength = cpu_to_le16(poke_size); > > > - memcpy(databuf, ghl_ps3wiiu_magic_data, poke_size); > > > - usb_fill_control_urb( > > > - sc->ghl_urb, usbdev, pipe, > > > - (unsigned char *) cr, databuf, poke_size, > > > - ghl_magic_poke_cb, sc); > > > - return 0; > > > +out: > > > + kfree(buf); > > > } > > > > > > static int guitar_mapping(struct hid_device *hdev, struct hid_input *hi, > > > @@ -2968,7 +2939,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) > > > int ret; > > > unsigned long quirks = id->driver_data; > > > struct sony_sc *sc; > > > - struct usb_device *usbdev; > > > unsigned int connect_mask = HID_CONNECT_DEFAULT; > > > > > > if (!strcmp(hdev->name, "FutureMax Dance Mat")) > > > @@ -2988,7 +2958,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) > > > sc->quirks = quirks; > > > hid_set_drvdata(hdev, sc); > > > sc->hdev = hdev; > > > - usbdev = to_usb_device(sc->hdev->dev.parent->parent); > > > > > > ret = hid_parse(hdev); > > > if (ret) { > > > @@ -3031,15 +3000,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) > > > } > > > > > > if (sc->quirks & GHL_GUITAR_PS3WIIU) { > > > - sc->ghl_urb = usb_alloc_urb(0, GFP_ATOMIC); > > > - if (!sc->ghl_urb) > > > - return -ENOMEM; > > > - ret = ghl_init_urb(sc, usbdev); > > > - if (ret) { > > > - hid_err(hdev, "error preparing URB\n"); > > > - return ret; > > > - } > > > - > > > timer_setup(&sc->ghl_poke_timer, ghl_magic_poke, 0); > > > mod_timer(&sc->ghl_poke_timer, > > > jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); > > > @@ -3054,7 +3014,6 @@ static void sony_remove(struct hid_device *hdev) > > > > > > if (sc->quirks & GHL_GUITAR_PS3WIIU) { > > > del_timer_sync(&sc->ghl_poke_timer); > > > - usb_free_urb(sc->ghl_urb); > > > } > > > > > > hid_hw_close(hdev); > > > --- > > > > Was your patch against the master branch of hid/hid.git ? > > It was against the branch for-next of hid/hid.git, to account for the > PS3 fix that was sent earlier. > > > I'm asking because it doesn't apply at all on my end, unless I use > > --ignore-whitespace in which case, 3 out of 8 hunks fail. > > > > I assume I may be doing something wrong. I tried both downloading the > > raw message from marc.info and from patchwork in case gmail would > > mangle spaces/tabs, same result. > > > > Any idea? > > Usually opening the source of the email, and doing `git am -3`, hit > enter, then paste the content of the patch, then Ctrl-D is the > quickest you can do to apply such inlined patches (at least, that's > what I do). > > But Daniel found out that the patch is buggy, so let's concentrate on > the next iteration. I dove deeper into the proposed method and verified the functions in order to better understand how they work. I noticed that the const that you created (ghl_ps3wiiu_magic_report = 1) is modifying the first byte of the data to 0x01. I also noticed that it is what creates the wValue = 0x201, which explains why you set it to 1. After testing, I noticed that if we maintain the wValue = 0x201, and therefore byte[0] = 0x01, the dongle does not function as it should. However, if we change the const (ghl_ps3wiiu_magic_report = 2), although we have wValue = 0x202, the dongle does function as expected. That's surprising to us. Do you have any insights into the role of wValue in this context? We came up with 0x201 as we were simply reproducing what we had sniffed. Despite the error message related to scheduling, I have not been able to pinpoint the source of the hard freeze. Any hint there would be appreciated. Lastly, I wanted to point out that for the PS4 dongle support, the magic data is not sent via control transfers, but through interrupt transfers. Does the HID infrastructure have the necessary mechanisms to do that as well or will we have to resort to the direct USB calls (like currently done in the patch I submitted)? Thank you for your patience. -Daniel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: sony: support for the ghlive ps4 dongles 2021-07-21 18:15 ` Daniel Nguyen @ 2021-07-21 18:44 ` Benjamin Tissoires 2021-07-23 19:07 ` Daniel Nguyen 0 siblings, 1 reply; 8+ messages in thread From: Benjamin Tissoires @ 2021-07-21 18:44 UTC (permalink / raw) To: Daniel Nguyen Cc: Pascal Giard, Colenbrander, Roelof, Jiri Kosina, open list:HID CORE LAYER On Wed, Jul 21, 2021 at 8:15 PM Daniel Nguyen <daniel.nguyen.1@ens.etsmtl.ca> wrote: > > Hello Benjamin, > > On Wed, Jul 21, 2021 at 6:16 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > On Tue, Jul 20, 2021 at 4:33 PM Pascal Giard <pascal.giard@etsmtl.ca> wrote: > > > > > > Hi Benjamin, > > > > > > On Tue, Jul 20, 2021 at 7:39 AM Benjamin Tissoires > > > <benjamin.tissoires@redhat.com> wrote: > > > > > > > > Hi Daniel (and Pascal), > > > > > > > > [adding Roderick in Cc who is dealing with the Sony driver from Sony > > > > itself :) ] > > > > > > > > > > > > On Thu, Jul 15, 2021 at 9:58 PM Daniel Nguyen <daniel.nguyen.1@ens.etsmtl.ca> wrote: > > > > > > > > > > This commit adds support for the Guitar Hero Live PS4 dongles. > > > > > > > > I was about to ask you to add some regression tests to > > > > https://gitlab.freedesktop.org/libevdev/hid-tools/-/blob/master/tests/test_sony.py > > > > > > > > This would likely have avoided the previous patch that was required and > > > > cc-ed to stable. > > > > > > > > But after looking slightly at the patch, I realized that the Guitar Hero > > > > code uses direct USB calls, which is not something we can emulate at the > > > > hid-tools level. > > > > > > > > However, after a second look at the code, I think that this part of the > > > > code just reimplements its own HID SET_OUTPUT code, and that is > > > > something we can easily emulate. > > > > > > > > Could you check if the following patch is still working properly on a > > > > PS3 dongle? and if so, add your PS4 support on top? > > > > > > > [...] > > > > > > I wasn't aware that this was possible. Of course we will check whether > > > that works or not. > > > > > > > --- > > > > commit 10e14f105553d2bd88bc7748e87154c5a8131e9e > > > > Author: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > Date: Tue Jul 20 11:44:10 2021 +0200 > > > > > > > > HID: sony: GHL: do not use raw USB calls > > > > > > > > We can rely on HID to do the job for us. > > > > This simplifies the code and also allows to implement regression tests. > > > > > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > > > > > diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c > > > > index b3722c51ec78..901f61d286e8 100644 > > > > --- a/drivers/hid/hid-sony.c > > > > +++ b/drivers/hid/hid-sony.c > > > > @@ -37,7 +37,6 @@ > > > > #include <linux/idr.h> > > > > #include <linux/input/mt.h> > > > > #include <linux/crc32.h> > > > > -#include <linux/usb.h> > > > > #include <linux/timer.h> > > > > #include <asm/unaligned.h> > > > > > > > > @@ -92,7 +91,7 @@ > > > > * https://github.com/ghlre/GHLtarUtility/blob/master/PS3Guitar.cs > > > > * Note: The Wii U and PS3 dongles happen to share the same! > > > > */ > > > > -static const u16 ghl_ps3wiiu_magic_value = 0x201; > > > > +static const u16 ghl_ps3wiiu_magic_report = 1; > > > > static const char ghl_ps3wiiu_magic_data[] = { > > > > 0x02, 0x08, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00 > > > > }; > > > > @@ -597,7 +596,6 @@ struct sony_sc { > > > > /* DS4 calibration data */ > > > > struct ds4_calibration_data ds4_calib_data[6]; > > > > /* GH Live */ > > > > - struct urb *ghl_urb; > > > > struct timer_list ghl_poke_timer; > > > > }; > > > > > > > > @@ -622,56 +620,29 @@ static inline void sony_schedule_work(struct sony_sc *sc, > > > > } > > > > } > > > > > > > > -static void ghl_magic_poke_cb(struct urb *urb) > > > > -{ > > > > - struct sony_sc *sc = urb->context; > > > > - > > > > - if (urb->status < 0) > > > > - hid_err(sc->hdev, "URB transfer failed : %d", urb->status); > > > > - > > > > - mod_timer(&sc->ghl_poke_timer, jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); > > > > -} > > > > - > > > > static void ghl_magic_poke(struct timer_list *t) > > > > { > > > > int ret; > > > > struct sony_sc *sc = from_timer(sc, t, ghl_poke_timer); > > > > + const int buf_size = ARRAY_SIZE(ghl_ps3wiiu_magic_data); > > > > + u8 *buf; > > > > > > > > - ret = usb_submit_urb(sc->ghl_urb, GFP_ATOMIC); > > > > - if (ret < 0) > > > > - hid_err(sc->hdev, "usb_submit_urb failed: %d", ret); > > > > -} > > > > - > > > > -static int ghl_init_urb(struct sony_sc *sc, struct usb_device *usbdev) > > > > -{ > > > > - struct usb_ctrlrequest *cr; > > > > - u16 poke_size; > > > > - u8 *databuf; > > > > - unsigned int pipe; > > > > - > > > > - poke_size = ARRAY_SIZE(ghl_ps3wiiu_magic_data); > > > > - pipe = usb_sndctrlpipe(usbdev, 0); > > > > + buf = kmemdup(ghl_ps3wiiu_magic_data, buf_size, GFP_KERNEL); > > > > + if (!buf) > > > > + return; > > > > > > > > - cr = devm_kzalloc(&sc->hdev->dev, sizeof(*cr), GFP_ATOMIC); > > > > - if (cr == NULL) > > > > - return -ENOMEM; > > > > + ret = hid_hw_raw_request(sc->hdev, ghl_ps3wiiu_magic_report, buf, > > > > + buf_size, > > > > + HID_OUTPUT_REPORT, HID_REQ_SET_REPORT); > > > > + if (ret < 0) { > > > > + hid_err(sc->hdev, "can't poke ghl magic\n"); > > > > + goto out; > > > > + } > > > > > > > > - databuf = devm_kzalloc(&sc->hdev->dev, poke_size, GFP_ATOMIC); > > > > - if (databuf == NULL) > > > > - return -ENOMEM; > > > > + mod_timer(&sc->ghl_poke_timer, jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); > > > > > > > > - cr->bRequestType = > > > > - USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_OUT; > > > > - cr->bRequest = USB_REQ_SET_CONFIGURATION; > > > > - cr->wValue = cpu_to_le16(ghl_ps3wiiu_magic_value); > > > > - cr->wIndex = 0; > > > > - cr->wLength = cpu_to_le16(poke_size); > > > > - memcpy(databuf, ghl_ps3wiiu_magic_data, poke_size); > > > > - usb_fill_control_urb( > > > > - sc->ghl_urb, usbdev, pipe, > > > > - (unsigned char *) cr, databuf, poke_size, > > > > - ghl_magic_poke_cb, sc); > > > > - return 0; > > > > +out: > > > > + kfree(buf); > > > > } > > > > > > > > static int guitar_mapping(struct hid_device *hdev, struct hid_input *hi, > > > > @@ -2968,7 +2939,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) > > > > int ret; > > > > unsigned long quirks = id->driver_data; > > > > struct sony_sc *sc; > > > > - struct usb_device *usbdev; > > > > unsigned int connect_mask = HID_CONNECT_DEFAULT; > > > > > > > > if (!strcmp(hdev->name, "FutureMax Dance Mat")) > > > > @@ -2988,7 +2958,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) > > > > sc->quirks = quirks; > > > > hid_set_drvdata(hdev, sc); > > > > sc->hdev = hdev; > > > > - usbdev = to_usb_device(sc->hdev->dev.parent->parent); > > > > > > > > ret = hid_parse(hdev); > > > > if (ret) { > > > > @@ -3031,15 +3000,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) > > > > } > > > > > > > > if (sc->quirks & GHL_GUITAR_PS3WIIU) { > > > > - sc->ghl_urb = usb_alloc_urb(0, GFP_ATOMIC); > > > > - if (!sc->ghl_urb) > > > > - return -ENOMEM; > > > > - ret = ghl_init_urb(sc, usbdev); > > > > - if (ret) { > > > > - hid_err(hdev, "error preparing URB\n"); > > > > - return ret; > > > > - } > > > > - > > > > timer_setup(&sc->ghl_poke_timer, ghl_magic_poke, 0); > > > > mod_timer(&sc->ghl_poke_timer, > > > > jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); > > > > @@ -3054,7 +3014,6 @@ static void sony_remove(struct hid_device *hdev) > > > > > > > > if (sc->quirks & GHL_GUITAR_PS3WIIU) { > > > > del_timer_sync(&sc->ghl_poke_timer); > > > > - usb_free_urb(sc->ghl_urb); > > > > } > > > > > > > > hid_hw_close(hdev); > > > > --- > > > > > > Was your patch against the master branch of hid/hid.git ? > > > > It was against the branch for-next of hid/hid.git, to account for the > > PS3 fix that was sent earlier. > > > > > I'm asking because it doesn't apply at all on my end, unless I use > > > --ignore-whitespace in which case, 3 out of 8 hunks fail. > > > > > > I assume I may be doing something wrong. I tried both downloading the > > > raw message from marc.info and from patchwork in case gmail would > > > mangle spaces/tabs, same result. > > > > > > Any idea? > > > > Usually opening the source of the email, and doing `git am -3`, hit > > enter, then paste the content of the patch, then Ctrl-D is the > > quickest you can do to apply such inlined patches (at least, that's > > what I do). > > > > But Daniel found out that the patch is buggy, so let's concentrate on > > the next iteration. > > I dove deeper into the proposed method and verified the functions in > order to better understand how they work. I noticed that the const that you > created (ghl_ps3wiiu_magic_report = 1) is modifying the first byte of the data > to 0x01. I also noticed that it is what creates the wValue = 0x201, which > explains why you set it to 1. > > After testing, I noticed that if we maintain the wValue = 0x201, and therefore > byte[0] = 0x01, the dongle does not function as it should. However, if we > change the const (ghl_ps3wiiu_magic_report = 2), although we have > wValue = 0x202, the dongle does function as expected. That's surprising > to us. Do you have any insights into the role of wValue in this context? > We came up with 0x201 as we were simply reproducing what > we had sniffed. The answer is in usbhid_set_raw_report() (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/usbhid/hid-core.c#n907) -> the wValue is used as `((rtype + 1) << 8) | reportnum` where rtype is the report type (declared in include/linux/hid.h) #define HID_INPUT_REPORT 0 #define HID_OUTPUT_REPORT 1 #define HID_FEATURE_REPORT 2 And reportnum is the report number (the one matching the report ID if you look at the report descriptors. I am as much as puzzled as you regarding the fact that writing the urb directly works with '1' when it doesn't with the HID stack... Actually, the HID stack would overwrite the first byte with the report ID, which explains why the report gets changed. However, why was the initial call setting a HID report ID of 1 but then addressing the report 2??? That's probably a mystery we won't solve :) > > Despite the error message related to scheduling, I have not been able to > pinpoint the source of the hard freeze. Any hint there would be appreciated. I would have to think about this a little bit more, but I have been dragged all day with other internal work :( > > Lastly, I wanted to point out that for the PS4 dongle support, the > magic data is > not sent via control transfers, but through interrupt transfers. Does the HID > infrastructure have the necessary mechanisms to do that as well or will we > have to resort to the direct USB calls (like currently done in the patch > I submitted)? We have usbhid_output_report in this file, which can be genetically called via hid_hw_output_report() Also, IIRC it happens that sometimes a device would accept both interrupt or control transfers for these kind of methods. It might be interesting to see if you can not have a common path between the 2. > > Thank you for your patience. Heh, no worries :) Cheers, Benjamin > -Daniel > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: sony: support for the ghlive ps4 dongles 2021-07-21 18:44 ` Benjamin Tissoires @ 2021-07-23 19:07 ` Daniel Nguyen 0 siblings, 0 replies; 8+ messages in thread From: Daniel Nguyen @ 2021-07-23 19:07 UTC (permalink / raw) To: Benjamin Tissoires Cc: Pascal Giard, Colenbrander, Roelof, Jiri Kosina, open list:HID CORE LAYER Good day Benjamin, On Wed, Jul 21, 2021 at 2:44 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Wed, Jul 21, 2021 at 8:15 PM Daniel Nguyen > <daniel.nguyen.1@ens.etsmtl.ca> wrote: > > > > Hello Benjamin, > > > > On Wed, Jul 21, 2021 at 6:16 AM Benjamin Tissoires > > <benjamin.tissoires@redhat.com> wrote: > > > > > > On Tue, Jul 20, 2021 at 4:33 PM Pascal Giard <pascal.giard@etsmtl.ca> wrote: > > > > > > > > Hi Benjamin, > > > > > > > > On Tue, Jul 20, 2021 at 7:39 AM Benjamin Tissoires > > > > <benjamin.tissoires@redhat.com> wrote: > > > > > > > > > > Hi Daniel (and Pascal), > > > > > > > > > > [adding Roderick in Cc who is dealing with the Sony driver from Sony > > > > > itself :) ] > > > > > > > > > > > > > > > On Thu, Jul 15, 2021 at 9:58 PM Daniel Nguyen <daniel.nguyen.1@ens.etsmtl.ca> wrote: > > > > > > > > > > > > This commit adds support for the Guitar Hero Live PS4 dongles. > > > > > > > > > > I was about to ask you to add some regression tests to > > > > > https://gitlab.freedesktop.org/libevdev/hid-tools/-/blob/master/tests/test_sony.py > > > > > > > > > > This would likely have avoided the previous patch that was required and > > > > > cc-ed to stable. > > > > > > > > > > But after looking slightly at the patch, I realized that the Guitar Hero > > > > > code uses direct USB calls, which is not something we can emulate at the > > > > > hid-tools level. > > > > > > > > > > However, after a second look at the code, I think that this part of the > > > > > code just reimplements its own HID SET_OUTPUT code, and that is > > > > > something we can easily emulate. > > > > > > > > > > Could you check if the following patch is still working properly on a > > > > > PS3 dongle? and if so, add your PS4 support on top? > > > > > > > > > [...] > > > > > > > > I wasn't aware that this was possible. Of course we will check whether > > > > that works or not. > > > > > > > > > --- > > > > > commit 10e14f105553d2bd88bc7748e87154c5a8131e9e > > > > > Author: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > > Date: Tue Jul 20 11:44:10 2021 +0200 > > > > > > > > > > HID: sony: GHL: do not use raw USB calls > > > > > > > > > > We can rely on HID to do the job for us. > > > > > This simplifies the code and also allows to implement regression tests. > > > > > > > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > > > > > > > diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c > > > > > index b3722c51ec78..901f61d286e8 100644 > > > > > --- a/drivers/hid/hid-sony.c > > > > > +++ b/drivers/hid/hid-sony.c > > > > > @@ -37,7 +37,6 @@ > > > > > #include <linux/idr.h> > > > > > #include <linux/input/mt.h> > > > > > #include <linux/crc32.h> > > > > > -#include <linux/usb.h> > > > > > #include <linux/timer.h> > > > > > #include <asm/unaligned.h> > > > > > > > > > > @@ -92,7 +91,7 @@ > > > > > * https://github.com/ghlre/GHLtarUtility/blob/master/PS3Guitar.cs > > > > > * Note: The Wii U and PS3 dongles happen to share the same! > > > > > */ > > > > > -static const u16 ghl_ps3wiiu_magic_value = 0x201; > > > > > +static const u16 ghl_ps3wiiu_magic_report = 1; > > > > > static const char ghl_ps3wiiu_magic_data[] = { > > > > > 0x02, 0x08, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00 > > > > > }; > > > > > @@ -597,7 +596,6 @@ struct sony_sc { > > > > > /* DS4 calibration data */ > > > > > struct ds4_calibration_data ds4_calib_data[6]; > > > > > /* GH Live */ > > > > > - struct urb *ghl_urb; > > > > > struct timer_list ghl_poke_timer; > > > > > }; > > > > > > > > > > @@ -622,56 +620,29 @@ static inline void sony_schedule_work(struct sony_sc *sc, > > > > > } > > > > > } > > > > > > > > > > -static void ghl_magic_poke_cb(struct urb *urb) > > > > > -{ > > > > > - struct sony_sc *sc = urb->context; > > > > > - > > > > > - if (urb->status < 0) > > > > > - hid_err(sc->hdev, "URB transfer failed : %d", urb->status); > > > > > - > > > > > - mod_timer(&sc->ghl_poke_timer, jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); > > > > > -} > > > > > - > > > > > static void ghl_magic_poke(struct timer_list *t) > > > > > { > > > > > int ret; > > > > > struct sony_sc *sc = from_timer(sc, t, ghl_poke_timer); > > > > > + const int buf_size = ARRAY_SIZE(ghl_ps3wiiu_magic_data); > > > > > + u8 *buf; > > > > > > > > > > - ret = usb_submit_urb(sc->ghl_urb, GFP_ATOMIC); > > > > > - if (ret < 0) > > > > > - hid_err(sc->hdev, "usb_submit_urb failed: %d", ret); > > > > > -} > > > > > - > > > > > -static int ghl_init_urb(struct sony_sc *sc, struct usb_device *usbdev) > > > > > -{ > > > > > - struct usb_ctrlrequest *cr; > > > > > - u16 poke_size; > > > > > - u8 *databuf; > > > > > - unsigned int pipe; > > > > > - > > > > > - poke_size = ARRAY_SIZE(ghl_ps3wiiu_magic_data); > > > > > - pipe = usb_sndctrlpipe(usbdev, 0); > > > > > + buf = kmemdup(ghl_ps3wiiu_magic_data, buf_size, GFP_KERNEL); > > > > > + if (!buf) > > > > > + return; > > > > > > > > > > - cr = devm_kzalloc(&sc->hdev->dev, sizeof(*cr), GFP_ATOMIC); > > > > > - if (cr == NULL) > > > > > - return -ENOMEM; > > > > > + ret = hid_hw_raw_request(sc->hdev, ghl_ps3wiiu_magic_report, buf, > > > > > + buf_size, > > > > > + HID_OUTPUT_REPORT, HID_REQ_SET_REPORT); > > > > > + if (ret < 0) { > > > > > + hid_err(sc->hdev, "can't poke ghl magic\n"); > > > > > + goto out; > > > > > + } > > > > > > > > > > - databuf = devm_kzalloc(&sc->hdev->dev, poke_size, GFP_ATOMIC); > > > > > - if (databuf == NULL) > > > > > - return -ENOMEM; > > > > > + mod_timer(&sc->ghl_poke_timer, jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); > > > > > > > > > > - cr->bRequestType = > > > > > - USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_OUT; > > > > > - cr->bRequest = USB_REQ_SET_CONFIGURATION; > > > > > - cr->wValue = cpu_to_le16(ghl_ps3wiiu_magic_value); > > > > > - cr->wIndex = 0; > > > > > - cr->wLength = cpu_to_le16(poke_size); > > > > > - memcpy(databuf, ghl_ps3wiiu_magic_data, poke_size); > > > > > - usb_fill_control_urb( > > > > > - sc->ghl_urb, usbdev, pipe, > > > > > - (unsigned char *) cr, databuf, poke_size, > > > > > - ghl_magic_poke_cb, sc); > > > > > - return 0; > > > > > +out: > > > > > + kfree(buf); > > > > > } > > > > > > > > > > static int guitar_mapping(struct hid_device *hdev, struct hid_input *hi, > > > > > @@ -2968,7 +2939,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) > > > > > int ret; > > > > > unsigned long quirks = id->driver_data; > > > > > struct sony_sc *sc; > > > > > - struct usb_device *usbdev; > > > > > unsigned int connect_mask = HID_CONNECT_DEFAULT; > > > > > > > > > > if (!strcmp(hdev->name, "FutureMax Dance Mat")) > > > > > @@ -2988,7 +2958,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) > > > > > sc->quirks = quirks; > > > > > hid_set_drvdata(hdev, sc); > > > > > sc->hdev = hdev; > > > > > - usbdev = to_usb_device(sc->hdev->dev.parent->parent); > > > > > > > > > > ret = hid_parse(hdev); > > > > > if (ret) { > > > > > @@ -3031,15 +3000,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) > > > > > } > > > > > > > > > > if (sc->quirks & GHL_GUITAR_PS3WIIU) { > > > > > - sc->ghl_urb = usb_alloc_urb(0, GFP_ATOMIC); > > > > > - if (!sc->ghl_urb) > > > > > - return -ENOMEM; > > > > > - ret = ghl_init_urb(sc, usbdev); > > > > > - if (ret) { > > > > > - hid_err(hdev, "error preparing URB\n"); > > > > > - return ret; > > > > > - } > > > > > - > > > > > timer_setup(&sc->ghl_poke_timer, ghl_magic_poke, 0); > > > > > mod_timer(&sc->ghl_poke_timer, > > > > > jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); > > > > > @@ -3054,7 +3014,6 @@ static void sony_remove(struct hid_device *hdev) > > > > > > > > > > if (sc->quirks & GHL_GUITAR_PS3WIIU) { > > > > > del_timer_sync(&sc->ghl_poke_timer); > > > > > - usb_free_urb(sc->ghl_urb); > > > > > } > > > > > > > > > > hid_hw_close(hdev); > > > > > --- > > > > > > > > Was your patch against the master branch of hid/hid.git ? > > > > > > It was against the branch for-next of hid/hid.git, to account for the > > > PS3 fix that was sent earlier. > > > > > > > I'm asking because it doesn't apply at all on my end, unless I use > > > > --ignore-whitespace in which case, 3 out of 8 hunks fail. > > > > > > > > I assume I may be doing something wrong. I tried both downloading the > > > > raw message from marc.info and from patchwork in case gmail would > > > > mangle spaces/tabs, same result. > > > > > > > > Any idea? > > > > > > Usually opening the source of the email, and doing `git am -3`, hit > > > enter, then paste the content of the patch, then Ctrl-D is the > > > quickest you can do to apply such inlined patches (at least, that's > > > what I do). > > > > > > But Daniel found out that the patch is buggy, so let's concentrate on > > > the next iteration. > > > > I dove deeper into the proposed method and verified the functions in > > order to better understand how they work. I noticed that the const that you > > created (ghl_ps3wiiu_magic_report = 1) is modifying the first byte of the data > > to 0x01. I also noticed that it is what creates the wValue = 0x201, which > > explains why you set it to 1. > > > > After testing, I noticed that if we maintain the wValue = 0x201, and therefore > > byte[0] = 0x01, the dongle does not function as it should. However, if we > > change the const (ghl_ps3wiiu_magic_report = 2), although we have > > wValue = 0x202, the dongle does function as expected. That's surprising > > to us. Do you have any insights into the role of wValue in this context? > > We came up with 0x201 as we were simply reproducing what > > we had sniffed. > > The answer is in usbhid_set_raw_report() > (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/usbhid/hid-core.c#n907) > > -> the wValue is used as `((rtype + 1) << 8) | reportnum` > > where rtype is the report type (declared in include/linux/hid.h) > #define HID_INPUT_REPORT 0 > #define HID_OUTPUT_REPORT 1 > #define HID_FEATURE_REPORT 2 > > And reportnum is the report number (the one matching the report ID if > you look at the report descriptors. > > I am as much as puzzled as you regarding the fact that writing the urb > directly works with '1' when it doesn't with the HID stack... > > Actually, the HID stack would overwrite the first byte with the report > ID, which explains why the report gets changed. > > However, why was the initial call setting a HID report ID of 1 but > then addressing the report 2??? That's probably a mystery we won't > solve :) > > > > > Despite the error message related to scheduling, I have not been able to > > pinpoint the source of the hard freeze. Any hint there would be appreciated. > > I would have to think about this a little bit more, but I have been > dragged all day with other internal work :( > > > > > Lastly, I wanted to point out that for the PS4 dongle support, the > > magic data is > > not sent via control transfers, but through interrupt transfers. Does the HID > > infrastructure have the necessary mechanisms to do that as well or will we > > have to resort to the direct USB calls (like currently done in the patch > > I submitted)? > > We have usbhid_output_report in this file, which can be genetically > called via hid_hw_output_report() > > Also, IIRC it happens that sometimes a device would accept both > interrupt or control transfers for these kind of methods. It might be > interesting to see if you can not have a common path between the 2. > Following your insights, I tested whether the PS4 support could be performed as a control transfer instead of interrupt. It can! However, the inverse does not, i.e., I tried using interrupt transfers instead of control transfers for the PS3 dongle and it didn't work. I found this very interesting. Thank you for mentioning it. Besides, I looked into hid_hw_raw_request() and noted that it uses usb_control_msg(). The documentation mentions the following about usb_control_msg(): "Don't use this function from within an interrupt context. If you need an asynchronous message, or need to send a message from within interrupt context, use usb_submit_urb()." Looking at how hid_hw_raw_request() is often used, it is performed during probe(), before HID reports begin polling. In our case, we are attempting to schedule a control transfer while the device is constantly polling for HID reports. Therefore, if I understand correctly, the likelihood of using usb_submit_urb() (via hid_hw_raw_request()) within interrupt context is high. Thus, we should not use hid_hw_raw_request(). Does this sound plausible? I tried to find a function from within HID that uses usb_submit_urb() to send a control transfer, but failed. Any insight? Lastly, based on the experiments mentioned above, I could change the PS4-dongle support to use control transfers instead of interrupt transfers, therefore sharing more codepaths with the PS3-dongle support. Is that something that would be appreciated? Thank you, -Daniel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-07-23 19:07 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-15 19:57 [PATCH] HID: sony: support for the ghlive ps4 dongles Daniel Nguyen 2021-07-20 11:36 ` Benjamin Tissoires 2021-07-20 14:33 ` Pascal Giard 2021-07-20 22:42 ` Nguyen, Daniel 2021-07-21 10:16 ` Benjamin Tissoires 2021-07-21 18:15 ` Daniel Nguyen 2021-07-21 18:44 ` Benjamin Tissoires 2021-07-23 19:07 ` Daniel Nguyen
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.