| Message ID | 20201009022722.123943-1-pascal.giard@etsmtl.ca |
|---|---|
| State | New |
| Headers | show |
| Series | HID: ghlive: support for ghlive ps3/wii u dongles | expand |
On Thu, 8 Oct 2020, Pascal Giard wrote: > This commit introduces the Guitar Hero Live driver which adds support > for the PS3 and Wii U dongles. Pascal, thanks for the patch. [ ... snip ... ] > --- > drivers/hid/Kconfig | 6 ++ > drivers/hid/Makefile | 1 + > drivers/hid/hid-ghlive.c | 220 +++++++++++++++++++++++++++++++++++++++ Would it make more sense (with respect to how we are structuring/naming the hid drivers) to incorporate this into hid-sony (irrespective of currently ongoing discussions about actually splitting that driver :) )? > +static void ghl_magic_poke(struct timer_list *t) > +{ > + struct ghlive_sc *sc = from_timer(sc, t, poke_timer); > + > + int ret; > + unsigned int pipe; > + struct urb *urb; > + struct usb_ctrlrequest *cr; > + const u16 poke_size = > + ARRAY_SIZE(ghl_ps3wiiu_magic_data); > + u8 *databuf; > + > + pipe = usb_sndctrlpipe(sc->usbdev, 0); > + > + cr = kzalloc(sizeof(*cr), GFP_ATOMIC); > + if (!cr) > + goto resched; > + > + databuf = kzalloc(poke_size, GFP_ATOMIC); > + if (!databuf) { > + kfree(cr); > + goto resched; > + } > + > + urb = usb_alloc_urb(0, GFP_ATOMIC); > + if (!urb) { > + kfree(databuf); > + kfree(cr); > + goto resched; So if one of the allocations above succeeds and a subsequent one fails, you're going to try re-allocate all of them next time again, leaking the ones that previously succeeded, right? Thanks,
On Thu, Oct 29, 2020 at 11:26 AM Jiri Kosina <jikos@kernel.org> wrote: > > On Thu, 8 Oct 2020, Pascal Giard wrote: > > > This commit introduces the Guitar Hero Live driver which adds support > > for the PS3 and Wii U dongles. > > Pascal, > > thanks for the patch. > Dear Jiri, thank you for reviewing my patch. > [ ... snip ... ] > > > --- > > drivers/hid/Kconfig | 6 ++ > > drivers/hid/Makefile | 1 + > > drivers/hid/hid-ghlive.c | 220 +++++++++++++++++++++++++++++++++++++++ > > Would it make more sense (with respect to how we are structuring/naming > the hid drivers) to incorporate this into hid-sony (irrespective of > currently ongoing discussions about actually splitting that driver :) )? > I think it would be most appropriate, yes. Note that there are 3 other dongles out there: - the xbox360 dongle does not need any special treatment, it just works with hid-generic; - the ps4 dongle obviously makes sense to go into hid-sony (although no one has reversed engineered that one (yet)); - the xboxone dongle: that's an unknown one to me. I don't have any information about that one unfortunately and do not own one. I wrote this as a separate hid driver as I saw that email [1] from Roderick Colenbrander in which he expressed a preference for a seperate driver in cases where the device is not from Sony proper. > > +static void ghl_magic_poke(struct timer_list *t) > > +{ > > + struct ghlive_sc *sc = from_timer(sc, t, poke_timer); > > + > > + int ret; > > + unsigned int pipe; > > + struct urb *urb; > > + struct usb_ctrlrequest *cr; > > + const u16 poke_size = > > + ARRAY_SIZE(ghl_ps3wiiu_magic_data); > > + u8 *databuf; > > + > > + pipe = usb_sndctrlpipe(sc->usbdev, 0); > > + > > + cr = kzalloc(sizeof(*cr), GFP_ATOMIC); > > + if (!cr) > > + goto resched; > > + > > + databuf = kzalloc(poke_size, GFP_ATOMIC); > > + if (!databuf) { > > + kfree(cr); > > + goto resched; > > + } > > + > > + urb = usb_alloc_urb(0, GFP_ATOMIC); > > + if (!urb) { > > + kfree(databuf); > > + kfree(cr); > > + goto resched; > > > So if one of the allocations above succeeds and a subsequent one fails, > you're going to try re-allocate all of them next time again, leaking the > ones that previously succeeded, right? > I attempted to avoid such a case. IIUC there are 4 possible scenarios tied to those 3 allocs (cr, databuf, and urb): 1) alloc of cr fails. nothing to be freed, we reschedule; 2) alloc of cr succeeds, alloc of databuf fails. we free cr and we reschedule; 3) allocs of cr and databuf succeed, alloc of urb fails. we free cr and databuf, and we reschedule; 4) all allocs succeed, we submit the urb, and free urb. once the control packet is sent, the callback is called and we free cr and databuf. Am I missing something? It's VERY possible that I am as this is my first patch and I wrote this by looking at other peoples' code. The only thing that comes to mind is if poke is called again before the control packet actually gets sent (and the callback called), in which case I'm not sure what would happen. But with a poke interval of 10 seconds, is that probability close enough to 0 to be ignored? Thanks again for your valuable input, :-) -Pascal [1] https://marc.info/?l=linux-input&m=157273970001101&w=2
On Thu, 29 Oct 2020, Pascal Giard wrote: > > > drivers/hid/Kconfig | 6 ++ > > > drivers/hid/Makefile | 1 + > > > drivers/hid/hid-ghlive.c | 220 +++++++++++++++++++++++++++++++++++++++ > > > > Would it make more sense (with respect to how we are structuring/naming > > the hid drivers) to incorporate this into hid-sony (irrespective of > > currently ongoing discussions about actually splitting that driver :) )? > > > > I think it would be most appropriate, yes. > > Note that there are 3 other dongles out there: > - the xbox360 dongle does not need any special treatment, it just > works with hid-generic; > - the ps4 dongle obviously makes sense to go into hid-sony (although > no one has reversed engineered that one (yet)); > - the xboxone dongle: that's an unknown one to me. I don't have any > information about that one unfortunately and do not own one. > > I wrote this as a separate hid driver as I saw that email [1] from > Roderick Colenbrander in which he expressed a preference for a > seperate driver in cases where the device is not from Sony proper. Yeah, but before that happens, I think we should just continue with status quo, meaning putting the support into hid-sony, if noone has any objections. > > So if one of the allocations above succeeds and a subsequent one fails, > > you're going to try re-allocate all of them next time again, leaking the > > ones that previously succeeded, right? > > > > I attempted to avoid such a case. IIUC there are 4 possible scenarios > tied to those 3 allocs (cr, databuf, and urb): > 1) alloc of cr fails. nothing to be freed, we reschedule; > 2) alloc of cr succeeds, alloc of databuf fails. we free cr and we reschedule; > 3) allocs of cr and databuf succeed, alloc of urb fails. we free cr > and databuf, and we reschedule; > 4) all allocs succeed, we submit the urb, and free urb. once the > control packet is sent, the callback is called and we free cr and > databuf. > > Am I missing something? No, I probably didn't have my first coffee of the day when looking at your patch, sorry. Still, the way it tries to allocate the buffers is sub-optimal, as if either (not necessarily the first one) fails, it retries the next iteration with attempting to allocate from scratch. If the system really is in tight memory condition, it'd make more sense to keep the ones for which the allocation succeeded already allocated, and retry only the failed one(s). Thanks,
On Sat, 7 Nov 2020, Pascal Giard wrote: > This commit adds support for the Guitar Hero Live PS3 and Wii U dongles. > > These dongles require a "magic" USB control message [1] to be sent > approximately every 10 seconds otherwise the dongle will not report > events where the strumbar is hit while a fret is being held. > > Also, inspired by a patch sent on linux-input by Sanjay Govind [2], the > accelerometer is mapped to ABS_RY for tilt. > > Interestingly, the Wii U and PS3 dongles share the same VID and PID. > > [1] https://github.com/ghlre/GHLtarUtility/ > [2] https://marc.info/?l=linux-input&m=157242835928542&w=2 > > Signed-off-by: Pascal Giard <pascal.giard@etsmtl.ca> > --- > differences from v1: > * Patches hid-sony instead of creating a new driver > * Changed memory allocation scheme in case of fail > --- > drivers/hid/Kconfig | 1 + > drivers/hid/hid-ids.h | 3 ++ > drivers/hid/hid-sony.c | 115 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 119 insertions(+) > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > index 34f07371716d..e2df2ae112a5 100644 > --- a/drivers/hid/Kconfig > +++ b/drivers/hid/Kconfig > @@ -897,6 +897,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 > > config SONY_FF > bool "Sony PS2/3/4 accessories force feedback support" > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > index 1c71a1aa76b2..e3a3942079cf 100644 > --- a/drivers/hid/hid-ids.h > +++ b/drivers/hid/hid-ids.h > @@ -1060,6 +1060,9 @@ > #define USB_DEVICE_ID_SONY_BUZZ_CONTROLLER 0x0002 > #define USB_DEVICE_ID_SONY_WIRELESS_BUZZ_CONTROLLER 0x1000 > > +#define USB_VENDOR_ID_SONY_GHLIVE 0x12ba > +#define USB_DEVICE_ID_SONY_PS3WIIU_GHLIVE_DONGLE 0x074b > + > #define USB_VENDOR_ID_SINO_LITE 0x1345 > #define USB_DEVICE_ID_SINO_LITE_CONTROLLER 0x3008 > > diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c > index 4c6ed6ef31f1..700bea6239f6 100644 > --- a/drivers/hid/hid-sony.c > +++ b/drivers/hid/hid-sony.c > @@ -11,6 +11,7 @@ > * 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> > */ > > /* > @@ -35,6 +36,8 @@ > #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> > > #include "hid-ids.h" > @@ -56,6 +59,8 @@ > #define NSG_MR5U_REMOTE_BT BIT(14) > #define NSG_MR7U_REMOTE_BT BIT(15) > #define SHANWAN_GAMEPAD BIT(16) > +#define GHL_GUITAR_PS3WIIU BIT(17) > +#define GHL_GUITAR_CONTROLLER BIT(18) Hi Pascal, thanks for fixing the previous version. This one looks good to me, I just have one remaining question -- why do we need both quirks here? Given the particular VID/PID gets both of them set anyway (and only that VID/PID), and the code is shared, what is the point of consuming the extra bit? Thanks, -- Jiri Kosina SUSE Labs
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index 34f07371716d..0bf8dd1629f7 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -362,6 +362,12 @@ config HID_GFRM ---help--- Support for Google Fiber TV Box remote controls +config HID_GHLIVE + tristate "Guitar Hero Live PS3/Wii U support" + depends on HID + help + Support for the Guitar Hero Live PS3 and Wii U guitar devices. + config HID_GLORIOUS tristate "Glorious PC Gaming Race mice" depends on HID diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile index d8ea4b8c95af..6394f5bbf8a5 100644 --- a/drivers/hid/Makefile +++ b/drivers/hid/Makefile @@ -48,6 +48,7 @@ obj-$(CONFIG_HID_ELO) += hid-elo.o obj-$(CONFIG_HID_EZKEY) += hid-ezkey.o obj-$(CONFIG_HID_GEMBIRD) += hid-gembird.o obj-$(CONFIG_HID_GFRM) += hid-gfrm.o +obj-$(CONFIG_HID_GHLIVE) += hid-ghlive.o obj-$(CONFIG_HID_GLORIOUS) += hid-glorious.o obj-$(CONFIG_HID_GOOGLE_HAMMER) += hid-google-hammer.o obj-$(CONFIG_HID_GT683R) += hid-gt683r.o diff --git a/drivers/hid/hid-ghlive.c b/drivers/hid/hid-ghlive.c new file mode 100644 index 000000000000..db5814aff17f --- /dev/null +++ b/drivers/hid/hid-ghlive.c @@ -0,0 +1,220 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * HID driver for Guitar Hero Live PS3 and Wii U Guitar devices. + * + * Copyright (c) 2020 Pascal Giard <pascal.giard@etsmtl.ca> + */ + +#include <linux/hid.h> +#include <linux/usb.h> +#include <linux/timer.h> +#include <linux/module.h> + +#include "hid-ids.h" + +MODULE_AUTHOR("Pascal Giard <pascal.giard@etsmtl.ca>"); +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("HID driver for Activision GH Live PS3 and Wii U Guitar devices"); + +#define GHL_GUITAR_PS3WIIU BIT(2) +#define GHL_GUITAR_CONTROLLER BIT(1) + +#define GHL_GUITAR_POKE_INTERVAL 10 /* In seconds */ + +#define GHL_GUITAR_TILT_USAGE 44 + +/* Magic value and data taken from GHLtarUtility: + * 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 char ghl_ps3wiiu_magic_data[] = { + 0x02, 0x08, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00 +}; + +struct ghlive_sc { + struct hid_device *hdev; + struct usb_device *usbdev; + unsigned long quirks; + int device_id; + unsigned int poke_current; + struct timer_list poke_timer; +}; + +static void ghl_magic_poke_cb(struct urb *urb) +{ + if (urb) { + /* Free cr and databuf allocated in ghl_magic_poke() */ + kfree(urb->setup_packet); + kfree(urb->transfer_buffer); + } +} + +static void ghl_magic_poke(struct timer_list *t) +{ + struct ghlive_sc *sc = from_timer(sc, t, poke_timer); + + int ret; + unsigned int pipe; + struct urb *urb; + struct usb_ctrlrequest *cr; + const u16 poke_size = + ARRAY_SIZE(ghl_ps3wiiu_magic_data); + u8 *databuf; + + pipe = usb_sndctrlpipe(sc->usbdev, 0); + + cr = kzalloc(sizeof(*cr), GFP_ATOMIC); + if (!cr) + goto resched; + + databuf = kzalloc(poke_size, GFP_ATOMIC); + if (!databuf) { + kfree(cr); + goto resched; + } + + urb = usb_alloc_urb(0, GFP_ATOMIC); + if (!urb) { + kfree(databuf); + kfree(cr); + goto resched; + } + + if (sc->quirks & (GHL_GUITAR_CONTROLLER | GHL_GUITAR_PS3WIIU)) { + 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( + urb, sc->usbdev, pipe, + (unsigned char *) cr, databuf, poke_size, + ghl_magic_poke_cb, NULL); + ret = usb_submit_urb(urb, GFP_ATOMIC); + if (ret < 0) { + kfree(databuf); + kfree(cr); + } + } + usb_free_urb(urb); + +resched: + /* Reschedule for next time */ + mod_timer(&sc->poke_timer, jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); +} + +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) +{ + if ((usage->hid & HID_USAGE_PAGE) == HID_UP_MSVENDOR) { + unsigned int abs = usage->hid & HID_USAGE; + + if (abs == GHL_GUITAR_TILT_USAGE) { + hid_map_usage_clear(hi, usage, bit, max, EV_ABS, ABS_RY); + return 1; + } + } + return 0; +} + +static int ghlive_mapping(struct hid_device *hdev, struct hid_input *hi, + struct hid_field *field, struct hid_usage *usage, + unsigned long **bit, int *max) +{ + struct ghlive_sc *sc = hid_get_drvdata(hdev); + + if (sc->quirks & GHL_GUITAR_CONTROLLER) + return guitar_mapping(hdev, hi, field, usage, bit, max); + + /* Let hid-core decide for the others */ + return 0; +} + +static int ghlive_probe(struct hid_device *hdev, + const struct hid_device_id *id) +{ + int ret; + unsigned long quirks = id->driver_data; + struct ghlive_sc *sc; + unsigned int connect_mask = HID_CONNECT_DEFAULT; + + sc = devm_kzalloc(&hdev->dev, sizeof(*sc), GFP_KERNEL); + if (sc == NULL) + return -ENOMEM; + + sc->quirks = quirks; + hid_set_drvdata(hdev, sc); + sc->hdev = hdev; + + ret = hid_parse(hdev); + if (ret) { + hid_err(hdev, "parse failed\n"); + return ret; + } + + ret = hid_hw_start(hdev, connect_mask); + if (ret) { + hid_err(hdev, "hw start failed\n"); + return ret; + } + + if (!(hdev->claimed & HID_CLAIMED_INPUT)) { + hid_err(hdev, "failed to claim input\n"); + hid_hw_stop(hdev); + return -ENODEV; + } + + if (sc->quirks & GHL_GUITAR_CONTROLLER) { + sc->usbdev = to_usb_device(hdev->dev.parent->parent); + sc->poke_current = 0; + timer_setup(&sc->poke_timer, ghl_magic_poke, 0); + mod_timer(&sc->poke_timer, + jiffies + GHL_GUITAR_POKE_INTERVAL*HZ); + } + + return ret; +} + +static void ghlive_remove(struct hid_device *hdev) +{ + struct ghlive_sc *sc = hid_get_drvdata(hdev); + + del_timer_sync(&sc->poke_timer); + hid_hw_close(hdev); + hid_hw_stop(hdev); +} + +static const struct hid_device_id ghlive_devices[] = { + { HID_USB_DEVICE(USB_VENDOR_ID_SONY_GHLIVE, USB_DEVICE_ID_SONY_PS3WIIU_GHLIVE_DONGLE), + .driver_data = GHL_GUITAR_CONTROLLER | GHL_GUITAR_PS3WIIU}, + { } +}; +MODULE_DEVICE_TABLE(hid, ghlive_devices); + +static struct hid_driver ghlive_driver = { + .name = "ghlive", + .id_table = ghlive_devices, + .input_mapping = ghlive_mapping, + .probe = ghlive_probe, + .remove = ghlive_remove, +}; + +static int __init ghlive_init(void) +{ + dbg_hid("GHLive:%s\n", __func__); + return hid_register_driver(&ghlive_driver); +} + +static void __exit ghlive_exit(void) +{ + dbg_hid("GHLive:%s\n", __func__); + + hid_unregister_driver(&ghlive_driver); +} +module_init(ghlive_init); +module_exit(ghlive_exit); diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index 1c71a1aa76b2..e3a3942079cf 100644 --- a/drivers/hid/hid-ids.h +++ b/drivers/hid/hid-ids.h @@ -1060,6 +1060,9 @@ #define USB_DEVICE_ID_SONY_BUZZ_CONTROLLER 0x0002 #define USB_DEVICE_ID_SONY_WIRELESS_BUZZ_CONTROLLER 0x1000 +#define USB_VENDOR_ID_SONY_GHLIVE 0x12ba +#define USB_DEVICE_ID_SONY_PS3WIIU_GHLIVE_DONGLE 0x074b + #define USB_VENDOR_ID_SINO_LITE 0x1345 #define USB_DEVICE_ID_SINO_LITE_CONTROLLER 0x3008
This commit introduces the Guitar Hero Live driver which adds support for the PS3 and Wii U dongles. These dongles require a "magic" USB control message [1] to be sent approximately every 10 seconds otherwise the dongle will not report events where the strumbar is hit while a fret is being held. Also, inspired by a patch sent on linux-input by Sanjay Govind [2], the accelerometer is mapped to ABS_RY for tilt. Interestingly, the Wii U and PS3 dongles share the same VID and PID. [1] https://github.com/ghlre/GHLtarUtility/ [2] https://marc.info/?l=linux-input&m=157242835928542&w=2 Signed-off-by: Pascal Giard <pascal.giard@etsmtl.ca> --- drivers/hid/Kconfig | 6 ++ drivers/hid/Makefile | 1 + drivers/hid/hid-ghlive.c | 220 +++++++++++++++++++++++++++++++++++++++ drivers/hid/hid-ids.h | 3 + 4 files changed, 230 insertions(+) create mode 100644 drivers/hid/hid-ghlive.c