[PATCH] HID: sony: fix freeze when inserting ghlive ps3/wii dongles

[PATCH] HID: sony: fix freeze when inserting ghlive ps3/wii dongles

[Pascal Giard]

This commit fixes a freeze on insertion of a Guitar Hero Live PS3/WiiU
USB dongle. Indeed, with the current implementation, inserting one of
those USB dongles will lead to a hard freeze. I apologize for not
catching this earlier, it didn't occur on my old laptop.

While the issue was isolated to memory alloc/free, I could not figure
out why it causes a freeze. So this patch fixes this issue by
simplifying memory allocation and usage.

We remind that for the dongle to work properly, a control URB needs to
be sent periodically. We used to alloc/free the URB each time this URB
needed to be sent.

With this patch, the memory for the URB is allocated on the probe, reused
for as long as the dongle is plugged in, and freed once the dongle is
unplugged.

Signed-off-by: Pascal Giard <pascal.giard@etsmtl.ca>
---
 drivers/hid/hid-sony.c | 98 +++++++++++++++++++++---------------------
 1 file changed, 49 insertions(+), 49 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 8319b0ce385a..b3722c51ec78 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -597,9 +597,8 @@ 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;
-	struct usb_ctrlrequest *ghl_cr;
-	u8 *ghl_databuf;
 };
 
 static void sony_set_leds(struct sony_sc *sc);
@@ -625,66 +624,54 @@ static inline void sony_schedule_work(struct sony_sc *sc,
 
 static void ghl_magic_poke_cb(struct urb *urb)
 {
-	if (urb) {
-		/* Free sc->ghl_cr and sc->ghl_databuf allocated in
-		 * ghl_magic_poke()
-		 */
-		kfree(urb->setup_packet);
-		kfree(urb->transfer_buffer);
-	}
+	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);
 
-	int ret;
+	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;
-	struct urb *urb;
-	struct usb_device *usbdev = to_usb_device(sc->hdev->dev.parent->parent);
-	const u16 poke_size =
-		ARRAY_SIZE(ghl_ps3wiiu_magic_data);
 
+	poke_size = ARRAY_SIZE(ghl_ps3wiiu_magic_data);
 	pipe = usb_sndctrlpipe(usbdev, 0);
 
-	if (!sc->ghl_cr) {
-		sc->ghl_cr = kzalloc(sizeof(*sc->ghl_cr), GFP_ATOMIC);
-		if (!sc->ghl_cr)
-			goto resched;
-	}
-
-	if (!sc->ghl_databuf) {
-		sc->ghl_databuf = kzalloc(poke_size, GFP_ATOMIC);
-		if (!sc->ghl_databuf)
-			goto resched;
-	}
+	cr = devm_kzalloc(&sc->hdev->dev, sizeof(*cr), GFP_ATOMIC);
+	if (cr == NULL)
+		return -ENOMEM;
 
-	urb = usb_alloc_urb(0, GFP_ATOMIC);
-	if (!urb)
-		goto resched;
+	databuf = devm_kzalloc(&sc->hdev->dev, poke_size, GFP_ATOMIC);
+	if (databuf == NULL)
+		return -ENOMEM;
 
-	sc->ghl_cr->bRequestType =
+	cr->bRequestType =
 		USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_OUT;
-	sc->ghl_cr->bRequest = USB_REQ_SET_CONFIGURATION;
-	sc->ghl_cr->wValue = cpu_to_le16(ghl_ps3wiiu_magic_value);
-	sc->ghl_cr->wIndex = 0;
-	sc->ghl_cr->wLength = cpu_to_le16(poke_size);
-	memcpy(sc->ghl_databuf, ghl_ps3wiiu_magic_data, poke_size);
-
+	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, usbdev, pipe,
-		(unsigned char *) sc->ghl_cr, sc->ghl_databuf,
-		poke_size, ghl_magic_poke_cb, NULL);
-	ret = usb_submit_urb(urb, GFP_ATOMIC);
-	if (ret < 0) {
-		kfree(sc->ghl_databuf);
-		kfree(sc->ghl_cr);
-	}
-	usb_free_urb(urb);
-
-resched:
-	/* Reschedule for next time */
-	mod_timer(&sc->ghl_poke_timer, jiffies + GHL_GUITAR_POKE_INTERVAL*HZ);
+		sc->ghl_urb, usbdev, pipe,
+		(unsigned char *) cr, databuf, poke_size,
+		ghl_magic_poke_cb, sc);
+	return 0;
 }
 
 static int guitar_mapping(struct hid_device *hdev, struct hid_input *hi,
@@ -2981,6 +2968,7 @@ 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"))
@@ -3000,6 +2988,7 @@ 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) {
@@ -3042,6 +3031,15 @@ 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,8 +3052,10 @@ 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) {
 		del_timer_sync(&sc->ghl_poke_timer);
+		usb_free_urb(sc->ghl_urb);
+	}
 
 	hid_hw_close(hdev);
 
-- 
2.32.0.rc2

Re: [PATCH] HID: sony: fix freeze when inserting ghlive ps3/wii dongles

[Greg KH]

On Fri, Jun 04, 2021 at 12:10:23PM -0400, Pascal Giard wrote:
> This commit fixes a freeze on insertion of a Guitar Hero Live PS3/WiiU
> USB dongle. Indeed, with the current implementation, inserting one of
> those USB dongles will lead to a hard freeze. I apologize for not
> catching this earlier, it didn't occur on my old laptop.
> 
> While the issue was isolated to memory alloc/free, I could not figure
> out why it causes a freeze. So this patch fixes this issue by
> simplifying memory allocation and usage.
> 
> We remind that for the dongle to work properly, a control URB needs to
> be sent periodically. We used to alloc/free the URB each time this URB
> needed to be sent.
> 
> With this patch, the memory for the URB is allocated on the probe, reused
> for as long as the dongle is plugged in, and freed once the dongle is
> unplugged.
> 
> Signed-off-by: Pascal Giard <pascal.giard@etsmtl.ca>
> ---
>  drivers/hid/hid-sony.c | 98 +++++++++++++++++++++---------------------
>  1 file changed, 49 insertions(+), 49 deletions(-)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

Re: [PATCH] HID: sony: fix freeze when inserting ghlive ps3/wii dongles

[Pascal Giard]

On Sat, Jun 5, 2021 at 2:44 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Jun 04, 2021 at 12:10:23PM -0400, Pascal Giard wrote:
> > This commit fixes a freeze on insertion of a Guitar Hero Live PS3/WiiU
> > USB dongle. Indeed, with the current implementation, inserting one of
> > those USB dongles will lead to a hard freeze. I apologize for not
> > catching this earlier, it didn't occur on my old laptop.
> >
> > While the issue was isolated to memory alloc/free, I could not figure
> > out why it causes a freeze. So this patch fixes this issue by
> > simplifying memory allocation and usage.
> >
> > We remind that for the dongle to work properly, a control URB needs to
> > be sent periodically. We used to alloc/free the URB each time this URB
> > needed to be sent.
> >
> > With this patch, the memory for the URB is allocated on the probe, reused
> > for as long as the dongle is plugged in, and freed once the dongle is
> > unplugged.
> >
> > Signed-off-by: Pascal Giard <pascal.giard@etsmtl.ca>
> > ---
> >  drivers/hid/hid-sony.c | 98 +++++++++++++++++++++---------------------
> >  1 file changed, 49 insertions(+), 49 deletions(-)
>
> <formletter>
>
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree.  Please read:
>     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
>
> </formletter>

Dear Greg,

I apologize for failing to follow the procedure. I had already read
these guidelines, and I actually thought I was following Option 1 :-/

I thought that I had to get my patch merged into next first (patch
against dtor's git) and that by adding stable@ as CC, it would
automatically get considered for inclusion into stable once merged
into Linus' tree. Based on your email, I got that wrong...

So I sent my patch to the right place BUT my patch should be against
this [1] tree instead?

Thank you for your patience,

-Pascal
[1] git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

Re: [PATCH] HID: sony: fix freeze when inserting ghlive ps3/wii dongles

[Greg KH]

On Wed, Jun 09, 2021 at 08:25:47PM -0400, Pascal Giard wrote:
> On Sat, Jun 5, 2021 at 2:44 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, Jun 04, 2021 at 12:10:23PM -0400, Pascal Giard wrote:
> > > This commit fixes a freeze on insertion of a Guitar Hero Live PS3/WiiU
> > > USB dongle. Indeed, with the current implementation, inserting one of
> > > those USB dongles will lead to a hard freeze. I apologize for not
> > > catching this earlier, it didn't occur on my old laptop.
> > >
> > > While the issue was isolated to memory alloc/free, I could not figure
> > > out why it causes a freeze. So this patch fixes this issue by
> > > simplifying memory allocation and usage.
> > >
> > > We remind that for the dongle to work properly, a control URB needs to
> > > be sent periodically. We used to alloc/free the URB each time this URB
> > > needed to be sent.
> > >
> > > With this patch, the memory for the URB is allocated on the probe, reused
> > > for as long as the dongle is plugged in, and freed once the dongle is
> > > unplugged.
> > >
> > > Signed-off-by: Pascal Giard <pascal.giard@etsmtl.ca>
> > > ---
> > >  drivers/hid/hid-sony.c | 98 +++++++++++++++++++++---------------------
> > >  1 file changed, 49 insertions(+), 49 deletions(-)
> >
> > <formletter>
> >
> > This is not the correct way to submit patches for inclusion in the
> > stable kernel tree.  Please read:
> >     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > for how to do this properly.
> >
> > </formletter>
> 
> Dear Greg,
> 
> I apologize for failing to follow the procedure. I had already read
> these guidelines, and I actually thought I was following Option 1 :-/

Is this commit already in Linus's tree?  If so then we just need a git
commit id and we can queue it up.

> I thought that I had to get my patch merged into next first (patch
> against dtor's git) and that by adding stable@ as CC, it would
> automatically get considered for inclusion into stable once merged
> into Linus' tree. Based on your email, I got that wrong...

It will, but you need to add that to the signed-off-by: area, as the
document says.

thanks,

greg k-h

Re: [PATCH] HID: sony: fix freeze when inserting ghlive ps3/wii dongles

[Pascal Giard]

On Thu, Jun 10, 2021 at 1:25 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jun 09, 2021 at 08:25:47PM -0400, Pascal Giard wrote:
>
> > I apologize for failing to follow the procedure. I had already read
> > these guidelines, and I actually thought I was following Option 1 :-/
>
> Is this commit already in Linus's tree?  If so then we just need a git
> commit id and we can queue it up.

No, it isn't yet. My patch has not been reviewed yet.

> > I thought that I had to get my patch merged into next first (patch
> > against dtor's git) and that by adding stable@ as CC, it would
> > automatically get considered for inclusion into stable once merged
> > into Linus' tree. Based on your email, I got that wrong...
>
> It will, but you need to add that to the signed-off-by: area, as the
> document says.

Oh dear, that's the bit I missed.

At this point I assume that I should not resubmit a patch (to avoid
unnecessary noise) and patiently wait for a review, e.g., by Jiri, for
it to be included in next.
From there, I'll try to do the right thing (CC stable in the
signed-off area) shall a new version be necessary or follow option 2
with the commit id when it gets merged to Linus' tree.

Once again, my apologies for failing to follow the procedure and thank
you for your patience.

-Pascal

Re: [PATCH] HID: sony: fix freeze when inserting ghlive ps3/wii dongles

[Jiri Kosina]

On Fri, 4 Jun 2021, Pascal Giard wrote:

> This commit fixes a freeze on insertion of a Guitar Hero Live PS3/WiiU
> USB dongle. Indeed, with the current implementation, inserting one of
> those USB dongles will lead to a hard freeze. I apologize for not
> catching this earlier, it didn't occur on my old laptop.

Applied to for-5.13/upstream-fixes branch, thanks.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: sony: fix freeze when inserting ghlive ps3/wii dongles

[Pascal Giard]

On Tue, Jun 15, 2021 at 4:54 AM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Fri, 4 Jun 2021, Pascal Giard wrote:
>
> > This commit fixes a freeze on insertion of a Guitar Hero Live PS3/WiiU
> > USB dongle. Indeed, with the current implementation, inserting one of
> > those USB dongles will lead to a hard freeze. I apologize for not
> > catching this earlier, it didn't occur on my old laptop.
>
> Applied to for-5.13/upstream-fixes branch, thanks.

Thanks Jiri!

I saw that it propagated to 5.13 and 5.12. It also made it to Linus'
tree, I can see it in his master branch.

We (Daniel and myself) have a patch that we want to submit that will
add support for the PS4 version of the Guitar Hero Live dongle ;-)
IIUC our patch should be against dtor/master and not Linus' tree.

However, we see that dtor/master is still behind, it does not include
the fix. Is there something I need to do? Do I just have to be
patient?

Best regards,

-Pascal

Re: [PATCH] HID: sony: fix freeze when inserting ghlive ps3/wii dongles

[Jiri Kosina]

On Tue, 13 Jul 2021, Pascal Giard wrote:

> We (Daniel and myself) have a patch that we want to submit that will add 
> support for the PS4 version of the Guitar Hero Live dongle ;-) IIUC our 
> patch should be against dtor/master and not Linus' tree.
> 
> However, we see that dtor/master is still behind, it does not include
> the fix. Is there something I need to do? Do I just have to be
> patient?

Just base the patch on Linus' tree, submit the patch to Dmitry and 
indicate that it's based on more recent upstream than his master. He will 
manage :)

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: sony: fix freeze when inserting ghlive ps3/wii dongles

[Benjamin Tissoires]

On Thu, Jul 15, 2021 at 9:44 PM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Tue, 13 Jul 2021, Pascal Giard wrote:
>
> > We (Daniel and myself) have a patch that we want to submit that will add
> > support for the PS4 version of the Guitar Hero Live dongle ;-) IIUC our
> > patch should be against dtor/master and not Linus' tree.
> >
> > However, we see that dtor/master is still behind, it does not include
> > the fix. Is there something I need to do? Do I just have to be
> > patient?
>
> Just base the patch on Linus' tree, submit the patch to Dmitry and
> indicate that it's based on more recent upstream than his master. He will
> manage :)

FWIW, if the patch in question is
https://patchwork.kernel.org/project/linux-input/patch/20210715195720.169385-1-daniel.nguyen.1@ens.etsmtl.ca/
then this patch is not destinated to Dmitry, but the HID tree like the
current one :)

Cheers,
Benjamin

>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>

Re: [PATCH] HID: sony: fix freeze when inserting ghlive ps3/wii dongles

[Pascal Giard]

On Tue, Jul 20, 2021 at 4:18 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Thu, Jul 15, 2021 at 9:44 PM Jiri Kosina <jikos@kernel.org> wrote:
> >
> > On Tue, 13 Jul 2021, Pascal Giard wrote:
> >
> > > We (Daniel and myself) have a patch that we want to submit that will add
> > > support for the PS4 version of the Guitar Hero Live dongle ;-) IIUC our
> > > patch should be against dtor/master and not Linus' tree.
> > >
> > > However, we see that dtor/master is still behind, it does not include
> > > the fix. Is there something I need to do? Do I just have to be
> > > patient?
> >
> > Just base the patch on Linus' tree, submit the patch to Dmitry and
> > indicate that it's based on more recent upstream than his master. He will
> > manage :)
>
> FWIW, if the patch in question is
> https://patchwork.kernel.org/project/linux-input/patch/20210715195720.169385-1-daniel.nguyen.1@ens.etsmtl.ca/
> then this patch is not destinated to Dmitry, but the HID tree like the
> current one :)

Hi Benjamin,

Yes, we figured since then. I was confused between both trees
(dtor/input and hid/hid), and following a brief discussion with Dmitry
on IRC, that confusion was resolved.
Bottom line: in case of doubt, remember to go back to
./scripts/get_maintainer.pl.

That lead to the submission of the patch you linked to above, applied
against hid/hid.

Sorry, I should have replied to clear this up earlier.

Best regards,

-Pascal