On Wed, 28 Aug 2013, Ming Lei wrote:
> > 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.
That's a totally separate issue. This email thread is about changes to
ehci-hcd, not bugs in other drivers.
Drivers that submit isochronous URBs from tasklets/workqueues/whatever
without URB_ISO_ASAP are broken and need to be fixed.
> 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.
What do you mean? We can't fix those other drivers by changing
ehci-hcd. It's their fault if they misuse the isochronous API.
> >> 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.
Yes. That case has to be handled correctly.
> >> 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, :-)
Why not? Problems in other drivers can't be fixed by making changes to
ehci-hcd. This change is needed so that drivers which aren't buggy
(such as snd-usb-audio) will continue to work as they should.
> > 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.
There's no reason to call this routine for non-periodic endpoints.
Also, this way I don't have to test whether to use high_prio_bh or
low_prio_bh.
There's no reason to pass an URB pointer. We don't care whether the
URB being submitted is the same as the one that is completing. All
that matters is whether a completion handler for some URB on this
endpoint is running.
> Also giveback have been completed, and it is complete in progress, :-)
Good point; I will change the name.
> > +{
> > + 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.
By that argument, _every_ variable that isn't on the stack and is used
only in preempt-disabled regions should be percpu. Obviously that
would be ridiculous overkill. Percpu variables should be used only
when there is significant contention. After all, using a percpu
variable has its own costs in time and space.
Besides, the CPU that runs this code already owns the cache line
containing bh's tasklet struct. There will not be any overhead when it
writes to bh->giveback_ep (guess I should rename that to
completing_ep). And when the completion handler submits an URB, it
will running be on this same CPU.
Alan Stern
--
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
