[PATCH 0/5] usb: xhci: fixes for OTG/DRD use

[PATCH 0/5] usb: xhci: fixes for OTG/DRD use

[Roger Quadros]

Hi,

While testing for OTG/DRD [1], I encountered a couple of
problems with the XHCI driver.

The first 2 patches clean up the HCD allocation logic as we want
both primary and shared HCDs to be allocated before the
primary HCD registers for OTG use. That's the only way the OTG
core will know that it needs to wait for the shared HCD to
register before firing up the OTG state machine. We let the HCD
core allocate memory for the xhci private structure.

For OTG/DRD case we want usb_add/remove_hcd() to work reliably
when called repeatedly. Patches 3 and 4 fix issues with
usb_add/remove_hcd() on xhci.

The last patch fixes an issue with suspend/resume.

[1] - OTG core support
http://thread.gmane.org/gmane.linux.kernel/1911689

cheers,
-roger

Roger Quadros (5):
  usb: xhci: cleanup xhci_hcd allocation
  usb: xhci: plat: Create both HCDs before adding them
  usb: xhci: Allow usb_add/remove_hcd() to be called repeatedly
  usb: xhci: fix xhci locking up during hcd remove
  usb: xhci: Fix suspend/resume when used with OTG core

 drivers/usb/host/xhci-pci.c  | 18 +++++++++---------
 drivers/usb/host/xhci-plat.c | 43 ++++++++++++++++++++++---------------------
 drivers/usb/host/xhci-ring.c |  5 ++++-
 drivers/usb/host/xhci.c      | 43 +++++++++++++++++++++++--------------------
 drivers/usb/host/xhci.h      | 19 +++++++++++++++++--
 5 files changed, 75 insertions(+), 53 deletions(-)

-- 
2.1.0

[PATCH 1/5] usb: xhci: cleanup xhci_hcd allocation

[Roger Quadros]

HCD core allocates memory for HCD private data in
usb_create_[shared_]hcd() so make use of that
mechanism to allocate the struct xhci_hcd.

Introduce struct xhci_driver_overrides to provide
the size of HCD private data and hc_driver operation
overrides. As of now we only need to override the
reset and start methods.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/host/xhci-pci.c  | 17 ++++++++---------
 drivers/usb/host/xhci-plat.c | 18 ++++++++++--------
 drivers/usb/host/xhci.c      | 30 +++++++++++++++++-------------
 drivers/usb/host/xhci.h      | 19 +++++++++++++++++--
 4 files changed, 52 insertions(+), 32 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 7f76c8a..4ad2c14 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -42,6 +42,13 @@ static const char hcd_name[] = "xhci_hcd";
 
 static struct hc_driver __read_mostly xhci_pci_hc_driver;
 
+static int xhci_pci_setup(struct usb_hcd *hcd);
+
+static const struct xhci_driver_overrides xhci_pci_overrides __initconst = {
+	.extra_priv_size = sizeof(struct xhci_hcd),
+	.reset = xhci_pci_setup,
+};
+
 /* called after powerup, by probe or system-pm "wakeup" */
 static int xhci_pci_reinit(struct xhci_hcd *xhci, struct pci_dev *pdev)
 {
@@ -182,7 +189,6 @@ static int xhci_pci_setup(struct usb_hcd *hcd)
 	if (!retval)
 		return retval;
 
-	kfree(xhci);
 	return retval;
 }
 
@@ -223,11 +229,6 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		goto dealloc_usb2_hcd;
 	}
 
-	/* Set the xHCI pointer before xhci_pci_setup() (aka hcd_driver.reset)
-	 * is called by usb_add_hcd().
-	 */
-	*((struct xhci_hcd **) xhci->shared_hcd->hcd_priv) = xhci;
-
 	retval = usb_add_hcd(xhci->shared_hcd, dev->irq,
 			IRQF_SHARED);
 	if (retval)
@@ -266,8 +267,6 @@ static void xhci_pci_remove(struct pci_dev *dev)
 	/* Workaround for spurious wakeups at shutdown with HSW */
 	if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
 		pci_set_power_state(dev, PCI_D3hot);
-
-	kfree(xhci);
 }
 
 #ifdef CONFIG_PM
@@ -349,7 +348,7 @@ static struct pci_driver xhci_pci_driver = {
 
 static int __init xhci_pci_init(void)
 {
-	xhci_init_driver(&xhci_pci_hc_driver, xhci_pci_setup);
+	xhci_init_driver(&xhci_pci_hc_driver, &xhci_pci_overrides);
 #ifdef CONFIG_PM
 	xhci_pci_hc_driver.pci_suspend = xhci_pci_suspend;
 	xhci_pci_hc_driver.pci_resume = xhci_pci_resume;
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 08d402b..517fb4c 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -25,6 +25,15 @@
 
 static struct hc_driver __read_mostly xhci_plat_hc_driver;
 
+static int xhci_plat_setup(struct usb_hcd *hcd);
+static int xhci_plat_start(struct usb_hcd *hcd);
+
+static const struct xhci_driver_overrides xhci_plat_overrides __initconst = {
+	.extra_priv_size = sizeof(struct xhci_hcd),
+	.reset = xhci_plat_setup,
+	.start = xhci_plat_start,
+};
+
 static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
 {
 	/*
@@ -147,11 +156,6 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if ((node && of_property_read_bool(node, "usb3-lpm-capable")) ||
 			(pdata && pdata->usb3_lpm_capable))
 		xhci->quirks |= XHCI_LPM_SUPPORT;
-	/*
-	 * Set the xHCI pointer before xhci_plat_setup() (aka hcd_driver.reset)
-	 * is called by usb_add_hcd().
-	 */
-	*((struct xhci_hcd **) xhci->shared_hcd->hcd_priv) = xhci;
 
 	if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
 		xhci->shared_hcd->can_do_streams = 1;
@@ -191,7 +195,6 @@ static int xhci_plat_remove(struct platform_device *dev)
 	if (!IS_ERR(clk))
 		clk_disable_unprepare(clk);
 	usb_put_hcd(hcd);
-	kfree(xhci);
 
 	return 0;
 }
@@ -255,8 +258,7 @@ MODULE_ALIAS("platform:xhci-hcd");
 
 static int __init xhci_plat_init(void)
 {
-	xhci_init_driver(&xhci_plat_hc_driver, xhci_plat_setup);
-	xhci_plat_hc_driver.start = xhci_plat_start;
+	xhci_init_driver(&xhci_plat_hc_driver, &xhci_plat_overrides);
 	return platform_driver_register(&usb_xhci_driver);
 }
 module_init(xhci_plat_init);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index ec8ac16..01118f7 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4832,10 +4832,7 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
 	hcd->self.no_stop_on_short = 1;
 
 	if (usb_hcd_is_primary_hcd(hcd)) {
-		xhci = kzalloc(sizeof(struct xhci_hcd), GFP_KERNEL);
-		if (!xhci)
-			return -ENOMEM;
-		*((struct xhci_hcd **) hcd->hcd_priv) = xhci;
+		xhci = hcd_to_xhci(hcd);
 		xhci->main_hcd = hcd;
 		/* Mark the first roothub as being USB 2.0.
 		 * The xHCI driver will register the USB 3.0 roothub.
@@ -4883,13 +4880,13 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
 	/* Make sure the HC is halted. */
 	retval = xhci_halt(xhci);
 	if (retval)
-		goto error;
+		return retval;
 
 	xhci_dbg(xhci, "Resetting HCD\n");
 	/* Reset the internal HC memory state and registers. */
 	retval = xhci_reset(xhci);
 	if (retval)
-		goto error;
+		return retval;
 	xhci_dbg(xhci, "Reset complete\n");
 
 	/* Set dma_mask and coherent_dma_mask to 64-bits,
@@ -4904,16 +4901,13 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
 	/* Initialize HCD and host controller data structures. */
 	retval = xhci_init(hcd);
 	if (retval)
-		goto error;
+		return retval;
 	xhci_dbg(xhci, "Called HCD init\n");
 
 	xhci_info(xhci, "hcc params 0x%08x hci version 0x%x quirks 0x%08x\n",
 		  xhci->hcc_params, xhci->hci_version, xhci->quirks);
 
 	return 0;
-error:
-	kfree(xhci);
-	return retval;
 }
 EXPORT_SYMBOL_GPL(xhci_gen_setup);
 
@@ -4978,11 +4972,21 @@ static const struct hc_driver xhci_hc_driver = {
 	.find_raw_port_number =	xhci_find_raw_port_number,
 };
 
-void xhci_init_driver(struct hc_driver *drv, int (*setup_fn)(struct usb_hcd *))
+void xhci_init_driver(struct hc_driver *drv,
+		      const struct xhci_driver_overrides *over)
 {
-	BUG_ON(!setup_fn);
+	BUG_ON(!over);
+
+	/* Copy the generic table to drv then apply the overrides */
 	*drv = xhci_hc_driver;
-	drv->reset = setup_fn;
+
+	if (over) {
+		drv->hcd_priv_size += over->extra_priv_size;
+		if (over->reset)
+			drv->reset = over->reset;
+		if (over->start)
+			drv->start = over->start;
+	}
 }
 EXPORT_SYMBOL_GPL(xhci_init_driver);
 
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 9745147..0567364 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1586,10 +1586,24 @@ struct xhci_hcd {
 #define COMP_MODE_RCVRY_MSECS 2000
 };
 
+/* Platform specific overrides to generic XHCI hc_driver ops */
+struct xhci_driver_overrides {
+	size_t extra_priv_size;
+	int (*reset)(struct usb_hcd *hcd);
+	int (*start)(struct usb_hcd *hcd);
+};
+
 /* convert between an HCD pointer and the corresponding EHCI_HCD */
 static inline struct xhci_hcd *hcd_to_xhci(struct usb_hcd *hcd)
 {
-	return *((struct xhci_hcd **) (hcd->hcd_priv));
+	struct usb_hcd *primary_hcd;
+
+	if (usb_hcd_is_primary_hcd(hcd))
+		primary_hcd = hcd;
+	else
+		primary_hcd = hcd->primary_hcd;
+
+	return (struct xhci_hcd *) (primary_hcd->hcd_priv);
 }
 
 static inline struct usb_hcd *xhci_to_hcd(struct xhci_hcd *xhci)
@@ -1743,7 +1757,8 @@ int xhci_run(struct usb_hcd *hcd);
 void xhci_stop(struct usb_hcd *hcd);
 void xhci_shutdown(struct usb_hcd *hcd);
 int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks);
-void xhci_init_driver(struct hc_driver *drv, int (*setup_fn)(struct usb_hcd *));
+void xhci_init_driver(struct hc_driver *drv,
+		      const struct xhci_driver_overrides *over);
 
 #ifdef	CONFIG_PM
 int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup);
-- 
2.1.0

Re: [PATCH 1/5] usb: xhci: cleanup xhci_hcd allocation

[Mathias Nyman]

Hi

On 02.04.2015 15:23, Roger Quadros wrote:
> HCD core allocates memory for HCD private data in
> usb_create_[shared_]hcd() so make use of that
> mechanism to allocate the struct xhci_hcd.
> 
> Introduce struct xhci_driver_overrides to provide
> the size of HCD private data and hc_driver operation
> overrides. As of now we only need to override the
> reset and start methods.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>

I'm not sure I fully understand the what's going on, or what the
intention of this patch is.

So currently xhci driver manages the allocation and freeing of
the xhci_hcd structure. We store a pointer to the xhci_hcd structure in
the content of both the primary and shared usb_hcds structures hcd_priv
field.

With this patch xhci would be part of the usb_hcd structure,
starting at hcd_priv[0]. (Like EHCI I think) It allocates enough space to include
the xhci_hcd in both the primary and shared usb_hcd, but always only use the one
in the primary hcd.
I'm not sure what to do with the space allocated for the shared hcd's
hcd_priv field.

This also means that xhci goes away together with the primary hcd. It's possible
this has some impact as the xhci driver expects xhci to always exists.

-Mathias

Re: [PATCH 1/5] usb: xhci: cleanup xhci_hcd allocation

[Roger Quadros]

Hi,

On 07/04/15 17:23, Mathias Nyman wrote:
> Hi
> 
> On 02.04.2015 15:23, Roger Quadros wrote:
>> HCD core allocates memory for HCD private data in
>> usb_create_[shared_]hcd() so make use of that
>> mechanism to allocate the struct xhci_hcd.
>>
>> Introduce struct xhci_driver_overrides to provide
>> the size of HCD private data and hc_driver operation
>> overrides. As of now we only need to override the
>> reset and start methods.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> 
> I'm not sure I fully understand the what's going on, or what the
> intention of this patch is.

The main intention is to have both primary and shared HCDs allocated
before calling usb_add_hcd() for the primary hcd.
This is so that at the first usb_add_hcd() the OTG core knows the HCD topology
(i.e. whether it uses a shared HCD or not).

>From the OTG perspective we have to prevent the actual usb_add_hcd() till the
OTG state machine says so.
This means that xhci_gen_setup() won't be necessarily called immediately and
so we need to allocate for xhci somewhere else.

> 
> So currently xhci driver manages the allocation and freeing of
> the xhci_hcd structure. We store a pointer to the xhci_hcd structure in
> the content of both the primary and shared usb_hcds structures hcd_priv
> field.
> 
> With this patch xhci would be part of the usb_hcd structure,
> starting at hcd_priv[0]. (Like EHCI I think) It allocates enough space to include
> the xhci_hcd in both the primary and shared usb_hcd, but always only use the one
> in the primary hcd.

precisely.

> I'm not sure what to do with the space allocated for the shared hcd's
> hcd_priv field.

we just ignore the space allocated for the shared hcd.

> 
> This also means that xhci goes away together with the primary hcd. It's possible
> this has some impact as the xhci driver expects xhci to always exists.

Can you please point out where this impact is.

I've been testing add/remove HCD extensively and didn't observe any issues after applying
these 5 patches. Well there is one issue that comes up but it has nothing to do with xhci
not being allocated. It has more to do with command being queued after the HCD has gone away
and so getting stuck forever without timing out.

cheers,
-roger

Re: [PATCH 1/5] usb: xhci: cleanup xhci_hcd allocation

[Mathias Nyman]

Hi

On 09.04.2015 12:22, Roger Quadros wrote:
> Hi,
> 
> On 07/04/15 17:23, Mathias Nyman wrote:
>> Hi
>>
>> On 02.04.2015 15:23, Roger Quadros wrote:
>>> HCD core allocates memory for HCD private data in
>>> usb_create_[shared_]hcd() so make use of that
>>> mechanism to allocate the struct xhci_hcd.
>>>
>>> Introduce struct xhci_driver_overrides to provide
>>> the size of HCD private data and hc_driver operation
>>> overrides. As of now we only need to override the
>>> reset and start methods.
>>>
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>
>> I'm not sure I fully understand the what's going on, or what the
>> intention of this patch is.
> 
> The main intention is to have both primary and shared HCDs allocated
> before calling usb_add_hcd() for the primary hcd.
> This is so that at the first usb_add_hcd() the OTG core knows the HCD topology
> (i.e. whether it uses a shared HCD or not).
> 
> From the OTG perspective we have to prevent the actual usb_add_hcd() till the
> OTG state machine says so.
> This means that xhci_gen_setup() won't be necessarily called immediately and
> so we need to allocate for xhci somewhere else.

Ok, thanks for explaining. I now understand the reason behind this.

> 
>>
>> So currently xhci driver manages the allocation and freeing of
>> the xhci_hcd structure. We store a pointer to the xhci_hcd structure in
>> the content of both the primary and shared usb_hcds structures hcd_priv
>> field.
>>
>> With this patch xhci would be part of the usb_hcd structure,
>> starting at hcd_priv[0]. (Like EHCI I think) It allocates enough space to include
>> the xhci_hcd in both the primary and shared usb_hcd, but always only use the one
>> in the primary hcd.
> 
> precisely.
> 
>> I'm not sure what to do with the space allocated for the shared hcd's
>> hcd_priv field.
> 
> we just ignore the space allocated for the shared hcd.

Ok, not a big loss.

> 
>>
>> This also means that xhci goes away together with the primary hcd. It's possible
>> this has some impact as the xhci driver expects xhci to always exists.
> 
> Can you please point out where this impact is.
> 
> I've been testing add/remove HCD extensively and didn't observe any issues after applying
> these 5 patches. Well there is one issue that comes up but it has nothing to do with xhci
> not being allocated. It has more to do with command being queued after the HCD has gone away
> and so getting stuck forever without timing out.

I went through the codepaths and you're right, should work fine. My concern wasn't valid. 
This patchset doesn't even touch the order how primary and shared HCDs are created and added
in the PCI case, only for the platform device case.   

I'll try it out and send forward once rc1 is out.

-Mathias

Re: [PATCH 1/5] usb: xhci: cleanup xhci_hcd allocation

[Roger Quadros]

On 13/04/15 15:48, Mathias Nyman wrote:
> Hi
> 
> On 09.04.2015 12:22, Roger Quadros wrote:
>> Hi,
>>
>> On 07/04/15 17:23, Mathias Nyman wrote:
>>> Hi
>>>
>>> On 02.04.2015 15:23, Roger Quadros wrote:
>>>> HCD core allocates memory for HCD private data in
>>>> usb_create_[shared_]hcd() so make use of that
>>>> mechanism to allocate the struct xhci_hcd.
>>>>
>>>> Introduce struct xhci_driver_overrides to provide
>>>> the size of HCD private data and hc_driver operation
>>>> overrides. As of now we only need to override the
>>>> reset and start methods.
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>
>>> I'm not sure I fully understand the what's going on, or what the
>>> intention of this patch is.
>>
>> The main intention is to have both primary and shared HCDs allocated
>> before calling usb_add_hcd() for the primary hcd.
>> This is so that at the first usb_add_hcd() the OTG core knows the HCD topology
>> (i.e. whether it uses a shared HCD or not).
>>
>> From the OTG perspective we have to prevent the actual usb_add_hcd() till the
>> OTG state machine says so.
>> This means that xhci_gen_setup() won't be necessarily called immediately and
>> so we need to allocate for xhci somewhere else.
> 
> Ok, thanks for explaining. I now understand the reason behind this.
> 
>>
>>>
>>> So currently xhci driver manages the allocation and freeing of
>>> the xhci_hcd structure. We store a pointer to the xhci_hcd structure in
>>> the content of both the primary and shared usb_hcds structures hcd_priv
>>> field.
>>>
>>> With this patch xhci would be part of the usb_hcd structure,
>>> starting at hcd_priv[0]. (Like EHCI I think) It allocates enough space to include
>>> the xhci_hcd in both the primary and shared usb_hcd, but always only use the one
>>> in the primary hcd.
>>
>> precisely.
>>
>>> I'm not sure what to do with the space allocated for the shared hcd's
>>> hcd_priv field.
>>
>> we just ignore the space allocated for the shared hcd.
> 
> Ok, not a big loss.
> 
>>
>>>
>>> This also means that xhci goes away together with the primary hcd. It's possible
>>> this has some impact as the xhci driver expects xhci to always exists.
>>
>> Can you please point out where this impact is.
>>
>> I've been testing add/remove HCD extensively and didn't observe any issues after applying
>> these 5 patches. Well there is one issue that comes up but it has nothing to do with xhci
>> not being allocated. It has more to do with command being queued after the HCD has gone away
>> and so getting stuck forever without timing out.
> 
> I went through the codepaths and you're right, should work fine. My concern wasn't valid. 
> This patchset doesn't even touch the order how primary and shared HCDs are created and added
> in the PCI case, only for the platform device case.   

I was not very sure how to do it for the PCI case.
usb_hcd_pci_probe() wants to create and add the hcd. We need something else to split
the create_hcd() and add_hcd() operations.

> 
> I'll try it out and send forward once rc1 is out.

Thanks.

cheers,
-roger

Re: [PATCH 1/5] usb: xhci: cleanup xhci_hcd allocation

[Roger Quadros]

Hi Mathias,

On 13/04/15 15:48, Mathias Nyman wrote:
> Hi
>
> On 09.04.2015 12:22, Roger Quadros wrote:
>> Hi,
>>
>> On 07/04/15 17:23, Mathias Nyman wrote:
>>> Hi
>>>
>>> On 02.04.2015 15:23, Roger Quadros wrote:
>>>> HCD core allocates memory for HCD private data in
>>>> usb_create_[shared_]hcd() so make use of that
>>>> mechanism to allocate the struct xhci_hcd.
>>>>
>>>> Introduce struct xhci_driver_overrides to provide
>>>> the size of HCD private data and hc_driver operation
>>>> overrides. As of now we only need to override the
>>>> reset and start methods.
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>
>>> I'm not sure I fully understand the what's going on, or what the
>>> intention of this patch is.
>>
>> The main intention is to have both primary and shared HCDs allocated
>> before calling usb_add_hcd() for the primary hcd.
>> This is so that at the first usb_add_hcd() the OTG core knows the HCD topology
>> (i.e. whether it uses a shared HCD or not).
>>
>>  From the OTG perspective we have to prevent the actual usb_add_hcd() till the
>> OTG state machine says so.
>> This means that xhci_gen_setup() won't be necessarily called immediately and
>> so we need to allocate for xhci somewhere else.
>
> Ok, thanks for explaining. I now understand the reason behind this.
>
>>
>>>
>>> So currently xhci driver manages the allocation and freeing of
>>> the xhci_hcd structure. We store a pointer to the xhci_hcd structure in
>>> the content of both the primary and shared usb_hcds structures hcd_priv
>>> field.
>>>
>>> With this patch xhci would be part of the usb_hcd structure,
>>> starting at hcd_priv[0]. (Like EHCI I think) It allocates enough space to include
>>> the xhci_hcd in both the primary and shared usb_hcd, but always only use the one
>>> in the primary hcd.
>>
>> precisely.
>>
>>> I'm not sure what to do with the space allocated for the shared hcd's
>>> hcd_priv field.
>>
>> we just ignore the space allocated for the shared hcd.
>
> Ok, not a big loss.
>
>>
>>>
>>> This also means that xhci goes away together with the primary hcd. It's possible
>>> this has some impact as the xhci driver expects xhci to always exists.
>>
>> Can you please point out where this impact is.
>>
>> I've been testing add/remove HCD extensively and didn't observe any issues after applying
>> these 5 patches. Well there is one issue that comes up but it has nothing to do with xhci
>> not being allocated. It has more to do with command being queued after the HCD has gone away
>> and so getting stuck forever without timing out.
>
> I went through the codepaths and you're right, should work fine. My concern wasn't valid.
> This patchset doesn't even touch the order how primary and shared HCDs are created and added
> in the PCI case, only for the platform device case.
>
> I'll try it out and send forward once rc1 is out.

did you get a chance to try this series?

cheers,
-roger

Re: [PATCH 1/5] usb: xhci: cleanup xhci_hcd allocation

[Mathias Nyman]

On 11.05.2015 17:18, Roger Quadros wrote:
> Hi Mathias,
> 
> On 13/04/15 15:48, Mathias Nyman wrote:
>> Hi
>>
>> On 09.04.2015 12:22, Roger Quadros wrote:
>>> Hi,
>>>
>>> On 07/04/15 17:23, Mathias Nyman wrote:
>>>> Hi
>>>>
>>>> On 02.04.2015 15:23, Roger Quadros wrote:
>>>>> HCD core allocates memory for HCD private data in
>>>>> usb_create_[shared_]hcd() so make use of that
>>>>> mechanism to allocate the struct xhci_hcd.
>>>>>
>>>>> Introduce struct xhci_driver_overrides to provide
>>>>> the size of HCD private data and hc_driver operation
>>>>> overrides. As of now we only need to override the
>>>>> reset and start methods.
>>>>>
>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>
>>>> I'm not sure I fully understand the what's going on, or what the
>>>> intention of this patch is.
>>>
>>> The main intention is to have both primary and shared HCDs allocated
>>> before calling usb_add_hcd() for the primary hcd.
>>> This is so that at the first usb_add_hcd() the OTG core knows the HCD topology
>>> (i.e. whether it uses a shared HCD or not).
>>>
>>>  From the OTG perspective we have to prevent the actual usb_add_hcd() till the
>>> OTG state machine says so.
>>> This means that xhci_gen_setup() won't be necessarily called immediately and
>>> so we need to allocate for xhci somewhere else.
>>
>> Ok, thanks for explaining. I now understand the reason behind this.
>>
>>>
>>>>
>>>> So currently xhci driver manages the allocation and freeing of
>>>> the xhci_hcd structure. We store a pointer to the xhci_hcd structure in
>>>> the content of both the primary and shared usb_hcds structures hcd_priv
>>>> field.
>>>>
>>>> With this patch xhci would be part of the usb_hcd structure,
>>>> starting at hcd_priv[0]. (Like EHCI I think) It allocates enough space to include
>>>> the xhci_hcd in both the primary and shared usb_hcd, but always only use the one
>>>> in the primary hcd.
>>>
>>> precisely.
>>>
>>>> I'm not sure what to do with the space allocated for the shared hcd's
>>>> hcd_priv field.
>>>
>>> we just ignore the space allocated for the shared hcd.
>>
>> Ok, not a big loss.
>>
>>>
>>>>
>>>> This also means that xhci goes away together with the primary hcd. It's possible
>>>> this has some impact as the xhci driver expects xhci to always exists.
>>>
>>> Can you please point out where this impact is.
>>>
>>> I've been testing add/remove HCD extensively and didn't observe any issues after applying
>>> these 5 patches. Well there is one issue that comes up but it has nothing to do with xhci
>>> not being allocated. It has more to do with command being queued after the HCD has gone away
>>> and so getting stuck forever without timing out.
>>
>> I went through the codepaths and you're right, should work fine. My concern wasn't valid.
>> This patchset doesn't even touch the order how primary and shared HCDs are created and added
>> in the PCI case, only for the platform device case.
>>
>> I'll try it out and send forward once rc1 is out.
> 
> did you get a chance to try this series?
> 

Sorry, not yet, got delayed by other internal tasks.
I'll try it out as soon as possible.

-Mathias

Re: [PATCH 1/5] usb: xhci: cleanup xhci_hcd allocation

[Mathias Nyman]

>>>>
>>>> I've been testing add/remove HCD extensively and didn't observe any issues after applying
>>>> these 5 patches. Well there is one issue that comes up but it has nothing to do with xhci
>>>> not being allocated. It has more to do with command being queued after the HCD has gone away
>>>> and so getting stuck forever without timing out.
>>>
>>> I went through the codepaths and you're right, should work fine. My concern wasn't valid.
>>> This patchset doesn't even touch the order how primary and shared HCDs are created and added
>>> in the PCI case, only for the platform device case.
>>>
>>> I'll try it out and send forward once rc1 is out.
>>
>> did you get a chance to try this series?
>>
> 
> Sorry, not yet, got delayed by other internal tasks.
> I'll try it out as soon as possible.
> 
>

Ok, back to this, I'd like to get both this series and Andrew's xhci-tegra
support to 4.2

I did similar changes to xhci-tegra.c as Roger did to xhci-pci.c and xhci-plat.c,
but I can't test them and would need both your eyes on this to make sure it looks ok.

Both series are in a tegra_otg_merge topic branch in:
  
git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git tegra_otg_merge

It contains Andrews full xhci tegra support series, but If I understood correctly only
patch 9/9 (maybe 8/9 as well?) will actually go through the xhci tree. 
Patch 1/9 is not needed with Roger's changes anymre  

The changes are in the last patch, here:
https://git.kernel.org/cgit/linux/kernel/git/mnyman/xhci.git/commit/?h=tegra_otg_merge

and would be squashed together with patch 9/9.

Thanks
Mathias 

Re: [PATCH 1/5] usb: xhci: cleanup xhci_hcd allocation

[Andrew Bresticker]

Hi Mathias,

On Mon, May 25, 2015 at 8:05 AM, Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
>>>>>
>>>>> I've been testing add/remove HCD extensively and didn't observe any issues after applying
>>>>> these 5 patches. Well there is one issue that comes up but it has nothing to do with xhci
>>>>> not being allocated. It has more to do with command being queued after the HCD has gone away
>>>>> and so getting stuck forever without timing out.
>>>>
>>>> I went through the codepaths and you're right, should work fine. My concern wasn't valid.
>>>> This patchset doesn't even touch the order how primary and shared HCDs are created and added
>>>> in the PCI case, only for the platform device case.
>>>>
>>>> I'll try it out and send forward once rc1 is out.
>>>
>>> did you get a chance to try this series?
>>>
>>
>> Sorry, not yet, got delayed by other internal tasks.
>> I'll try it out as soon as possible.
>>
>>
>
> Ok, back to this, I'd like to get both this series and Andrew's xhci-tegra
> support to 4.2
>
> I did similar changes to xhci-tegra.c as Roger did to xhci-pci.c and xhci-plat.c,
> but I can't test them and would need both your eyes on this to make sure it looks ok.
>
> Both series are in a tegra_otg_merge topic branch in:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git tegra_otg_merge
>
> It contains Andrews full xhci tegra support series, but If I understood correctly only
> patch 9/9 (maybe 8/9 as well?) will actually go through the xhci tree.
> Patch 1/9 is not needed with Roger's changes anymre
>
> The changes are in the last patch, here:
> https://git.kernel.org/cgit/linux/kernel/git/mnyman/xhci.git/commit/?h=tegra_otg_merge
>
> and would be squashed together with patch 9/9.

No need to apply patch 8/9 or 9/9... Lee Jones has NAK'ed the MFD
driver on which the xhci-tegra driver depends, so I'll need to re-spin
the series.  It would be great if you could ACK those two patches
though so that the entire series could go through the Tegra tree for
4.3.

Thanks,
Andrew

[PATCH 2/5] usb: xhci: plat: Create both HCDs before adding them

[Roger Quadros]

As xhci_hcd is now allocated by usb_create_hcd(), we don't
need to add the primary HCD before creating the shared HCD.

Creating the shared HCD before adding the primary HCD is particularly
useful for the OTG use case so that we know at the OTG core if
the HCD is in single configuration or dual (primary + shared)
configuration.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/host/xhci-plat.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 517fb4c..00f23f5 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -136,21 +136,15 @@ static int xhci_plat_probe(struct platform_device *pdev)
 			goto put_hcd;
 	}
 
-	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
-	if (ret)
-		goto disable_clk;
-
 	device_wakeup_enable(hcd->self.controller);
 
-	/* USB 2.0 roothub is stored in the platform_device now. */
-	hcd = platform_get_drvdata(pdev);
 	xhci = hcd_to_xhci(hcd);
 	xhci->clk = clk;
 	xhci->shared_hcd = usb_create_shared_hcd(driver, &pdev->dev,
 			dev_name(&pdev->dev), hcd);
 	if (!xhci->shared_hcd) {
 		ret = -ENOMEM;
-		goto dealloc_usb2_hcd;
+		goto disable_clk;
 	}
 
 	if ((node && of_property_read_bool(node, "usb3-lpm-capable")) ||
@@ -160,18 +154,22 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
 		xhci->shared_hcd->can_do_streams = 1;
 
-	ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
+	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
 	if (ret)
 		goto put_usb3_hcd;
 
-	return 0;
+	ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
+	if (ret)
+		goto dealloc_usb2_hcd;
 
-put_usb3_hcd:
-	usb_put_hcd(xhci->shared_hcd);
+	return 0;
 
 dealloc_usb2_hcd:
 	usb_remove_hcd(hcd);
 
+put_usb3_hcd:
+	usb_put_hcd(xhci->shared_hcd);
+
 disable_clk:
 	if (!IS_ERR(clk))
 		clk_disable_unprepare(clk);
@@ -189,9 +187,9 @@ static int xhci_plat_remove(struct platform_device *dev)
 	struct clk *clk = xhci->clk;
 
 	usb_remove_hcd(xhci->shared_hcd);
-	usb_put_hcd(xhci->shared_hcd);
-
 	usb_remove_hcd(hcd);
+
+	usb_put_hcd(xhci->shared_hcd);
 	if (!IS_ERR(clk))
 		clk_disable_unprepare(clk);
 	usb_put_hcd(hcd);
-- 
2.1.0

Re: [PATCH 2/5] usb: xhci: plat: Create both HCDs before adding them

[Mathias Nyman]

Hi

On 02.04.2015 15:23, Roger Quadros wrote:
> As xhci_hcd is now allocated by usb_create_hcd(), we don't
> need to add the primary HCD before creating the shared HCD.
> 
> Creating the shared HCD before adding the primary HCD is particularly
> useful for the OTG use case so that we know at the OTG core if
> the HCD is in single configuration or dual (primary + shared)
> configuration.
> 

This doesn't apply as 

commit 7b8ef22ea547b80475ac7feab06fb15e0fc143d8
    usb: xhci: plat: Add USB phy support

changed xhci-plat.c since.

I rebased it, and the changed version is sitting in the for-usb-next branch in:
git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git

But it appeared to me that usb_add_hcd() and usb_remove_hcd() will also
call phy init and remove functions. As the order how hcds are created and added
would change I'd need some more eyes on this to see if it will cause regression.

Or maybe in the best case we could get rid of the "Add USB phy support" patch as
we will call xhci_add_hcd() for the first hcd much later, and it could maybe init
the phy for us?

I don't have a board that enumerates xhci using xhci-plat.c myself. 

-Mathias

Re: [PATCH 2/5] usb: xhci: plat: Create both HCDs before adding them

[Roger Quadros]

On 20/04/15 15:35, Mathias Nyman wrote:
> Hi
> 
> On 02.04.2015 15:23, Roger Quadros wrote:
>> As xhci_hcd is now allocated by usb_create_hcd(), we don't
>> need to add the primary HCD before creating the shared HCD.
>>
>> Creating the shared HCD before adding the primary HCD is particularly
>> useful for the OTG use case so that we know at the OTG core if
>> the HCD is in single configuration or dual (primary + shared)
>> configuration.
>>
> 
> This doesn't apply as 
> 
> commit 7b8ef22ea547b80475ac7feab06fb15e0fc143d8
>     usb: xhci: plat: Add USB phy support
> 
> changed xhci-plat.c since.
> 
> I rebased it, and the changed version is sitting in the for-usb-next branch in:
> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git
> 
> But it appeared to me that usb_add_hcd() and usb_remove_hcd() will also
> call phy init and remove functions. As the order how hcds are created and added
> would change I'd need some more eyes on this to see if it will cause regression.
> 
> Or maybe in the best case we could get rid of the "Add USB phy support" patch as
> we will call xhci_add_hcd() for the first hcd much later, and it could maybe init
> the phy for us?

I thought usb_phy_*() stuff would be deprecated and we should use phy framework
instead i.e. phy_init() and friends.

In fact usb_add_hcd() is already handling the phy for us.
So I'm in favor of getting rid of commit 7b8ef22ea547b80475ac7feab06fb15e0fc143d8
(usb: xhci: plat: Add USB phy support)

cheers,
-roger

Re: [PATCH 2/5] usb: xhci: plat: Create both HCDs before adding them

[Roger Quadros]

fixed Kishon's id.

On 21/04/15 12:49, Roger Quadros wrote:
> On 20/04/15 15:35, Mathias Nyman wrote:
>> Hi
>>
>> On 02.04.2015 15:23, Roger Quadros wrote:
>>> As xhci_hcd is now allocated by usb_create_hcd(), we don't
>>> need to add the primary HCD before creating the shared HCD.
>>>
>>> Creating the shared HCD before adding the primary HCD is particularly
>>> useful for the OTG use case so that we know at the OTG core if
>>> the HCD is in single configuration or dual (primary + shared)
>>> configuration.
>>>
>>
>> This doesn't apply as 
>>
>> commit 7b8ef22ea547b80475ac7feab06fb15e0fc143d8
>>     usb: xhci: plat: Add USB phy support
>>
>> changed xhci-plat.c since.
>>
>> I rebased it, and the changed version is sitting in the for-usb-next branch in:
>> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git
>>
>> But it appeared to me that usb_add_hcd() and usb_remove_hcd() will also
>> call phy init and remove functions. As the order how hcds are created and added
>> would change I'd need some more eyes on this to see if it will cause regression.
>>
>> Or maybe in the best case we could get rid of the "Add USB phy support" patch as
>> we will call xhci_add_hcd() for the first hcd much later, and it could maybe init
>> the phy for us?
> 
> I thought usb_phy_*() stuff would be deprecated and we should use phy framework
> instead i.e. phy_init() and friends.
> 
> In fact usb_add_hcd() is already handling the phy for us.
> So I'm in favor of getting rid of commit 7b8ef22ea547b80475ac7feab06fb15e0fc143d8
> (usb: xhci: plat: Add USB phy support)
> 
> cheers,
> -roger
> 

Re: [PATCH 2/5] usb: xhci: plat: Create both HCDs before adding them

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1816 bytes --]

On Tue, Apr 21, 2015 at 12:49:54PM +0300, Roger Quadros wrote:
> On 20/04/15 15:35, Mathias Nyman wrote:
> > Hi
> > 
> > On 02.04.2015 15:23, Roger Quadros wrote:
> >> As xhci_hcd is now allocated by usb_create_hcd(), we don't
> >> need to add the primary HCD before creating the shared HCD.
> >>
> >> Creating the shared HCD before adding the primary HCD is particularly
> >> useful for the OTG use case so that we know at the OTG core if
> >> the HCD is in single configuration or dual (primary + shared)
> >> configuration.
> >>
> > 
> > This doesn't apply as 
> > 
> > commit 7b8ef22ea547b80475ac7feab06fb15e0fc143d8
> >     usb: xhci: plat: Add USB phy support
> > 
> > changed xhci-plat.c since.
> > 
> > I rebased it, and the changed version is sitting in the for-usb-next branch in:
> > git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git
> > 
> > But it appeared to me that usb_add_hcd() and usb_remove_hcd() will also
> > call phy init and remove functions. As the order how hcds are created and added
> > would change I'd need some more eyes on this to see if it will cause regression.
> > 
> > Or maybe in the best case we could get rid of the "Add USB phy support" patch as
> > we will call xhci_add_hcd() for the first hcd much later, and it could maybe init
> > the phy for us?
> 
> I thought usb_phy_*() stuff would be deprecated and we should use
> phy framework instead i.e. phy_init() and friends.

Except that all drivers have not been converted yet... So it's not
really an option until then.

> In fact usb_add_hcd() is already handling the phy for us.

If it handles USB phy, then I don't really have an issue with it.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/5] usb: xhci: plat: Create both HCDs before adding them

[Roger Quadros]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 21/04/15 11:08, Maxime Ripard wrote:
> On Tue, Apr 21, 2015 at 12:49:54PM +0300, Roger Quadros wrote:
>> On 20/04/15 15:35, Mathias Nyman wrote:
>>> Hi
>>>
>>> On 02.04.2015 15:23, Roger Quadros wrote:
>>>> As xhci_hcd is now allocated by usb_create_hcd(), we don't
>>>> need to add the primary HCD before creating the shared HCD.
>>>>
>>>> Creating the shared HCD before adding the primary HCD is particularly
>>>> useful for the OTG use case so that we know at the OTG core if
>>>> the HCD is in single configuration or dual (primary + shared)
>>>> configuration.
>>>>
>>>
>>> This doesn't apply as 
>>>
>>> commit 7b8ef22ea547b80475ac7feab06fb15e0fc143d8
>>>     usb: xhci: plat: Add USB phy support
>>>
>>> changed xhci-plat.c since.
>>>
>>> I rebased it, and the changed version is sitting in the for-usb-next branch in:
>>> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git
>>>
>>> But it appeared to me that usb_add_hcd() and usb_remove_hcd() will also
>>> call phy init and remove functions. As the order how hcds are created and added
>>> would change I'd need some more eyes on this to see if it will cause regression.
>>>
>>> Or maybe in the best case we could get rid of the "Add USB phy support" patch as
>>> we will call xhci_add_hcd() for the first hcd much later, and it could maybe init
>>> the phy for us?
>>
>> I thought usb_phy_*() stuff would be deprecated and we should use
>> phy framework instead i.e. phy_init() and friends.
> 
> Except that all drivers have not been converted yet... So it's not
> really an option until then.
> 
>> In fact usb_add_hcd() is already handling the phy for us.
> 
> If it handles USB phy, then I don't really have an issue with it.

It handles usb_phy as well. It just expects hcd->usb_phy to be set.

cheers,
- -roger
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJVNiqDAAoJENJaa9O+djCTwy0QAMCz+99lbMbp9feC7wVOThgd
MNjFEpWYMS+Cz0nlwoLQB+uV46WRclR5HNfHCJAclcfp/O+iNXmxrRgAmCVqdzAd
d+54nquoWV4IJy/YdJ1GCiG7IDpEYBjJ2Cp99ZAUtqx3KchR6yVHH8CpM0738X9+
SZj9x2xqNeX29UaOIQtRRfoMB2g7SZjJqEa/sILXptVSVvVSbWnyFpKVelQD/MpX
yIyFVapFvZ4podkB09YryHTj0xqo6VXTy4ngk9BTtO0Cf9f3Yea6G/hTJbCJ8WZA
cyYaO0qJyMKCNQDYvabQ3UZk9IG7XPs//qDnmtzYMdZIJ9piBgQN8gw1cSrv/txi
FVKdKSuR2lNDAB0XaRFnB9WIbpEr4Q/OKkTFKT6Cu7T1iMkTMrlartpboiWpRXQJ
B4lz9sxiNGcYThOfXqdwQX0s62RZNwfOYA4qX61DJwttYiEyuagXdR/WtfkkAJ6C
HYN5zEQhw+YkBuU+28+ulnJW4057nNAYUD2qbXJ7Dr7D7ThX8PkUYZu5D0BFjrF6
cIBksnbmgWuoUdBYtTDlYRbwGv/iZ/NnBZFE0xMqzNMKM7j7YQR/BkS2fbHZw+lk
RvG5XUhpCeJ7olW/xgxSNmH9tSIxkRLHNJZ4xcmpjR1Ba3QRYDG22GhEX+pX9bcy
0TcyxgHvprTGH5OFZuHX
=OA20
-----END PGP SIGNATURE-----

Re: [PATCH 2/5] usb: xhci: plat: Create both HCDs before adding them

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2112 bytes --]

On Tue, Apr 21, 2015 at 01:46:36PM +0300, Roger Quadros wrote:
> On 21/04/15 11:08, Maxime Ripard wrote:
> > On Tue, Apr 21, 2015 at 12:49:54PM +0300, Roger Quadros wrote:
> >> On 20/04/15 15:35, Mathias Nyman wrote:
> >>> Hi
> >>>
> >>> On 02.04.2015 15:23, Roger Quadros wrote:
> >>>> As xhci_hcd is now allocated by usb_create_hcd(), we don't
> >>>> need to add the primary HCD before creating the shared HCD.
> >>>>
> >>>> Creating the shared HCD before adding the primary HCD is particularly
> >>>> useful for the OTG use case so that we know at the OTG core if
> >>>> the HCD is in single configuration or dual (primary + shared)
> >>>> configuration.
> >>>>
> >>>
> >>> This doesn't apply as 
> >>>
> >>> commit 7b8ef22ea547b80475ac7feab06fb15e0fc143d8
> >>>     usb: xhci: plat: Add USB phy support
> >>>
> >>> changed xhci-plat.c since.
> >>>
> >>> I rebased it, and the changed version is sitting in the for-usb-next branch in:
> >>> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git
> >>>
> >>> But it appeared to me that usb_add_hcd() and usb_remove_hcd() will also
> >>> call phy init and remove functions. As the order how hcds are created and added
> >>> would change I'd need some more eyes on this to see if it will cause regression.
> >>>
> >>> Or maybe in the best case we could get rid of the "Add USB phy support" patch as
> >>> we will call xhci_add_hcd() for the first hcd much later, and it could maybe init
> >>> the phy for us?
> >>
> >> I thought usb_phy_*() stuff would be deprecated and we should use
> >> phy framework instead i.e. phy_init() and friends.
> > 
> > Except that all drivers have not been converted yet... So it's not
> > really an option until then.
> > 
> >> In fact usb_add_hcd() is already handling the phy for us.
> > 
> > If it handles USB phy, then I don't really have an issue with it.
> 
> It handles usb_phy as well. It just expects hcd->usb_phy to be set.

Ok, perfect then.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 3/5] usb: xhci: Allow usb_add/remove_hcd() to be called repeatedly

[Roger Quadros]

Don't set xhci->shared_hcd to NULL in xhci_stop() as we have
still not de-allocated it. It was resulting in a NULL pointer
de-reference if usb_add/remove_hcd() is called repeatedly.

We want repeated add/remove to work for the OTG use case.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/host/xhci.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 01118f7..8a7e319 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -660,12 +660,6 @@ static void xhci_only_stop_hcd(struct usb_hcd *hcd)
 
 	spin_lock_irq(&xhci->lock);
 	xhci_halt(xhci);
-
-	/* The shared_hcd is going to be deallocated shortly (the USB core only
-	 * calls this function when allocation fails in usb_add_hcd(), or
-	 * usb_remove_hcd() is called).  So we need to unset xHCI's pointer.
-	 */
-	xhci->shared_hcd = NULL;
 	spin_unlock_irq(&xhci->lock);
 }
 
-- 
2.1.0

[PATCH 4/5] usb: xhci: fix xhci locking up during hcd remove

[Roger Quadros]

The problem seems to be that if a new device is detected
while we have already removed the shared HCD, then many of the
xhci operations (e.g.  xhci_alloc_dev(), xhci_setup_device())
hang as command never completes.

I don't think XHCI can operate without the shared HCD as we've
already called xhci_halt() in xhci_only_stop_hcd() when shared HCD
goes away. We need to prevent new commands from being queued
not only when HCD is dying but also when HCD is halted.

The following lockup was detected while testing the otg state
machine.

[  178.199951] xhci-hcd xhci-hcd.0.auto: xHCI Host Controller
[  178.205799] xhci-hcd xhci-hcd.0.auto: new USB bus registered, assigned bus number 1
[  178.214458] xhci-hcd xhci-hcd.0.auto: hcc params 0x0220f04c hci version 0x100 quirks 0x00010010
[  178.223619] xhci-hcd xhci-hcd.0.auto: irq 400, io mem 0x48890000
[  178.230677] usb usb1: New USB device found, idVendor=1d6b, idProduct=0002
[  178.237796] usb usb1: New USB device strings: Mfr=3, Product=2, SerialNumber=1
[  178.245358] usb usb1: Product: xHCI Host Controller
[  178.250483] usb usb1: Manufacturer: Linux 4.0.0-rc1-00024-g6111320 xhci-hcd
[  178.257783] usb usb1: SerialNumber: xhci-hcd.0.auto
[  178.267014] hub 1-0:1.0: USB hub found
[  178.272108] hub 1-0:1.0: 1 port detected
[  178.278371] xhci-hcd xhci-hcd.0.auto: xHCI Host Controller
[  178.284171] xhci-hcd xhci-hcd.0.auto: new USB bus registered, assigned bus number 2
[  178.294038] usb usb2: New USB device found, idVendor=1d6b, idProduct=0003
[  178.301183] usb usb2: New USB device strings: Mfr=3, Product=2, SerialNumber=1
[  178.308776] usb usb2: Product: xHCI Host Controller
[  178.313902] usb usb2: Manufacturer: Linux 4.0.0-rc1-00024-g6111320 xhci-hcd
[  178.321222] usb usb2: SerialNumber: xhci-hcd.0.auto
[  178.329061] hub 2-0:1.0: USB hub found
[  178.333126] hub 2-0:1.0: 1 port detected
[  178.567585] dwc3 48890000.usb: usb_otg_start_host 0
[  178.572707] xhci-hcd xhci-hcd.0.auto: remove, state 4
[  178.578064] usb usb2: USB disconnect, device number 1
[  178.586565] xhci-hcd xhci-hcd.0.auto: USB bus 2 deregistered
[  178.592585] xhci-hcd xhci-hcd.0.auto: remove, state 1
[  178.597924] usb usb1: USB disconnect, device number 1
[  178.603248] usb 1-1: new high-speed USB device number 2 using xhci-hcd
[  190.597337] INFO: task kworker/u4:0:6 blocked for more than 10 seconds.
[  190.604273]       Not tainted 4.0.0-rc1-00024-g6111320 #1058
[  190.610228] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[  190.618443] kworker/u4:0    D c05c0ac0     0     6      2 0x00000000
[  190.625120] Workqueue: usb_otg usb_otg_work
[  190.629533] [<c05c0ac0>] (__schedule) from [<c05c10ac>] (schedule+0x34/0x98)
[  190.636915] [<c05c10ac>] (schedule) from [<c05c1318>] (schedule_preempt_disabled+0xc/0x10)
[  190.645591] [<c05c1318>] (schedule_preempt_disabled) from [<c05c23d0>] (mutex_lock_nested+0x1ac/0x3fc)
[  190.655353] [<c05c23d0>] (mutex_lock_nested) from [<c046cf8c>] (usb_disconnect+0x3c/0x208)
[  190.664043] [<c046cf8c>] (usb_disconnect) from [<c0470cf0>] (_usb_remove_hcd+0x98/0x1d8)
[  190.672535] [<c0470cf0>] (_usb_remove_hcd) from [<c0485da8>] (usb_otg_start_host+0x50/0xf4)
[  190.681299] [<c0485da8>] (usb_otg_start_host) from [<c04849a4>] (otg_set_protocol+0x5c/0xd0)
[  190.690153] [<c04849a4>] (otg_set_protocol) from [<c0484b88>] (otg_set_state+0x170/0xbfc)
[  190.698735] [<c0484b88>] (otg_set_state) from [<c0485740>] (otg_statemachine+0x12c/0x470)
[  190.707326] [<c0485740>] (otg_statemachine) from [<c0053c84>] (process_one_work+0x1b4/0x4a0)
[  190.716162] [<c0053c84>] (process_one_work) from [<c00540f8>] (worker_thread+0x154/0x44c)
[  190.724742] [<c00540f8>] (worker_thread) from [<c0058f88>] (kthread+0xd4/0xf0)
[  190.732328] [<c0058f88>] (kthread) from [<c000e810>] (ret_from_fork+0x14/0x24)
[  190.739898] 5 locks held by kworker/u4:0/6:
[  190.744274]  #0:  ("%s""usb_otg"){.+.+.+}, at: [<c0053bf4>] process_one_work+0x124/0x4a0
[  190.752799]  #1:  ((&otgd->work)){+.+.+.}, at: [<c0053bf4>] process_one_work+0x124/0x4a0
[  190.761326]  #2:  (&otgd->fsm.lock){+.+.+.}, at: [<c048562c>] otg_statemachine+0x18/0x470
[  190.769934]  #3:  (usb_bus_list_lock){+.+.+.}, at: [<c0470ce8>] _usb_remove_hcd+0x90/0x1d8
[  190.778635]  #4:  (&dev->mutex){......}, at: [<c046cf8c>] usb_disconnect+0x3c/0x208
[  190.786700] INFO: task kworker/1:0:14 blocked for more than 10 seconds.
[  190.793633]       Not tainted 4.0.0-rc1-00024-g6111320 #1058
[  190.799567] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[  190.807783] kworker/1:0     D c05c0ac0     0    14      2 0x00000000
[  190.814457] Workqueue: usb_hub_wq hub_event
[  190.818866] [<c05c0ac0>] (__schedule) from [<c05c10ac>] (schedule+0x34/0x98)
[  190.826252] [<c05c10ac>] (schedule) from [<c05c4e40>] (schedule_timeout+0x13c/0x1ec)
[  190.834377] [<c05c4e40>] (schedule_timeout) from [<c05c19f0>] (wait_for_common+0xbc/0x150)
[  190.843062] [<c05c19f0>] (wait_for_common) from [<bf068a3c>] (xhci_setup_device+0x164/0x5cc [xhci_hcd])
[  190.852986] [<bf068a3c>] (xhci_setup_device [xhci_hcd]) from [<c046b7f4>] (hub_port_init+0x3f4/0xb10)
[  190.862667] [<c046b7f4>] (hub_port_init) from [<c046eb64>] (hub_event+0x704/0x1018)
[  190.870704] [<c046eb64>] (hub_event) from [<c0053c84>] (process_one_work+0x1b4/0x4a0)
[  190.878919] [<c0053c84>] (process_one_work) from [<c00540f8>] (worker_thread+0x154/0x44c)
[  190.887503] [<c00540f8>] (worker_thread) from [<c0058f88>] (kthread+0xd4/0xf0)
[  190.895076] [<c0058f88>] (kthread) from [<c000e810>] (ret_from_fork+0x14/0x24)
[  190.902650] 5 locks held by kworker/1:0/14:
[  190.907023]  #0:  ("usb_hub_wq"){.+.+.+}, at: [<c0053bf4>] process_one_work+0x124/0x4a0
[  190.915454]  #1:  ((&hub->events)){+.+.+.}, at: [<c0053bf4>] process_one_work+0x124/0x4a0
[  190.924070]  #2:  (&dev->mutex){......}, at: [<c046e490>] hub_event+0x30/0x1018
[  190.931768]  #3:  (&port_dev->status_lock){+.+.+.}, at: [<c046eb50>] hub_event+0x6f0/0x1018
[  190.940558]  #4:  (&bus->usb_address0_mutex){+.+.+.}, at: [<c046b458>] hub_port_init+0x58/0xb10

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/host/xhci-ring.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 88da8d6..7c49dfe 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3770,8 +3770,11 @@ static int queue_command(struct xhci_hcd *xhci, struct xhci_command *cmd,
 {
 	int reserved_trbs = xhci->cmd_ring_reserved_trbs;
 	int ret;
-	if (xhci->xhc_state & XHCI_STATE_DYING)
+
+	if (xhci->xhc_state) {
+		xhci_dbg(xhci, "xHCI dying or halted, can't queue_command\n");
 		return -ESHUTDOWN;
+	}
 
 	if (!command_must_succeed)
 		reserved_trbs++;
-- 
2.1.0

[PATCH 5/5] usb: xhci: Fix suspend/resume when used with OTG core

[Roger Quadros]

In the OTG case, the controller might not yet have been
added or is removed before the system suspends.

Assign xhci->main_hcd during probe to prevent NULL
pointer de-reference in xhci_suspend/resume().

Use the hcd->state flag to check if HCD is halted
and if that is so do nothing for xhci_suspend/resume().

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/host/xhci-pci.c  | 1 +
 drivers/usb/host/xhci-plat.c | 1 +
 drivers/usb/host/xhci.c      | 7 ++++++-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 4ad2c14..5474e0b 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -222,6 +222,7 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	/* USB 2.0 roothub is stored in the PCI device now. */
 	hcd = dev_get_drvdata(&dev->dev);
 	xhci = hcd_to_xhci(hcd);
+	xhci->main_hcd = hcd;
 	xhci->shared_hcd = usb_create_shared_hcd(driver, &dev->dev,
 				pci_name(dev), hcd);
 	if (!xhci->shared_hcd) {
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 00f23f5..68d67f5 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -140,6 +140,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
 
 	xhci = hcd_to_xhci(hcd);
 	xhci->clk = clk;
+	xhci->main_hcd = hcd;
 	xhci->shared_hcd = usb_create_shared_hcd(driver, &pdev->dev,
 			dev_name(&pdev->dev), hcd);
 	if (!xhci->shared_hcd) {
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 8a7e319..22ec235 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -891,6 +891,9 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
 	struct usb_hcd		*hcd = xhci_to_hcd(xhci);
 	u32			command;
 
+	if (!hcd->state)
+		return 0;
+
 	if (hcd->state != HC_STATE_SUSPENDED ||
 			xhci->shared_hcd->state != HC_STATE_SUSPENDED)
 		return -EINVAL;
@@ -977,6 +980,9 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
 	int			retval = 0;
 	bool			comp_timer_running = false;
 
+	if (!hcd->state)
+		return 0;
+
 	/* Wait a bit if either of the roothubs need to settle from the
 	 * transition into bus suspend.
 	 */
@@ -4827,7 +4833,6 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
 
 	if (usb_hcd_is_primary_hcd(hcd)) {
 		xhci = hcd_to_xhci(hcd);
-		xhci->main_hcd = hcd;
 		/* Mark the first roothub as being USB 2.0.
 		 * The xHCI driver will register the USB 3.0 roothub.
 		 */
-- 
2.1.0

[PATCH] usb: host: xhci-pci: Fix NULL pointer dereference error

[Roger Quadros]

From: Kishon Vijay Abraham I <kishon@ti.com>

commit 3b8295d5cbf2 (usb: xhci: Fix suspend/resume when used
with OTG core) removes assigning xhci->main_hcd from xhci_gen_setup
and adds it in the probe of xhci-plat and xhci-pci.

In the case of xhci-pci, xhci_mem_init is invoked before main_hcd is
initialized in the probe causing a null pointer deferencing error
when a PCIe usb controller card is plugged in.

Fix it by initializing xhci->main_hcd in xhci_gen_setup and removing
it from xhci_pci_probe().

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Acked-by: Roger Quadros <rogerq@ti.com>
---

  drivers/usb/host/xhci-pci.c |    1 -
  drivers/usb/host/xhci.c     |    1 +
  2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 11f16cb..67b9529 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -230,7 +230,6 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
  	/* USB 2.0 roothub is stored in the PCI device now. */
  	hcd = dev_get_drvdata(&dev->dev);
  	xhci = hcd_to_xhci(hcd);
-	xhci->main_hcd = hcd;
  	xhci->shared_hcd = usb_create_shared_hcd(driver, &dev->dev,
  				pci_name(dev), hcd);
  	if (!xhci->shared_hcd) {
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 397c0dd..b14f572 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4842,6 +4842,7 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
  		 */
  		hcd->speed = HCD_USB2;
  		hcd->self.root_hub->speed = USB_SPEED_HIGH;
+		xhci->main_hcd = hcd;
  		/*
  		 * USB 2.0 roothub under xHCI has an integrated TT,
  		 * (rate matching hub) as opposed to having an OHCI/UHCI
-- 
1.7.9.5

Re: [PATCH] usb: host: xhci-pci: Fix NULL pointer dereference error

[Mathias Nyman]

On 26.05.2015 17:15, Roger Quadros wrote:
> From: Kishon Vijay Abraham I <kishon@ti.com>
> 
> commit 3b8295d5cbf2 (usb: xhci: Fix suspend/resume when used
> with OTG core) removes assigning xhci->main_hcd from xhci_gen_setup
> and adds it in the probe of xhci-plat and xhci-pci.
> 
> In the case of xhci-pci, xhci_mem_init is invoked before main_hcd is
> initialized in the probe causing a null pointer deferencing error
> when a PCIe usb controller card is plugged in.
> 
> Fix it by initializing xhci->main_hcd in xhci_gen_setup and removing
> it from xhci_pci_probe().
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Acked-by: Roger Quadros <rogerq@ti.com>
> ---
> 
>  drivers/usb/host/xhci-pci.c |    1 -
>  drivers/usb/host/xhci.c     |    1 +
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 11f16cb..67b9529 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -230,7 +230,6 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>      /* USB 2.0 roothub is stored in the PCI device now. */
>      hcd = dev_get_drvdata(&dev->dev);
>      xhci = hcd_to_xhci(hcd);
> -    xhci->main_hcd = hcd;
>      xhci->shared_hcd = usb_create_shared_hcd(driver, &dev->dev,
>                  pci_name(dev), hcd);
>      if (!xhci->shared_hcd) {
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 397c0dd..b14f572 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -4842,6 +4842,7 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
>           */
>          hcd->speed = HCD_USB2;
>          hcd->self.root_hub->speed = USB_SPEED_HIGH;
> +        xhci->main_hcd = hcd;
>          /*
>           * USB 2.0 roothub under xHCI has an integrated TT,
>           * (rate matching hub) as opposed to having an OHCI/UHCI

Thanks, 
As your original patch causing this isn't out yet I'll undo this part from your 5/5 patch.
(prevent broken xhci for some unlucky bisecter)

-Mathias

Re: [PATCH] usb: host: xhci-pci: Fix NULL pointer dereference error

[Roger Quadros]

Mathias,

On 29/05/15 16:19, Mathias Nyman wrote:
> On 26.05.2015 17:15, Roger Quadros wrote:
>> From: Kishon Vijay Abraham I <kishon@ti.com>
>>
>> commit 3b8295d5cbf2 (usb: xhci: Fix suspend/resume when used
>> with OTG core) removes assigning xhci->main_hcd from xhci_gen_setup
>> and adds it in the probe of xhci-plat and xhci-pci.
>>
>> In the case of xhci-pci, xhci_mem_init is invoked before main_hcd is
>> initialized in the probe causing a null pointer deferencing error
>> when a PCIe usb controller card is plugged in.
>>
>> Fix it by initializing xhci->main_hcd in xhci_gen_setup and removing
>> it from xhci_pci_probe().
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> Acked-by: Roger Quadros <rogerq@ti.com>
>> ---
>>
>>   drivers/usb/host/xhci-pci.c |    1 -
>>   drivers/usb/host/xhci.c     |    1 +
>>   2 files changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
>> index 11f16cb..67b9529 100644
>> --- a/drivers/usb/host/xhci-pci.c
>> +++ b/drivers/usb/host/xhci-pci.c
>> @@ -230,7 +230,6 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>       /* USB 2.0 roothub is stored in the PCI device now. */
>>       hcd = dev_get_drvdata(&dev->dev);
>>       xhci = hcd_to_xhci(hcd);
>> -    xhci->main_hcd = hcd;
>>       xhci->shared_hcd = usb_create_shared_hcd(driver, &dev->dev,
>>                   pci_name(dev), hcd);
>>       if (!xhci->shared_hcd) {
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index 397c0dd..b14f572 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -4842,6 +4842,7 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
>>            */
>>           hcd->speed = HCD_USB2;
>>           hcd->self.root_hub->speed = USB_SPEED_HIGH;
>> +        xhci->main_hcd = hcd;
>>           /*
>>            * USB 2.0 roothub under xHCI has an integrated TT,
>>            * (rate matching hub) as opposed to having an OHCI/UHCI
>
> Thanks,
> As your original patch causing this isn't out yet I'll undo this part from your 5/5 patch.
> (prevent broken xhci for some unlucky bisecter)
>

That is fine. Thanks.

cheers,
-roger