On Tue, Aug 27, 2013 at 10:55 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> 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.
Right, but drivers which submit URBs from tasklet/workque/... scheduled
from complete() may need these patches too, even without giveback URB
in complete() change.
>
>> > 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.
You have said the same problem exists on these drivers already
without giveback in tasklet patch, so for the problem and solution,
there is no difference.
>
>> >> > 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.
Actually the problem only happened when underrun without
URB_ISO_ASAP.
I have another fix which uses rebase trick and is simper(less change), could
you comment on the attachment patch?
>
>> 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.
Exactly, that is why I wouldn't like to count the change only on the
giveback in tasklet patch, :-)
>
> Below is an updated version of the last 1/3 RFC patch (still without
> percpu variables). What do you think of it?
Just little minor things.
>
> 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)
Suggest not mention periodic since it can be used generally to decide if
one urb is schedule in its complete, then better to pass urb instead of ep.
Also giveback have been completed, and it is complete in progress, :-)
> +{
> + 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;
> }
Considered it can be used generally, it is better to keep it as percpu
operation to avoid probable incorrectness, also percpu write is more
economical, but it isn't a big deal.
Thanks,
--
Ming Lei
Attachment:
ehci-rebase.patch
Description: Binary data
