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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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