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

[Johan Hovold <johan@kernel.org>]

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