Message-ID: <c4e88c28-28ee-4e37-9822-8e2999d0f0ee@rowland.harvard.edu>
Date: Tue, 15 Jul 2025 21:30:02 -0400
From: Alan Stern <stern@...land.harvard.edu>
To: Sean Young <sean@...s.org>
Cc: Hillf Danton <hdanton@...a.com>,
	Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
	syzbot+592e2ab8775dbe0bf09a@...kaller.appspotmail.com,
	LKML <linux-kernel@...r.kernel.org>,
	Mauro Carvalho Chehab <mchehab@...nel.org>
Subject: Re: [PATCH] media: imon: make send_packet() more robust

On Tue, Jul 15, 2025 at 09:19:18PM +0100, Sean Young wrote:
> Hi Alan,
> 
> On Sun, Jul 13, 2025 at 11:21:24AM -0400, Alan Stern wrote:
> > On Sun, Jul 13, 2025 at 04:11:47PM +0800, Hillf Danton wrote:
> > > [loop Alan in]
> > 
> > I assume you're interested in the question of when to avoid resubmitting 
> > URBs.

> > In theory it's okay to resubmit _if_ the driver has a robust 
> > error-recovery scheme (such as giving up after some fixed limit on the 
> > number of errors or after some fixed time has elapsed, perhaps with a 
> > time delay to prevent a flood of errors).  Most drivers don't bother to 
> > do this; they simply give up right away.  This makes them more 
> > vulnerable to short-term noise interference during USB transfers, but in 
> > reality such interference is quite rare.  There's nothing really wrong 
> > with giving up right away.
> > 
> > As to which error codes drivers should pay attention to...  In most 
> > cases they only look at -EPROTO.  According to 
> > Documentation/driver-api/usb/error-codes.rst, -EILSEQ and -ETIME are 
> > also possible errors when a device has been unplugged, so it wouldn't 
> > hurt to check for them too.  But most host controller drivers don't 
> > bother to issue them; -EPROTO is by far the most common error code 
> > following an unplug.
> 
> Thank you for explaining that, very helpful. Would it be useful to have
> this in the USB completion handler documentation?

I don't know what USB completion handler documentation you're talking 
about.  Is it something in the Documentation/ directory?  If it is then 
it should already include or refer to error-codes.rst.

Or perhaps you're talking about the kerneldoc for this particular 
completion handler?  There's no reason for that to include all the 
material that's already in error-codes.rst.  But you might put a comment 
in the code at the point where -EPROTO errors are handled, explaining 
that they generally indicate that the device has been unplugged.

> > If the error occurred because the device was unplugged then unlinking 
> > the outstanding URBs isn't necessary; the USB core will unlink them for 
> > you after the device's parent hub reports that the unplug took place.
> 
> Are you saying there is a case when usb_unlink_urb() is necessary: if the
> device was not unplugged and -EPROTO is reported?

That depends on the driver.  If it wants to cancel other outstanding 
URBs merely because one URB got a -EPROTO error but the device wasn't 
unplugged, then it has to call usb_unlink_urb() or something equivalent.  
Otherwise it will have to wait for those other URBs to complete in the 
usual way.

(Of course, when the -EPROTO error code shows up in the completion 
handler, the driver doesn't know yet whether the device has been 
unplugged...)

> > > > Second problem is that when usb_rx_callback_intf0() once got -EPROTO error
> > > > before ictx->dev_present_intf0 becomes true, usb_rx_callback_intf0() always
> > > > resubmits urb due to commit 8791d63af0cf ("[media] imon: don't wedge
> > > > hardware after early callbacks"). If some errors should stop resubmitting
> > > > urb regardless of whether configuring the hardware has completed or not,
> > > > what that commit is doing is wrong. The ictx->dev_present_intf0 test was
> > > > introduced by commit 6f6b90c9231a ("[media] imon: don't parse scancodes
> > > > until intf configured"), but that commit did not call usb_unlink_urb()
> > > > when usb_rx_callback_intf0() got an error. Move the ictx->dev_present_intf0
> > > > test to immediately before imon_incoming_packet() so that we can call
> > > > usb_unlink_urb() as needed, or the first problem explained above happens
> > > > without printk() flooding (i.e. hung task).
> > 
> > It seems odd for a driver to set up normal communications with a device 
> > before the device has been configured, but of course that decision is up 
> > to the creators and maintainers of the driver.
> 
> The usb device has two interfaces, and we need both of them before we can
> do anything useful. Badly designed hardware.
> 
> I think that is why this driver code is so awkward.

That's what usb_driver_claim_interface() is for.  IIRC, the cdc-acm 
driver uses it in exactly this way.

Alan Stern

Powered by blists - more mailing lists