On Tue, 27 Aug 2013, Ming Lei wrote:
> > I don't understand your argument. The uhci-hcd _does_ support
> > interrupt URBs being submitted from tasklets, workqueues, kernel
> > threads, or whatever. The guarantee is that the interrupt URB will be
> > scheduled right away if the endpoint queue has not emptied out.
>
> Considered it is common for interrupt queue that only one URB is
> submitted, there is no the guarantee if drivers submit URB in
> tasklet scheduled from complete(), so the URB isn't submitted
> right away always for the driver.
Yes. I'm not sure what is the best way to explain this...
Think about it this way: Why did you write the "USB: EHCI: improve
interrupt qh unlink" patch in the first place? Essentially the same
reason applies to uhci-hcd and ohci-hcd, and they will need similar
patches.
> > The problem is that the HCDs currently _do_ guarantee the isochronous
> > URBs submitted by the completion handler will be scheduled immediately
> > following the preceding URB (to the best of their ability -- not all
> > the HCDs can do this). Maintaining this guarantee while giving back
> > URBs in a tasklet is difficult. See the 3/3 patch in the RFC series I
> > just posted to get an idea of the required changes.
>
> There are already several drivers which submits URB from tasklet
> scheduled in complete(), which doesn't submit URB directly. Is there
> any difference with what does in giveback in tasklet patch from view
> of HCD?
No. There is a difference only when the completion submits an
isochronous URB directly.
> >> > Suppose an URB is submitted while the endpoint queue is non-empty, so
> >> > it is scheduled to start in the next slot after the end of the last URB
> >> > in the queue. Does that next slot occur after the frame stored in
> >> > ehci->last_iso_frame?
> >>
> >> I think it is yes, actually the new slot is always after current uframe
> >> during iso_stream_schedule from tasklet, and it is behind
> >> ehci->last_iso_frame which is updated in ehci_irq path.
> >
> > Wrong. Here are a couple of examples. Assume first that the
> > endpoint's interval is 8 frames (it's a full-speed device):
> >
> > The last packet of the last URB on the queue is scheduled
> > for frame 100. Because of an SMI, IRQs are delayed until
> > the controller reaches frame 105.
> >
> > When the interrupt occurs, ehci-hcd will scan the siTDs
> > for the endpoint and give them all back. At the end,
> > last_iso_frame will be set to 104 (it lags one behind the
> > current frame number).
> >
> > Let's say the tasklet takes 1 ms to start up, so the
> > completion handler runs during frame 106. It submits an
> > URB. This URB is scheduled for the next unused slot, which
> > is 108 (i.e., 8 frames after the last packet of the last
> > URB). Since 108 is larger than both 104 and 106, the next
> > slot does indeed come after both ehci->last_iso_frame and
> > the current frame number.
>
> Yes, but the TDs(URB) can't be missed, suppose there are 8
> packets, so the last packet of the new URB will be completed in
> 164, when IRQ comes at 165, ehci_irq() will scan from 104 to
> 165, so the URB can be completed successfully, then update
> last_iso_frame as 164.
>
> Then what is wrong?
Nothing is wrong, either with my example or with yours.
> > Now assume the endpoint's interval is 1 frame:
> >
> > The last packet of the last URB on the queue is scheduled
> > for frame 200. Because of an SMI, IRQs are delayed until
> > the controller reaches frame 205.
> >
> > When the interrupt occurs, ehci-hcd will scan the siTDs
> > for the endpoint and give them all back. At the end,
> > last_iso_frame will be set to 204.
> >
> > The tasklet takes 1 ms to start up, so the completion handler
> > runs during frame 206. It submits an URB. This URB is
> > scheduled for the next unused slot, which is 201 (i.e., 1 frame
> > after the last packet of the last URB). Since 201 is smaller
> > than both 204 and 206, the next slot does indeed come before
> > both ehci->last_iso_frame and the current frame number.
>
> iso_stream_schedule() will figure out that the next slot(201) is behind
> the scheduling threshold(case of 'start < next'), then update the next
> slot as one new slot which is larger 206 if URB_ISO_ASAP, so looks
> the TDs(USB) can't be missed too.
>
> Without URB_ISO_ASAP, there might be problem since 201 won't
> be scanned next time in scan_isoc, so is it what your RFC3 is fixing?
Yes, it is. Now you understand my point.
> If I understand the above problem correctly, the same problem exists
> on drivers which submit URB in tasklet scheduled from complete() too
> before the moving, doesn't it?
That's right. Probably those drivers always use URB_ISO_ASAP, so the
problem doesn't arise. If they don't use URB_ISO_ASAP ... well, then
they will get inconsistent results when an underrun occurs.
Below is an updated version of the last 1/3 RFC patch (still without
percpu variables). What do you think of it?
Alan Stern
Index: usb-3.11/include/linux/usb/hcd.h
===================================================================
--- usb-3.11.orig/include/linux/usb/hcd.h
+++ usb-3.11/include/linux/usb/hcd.h
@@ -73,6 +73,7 @@ struct giveback_urb_bh {
spinlock_t lock;
struct list_head head;
struct tasklet_struct bh;
+ struct usb_host_endpoint *giveback_ep;
};
struct usb_hcd {
@@ -378,6 +379,12 @@ static inline int hcd_giveback_urb_in_bh
return hcd->driver->flags & HCD_BH;
}
+static inline bool hcd_periodic_giveback_in_progress(struct usb_hcd *hcd,
+ struct usb_host_endpoint *ep)
+{
+ return hcd->high_prio_bh.giveback_ep == ep;
+}
+
extern int usb_hcd_link_urb_to_ep(struct usb_hcd *hcd, struct urb *urb);
extern int usb_hcd_check_unlink_urb(struct usb_hcd *hcd, struct urb *urb,
int status);
Index: usb-3.11/drivers/usb/core/hcd.c
===================================================================
--- usb-3.11.orig/drivers/usb/core/hcd.c
+++ usb-3.11/drivers/usb/core/hcd.c
@@ -1690,7 +1690,9 @@ static void usb_giveback_urb_bh(unsigned
urb = list_entry(local_list.next, struct urb, urb_list);
list_del_init(&urb->urb_list);
+ bh->giveback_ep = urb->ep;
__usb_hcd_giveback_urb(urb);
+ bh->giveback_ep = NULL;
}
/* check if there are new URBs to giveback */
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
