On Sat, 31 May 2014, Pantelis Koukousoulas wrote:
> When dummy_hcd is created as a SuperSpeed controller (configurable via
> a module parameter) dummy_start() is called twice, once to make the
Not to "make" the hcds, but to start them running. The hcd structures
are created in dummy_hcd_probe().
> SuperSpeed HCD and once to make the Highspeed HCD. When it is created
> as a HighSpeed controller, then dummy_start() is only called once.
>
> The way dummy_start() was written, the URB list and the timer are
> initialized twice (in the superspeed case). Besides looking weird
> it is also possible that this can be buggy, because the way it works
> is that we initialize the timer and the list when starting the
> SuperSpeed HCD, we return all the way back to the USB core (so
> possibly the timer and list start to get used due to requests
> by the hub) and then we initialize them again while starting
> the HighSpeed HCD. It is not impossible for this to result at
> least in lost/leaked URBs.
No, this is completely wrong. When the driver uses SuperSpeed support,
there are two hcds: the high speed one and the SuperSpeed one. They
are different structures and they both need to be initialized.
It sounds like you have confused struct dummy (there's only one of
these) with struct dummy_hcd (there are two of these).
> So, rework dummy_start() to only initialize dummy_hcd struct
> fields once and delete dummy_start_ss() helper function
> as it is no longer useful.
>
> Signed-off-by: Pantelis Koukousoulas <pktoss@xxxxxxxxx>
> ---
> drivers/usb/gadget/dummy_hcd.c | 50 ++++++++++++++----------------------------
> 1 file changed, 17 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
> index 8c06430..dbbee3e 100644
> --- a/drivers/usb/gadget/dummy_hcd.c
> +++ b/drivers/usb/gadget/dummy_hcd.c
> @@ -2314,45 +2314,23 @@ static ssize_t urbs_show(struct device *dev, struct device_attribute *attr,
> }
> static DEVICE_ATTR_RO(urbs);
>
> -static int dummy_start_ss(struct dummy_hcd *dum_hcd)
> -{
> - init_timer(&dum_hcd->timer);
> - dum_hcd->timer.function = dummy_timer;
> - dum_hcd->timer.data = (unsigned long)dum_hcd;
> - dum_hcd->rh_state = DUMMY_RH_RUNNING;
> - dum_hcd->stream_en_ep = 0;
> - INIT_LIST_HEAD(&dum_hcd->urbp_list);
> - dummy_hcd_to_hcd(dum_hcd)->power_budget = POWER_BUDGET;
> - dummy_hcd_to_hcd(dum_hcd)->state = HC_STATE_RUNNING;
> - dummy_hcd_to_hcd(dum_hcd)->uses_new_polling = 1;
> -#ifdef CONFIG_USB_OTG
> - dummy_hcd_to_hcd(dum_hcd)->self.otg_port = 1;
> -#endif
> - return 0;
> -
> - /* FIXME 'urbs' should be a per-device thing, maybe in usbcore */
> - return device_create_file(dummy_dev(dum_hcd), &dev_attr_urbs);
> -}
> -
> static int dummy_start(struct usb_hcd *hcd)
> {
> struct dummy_hcd *dum_hcd = hcd_to_dummy_hcd(hcd);
>
> /*
> - * MASTER side init ... we emulate a root hub that'll only ever
> - * talk to one device (the slave side). Also appears in sysfs,
> - * just like more familiar pci-based HCDs.
Don't remove this comment. It's still correct.
> + * These are per-device, not per HCD, so they should only be
I don't know what "These" refers to, but the data structures used in
this subroutine _are_ per-hcd.
> + * initialized once although in the superspeed case this
> + * function will be called twice. Thus we use a conditional.
> */
> - if (!usb_hcd_is_primary_hcd(hcd))
> - return dummy_start_ss(dum_hcd);
> -
> - spin_lock_init(&dum_hcd->dum->lock);
> - init_timer(&dum_hcd->timer);
> - dum_hcd->timer.function = dummy_timer;
> - dum_hcd->timer.data = (unsigned long)dum_hcd;
> - dum_hcd->rh_state = DUMMY_RH_RUNNING;
> -
> - INIT_LIST_HEAD(&dum_hcd->urbp_list);
> + if (dum_hcd->rh_state != DUMMY_RH_RUNNING) {
> + spin_lock_init(&dum_hcd->dum->lock);
> + init_timer(&dum_hcd->timer);
> + dum_hcd->timer.function = dummy_timer;
> + dum_hcd->timer.data = (unsigned long)dum_hcd;
> + INIT_LIST_HEAD(&dum_hcd->urbp_list);
> + dum_hcd->rh_state = DUMMY_RH_RUNNING;
> + }
This stuff needs to be executed unconditionally.
> hcd->power_budget = POWER_BUDGET;
> hcd->state = HC_STATE_RUNNING;
> @@ -2362,6 +2340,12 @@ static int dummy_start(struct usb_hcd *hcd)
> hcd->self.otg_port = 1;
> #endif
>
> + /* In dummy_hcd the SuperSpeed HCD is the secondary one */
This is true for all HCDs that have SuperSpeed support, not just
dummy_hcd.
> + if (!usb_hcd_is_primary_hcd(hcd)) {
> + dum_hcd->stream_en_ep = 0;
> + return 0;
> + }
> +
> /* FIXME 'urbs' should be a per-device thing, maybe in usbcore */
> return device_create_file(dummy_dev(dum_hcd), &dev_attr_urbs);
> }
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
