On Wed, Jun 19, 2013 at 11:37 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, 19 Jun 2013, Ming Lei wrote:
>
>> >> @@ -835,9 +839,11 @@ static int usb_rh_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
>> >> hcd->status_urb = NULL;
>> >> usb_hcd_unlink_urb_from_ep(hcd, urb);
>> >>
>> >> - spin_unlock(&hcd_root_hub_lock);
>> >> + if (!hcd_giveback_urb_in_bh(hcd))
>> >> + spin_unlock(&hcd_root_hub_lock);
>> >> usb_hcd_giveback_urb(hcd, urb, status);
>> >> - spin_lock(&hcd_root_hub_lock);
>> >> + if (!hcd_giveback_urb_in_bh(hcd))
>> >> + spin_lock(&hcd_root_hub_lock);
>> >> }
>> >> }
>> >> done:
>> >
>> > None of these tests are necessary. Root hubs are different from normal
>> > devices; their URBs are handled mostly by usbcore. The only part done
>> > by the HCD is always synchronous. And we know that root-hub URBs
>>
>> Looks not always synchronous, control transfer is synchronous, and
>> interrupt transfer is still asynchronous. No drivers(hub, usbfs) depend
>> on that, and IMO, treating root hub same as hub will simplify HCD core,
>> and finally we can remove all the above lock releasing & acquiring if
>> all HCDs set HCD_BH.
>>
>> Also there is very less roothub transfers and always letting tasklet
>> handle URB giveback of roothub won't have performance problem, so
>> how about keeping the above tests?
>
> If you want to use the tasklets for root-hub URBs, okay. There's no
> reason to check the HCD_BH flag, though, because HCDs have only minimal
> involvement in root-hub URBs. In particular, HCD's don't call
> usb_hcd_giveback_urb() for them.
Looks both root hub's control and interrupt transfer call usb_hcd_giveback_urb()
to complete URBs, don't they?
> So you can use the tasklets for _all_ root-hub URBs. Then the tests
> above aren't necessary, and neither are the spinlock operations.
Yes, that is what I am going to do.
>
>> >> @@ -2573,6 +2687,16 @@ int usb_add_hcd(struct usb_hcd *hcd,
>> >> && device_can_wakeup(&hcd->self.root_hub->dev))
>> >> dev_dbg(hcd->self.controller, "supports USB remote wakeup\n");
>> >>
>> >> + if (usb_hcd_is_primary_hcd(hcd)) {
>> >> + retval = init_giveback_urb_bh(hcd);
>> >> + if (retval)
>> >> + goto err_init_giveback_bh;
>> >> + } else {
>> >> + /* share tasklet handling with primary hcd */
>> >> + hcd->async_bh = hcd->primary_hcd->async_bh;
>> >> + hcd->periodic_bh = hcd->primary_hcd->periodic_bh;
>> >> + }
>> >
>> > Is there any reason why a secondary HCD can't have its own tasklets?
>>
>> I didn't do that because both primary and secondary HCDs share one
>> hard interrupt handler, so basically there is no obvious advantage to
>> do that.
>
> If the bh structures are embedded directly in the hcd structure, it
> won't be possible for a secondary hcd to share its tasklets with the
> primary hcd. Not sharing seems simpler, and there's no obvious
> disadvantage either.
OK, I will let secondary HCD have its own tasklet in v2.
Thanks,
--
Ming Lei
--
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
