[PATCH] USB: serial: garmin_gps: queue the device reset

[PATCH] USB: serial: garmin_gps: queue the device reset

[Greg Kroah-Hartman]

From: Josef Bacik <josef@toxicpanda.com>

A private packet with the ID PRIV_PKTID_RESET_REQ that is written to the
port makes garmin_write() call usb_kill_urb() and usb_reset_device().
Both sleep, but the write operation of a tty driver must not: a line
discipline may call it in atomic context, synchronous PPP for one under
a spinlock with bottom halves disabled.  In addition usb_reset_device()
has to be called with the device locked, which it is not here.

The reset also unbinds the driver inside the write: the disconnect hangs
the tty up, ppp_sync_close() calls ppp_unregister_channel(), and that
spins on pch->downl, which the writer holds.  The CPU is lost.

Unlink the URB and use usb_queue_reset_device(), which locks the device
and resets it from a work item.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable <stable@kernel.org>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
Johan, this was sent to me, but I really don't know if PPP is used for
this device to talk to the port or not.  If not, this isn't a real
issue, but if it is, then maybe it is?  I can't tell so I'm tossing it
to you :)

 drivers/usb/serial/garmin_gps.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/serial/garmin_gps.c b/drivers/usb/serial/garmin_gps.c
index 8020149f5658..c2b29a7a6674 100644
--- a/drivers/usb/serial/garmin_gps.c
+++ b/drivers/usb/serial/garmin_gps.c
@@ -818,10 +818,9 @@ static void priv_status_resp(struct usb_serial_port *port)
  * Garmin specific driver functions
  ******************************************************************************/
 
-static int process_resetdev_request(struct usb_serial_port *port)
+static void process_resetdev_request(struct usb_serial_port *port)
 {
 	unsigned long flags;
-	int status;
 	struct garmin_data *garmin_data_p = usb_get_serial_port_data(port);
 
 	spin_lock_irqsave(&garmin_data_p->lock, flags);
@@ -830,13 +829,10 @@ static int process_resetdev_request(struct usb_serial_port *port)
 	garmin_data_p->serial_num = 0;
 	spin_unlock_irqrestore(&garmin_data_p->lock, flags);
 
-	usb_kill_urb(port->interrupt_in_urb);
-	dev_dbg(&port->dev, "%s - usb_reset_device\n", __func__);
-	status = usb_reset_device(port->serial->dev);
-	if (status)
-		dev_dbg(&port->dev, "%s - usb_reset_device failed: %d\n",
-			__func__, status);
-	return status;
+	/* This is called by the write operation, which must not sleep */
+	usb_unlink_urb(port->interrupt_in_urb);
+	dev_dbg(&port->dev, "%s - usb_queue_reset_device\n", __func__);
+	usb_queue_reset_device(port->serial->interface);
 }
 
 
-- 
2.56.0

Re: [PATCH] USB: serial: garmin_gps: queue the device reset

[sashiko-bot]

> From: Josef Bacik <josef@toxicpanda.com>
> 
> A private packet with the ID PRIV_PKTID_RESET_REQ that is written to the
> port makes garmin_write() call usb_kill_urb() and usb_reset_device().
> Both sleep, but the write operation of a tty driver must not: a line
> discipline may call it in atomic context, synchronous PPP for one under
> a spinlock with bottom halves disabled.  In addition usb_reset_device()
> has to be called with the device locked, which it is not here.
> 
> The reset also unbinds the driver inside the write: the disconnect hangs
> the tty up, ppp_sync_close() calls ppp_unregister_channel(), and that
> spins on pch->downl, which the writer holds.  The CPU is lost.
> 
> Unlink the URB and use usb_queue_reset_device(), which locks the device
> and resets it from a work item.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable <stable@kernel.org>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/2026102733-prayer-drainer-56f3@gregkh?part=1

Re: [PATCH] USB: serial: garmin_gps: queue the device reset

[Johan Hovold]

On Tue, Oct 27, 2026 at 03:29:34PM +0100, Greg Kroah-Hartman wrote:
> From: Josef Bacik <josef@toxicpanda.com>
> 
> A private packet with the ID PRIV_PKTID_RESET_REQ that is written to the
> port makes garmin_write() call usb_kill_urb() and usb_reset_device().
> Both sleep, but the write operation of a tty driver must not: a line
> discipline may call it in atomic context, synchronous PPP for one under
> a spinlock with bottom halves disabled.  In addition usb_reset_device()
> has to be called with the device locked, which it is not here.
> 
> The reset also unbinds the driver inside the write: the disconnect hangs
> the tty up, ppp_sync_close() calls ppp_unregister_channel(), and that
> spins on pch->downl, which the writer holds.  The CPU is lost.
> 
> Unlink the URB and use usb_queue_reset_device(), which locks the device
> and resets it from a work item.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable <stable@kernel.org>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> Johan, this was sent to me, but I really don't know if PPP is used for
> this device to talk to the port or not.  If not, this isn't a real
> issue, but if it is, then maybe it is?  I can't tell so I'm tossing it
> to you :)

No, you would not use PPP with this device, but if someone tried to I
don't believe they would be able to trigger the reset due the PPP
packet framing not matching the garmin_gps header format.

So basically the patch is fine but the commit message is misleading.

You will still get a deadlock with the default line discipline however
so that's enough of a motivation to fix this (even if that doesn't wedge
a CPU).

> -	usb_kill_urb(port->interrupt_in_urb);
> -	dev_dbg(&port->dev, "%s - usb_reset_device\n", __func__);
> -	status = usb_reset_device(port->serial->dev);
> -	if (status)
> -		dev_dbg(&port->dev, "%s - usb_reset_device failed: %d\n",
> -			__func__, status);
> -	return status;
> +	/* This is called by the write operation, which must not sleep */
> +	usb_unlink_urb(port->interrupt_in_urb);

USB core will unlink (and the driver will kill) all three URBs as part
of disconnect so I think we can just drop the interrupt-in unlink here.

> +	dev_dbg(&port->dev, "%s - usb_queue_reset_device\n", __func__);
> +	usb_queue_reset_device(port->serial->interface);
>  }

Josef, do you want a send a v2 of this or shall I treat this patch as a
bug report and fix it up myself?

Johan

Re: [PATCH] USB: serial: garmin_gps: queue the device reset

[Greg Kroah-Hartman]

On Thu, Oct 29, 2026 at 09:30:58AM +0100, Johan Hovold wrote:
> On Tue, Oct 27, 2026 at 03:29:34PM +0100, Greg Kroah-Hartman wrote:
> > From: Josef Bacik <josef@toxicpanda.com>
> > 
> > A private packet with the ID PRIV_PKTID_RESET_REQ that is written to the
> > port makes garmin_write() call usb_kill_urb() and usb_reset_device().
> > Both sleep, but the write operation of a tty driver must not: a line
> > discipline may call it in atomic context, synchronous PPP for one under
> > a spinlock with bottom halves disabled.  In addition usb_reset_device()
> > has to be called with the device locked, which it is not here.
> > 
> > The reset also unbinds the driver inside the write: the disconnect hangs
> > the tty up, ppp_sync_close() calls ppp_unregister_channel(), and that
> > spins on pch->downl, which the writer holds.  The CPU is lost.
> > 
> > Unlink the URB and use usb_queue_reset_device(), which locks the device
> > and resets it from a work item.
> > 
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Cc: stable <stable@kernel.org>
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > Johan, this was sent to me, but I really don't know if PPP is used for
> > this device to talk to the port or not.  If not, this isn't a real
> > issue, but if it is, then maybe it is?  I can't tell so I'm tossing it
> > to you :)
> 
> No, you would not use PPP with this device, but if someone tried to I
> don't believe they would be able to trigger the reset due the PPP
> packet framing not matching the garmin_gps header format.
> 
> So basically the patch is fine but the commit message is misleading.
> 
> You will still get a deadlock with the default line discipline however
> so that's enough of a motivation to fix this (even if that doesn't wedge
> a CPU).
> 
> > -	usb_kill_urb(port->interrupt_in_urb);
> > -	dev_dbg(&port->dev, "%s - usb_reset_device\n", __func__);
> > -	status = usb_reset_device(port->serial->dev);
> > -	if (status)
> > -		dev_dbg(&port->dev, "%s - usb_reset_device failed: %d\n",
> > -			__func__, status);
> > -	return status;
> > +	/* This is called by the write operation, which must not sleep */
> > +	usb_unlink_urb(port->interrupt_in_urb);
> 
> USB core will unlink (and the driver will kill) all three URBs as part
> of disconnect so I think we can just drop the interrupt-in unlink here.
> 
> > +	dev_dbg(&port->dev, "%s - usb_queue_reset_device\n", __func__);
> > +	usb_queue_reset_device(port->serial->interface);
> >  }
> 
> Josef, do you want a send a v2 of this or shall I treat this patch as a
> bug report and fix it up myself?

I'm guessing at Josef's current load, so it might just be faster for you
to fix it up yourself :)

thanks,

greg k-h

Re: [PATCH] USB: serial: garmin_gps: queue the device reset

[Johan Hovold]

On Thu, Oct 29, 2026 at 10:38:31AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Oct 29, 2026 at 09:30:58AM +0100, Johan Hovold wrote:

> > Josef, do you want a send a v2 of this or shall I treat this patch as a
> > bug report and fix it up myself?
> 
> I'm guessing at Josef's current load, so it might just be faster for you
> to fix it up yourself :)

That was my guess as well. I'll do so.

Johan