Re: [PATCH 1/3] usb: gadget: dummy_hcd: refactor dummy_start to be DRY
On Tue, 3 Jun 2014, Pantelis Koukousoulas wrote:
> So, in the end dummy_start_ss() can be replaced by just adding
> an
>
> if (usb_is_primary_hcd(hcd))
> spin_lock_init(&dum_hcd->dum->lock);
>
> near the top of dummy_start() and
>
> if(!usb_is_primary_hcd(hcd)) {
>dum_hcd->stream_en_ep = 0;
>return 0;
> }
>
> near the bottom, right? (so that the device file will only be created
> once).
That's right.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] usb: gadget: dummy_hcd: refactor dummy_start to be DRY
On Tue, Jun 3, 2014 at 5:21 PM, Alan Stern wrote:
> On Tue, 3 Jun 2014, Pantelis Koukousoulas wrote:
> That's the same mistake as before -- struct dummy_hcd _isn't_ shared.
> Only struct dummy is shared.
Oops! Indeed it turns out I was confused about the structs as well :S
> The current version of xhci-pci prevents any suspends from occurring
> between the initialization of the two hcds; see commit bcffae7708eb.
> You might need to make dummy-hcd do the same thing.
I noticed that although xhci-pci does this, xhci-plat doesn't, was this
on purpose? In any case I think this is a separate issue and not as
serious as I thought.
So, in the end dummy_start_ss() can be replaced by just adding
an
if (usb_is_primary_hcd(hcd))
spin_lock_init(&dum_hcd->dum->lock);
near the top of dummy_start() and
if(!usb_is_primary_hcd(hcd)) {
dum_hcd->stream_en_ep = 0;
return 0;
}
near the bottom, right? (so that the device file will only be created
once).
If this is correct then I can submit a patch to do it.
Thanks a lot for the comments!
Regards,
Pantelis
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] usb: gadget: dummy_hcd: refactor dummy_start to be DRY
On Tue, 3 Jun 2014, Pantelis Koukousoulas wrote: > On Mon, Jun 2, 2014 at 6:04 PM, Alan Stern wrote: > > 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). > > I think that indeed I was completely wrong but I wasn't confused about the > structures, but regarding some details of what is executed when, in which > order and in which context. > > The reason was that when I simply removed the timer / list initialization > from dummy_start_ss(), (because they belong to the shared struct dummy_hcd > so they better only be initialized in one place) That's the same mistake as before -- struct dummy_hcd _isn't_ shared. Only struct dummy is shared. > I got a hard lockup > during testing, > when I started dummy_hcd in superspeed mode and added a device. > > I think that now I understand why this happened, but I will read the > relevant bits > from dummy_hcd and xhci once more (especially what happens if there is > a suspend / resume between the initialization of the highspeed hcd and the > superspeed hcd) and come back with a different approach that will hopefully > be both correct and DRY. The current version of xhci-pci prevents any suspends from occurring between the initialization of the two hcds; see commit bcffae7708eb. You might need to make dummy-hcd do the same thing. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] usb: gadget: dummy_hcd: refactor dummy_start to be DRY
On Mon, Jun 2, 2014 at 6:04 PM, Alan Stern wrote: > 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). I think that indeed I was completely wrong but I wasn't confused about the structures, but regarding some details of what is executed when, in which order and in which context. The reason was that when I simply removed the timer / list initialization from dummy_start_ss(), (because they belong to the shared struct dummy_hcd so they better only be initialized in one place) I got a hard lockup during testing, when I started dummy_hcd in superspeed mode and added a device. I think that now I understand why this happened, but I will read the relevant bits from dummy_hcd and xhci once more (especially what happens if there is a suspend / resume between the initialization of the highspeed hcd and the superspeed hcd) and come back with a different approach that will hopefully be both correct and DRY. Thanks a lot for the comments, they have been very helpful! Regards, Pantelis -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] usb: gadget: dummy_hcd: refactor dummy_start to be DRY
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
> ---
> 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
