Hello Benjamin, On Wed, Jul 21, 2021 at 6:16 AM Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> wrote: > > On Tue, Jul 20, 2021 at 4:33 PM Pascal Giard <pascal.giard@xxxxxxxxx> wrote: > > > > Hi Benjamin, > > > > On Tue, Jul 20, 2021 at 7:39 AM Benjamin Tissoires > > <benjamin.tissoires@xxxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxx> > > > 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@xxxxxxxxxx> > > > > > > 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
