usb: xhci: fixes for OTG/DRD use

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 <[email protected]>

Hi

Quoted 9 lines · 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.

Quoted 1 line

Signed-off-by Roger Quadros <[email protected]>

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

From the OTG perspective we have to prevent the actual usb_add_hcd() till the

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.

Hi,

Quoted 12 lines · Mathias Nyman

Hi

Quoted 9 lines · 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.

Quoted 4 lines

Quoted 1 line

Signed-off-by Roger Quadros <[email protected]>

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.

Quoted 11 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

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.

Hi

Quoted 25 lines · Roger Quadros

Hi,

Quoted 12 lines · Mathias Nyman

Hi

Quoted 9 lines · 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.

Quoted 4 lines

Quoted 1 line

Signed-off-by Roger Quadros <[email protected]>

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).

Quoted 4 lines

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.

Quoted 12 lines

Quoted 11 lines

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.

Quoted 6 lines

precisely.

Quoted 2 lines

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 4 lines

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.

Hi

Quoted 25 lines · Roger Quadros

Hi,

Quoted 12 lines · Mathias Nyman

Hi

Quoted 9 lines · 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.

Quoted 4 lines

Quoted 1 line

Signed-off-by Roger Quadros <[email protected]>

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).

Quoted 4 lines

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.

Quoted 12 lines

Quoted 11 lines

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.

Quoted 6 lines

precisely.

Quoted 2 lines

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.

Quoted 11 lines

Quoted 4 lines

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.

Hi Mathias,

Quoted 68 lines · Mathias Nyman

Hi

Quoted 25 lines · Roger Quadros

Hi,

Quoted 12 lines · Mathias Nyman

Hi

Quoted 9 lines · 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.

Quoted 4 lines

Quoted 1 line

Signed-off-by Roger Quadros <[email protected]>

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).

Quoted 4 lines

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.

Quoted 12 lines

Quoted 11 lines

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.

Quoted 6 lines

precisely.

Quoted 2 lines

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.

Quoted 11 lines

Quoted 4 lines

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.

Quoted 5 lines

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?

Quoted 10 lines

Quoted 10 lines

Quoted 5 lines

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.

Quoted 4 lines

Quoted 1 line

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.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 5 lines

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.

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

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.

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.

Hi

Quoted 8 lines · 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.

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?

Quoted 22 lines · Mathias Nyman

Hi

Quoted 8 lines · 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.

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

Quoted 7 lines

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

Quoted 26 lines · Mathias Nyman

Hi

Quoted 8 lines · 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.

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.

Quoted 3 lines

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.

Quoted 27 lines · Roger Quadros

Quoted 26 lines · Mathias Nyman

Hi

Quoted 8 lines · 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.

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 1 line

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.

Quoted 35 lines · Maxime Ripard

Quoted 33 lines · Roger Quadros

Quoted 22 lines · Mathias Nyman

Hi

Quoted 8 lines · 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.

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

Quoted 7 lines

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.

Quoted 6 lines

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

Quoted 1 line

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.

From: Kishon Vijay Abraham I <[email protected]>

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 <[email protected]>

Acked-by Roger Quadros <[email protected]>

---

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

drivers/usb/host/xhci-pci.c +0 -1

index 11f16cb..67b9529 100644
@@ -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) {

drivers/usb/host/xhci.c +0 -0

index 397c0dd..b14f572 100644

Quoted 41 lines · Roger Quadros

From: Kishon Vijay Abraham I <[email protected]>

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 <[email protected]>

Acked-by Roger Quadros <[email protected]>

---

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

drivers/usb/host/xhci-pci.c +0 -1

index 11f16cb..67b9529 100644
@@ -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) {

drivers/usb/host/xhci.c +0 -0

index 397c0dd..b14f572 100644
@@ -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;

drivers/usb/host/xhci.c int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks) · line 4848

Quoted 4 lines

drivers/usb/host/xhci.c +1 -0

+        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)