Re: [PATCH] usb: select NOP_USB_XCEIV by drivers that require it

[Michal Suchanek]

On 27 May 2016 at 05:41, Peter Chen  wrote:
> On Thu, May 26, 2016 at 07:25:23PM -, Michal Suchanek wrote:
>> Hello,
>>
>> I was updating my config by make oldconfig for a while and noticed that my 
>> USB
>> OTG controller is not working. Apparently it grew dependency on NOP_USB_XCEIV
>> over time.
>>
>> Looking through defconfigs some have it included and some which seem in need 
>> of
>> it don't.
>>
>> Since the dependency is not obvious I think it would be better to select it
>> where possible.
>
> From Documentation/kbuild/kconfig-language.txt
> In general use select only for non-visible symbols
> (no prompts anywhere) and for symbols with no dependencies.
>
> But NOP_USB_XCEIV is a visible symbol and can be chosen, besides,
> NOP_USB_XCEIV has already selected USB_PHY. Using select may cause
> dependency problem in future, so unless it is necessary, use it
> as least as possible.
>
> If you are using new code, and it adds new dependency code, it is
> reasonable you may need to update your defconfig.

If the driver gets split into multiple parts that need to be
configured separately that's reasonable.

If the newly required option is some obscure feature internal to the
Linux implementation like NOP_USB_XCEIV it's not reasonable in my
book.

Thanks

Michal
--
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] usb: core: fix a double free in the usb driver

[gre...@linuxfoundation.org]

On Fri, May 27, 2016 at 01:38:17AM +, Chung-Geol Kim wrote:
> There is a double free problem in the usb driver.

Which driver?

> This is caused by delayed deregister for scsi device.
> <*> at Insert USB Storage
> -  USB bus #1 register
>  usb_create_hcd (primary-kref==1)
>  * primary-bandwidth_mutex(alloc))
> usb_get_hcd(primary-kref==2)
> -  USB bus #2 register
> usb_create_hcd (second-kref==1)
>  * second-bandwidth_mutex==primary-bandwidth_mutex
>  usb_get_hcd(second-kref==2)
> -  scsi_device_get
> usb_get_hcd(second-kref==3)
> 
> <*> at remove USB Storage (Normal)
> -  scsi_device_put
> usb_put_hcd(second-kref==2)
> -  USB bus #2 deregister
>  usb_release_dev(second-kref==1)
> usb_release_dev(second-kref==0)  -> hcd_release()
> -  USB bus #1 deregister
>  usb_release_dev(primary-kref==1)
>  usb_release_dev(primary-kref==0) -> hcd_release()
> *(primary-bandwidth_mutex free)
> 
> at remove USB Storage 
> -  USB bus #2 deregister
>  usb_release_dev(second-kref==2)
> usb_release_dev(second-kref==1)
> -  USB bus #1 deregister
>  usb_release_dev(primary-kref==1)
>  usb_release_dev(primary-kref==0) -> hcd_release()
>  *(primary-bandwidth_mutex free)
> -  scsi_device_put
>  usb_put_hcd(second-kref==0)  -> hcd_release(*)
>  * at this, second->primary==0 therefore try to
> free the primary-bandwidth_mutex.(already freed)

The formatting for this is all confused, can you fix it up?

> 
> To fix this problem kfree(hcd->bandwidth_mutex);
> should be executed at only (hcd->primary_hcd==hcd).
> 
> Signed-off-by: Chunggeol Kim 

We need an email address at the end of this line, look at how the
commits in the kernel git history look like for examples.

> ---
> drivers/usb/core/hcd.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 34b837a..60077f3 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2608,7 +2608,7 @@ static void hcd_release(struct kref *kref)
> struct usb_hcd *hcd = container_of (kref, struct usb_hcd, kref);
> 
> mutex_lock(_port_peer_mutex);
> - if (usb_hcd_is_primary_hcd(hcd)) {
> + if (hcd == hcd->primary_hcd) {

That doesn't make sense, usb_hcd_is_primary_hcd() is the same as this
check, what are you changing here?

> kfree(hcd->address0_mutex);
> kfree(hcd->bandwidth_mutex);
> }

Your patch itself is also corrupted, and can't be applied, can you also
resolve this and resend?

thanks,

greg k-h
--
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] usb: select NOP_USB_XCEIV by drivers that require it

[Peter Chen]

On Thu, May 26, 2016 at 07:25:23PM -, Michal Suchanek wrote:
> Hello,
> 
> I was updating my config by make oldconfig for a while and noticed that my USB
> OTG controller is not working. Apparently it grew dependency on NOP_USB_XCEIV
> over time.
> 
> Looking through defconfigs some have it included and some which seem in need 
> of
> it don't.
> 
> Since the dependency is not obvious I think it would be better to select it
> where possible.

>From Documentation/kbuild/kconfig-language.txt
In general use select only for non-visible symbols
(no prompts anywhere) and for symbols with no dependencies.

But NOP_USB_XCEIV is a visible symbol and can be chosen, besides,
NOP_USB_XCEIV has already selected USB_PHY. Using select may cause
dependency problem in future, so unless it is necessary, use it
as least as possible.

If you are using new code, and it adds new dependency code, it is
reasonable you may need to update your defconfig.

Peter
> 
> Attaching a patch.
> 
> Thanks
> 
> Michal
> 
> 8<--
> NOP_USB_XCEIV is non-obvious dependency for MUSB and other drivers.
> 
> This is a virtual driver in the sense that there is no actual piece of
> hardware you can point at and say you did not include driver for this so
> it won't work.
> 
> So just change all depends on it to select.
> 
> Signed-off-by: Michal Suchanek 
> ---
>  drivers/usb/chipidea/Kconfig |  3 ++-
>  drivers/usb/musb/Kconfig | 19 +--
>  drivers/usb/phy/Kconfig  |  2 ++
>  3 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
> index 3644a35..8d08ebd 100644
> --- a/drivers/usb/chipidea/Kconfig
> +++ b/drivers/usb/chipidea/Kconfig
> @@ -19,7 +19,8 @@ config USB_CHIPIDEA_OF
>  config USB_CHIPIDEA_PCI
>   tristate
>   depends on PCI
> - depends on NOP_USB_XCEIV
> + select NOP_USB_XCEIV
> + select USB_PHY
>   default USB_CHIPIDEA
>  
>  config USB_CHIPIDEA_UDC
> diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
> index 886526b..91717b9 100644
> --- a/drivers/usb/musb/Kconfig
> +++ b/drivers/usb/musb/Kconfig
> @@ -66,7 +66,8 @@ comment "Platform Glue Layer"
>  config USB_MUSB_SUNXI
>   tristate "Allwinner (sunxi)"
>   depends on ARCH_SUNXI
> - depends on NOP_USB_XCEIV
> + select NOP_USB_XCEIV
> + select USB_PHY
>   depends on PHY_SUN4I_USB
>   depends on EXTCON
>   depends on GENERIC_PHY
> @@ -75,13 +76,15 @@ config USB_MUSB_SUNXI
>  config USB_MUSB_DAVINCI
>   tristate "DaVinci"
>   depends on ARCH_DAVINCI_DMx
> - depends on NOP_USB_XCEIV
> + select NOP_USB_XCEIV
> + select USB_PHY
>   depends on BROKEN
>  
>  config USB_MUSB_DA8XX
>   tristate "DA8xx/OMAP-L1x"
>   depends on ARCH_DAVINCI_DA8XX
> - depends on NOP_USB_XCEIV
> + select NOP_USB_XCEIV
> + select USB_PHY
>   depends on BROKEN
>  
>  config USB_MUSB_TUSB6010
> @@ -89,6 +92,7 @@ config USB_MUSB_TUSB6010
>   depends on HAS_IOMEM
>   depends on ARCH_OMAP2PLUS || COMPILE_TEST
>   depends on NOP_USB_XCEIV = USB_MUSB_HDRC # both built-in or both modules
> + # cannot select NOP_USB_XCEIV because of the dependency above
>  
>  config USB_MUSB_OMAP2PLUS
>   tristate "OMAP2430 and onwards"
> @@ -99,7 +103,8 @@ config USB_MUSB_OMAP2PLUS
>  config USB_MUSB_AM35X
>   tristate "AM35x"
>   depends on ARCH_OMAP
> - depends on NOP_USB_XCEIV
> + select NOP_USB_XCEIV
> + select USB_PHY
>  
>  config USB_MUSB_DSPS
>   tristate "TI DSPS platforms"
> @@ -110,7 +115,8 @@ config USB_MUSB_DSPS
>  config USB_MUSB_BLACKFIN
>   tristate "Blackfin"
>   depends on (BF54x && !BF544) || (BF52x && ! BF522 && !BF523)
> - depends on NOP_USB_XCEIV
> + select NOP_USB_XCEIV
> + select USB_PHY
>  
>  config USB_MUSB_UX500
>   tristate "Ux500 platforms"
> @@ -118,7 +124,8 @@ config USB_MUSB_UX500
>  
>  config USB_MUSB_JZ4740
>   tristate "JZ4740"
> - depends on NOP_USB_XCEIV
> + select NOP_USB_XCEIV
> + select USB_PHY
>   depends on MACH_JZ4740 || COMPILE_TEST
>   depends on USB_MUSB_GADGET
>   depends on USB_OTG_BLACKLIST_HUB
> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
> index c690474..a0bdfd3 100644
> --- a/drivers/usb/phy/Kconfig
> +++ b/drivers/usb/phy/Kconfig
> @@ -57,6 +57,8 @@ config NOP_USB_XCEIV
> built-in with usb ip or which are autonomous and doesn't require any
> phy programming such as ISP1x04 etc.
>  
> +   Should be automatically selected by the relevant driver.
> +
>  config AM335X_CONTROL_USB
>   tristate
>  
> -- 
> 2.8.1
> 
> --
> 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

-- 

Best Regards,
Peter 

Re: [RFC 4/5] ARM: tegra: Enable UDC on Dalmore

[Peter Chen]

On Thu, May 26, 2016 at 05:40:04PM +0200, Thierry Reding wrote:
> From: Thierry Reding 
> 
> Override the compatible string of the first USB controller to enable
> device mode.
> 
> Signed-off-by: Thierry Reding 
> ---
>  arch/arm/boot/dts/tegra114-dalmore.dts | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts 
> b/arch/arm/boot/dts/tegra114-dalmore.dts
> index c970bf65c74c..53d664f718ff 100644
> --- a/arch/arm/boot/dts/tegra114-dalmore.dts
> +++ b/arch/arm/boot/dts/tegra114-dalmore.dts
> @@ -1122,6 +1122,17 @@
>   non-removable;
>   };
>  
> + usb@7d00 {
> + compatible = "nvidia,tegra114-udc";
> + status = "okay";
> + dr_mode = "otg";
> + };
> +
> + usb-phy@7d00 {
> + status = "okay";
> + dr_mode = "otg";
> + };
> +

It is a USB PHY node, you don't need to set dr_mode for it.

-- 

Best Regards,
Peter Chen
--
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: [RFC 0/5] usb: chipidea: Add support for Tegra20 through Tegra124

[Peter Chen]

On Thu, May 26, 2016 at 05:40:00PM +0200, Thierry Reding wrote:
> From: Thierry Reding 
> 
> All Tegra SoC generations from Tegra20 through Tegra124 have a ChipIdea
> USB device controller. This set of patches adds very rudimentary support
> for it to the existing ChipIdea driver and enables them on the set of
> boards that I could easily test on.
> 
> I'm sending this out as RFC because I'm not sure yet how to merge this.
> While the driver seems to work fine (tested by exporting a USB driver or
> eMMC via the mass storage function) I don't yet understand how to make
> the driver switch between host and device modes dynamically. It might be
> useful to get this merged before, but I'd like to have some feedback on
> this, because doing so would mean that we need to use device mode on the
> devices where it's enabled and can't use the USBD port in host mode.
> 

Chipidea driver supports many ways to switch between host and device
mode. It can support switching with/without disconnecting cable.

Most of cases need to disconnect cable (Micro-AB) to switch between
host and device mode, I just take this as an example:

Using ID pin which is at Micro-B receptacle on the board to determine host (ID 
= 0)
or device (ID = 1 )mode.

- ID pin connects to CPU, and ID interrupt and value can be get through
register OTGSC.
- ID pin does not connect to CPU, and there is a dedicated GPIO for ID.

-- 

Best Regards,
Peter Chen
--
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: composite gadget with _real_ USB device

[Shea Ako]

> How about setting up USBIP, using the same machine as both the server
> and the client?

That’s an interesting idea, I’ll check it out. Thanks for the tip!

And thanks Felipe for your input too, I really appreciate the help.


> On May 20, 2016, at 9:38 AM, Alan Stern  wrote:
> 
> On Fri, 20 May 2016, Felipe Balbi wrote:
> 
>> 
>> Hi,
>> 
>> Shea Ako  writes:
>>> I’ve been learning about and playing with configfs and functionfs to
>>> create composite user space USB gadgets. My objective is to create a
>>> composite USB gadget that incorporates a custom functionfs function of
>>> my own creation along with some _real_ USB devices connected to my
>>> linux platform.
>>> 
>>> Is there an easy existing way to bundle _real_ USB devices into a
>>> composite gadget like this or am I looking at writing my own user
>>> space functionfs functions which handle wrapping and forwarding the
>>> interfaces/endpoints/data of the connected _real_ USB devices?
>> 
>> heh, you're really on your own. Sounds pretty interesting but you're
>> gonna spend a lot of time with this :p
>> 
>>> I haven’t found any documented existing way to do this, but I thought
>>> I should ask before I go off an implement it myself as it seems that
>>> this might be a use case which isn’t entirely off the wall and there
>>> could already be support for this somewhere that I haven’t yet
>>> encountered.
>> 
>> I don't think anybody has done anything like this yet. The closest I got
>> was to attach some USB sticks to host port via HUB, setup RAID and use
>> that RAID as backing store for g_mass_storage, then connect
>> g_mass_storage back to same host which has the RAID of several USB
>> sticks (same machine has host and peripheral controllers).
> 
> How about setting up USBIP, using the same machine as both the server
> and the client?
> 
> 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

[PATCH] usb: core: fix a double free in the usb driver

[Chung-Geol Kim]

There is a double free problem in the usb driver.
This is caused by delayed deregister for scsi device.
<*> at Insert USB Storage
-  USB bus #1 register
 usb_create_hcd (primary-kref==1)
 * primary-bandwidth_mutex(alloc))
usb_get_hcd(primary-kref==2)
-  USB bus #2 register
usb_create_hcd (second-kref==1)
 * second-bandwidth_mutex==primary-bandwidth_mutex
 usb_get_hcd(second-kref==2)
-  scsi_device_get
usb_get_hcd(second-kref==3)

<*> at remove USB Storage (Normal)
-  scsi_device_put
usb_put_hcd(second-kref==2)
-  USB bus #2 deregister
 usb_release_dev(second-kref==1)
usb_release_dev(second-kref==0)  -> hcd_release()
-  USB bus #1 deregister
 usb_release_dev(primary-kref==1)
 usb_release_dev(primary-kref==0) -> hcd_release()
*(primary-bandwidth_mutex free)

at remove USB Storage 
-  USB bus #2 deregister
 usb_release_dev(second-kref==2)
usb_release_dev(second-kref==1)
-  USB bus #1 deregister
 usb_release_dev(primary-kref==1)
 usb_release_dev(primary-kref==0) -> hcd_release()
 *(primary-bandwidth_mutex free)
-  scsi_device_put
 usb_put_hcd(second-kref==0)  -> hcd_release(*)
 * at this, second->primary==0 therefore try to
free the primary-bandwidth_mutex.(already freed)

To fix this problem kfree(hcd->bandwidth_mutex);
should be executed at only (hcd->primary_hcd==hcd).

Signed-off-by: Chunggeol Kim 
---
drivers/usb/core/hcd.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 34b837a..60077f3 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2608,7 +2608,7 @@ static void hcd_release(struct kref *kref)
struct usb_hcd *hcd = container_of (kref, struct usb_hcd, kref);

mutex_lock(_port_peer_mutex);
- if (usb_hcd_is_primary_hcd(hcd)) {
+ if (hcd == hcd->primary_hcd) {
kfree(hcd->address0_mutex);
kfree(hcd->bandwidth_mutex);
}
-- 
1.7.9.5N떑꿩�r툤y鉉싕b쾊Ф푤v�^�)頻{.n�+돴쪐{군펐왲^n뇊⊆쫧�곷h솳鈺�&��췍쳺�h�(��쉸듶줷"얎�m��곴�z받뻿筬f＂톒쉱�~늤

[PATCH v2] usb: dwc3: Set the ClearPendIN bit on Clear Stall EP command

[John Youn]

As of core revision 2.60a the recommended programming model is to set
the ClearPendIN bit when issuing a Clear Stall EP command for IN
endpoints. This is to prevent an issue where some (non-compliant) hosts
may not send ACK TPs for pending IN transfers due to a mishandled error
condition. Synopsys STAR 9000614252.

Signed-off-by: John Youn 
---
v2:
* Removed the call to dwc3_is_usb31(). We set the high bit in the
  version for USB 31 IP so that we can just do a comparison for these
  cases.
* Rewrote the commit message.
* Added comment

 drivers/usb/dwc3/core.h   |  1 +
 drivers/usb/dwc3/gadget.c | 30 --
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 74dff47..dcdba14 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -417,6 +417,7 @@
 #define DWC3_DEPCMD_GET_RSC_IDX(x) (((x) >> DWC3_DEPCMD_PARAM_SHIFT) & 
0x7f)
 #define DWC3_DEPCMD_STATUS(x)  (((x) >> 12) & 0x0F)
 #define DWC3_DEPCMD_HIPRI_FORCERM  (1 << 11)
+#define DWC3_DEPCMD_CLEARPENDIN(1 << 11)
 #define DWC3_DEPCMD_CMDACT (1 << 10)
 #define DWC3_DEPCMD_CMDIOC (1 << 8)
 
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 5d82ab6..b4779dd 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -334,6 +334,28 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned 
cmd,
return ret;
 }
 
+static int dwc3_send_clear_stall_ep_cmd(struct dwc3_ep *dep)
+{
+   struct dwc3 *dwc = dep->dwc;
+   struct dwc3_gadget_ep_cmd_params params;
+   u32 cmd = DWC3_DEPCMD_CLEARSTALL;
+
+   /*
+* As of core revision 2.60a the recommended programming model
+* is to set the ClearPendIN bit when issuing a Clear Stall EP
+* command for IN endpoints. This is to prevent an issue where
+* some (non-compliant) hosts may not send ACK TPs for pending
+* IN transfers due to a mishandled error condition. Synopsys
+* STAR 9000614252.
+*/
+   if (dep->direction && (dwc->revision >= DWC3_REVISION_260A))
+   cmd |= DWC3_DEPCMD_CLEARPENDIN;
+
+   memset(, 0, sizeof(params));
+
+   return dwc3_send_gadget_ep_cmd(dep, cmd, );
+}
+
 static dma_addr_t dwc3_trb_dma_offset(struct dwc3_ep *dep,
struct dwc3_trb *trb)
 {
@@ -1290,8 +1312,7 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int 
value, int protocol)
else
dep->flags |= DWC3_EP_STALL;
} else {
-   ret = dwc3_send_gadget_ep_cmd(dep, DWC3_DEPCMD_CLEARSTALL,
-   );
+   ret = dwc3_send_clear_stall_ep_cmd(dep);
if (ret)
dev_err(dwc->dev, "failed to clear STALL on %s\n",
dep->name);
@@ -2300,7 +2321,6 @@ static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)
 
for (epnum = 1; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
struct dwc3_ep *dep;
-   struct dwc3_gadget_ep_cmd_params params;
int ret;
 
dep = dwc->eps[epnum];
@@ -2312,9 +2332,7 @@ static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)
 
dep->flags &= ~DWC3_EP_STALL;
 
-   memset(, 0, sizeof(params));
-   ret = dwc3_send_gadget_ep_cmd(dep, DWC3_DEPCMD_CLEARSTALL,
-   );
+   ret = dwc3_send_clear_stall_ep_cmd(dep);
WARN_ON_ONCE(ret);
}
 }
-- 
2.8.2

--
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] reset: Put back *_optional variants

[John Youn]

On 5/26/2016 1:25 PM, Hans de Goede wrote:
> Hi,
> 
> On 26-05-16 03:15, John Youn wrote:
>> Prior to commit 6c96f05c8bb8 ("reset: Make [of_]reset_control_get[_foo]
>> functions wrappers"), the optional variants returned -ENOTSUPP when
>> CONFIG_RESET_CONTROLLER was not set. This patch reverts to this
>> behavior. Otherwise those calls will return -EINVAL causing users to
>> think that an error occurred when CONFIG_RESET_CONTROLLER is not set.
>>
>> Fixes: 6c96f05c8bb8 ("reset: Make [of_]reset_control_get[_foo] functions 
>> wrappers")
>> Signed-off-by: John Youn 
>> ---
>>
>> Hi Philipp, Hans,
>>
>> The commit referenced above breaks an upcoming patch for the dwc2
>> driver that adds an optional reset control.
>>
>> https://marc.info/?l=linux-usb=146161328211584=2
>>
>> I've attempted to add the optional variants back the way they were
>> working before. Let me know if I need to do anything else to fix it or
>> if it should be done another way.
>>
>> Regards,
>> John
> 
> Hmm, I don't like all the extra code your patch adds just to fix
> a return value...
> 
> Looking at the code before my "reset: Make [of_]reset_control_get[_foo]
> functions wrappers" patch, all the dev*_get* functions were
> returning ENOTSUPP except for [devm_]reset_control_get, so following
> your logic we should also change the of_reset_control_get_by_index
> variant to return -ENOTSUP.
> 
> Or better, simply make them all return -ENOTSUP, that seems both
> consistent and more KISS to me, this would mean an error code
> change for [devm_]reset_control_get, but will fix all the other
> getters from having a changed error-code, and I would callers
> of [devm_]reset_control_get to not care which error code they
> get, except for -EPROBE_DEFER.
> 
> So IMHO the following change would be a better way to fix this:
> 
> --- a/include/linux/reset.h
> +++ b/include/linux/reset.h
> @@ -65,14 +65,14 @@ static inline struct reset_control 
> *__of_reset_control_get(
>  struct device_node *node,
>  const char *id, int index, int 
> shared)
>   {
> -   return ERR_PTR(-EINVAL);
> + return ERR_PTR(-ENOTSUPP);
>   }
> 
>   static inline struct reset_control *__devm_reset_control_get(
>  struct device *dev,
>  const char *id, int index, int 
> shared)
>   {
> -   return ERR_PTR(-EINVAL);
> + return ERR_PTR(-ENOTSUPP);
>   }
> 
>   #endif /* CONFIG_RESET_CONTROLLER */


I'm good with this. However, per Philipp on a previous thread, the
intended behavior is to return -EINVAL for the non-optional functions.

http://marc.info/?l=linux-usb=146156945528848=2

Philipp,

Any suggestions?

Regards,
John

--
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: [RFC 1/5] usb: chipidea: Add support for Tegra20/30/114/124

[Stephen Warren]

On 05/26/2016 03:17 PM, Stephen Warren wrote:

On 05/26/2016 09:40 AM, Thierry Reding wrote:

From: Thierry Reding 

All of these Tegra SoC generations have a ChipIdea UDC IP block that can
be used for device mode communication with a host. Implement rudimentary
support that doesn't allow switching between host and device modes.


Are you sure this is correct for Tegra20? I ask because for the /host/
mode driver, there's a "has_hostpc" flag which is set to false for
Tegra20 and true for all other SoCs. In the U-Boot device mode driver
(if not in the kernel driver; I didn't check), there's a concept of "has
hostpc" too. I might expect that flag to be set the same way for both
drivers. That said, I /think/ the host and device HW are unrelated, so
it's possible has_hostpc might be set differently for them.
Unfortunately, we haven't enabled the device mode driver for any Tegra20
system in U-Boot so I can't tell whether we should enable has_hostpc for
Tegra20's device mode driver.

Still, if this code works then I guess it's likely correct...


On the other hand, it looks like the kernel device mode driver might 
auto-detect this; in core.c:hw_device_init(), I see:


reg = hw_read(ci, CAP_HCCPARAMS, HCCPARAMS_LEN) >>
__ffs(HCCPARAMS_LEN);
ci->hw_bank.lpm  = reg;

... and in host.c:host_start(), I see:

ehci->has_hostpc = ci->hw_bank.lpm;

--
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: [RFC 1/5] usb: chipidea: Add support for Tegra20/30/114/124

[Stephen Warren]

On 05/26/2016 09:40 AM, Thierry Reding wrote:

From: Thierry Reding 

All of these Tegra SoC generations have a ChipIdea UDC IP block that can
be used for device mode communication with a host. Implement rudimentary
support that doesn't allow switching between host and device modes.


Are you sure this is correct for Tegra20? I ask because for the /host/ 
mode driver, there's a "has_hostpc" flag which is set to false for 
Tegra20 and true for all other SoCs. In the U-Boot device mode driver 
(if not in the kernel driver; I didn't check), there's a concept of "has 
hostpc" too. I might expect that flag to be set the same way for both 
drivers. That said, I /think/ the host and device HW are unrelated, so 
it's possible has_hostpc might be set differently for them. 
Unfortunately, we haven't enabled the device mode driver for any Tegra20 
system in U-Boot so I can't tell whether we should enable has_hostpc for 
Tegra20's device mode driver.


Still, if this code works then I guess it's likely correct...
--
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] drivers: usb: dwc3 : Configure DMA properties and ops from DT

[Leo Li]

On Thu, May 26, 2016 at 3:30 AM, Felipe Balbi
 wrote:
>
> Hi,
>
> Leo Li  writes:
 On certain platforms (e.g. ARM64) the dma_ops needs to be explicitly set
 to be able to do DMA allocations, so use the of_dma_configure() helper
 to populate the dma properties and assign an appropriate dma_ops.

 Signed-off-by: Rajesh Bhagat 
 Reviewed-by: Yang-Leo Li 
>>>
>>> Cool, nxp is also using dwc3 :-) C'mon Rajesh, send us a glue layer :)
>>>
 diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
 index c679f63..4d5b783 100644
 --- a/drivers/usb/dwc3/host.c
 +++ b/drivers/usb/dwc3/host.c
 @@ -17,6 +17,7 @@

  #include 
  #include 
 +#include 

  #include "core.h"

 @@ -32,6 +33,9 @@ int dwc3_host_init(struct dwc3 *dwc)
   return -ENOMEM;
   }

 + if (IS_ENABLED(CONFIG_OF) && dwc->dev->of_node)
 + of_dma_configure(>dev, dwc->dev->of_node);
>>>
>>> okay, so we have a long discussion about this going on. You can catch up
>>> with it starting here:
>>>
>>> http://marc.info/?i=1461612094-30939-1-git-send-email-grygorii.stras...@ti.com
>>>
>>> At least for now, this patch will be applied. We need to have a better
>>> solution for this, one that helps not only DT platforms.
>>
>> Balbi,
>>
>> Has the patch from Grygorii been applied?  I don't see it in the
>> mainline tree yet.  Without fix, the dwc3 driver will fail for all
>> ARM64 SoCs.
>
> right, it's still broken. But we don't want something that fixes only
> OF, right? dwc3 is also broken for PCI when IOMMU is enabled. It breaks
> for the same reasons.
>
> We really need a way to inherit DMA bits from parent device here.

I agree with your proposal, but the original discussion seems to be on
halt right now.  If it need more time to get to an agreement on proper
fix, probably it's better to have a temporary fix right now to make
the driver working again.

Regards,
Leo
--
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] reset: Put back *_optional variants

[Hans de Goede]

Hi,

On 26-05-16 03:15, John Youn wrote:

Prior to commit 6c96f05c8bb8 ("reset: Make [of_]reset_control_get[_foo]
functions wrappers"), the optional variants returned -ENOTSUPP when
CONFIG_RESET_CONTROLLER was not set. This patch reverts to this
behavior. Otherwise those calls will return -EINVAL causing users to
think that an error occurred when CONFIG_RESET_CONTROLLER is not set.

Fixes: 6c96f05c8bb8 ("reset: Make [of_]reset_control_get[_foo] functions 
wrappers")
Signed-off-by: John Youn 
---

Hi Philipp, Hans,

The commit referenced above breaks an upcoming patch for the dwc2
driver that adds an optional reset control.

https://marc.info/?l=linux-usb=146161328211584=2

I've attempted to add the optional variants back the way they were
working before. Let me know if I need to do anything else to fix it or
if it should be done another way.

Regards,
John


Hmm, I don't like all the extra code your patch adds just to fix
a return value...

Looking at the code before my "reset: Make [of_]reset_control_get[_foo]
functions wrappers" patch, all the dev*_get* functions were
returning ENOTSUPP except for [devm_]reset_control_get, so following
your logic we should also change the of_reset_control_get_by_index
variant to return -ENOTSUP.

Or better, simply make them all return -ENOTSUP, that seems both
consistent and more KISS to me, this would mean an error code
change for [devm_]reset_control_get, but will fix all the other
getters from having a changed error-code, and I would callers
of [devm_]reset_control_get to not care which error code they
get, except for -EPROBE_DEFER.

So IMHO the following change would be a better way to fix this:

--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -65,14 +65,14 @@ static inline struct reset_control *__of_reset_control_get(
struct device_node *node,
const char *id, int index, int shared)
 {
-   return ERR_PTR(-EINVAL);
+ return ERR_PTR(-ENOTSUPP);
 }

 static inline struct reset_control *__devm_reset_control_get(
struct device *dev,
const char *id, int index, int shared)
 {
-   return ERR_PTR(-EINVAL);
+ return ERR_PTR(-ENOTSUPP);
 }

 #endif /* CONFIG_RESET_CONTROLLER */



Regards,

Hans





 include/linux/reset.h | 33 +++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/include/linux/reset.h b/include/linux/reset.h
index ec0306ce..739d33d 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -14,10 +14,24 @@ int reset_control_status(struct reset_control *rstc);

 struct reset_control *__of_reset_control_get(struct device_node *node,
 const char *id, int index, int shared);
+
+struct reset_control *__of_reset_control_get_optional(struct device_node *node,
+const char *id, int index, int shared)
+{
+   return __of_reset_control_get(node, id, index, shared);
+}
+
 void reset_control_put(struct reset_control *rstc);
 struct reset_control *__devm_reset_control_get(struct device *dev,
 const char *id, int index, int shared);

+static inline struct reset_control *__devm_reset_control_get_optional(
+   struct device *dev,
+   const char *id, int index, int shared)
+{
+   return __devm_reset_control_get(dev, id, index, shared);
+}
+
 int __must_check device_reset(struct device *dev);

 static inline int device_reset_optional(struct device *dev)
@@ -74,6 +88,13 @@ static inline struct reset_control *__of_reset_control_get(
return ERR_PTR(-EINVAL);
 }

+static inline struct reset_control *__of_reset_control_get_optional(
+   struct device_node *node,
+   const char *id, int index, int shared)
+{
+   return ERR_PTR(-ENOTSUPP);
+}
+
 static inline struct reset_control *__devm_reset_control_get(
struct device *dev,
const char *id, int index, int shared)
@@ -81,6 +102,13 @@ static inline struct reset_control 
*__devm_reset_control_get(
return ERR_PTR(-EINVAL);
 }

+static inline struct reset_control *__devm_reset_control_get_optional(
+   struct device *dev,
+   const char *id, int index, int shared)
+{
+   return ERR_PTR(-ENOTSUPP);
+}
+
 #endif /* CONFIG_RESET_CONTROLLER */

 /**
@@ -110,7 +138,8 @@ static inline struct reset_control *__must_check 
reset_control_get(
 static inline struct reset_control *reset_control_get_optional(
struct device *dev, const char *id)
 {
-   return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 0);
+   return 

[PATCH] usb: select NOP_USB_XCEIV by drivers that require it

[Michal Suchanek]

Hello,

I was updating my config by make oldconfig for a while and noticed that my USB
OTG controller is not working. Apparently it grew dependency on NOP_USB_XCEIV
over time.

Looking through defconfigs some have it included and some which seem in need of
it don't.

Since the dependency is not obvious I think it would be better to select it
where possible.

Attaching a patch.

Thanks

Michal

8<--
NOP_USB_XCEIV is non-obvious dependency for MUSB and other drivers.

This is a virtual driver in the sense that there is no actual piece of
hardware you can point at and say you did not include driver for this so
it won't work.

So just change all depends on it to select.

Signed-off-by: Michal Suchanek 
---
 drivers/usb/chipidea/Kconfig |  3 ++-
 drivers/usb/musb/Kconfig | 19 +--
 drivers/usb/phy/Kconfig  |  2 ++
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
index 3644a35..8d08ebd 100644
--- a/drivers/usb/chipidea/Kconfig
+++ b/drivers/usb/chipidea/Kconfig
@@ -19,7 +19,8 @@ config USB_CHIPIDEA_OF
 config USB_CHIPIDEA_PCI
tristate
depends on PCI
-   depends on NOP_USB_XCEIV
+   select NOP_USB_XCEIV
+   select USB_PHY
default USB_CHIPIDEA
 
 config USB_CHIPIDEA_UDC
diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
index 886526b..91717b9 100644
--- a/drivers/usb/musb/Kconfig
+++ b/drivers/usb/musb/Kconfig
@@ -66,7 +66,8 @@ comment "Platform Glue Layer"
 config USB_MUSB_SUNXI
tristate "Allwinner (sunxi)"
depends on ARCH_SUNXI
-   depends on NOP_USB_XCEIV
+   select NOP_USB_XCEIV
+   select USB_PHY
depends on PHY_SUN4I_USB
depends on EXTCON
depends on GENERIC_PHY
@@ -75,13 +76,15 @@ config USB_MUSB_SUNXI
 config USB_MUSB_DAVINCI
tristate "DaVinci"
depends on ARCH_DAVINCI_DMx
-   depends on NOP_USB_XCEIV
+   select NOP_USB_XCEIV
+   select USB_PHY
depends on BROKEN
 
 config USB_MUSB_DA8XX
tristate "DA8xx/OMAP-L1x"
depends on ARCH_DAVINCI_DA8XX
-   depends on NOP_USB_XCEIV
+   select NOP_USB_XCEIV
+   select USB_PHY
depends on BROKEN
 
 config USB_MUSB_TUSB6010
@@ -89,6 +92,7 @@ config USB_MUSB_TUSB6010
depends on HAS_IOMEM
depends on ARCH_OMAP2PLUS || COMPILE_TEST
depends on NOP_USB_XCEIV = USB_MUSB_HDRC # both built-in or both modules
+   # cannot select NOP_USB_XCEIV because of the dependency above
 
 config USB_MUSB_OMAP2PLUS
tristate "OMAP2430 and onwards"
@@ -99,7 +103,8 @@ config USB_MUSB_OMAP2PLUS
 config USB_MUSB_AM35X
tristate "AM35x"
depends on ARCH_OMAP
-   depends on NOP_USB_XCEIV
+   select NOP_USB_XCEIV
+   select USB_PHY
 
 config USB_MUSB_DSPS
tristate "TI DSPS platforms"
@@ -110,7 +115,8 @@ config USB_MUSB_DSPS
 config USB_MUSB_BLACKFIN
tristate "Blackfin"
depends on (BF54x && !BF544) || (BF52x && ! BF522 && !BF523)
-   depends on NOP_USB_XCEIV
+   select NOP_USB_XCEIV
+   select USB_PHY
 
 config USB_MUSB_UX500
tristate "Ux500 platforms"
@@ -118,7 +124,8 @@ config USB_MUSB_UX500
 
 config USB_MUSB_JZ4740
tristate "JZ4740"
-   depends on NOP_USB_XCEIV
+   select NOP_USB_XCEIV
+   select USB_PHY
depends on MACH_JZ4740 || COMPILE_TEST
depends on USB_MUSB_GADGET
depends on USB_OTG_BLACKLIST_HUB
diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
index c690474..a0bdfd3 100644
--- a/drivers/usb/phy/Kconfig
+++ b/drivers/usb/phy/Kconfig
@@ -57,6 +57,8 @@ config NOP_USB_XCEIV
  built-in with usb ip or which are autonomous and doesn't require any
  phy programming such as ISP1x04 etc.
 
+ Should be automatically selected by the relevant driver.
+
 config AM335X_CONTROL_USB
tristate
 
-- 
2.8.1

--
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: SuperH 7760 OHCI

[Alan Stern]

On Thu, 26 May 2016, Martin Townsend wrote:

> >> and setting the HCD_MEMORY_LOCAL flag in the HC driver.
> >
> > Did you do this correctly?  That is, in the correct driver?
> >
> I put the code for the declaring the DMA coherent memory into ohci-platform.c
> and set the flag in ohci-hcd.c

Okay.  It's not portable, but it's fine for a single system with only 
one controller.

> >> and I get the following error
> >
> > ...
> >> [1.51] usb 1-1: new full-speed USB device number 2 using 
> >> ohci-platform
> >> [1.69] usb 1-1: device descriptor read/64, error -12
> >> [1.98] usb 1-1: device descriptor read/64, error -12
> >> [2.27] usb 1-1: new full-speed USB device number 3 using 
> >> ohci-platform
> >> [2.45] usb 1-1: device descriptor read/64, error -12
> >> [2.74] usb 1-1: device descriptor read/64, error -12
> >> [3.03] usb 1-1: new full-speed USB device number 4 using 
> >> ohci-platform
> >> [3.45] usb 1-1: device not accepting address 4, error -12
> >> [3.63] usb 1-1: new full-speed USB device number 5 using 
> >> ohci-platform
> >> [4.05] usb 1-1: device not accepting address 5, error -12
> >> [4.05] usb usb1-port1: unable to enumerate USB device
> >
> > -12 is -ENOMEM on your system?
> Yes
> >
> > ...
> >> which looks good. Anyone have an idea as to what's wrong or what the
> >> error messages mean.  I have nothing plugged into the USB ports.
> >
> > Nothing plugged into the USB ports?  Then why does the system think
> > something is plugged in?  It sounds like the driver is not accessing
> > the right registers.
> >
> >>  Also
> >> any ideas on where to start with debugging this would be appreciated.
> >
> > What do you see in /sys/kernel/debug/usb/ohci/*/registers?
> >
> root@sh7760:~# cat /sys/kernel/debug/usb/ohci/ohci-platform/registers
> bus platform, device ohci-platform
> Generic Platform OHCI controller
> ohci_hcd
> OHCI 1.0, NO legacy support registers, rh state running
> control 0x083 HCFS=operational CBSR=3
> cmdstatus 0x0 SOC=0
> intrstatus 0x0024 FNO SF
> intrenable 0x805a MIE RHSC UE RD WDH
> hcca frame 0x
> fmintvl 0xa7782edf FIT FSMPS=0xa778 FI=0x2edf
> fmremaining 0x88b0 FRT FR=0x08b0
> periodicstart 0x2a2f
> lsthresh 0x0628
> hub poll timer off
> roothub.a 02000202 POTPGT=2 NPS NDP=2(2)
> roothub.b  PPCM= DR=
> roothub.status 8002 DRWE OCI
> roothub.portstatus [0] 0x0101 PPS CCS
> roothub.portstatus [1] 0x0100 PPS
> 
> Does this look sane?

Yes, except for two things:

The OCI bit in roothub.status indicates that an OverCurrent
condition was present at some point.

The CCS bit in roothub.portstats[0] indicates that something
is plugged into port 1.  If there really is nothing plugged
into that port then this is a hardware failure.

The reason for the ENOMEM errors isn't clear.  You could try adding 
some debugging statements to see exactly where it comes from.  A likely 
spot is the call to hcd_alloc_coherent() in usb_hcd_map_urb_for_dma().

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: SuperH 7760 OHCI

[Martin Townsend]

On Thu, May 26, 2016 at 6:31 PM, Alan Stern  wrote:
> On Thu, 26 May 2016, Martin Townsend wrote:
>
>> I tried hacking in the relevant code straight into the OHCI platform driver
>> res_mem = platform_get_resource(dev, IORESOURCE_MEM, 1);
>> if (res_mem == NULL) {
>> dev_err(>dev, "no resource definition for memory\n");
>> err = -ENOENT;
>> goto err_power;
>> }
>>
>> if (!request_mem_region(res_mem->start, resource_size(res_mem),
>> dev->name)) {
>> dev_err(>dev, "request_mem_region failed\n");
>> err = -EBUSY;
>> goto err_power;
>> }
>>
>> /*
>>  * Use SH7760 Shared Memory
>>  */
>> if (!dma_declare_coherent_memory(>dev, res_mem->start,
>>  res_mem->start - res_mem->parent->start,
>>  resource_size(res_mem),
>>  DMA_MEMORY_MAP |
>>  DMA_MEMORY_EXCLUSIVE)) {
>> dev_err(>dev, "cannot declare coherent memory\n");
>> err = -ENXIO;
>> goto err_power;
>> }
>> and setting the HCD_MEMORY_LOCAL flag in the HC driver.
>
> Did you do this correctly?  That is, in the correct driver?
>
I put the code for the declaring the DMA coherent memory into ohci-platform.c
and set the flag in ohci-hcd.c

>> and I get the following error
>
> ...
>> [1.51] usb 1-1: new full-speed USB device number 2 using 
>> ohci-platform
>> [1.69] usb 1-1: device descriptor read/64, error -12
>> [1.98] usb 1-1: device descriptor read/64, error -12
>> [2.27] usb 1-1: new full-speed USB device number 3 using 
>> ohci-platform
>> [2.45] usb 1-1: device descriptor read/64, error -12
>> [2.74] usb 1-1: device descriptor read/64, error -12
>> [3.03] usb 1-1: new full-speed USB device number 4 using 
>> ohci-platform
>> [3.45] usb 1-1: device not accepting address 4, error -12
>> [3.63] usb 1-1: new full-speed USB device number 5 using 
>> ohci-platform
>> [4.05] usb 1-1: device not accepting address 5, error -12
>> [4.05] usb usb1-port1: unable to enumerate USB device
>
> -12 is -ENOMEM on your system?
Yes
>
> ...
>> which looks good. Anyone have an idea as to what's wrong or what the
>> error messages mean.  I have nothing plugged into the USB ports.
>
> Nothing plugged into the USB ports?  Then why does the system think
> something is plugged in?  It sounds like the driver is not accessing
> the right registers.
>
>>  Also
>> any ideas on where to start with debugging this would be appreciated.
>
> What do you see in /sys/kernel/debug/usb/ohci/*/registers?
>
root@sh7760:~# cat /sys/kernel/debug/usb/ohci/ohci-platform/registers
bus platform, device ohci-platform
Generic Platform OHCI controller
ohci_hcd
OHCI 1.0, NO legacy support registers, rh state running
control 0x083 HCFS=operational CBSR=3
cmdstatus 0x0 SOC=0
intrstatus 0x0024 FNO SF
intrenable 0x805a MIE RHSC UE RD WDH
hcca frame 0x
fmintvl 0xa7782edf FIT FSMPS=0xa778 FI=0x2edf
fmremaining 0x88b0 FRT FR=0x08b0
periodicstart 0x2a2f
lsthresh 0x0628
hub poll timer off
roothub.a 02000202 POTPGT=2 NPS NDP=2(2)
roothub.b  PPCM= DR=
roothub.status 8002 DRWE OCI
roothub.portstatus [0] 0x0101 PPS CCS
roothub.portstatus [1] 0x0100 PPS

Does this look sane?

Cheers,
Martin.
> 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: SuperH 7760 OHCI

[Alan Stern]

On Thu, 26 May 2016, Martin Townsend wrote:

> I tried hacking in the relevant code straight into the OHCI platform driver
> res_mem = platform_get_resource(dev, IORESOURCE_MEM, 1);
> if (res_mem == NULL) {
> dev_err(>dev, "no resource definition for memory\n");
> err = -ENOENT;
> goto err_power;
> }
> 
> if (!request_mem_region(res_mem->start, resource_size(res_mem),
> dev->name)) {
> dev_err(>dev, "request_mem_region failed\n");
> err = -EBUSY;
> goto err_power;
> }
> 
> /*
>  * Use SH7760 Shared Memory
>  */
> if (!dma_declare_coherent_memory(>dev, res_mem->start,
>  res_mem->start - res_mem->parent->start,
>  resource_size(res_mem),
>  DMA_MEMORY_MAP |
>  DMA_MEMORY_EXCLUSIVE)) {
> dev_err(>dev, "cannot declare coherent memory\n");
> err = -ENXIO;
> goto err_power;
> }
> and setting the HCD_MEMORY_LOCAL flag in the HC driver.

Did you do this correctly?  That is, in the correct driver?

> and I get the following error

...
> [1.51] usb 1-1: new full-speed USB device number 2 using ohci-platform
> [1.69] usb 1-1: device descriptor read/64, error -12
> [1.98] usb 1-1: device descriptor read/64, error -12
> [2.27] usb 1-1: new full-speed USB device number 3 using ohci-platform
> [2.45] usb 1-1: device descriptor read/64, error -12
> [2.74] usb 1-1: device descriptor read/64, error -12
> [3.03] usb 1-1: new full-speed USB device number 4 using ohci-platform
> [3.45] usb 1-1: device not accepting address 4, error -12
> [3.63] usb 1-1: new full-speed USB device number 5 using ohci-platform
> [4.05] usb 1-1: device not accepting address 5, error -12
> [4.05] usb usb1-port1: unable to enumerate USB device

-12 is -ENOMEM on your system?

...
> which looks good. Anyone have an idea as to what's wrong or what the
> error messages mean.  I have nothing plugged into the USB ports.

Nothing plugged into the USB ports?  Then why does the system think 
something is plugged in?  It sounds like the driver is not accessing 
the right registers.

>  Also
> any ideas on where to start with debugging this would be appreciated.

What do you see in /sys/kernel/debug/usb/ohci/*/registers?

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: SuperH 7760 OHCI

[Alan Stern]

On Thu, 26 May 2016, Martin Townsend wrote:

> On Thu, May 26, 2016 at 4:24 PM, Alan Stern  wrote:
> > On Thu, 26 May 2016, Martin Townsend wrote:
> >
> >> Hi,
> >>
> >> I'm currently trying to get the USB Host working on the SH7760.  I
> >> tried the platform driver to start with and get the following error on
> >> boot:
> >> [3.60] usb 1-1: new full-speed USB device number 2 using 
> >> ohci-platform
> >> [3.872000] ohci-platform ohci-platform: frame counter not updating; 
> >> disabled
> >> [3.872000] ohci-platform ohci-platform: HC died; cleaning up
> >>
> >> So I dug a bit further and see that the SH7760 driver in the 2.6
> >> kernel makes use of the 8KB shared memory for HCCA and ED/TD buffers.
> >> After looking through the code for the 4.1 Kernel I am currently
> >> trying to port to I think I need to write my own platform driver that
> >> calls dma_declare_coherent_memory so that the OHCI driver uses this
> >> 8KB shared memory.  Then set HCD_LOCAL_MEM in the hc_driver flags to
> >> ensure that it uses dma_alloc_coherent.  In other words copy what the
> >> ohci-sm501.c file is doing.  I just wanted to confirm that this is
> >> what I should be doing or is there a better generic way of telling the
> >> OHCI driver to use this 8KB shared memory.
> >
> > There isn't a generic way of doing it, but you could such a thing to
> > the ohci-platform driver.  That would be preferable to adding a new,
> > separate platform driver.
> >
> > Alan Stern
> >
> Hi Alan,
> 
> Thanks for the reply.  So would that entail looking for a second
> IORESOURCE_MEM resource and taking this as the device's shared memory
> and if present call dma_declare_coherent_memory?

Something like that.  Actually you would probably need to add something
to the usb_ohci_pdata structure to represent the memory resource.

> Also how would I or in the HCD_LOCAL_MEM to flags to the hc_driver
> struct that's in ohci-hcd.c?

Well, if you use ohci-platform.c then the hc_driver struct is in that 
file, not in ohci-hcd.c -- it's called ohci_platform_hc_driver.  You 
can modify that structure's flags in ohci_platform_probe.

The problem is that this one structure gets used by the ohci-platform
driver for all its OHCI controllers, so it would be impossible to have
one controller that uses local memory and another that doesn't, all in
the same system.

If that's not an issue for you then feel free to submit changes for 
ohci-platform.c.  If it is an issue then there's no choice but to 
create a new platform driver.

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: SuperH 7760 OHCI

[Martin Townsend]

On Thu, May 26, 2016 at 4:05 PM, Martin Townsend
 wrote:
> Hi,
>
> I'm currently trying to get the USB Host working on the SH7760.  I
> tried the platform driver to start with and get the following error on
> boot:
> [3.60] usb 1-1: new full-speed USB device number 2 using ohci-platform
> [3.872000] ohci-platform ohci-platform: frame counter not updating; 
> disabled
> [3.872000] ohci-platform ohci-platform: HC died; cleaning up
>
> So I dug a bit further and see that the SH7760 driver in the 2.6
> kernel makes use of the 8KB shared memory for HCCA and ED/TD buffers.
> After looking through the code for the 4.1 Kernel I am currently
> trying to port to I think I need to write my own platform driver that
> calls dma_declare_coherent_memory so that the OHCI driver uses this
> 8KB shared memory.  Then set HCD_LOCAL_MEM in the hc_driver flags to
> ensure that it uses dma_alloc_coherent.  In other words copy what the
> ohci-sm501.c file is doing.  I just wanted to confirm that this is
> what I should be doing or is there a better generic way of telling the
> OHCI driver to use this 8KB shared memory.
>

I tried hacking in the relevant code straight into the OHCI platform driver
res_mem = platform_get_resource(dev, IORESOURCE_MEM, 1);
if (res_mem == NULL) {
dev_err(>dev, "no resource definition for memory\n");
err = -ENOENT;
goto err_power;
}

if (!request_mem_region(res_mem->start, resource_size(res_mem),
dev->name)) {
dev_err(>dev, "request_mem_region failed\n");
err = -EBUSY;
goto err_power;
}

/*
 * Use SH7760 Shared Memory
 */
if (!dma_declare_coherent_memory(>dev, res_mem->start,
 res_mem->start - res_mem->parent->start,
 resource_size(res_mem),
 DMA_MEMORY_MAP |
 DMA_MEMORY_EXCLUSIVE)) {
dev_err(>dev, "cannot declare coherent memory\n");
err = -ENXIO;
goto err_power;
}
and setting the HCD_MEMORY_LOCAL flag in the HC driver.
and I get the following error
[1.04] Found resource sh7760-usb-irq
[1.06] Found resource sh7760-usb-shared-memory
[1.06] Found resource sh7760-usb-io-memory
[1.13] usb usb1: New USB device found, idVendor=1d6b, idProduct=0001
[1.13] usb usb1: New USB device strings: Mfr=3, Product=2,
SerialNumber=1
[1.13] usb usb1: Product: Generic Platform OHCI controller
[1.13] usb usb1: Manufacturer: Linux 4.1.17-yocto-standard ohci_hcd
[1.13] usb usb1: SerialNumber: ohci-platform
[1.13] device: 'usb1': device_add
[1.13] bus: 'usb': add device usb1
[1.13] bus: 'usb': driver_probe_device: matched device usb1
with driver usb
[1.13] bus: 'usb': really_probe: probing driver usb with device usb1
[1.13] bus: 'usb': add device 1-0:1.0
[1.13] bus: 'usb': driver_probe_device: matched device 1-0:1.0
with driver hub
[1.13] bus: 'usb': really_probe: probing driver hub with device 1-0:1.0
[1.13] device: 'usb1-port1': device_add
[1.13] device: 'usb1-port2': device_add
[1.13] bus: 'usb': really_probe: bound device 1-0:1.0 to driver hub
[1.13] driver: 'usb': driver_bound: bound to device 'usb1'
[1.13] bus: 'usb': really_probe: bound device usb1 to driver usb
[1.14] bus: 'usb': add driver usbhid
[1.14] usbcore: registered new interface driver usbhid
[1.14] usbhid: USB HID core driver
[1.51] usb 1-1: new full-speed USB device number 2 using ohci-platform
[1.69] usb 1-1: device descriptor read/64, error -12
[1.98] usb 1-1: device descriptor read/64, error -12
[2.27] usb 1-1: new full-speed USB device number 3 using ohci-platform
[2.45] usb 1-1: device descriptor read/64, error -12
[2.74] usb 1-1: device descriptor read/64, error -12
[3.03] usb 1-1: new full-speed USB device number 4 using ohci-platform
[3.45] usb 1-1: device not accepting address 4, error -12
[3.63] usb 1-1: new full-speed USB device number 5 using ohci-platform
[4.05] usb 1-1: device not accepting address 5, error -12
[4.05] usb usb1-port1: unable to enumerate USB device

Here's the board support definitions
/* No power control needed so a blank platform data */
static struct usb_ohci_pdata usb_ohci_pdata;
static u64 usb_ohci_dma_mask = 0x;


static struct resource sh7760_usb_resources[] = {
DEFINE_RES_MEM_NAMED(SH7760_USB_BASE, SH7760_USB_IOLEN,
 "sh7760-usb-io-memory"),
DEFINE_RES_MEM_NAMED(SH7760_USB_SHARED_MEM_BASE_ADDR,
 SH7760_USB_SHARED_MEM_LEN,
 "sh7760-usb-shared-memory"),
DEFINE_RES_IRQ_NAMED(USB_IRQ, "sh7760-usb-irq"),
};

static struct platform_device sh7760_usb_host_device = {
.name= "ohci-platform",
.id= -1,
.dev = {
.dma_mask= 

[PATCH v3] usb: gadget: fix spinlock dead lock in gadgetfs

[Bin Liu]

[   40.467381] =
[   40.473013] [ INFO: possible recursive locking detected ]
[   40.478651] 4.6.0-08691-g7f3db9a #37 Not tainted
[   40.483466] -
[   40.489098] usb/733 is trying to acquire lock:
[   40.493734]  (&(>lock)->rlock){-.}, at: [] 
ep0_complete+0x18/0xdc [gadgetfs]
[   40.502882]
[   40.502882] but task is already holding lock:
[   40.508967]  (&(>lock)->rlock){-.}, at: [] 
ep0_read+0x20/0x5e0 [gadgetfs]
[   40.517811]
[   40.517811] other info that might help us debug this:
[   40.524623]  Possible unsafe locking scenario:
[   40.524623]
[   40.530798]CPU0
[   40.533346]
[   40.535894]   lock(&(>lock)->rlock);
[   40.540088]   lock(&(>lock)->rlock);
[   40.544284]
[   40.544284]  *** DEADLOCK ***
[   40.544284]
[   40.550461]  May be due to missing lock nesting notation
[   40.550461]
[   40.557544] 2 locks held by usb/733:
[   40.561271]  #0:  (>f_pos_lock){+.+.+.}, at: [] 
__fdget_pos+0x40/0x48
[   40.569219]  #1:  (&(>lock)->rlock){-.}, at: [] 
ep0_read+0x20/0x5e0 [gadgetfs]
[   40.578523]
[   40.578523] stack backtrace:
[   40.583075] CPU: 0 PID: 733 Comm: usb Not tainted 4.6.0-08691-g7f3db9a #37
[   40.590246] Hardware name: Generic AM33XX (Flattened Device Tree)
[   40.596625] [] (unwind_backtrace) from [] 
(show_stack+0x10/0x14)
[   40.604718] [] (show_stack) from [] 
(dump_stack+0xb0/0xe4)
[   40.612267] [] (dump_stack) from [] 
(__lock_acquire+0xf68/0x1994)
[   40.620440] [] (__lock_acquire) from [] 
(lock_acquire+0xd8/0x238)
[   40.628621] [] (lock_acquire) from [] 
(_raw_spin_lock_irqsave+0x38/0x4c)
[   40.637440] [] (_raw_spin_lock_irqsave) from [] 
(ep0_complete+0x18/0xdc [gadgetfs])
[   40.647339] [] (ep0_complete [gadgetfs]) from [] 
(musb_g_giveback+0x118/0x1b0 [musb_hdrc])
[   40.657842] [] (musb_g_giveback [musb_hdrc]) from [] 
(musb_g_ep0_queue+0x16c/0x188 [musb_hdrc])
[   40.668772] [] (musb_g_ep0_queue [musb_hdrc]) from [] 
(ep0_read+0x544/0x5e0 [gadgetfs])
[   40.678963] [] (ep0_read [gadgetfs]) from [] 
(__vfs_read+0x20/0x110)
[   40.687414] [] (__vfs_read) from [] (vfs_read+0x88/0x114)
[   40.694864] [] (vfs_read) from [] (SyS_read+0x44/0x9c)
[   40.702051] [] (SyS_read) from [] 
(ret_fast_syscall+0x0/0x1c)

This is caused by the spinlock bug in ep0_read().
Fix the two other deadlock sources in gadgetfs_setup() too.

Cc:  # v3.16+
Signed-off-by: Bin Liu 
---
v3:
  - update commit comments mentioning the fix in gadgetfs_setup()
  - use spin_(un)lock() for fix in gadgetfs_setup(), instead of
spin_(un)lock_irq(), since locked by spin_lock()
v2:
  - lock protects setup_req(), only unlock for usb_ep_queue()
  - use GFP_KERNEL in usb_ep_queue()
  - fix the same in two places in gadgetfs_setup() too

 drivers/usb/gadget/legacy/inode.c | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/legacy/inode.c 
b/drivers/usb/gadget/legacy/inode.c
index e64479f..aa3707b 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c
@@ -938,8 +938,11 @@ ep0_read (struct file *fd, char __user *buf, size_t len, 
loff_t *ptr)
struct usb_ep   *ep = dev->gadget->ep0;
struct usb_request  *req = dev->req;
 
-   if ((retval = setup_req (ep, req, 0)) == 0)
-   retval = usb_ep_queue (ep, req, GFP_ATOMIC);
+   if ((retval = setup_req (ep, req, 0)) == 0) {
+   spin_unlock_irq (>lock);
+   retval = usb_ep_queue (ep, req, GFP_KERNEL);
+   spin_lock_irq (>lock);
+   }
dev->state = STATE_DEV_CONNECTED;
 
/* assume that was SET_CONFIGURATION */
@@ -1457,8 +1460,11 @@ delegate:
w_length);
if (value < 0)
break;
+
+   spin_unlock (>lock);
value = usb_ep_queue (gadget->ep0, dev->req,
-   GFP_ATOMIC);
+   GFP_KERNEL);
+   spin_lock (>lock);
if (value < 0) {
clean_req (gadget->ep0, dev->req);
break;
@@ -1481,11 +1487,14 @@ delegate:
if (value >= 0 && dev->state != STATE_DEV_SETUP) {
req->length = value;
req->zero = value < w_length;
-   value = usb_ep_queue (gadget->ep0, req, GFP_ATOMIC);
+
+   spin_unlock (>lock);
+   value = usb_ep_queue (gadget->ep0, req, GFP_KERNEL);
if (value < 0) 

Re: [PATCH v2] usb: gadget: fix spinlock dead lock in gadgetfs

[Bin Liu]

Hi,

On Thu, May 26, 2016 at 07:26:44PM +0300, Felipe Balbi wrote:
> 
> >> Bin Liu  writes:
> >> > On Wed, May 25, 2016 at 02:18:34PM -0500, Bin Liu wrote:
> >> >> [   40.467381] =
> >> >> [   40.473013] [ INFO: possible recursive locking detected ]
> >> >> [   40.478651] 4.6.0-08691-g7f3db9a #37 Not tainted
> >> >> [   40.483466] -
> >> >> [   40.489098] usb/733 is trying to acquire lock:
> >> >> [   40.493734]  (&(>lock)->rlock){-.}, at: [] 
> >> >> ep0_complete+0x18/0xdc [gadgetfs]
> >> >> [   40.502882]
> >> >> [   40.502882] but task is already holding lock:
> >> >> [   40.508967]  (&(>lock)->rlock){-.}, at: [] 
> >> >> ep0_read+0x20/0x5e0 [gadgetfs]
> >> >> [   40.517811]
> >> >> [   40.517811] other info that might help us debug this:
> >> >> [   40.524623]  Possible unsafe locking scenario:
> >> >> [   40.524623]
> >> >> [   40.530798]CPU0
> >> >> [   40.533346]
> >> >> [   40.535894]   lock(&(>lock)->rlock);
> >> >> [   40.540088]   lock(&(>lock)->rlock);
> >> >> [   40.544284]
> >> >> [   40.544284]  *** DEADLOCK ***
> >> >> [   40.544284]
> >> >> [   40.550461]  May be due to missing lock nesting notation
> >> >> [   40.550461]
> >> >> [   40.557544] 2 locks held by usb/733:
> >> >> [   40.561271]  #0:  (>f_pos_lock){+.+.+.}, at: [] 
> >> >> __fdget_pos+0x40/0x48
> >> >> [   40.569219]  #1:  (&(>lock)->rlock){-.}, at: [] 
> >> >> ep0_read+0x20/0x5e0 [gadgetfs]
> >> >> [   40.578523]
> >> >> [   40.578523] stack backtrace:
> >> >> [   40.583075] CPU: 0 PID: 733 Comm: usb Not tainted 
> >> >> 4.6.0-08691-g7f3db9a #37
> >> >> [   40.590246] Hardware name: Generic AM33XX (Flattened Device Tree)
> >> >> [   40.596625] [] (unwind_backtrace) from [] 
> >> >> (show_stack+0x10/0x14)
> >> >> [   40.604718] [] (show_stack) from [] 
> >> >> (dump_stack+0xb0/0xe4)
> >> >> [   40.612267] [] (dump_stack) from [] 
> >> >> (__lock_acquire+0xf68/0x1994)
> >> >> [   40.620440] [] (__lock_acquire) from [] 
> >> >> (lock_acquire+0xd8/0x238)
> >> >> [   40.628621] [] (lock_acquire) from [] 
> >> >> (_raw_spin_lock_irqsave+0x38/0x4c)
> >> >> [   40.637440] [] (_raw_spin_lock_irqsave) from [] 
> >> >> (ep0_complete+0x18/0xdc [gadgetfs])
> >> >> [   40.647339] [] (ep0_complete [gadgetfs]) from [] 
> >> >> (musb_g_giveback+0x118/0x1b0 [musb_hdrc])
> >> >> [   40.657842] [] (musb_g_giveback [musb_hdrc]) from 
> >> >> [] (musb_g_ep0_queue+0x16c/0x188 [musb_hdrc])
> >> >> [   40.668772] [] (musb_g_ep0_queue [musb_hdrc]) from 
> >> >> [] (ep0_read+0x544/0x5e0 [gadgetfs])
> >> >> [   40.678963] [] (ep0_read [gadgetfs]) from [] 
> >> >> (__vfs_read+0x20/0x110)
> >> >> [   40.687414] [] (__vfs_read) from [] 
> >> >> (vfs_read+0x88/0x114)
> >> >> [   40.694864] [] (vfs_read) from [] 
> >> >> (SyS_read+0x44/0x9c)
> >> >> [   40.702051] [] (SyS_read) from [] 
> >> >> (ret_fast_syscall+0x0/0x1c)
> >> >> 
> >> >> Cc:  # v3.16+
> >> >> Signed-off-by: Bin Liu 
> >> >> ---
> >> >> v2:
> >> >>   - lock protects setup_req(), only unlock for usb_ep_queue()
> >> >>   - use GFP_KERNEL in usb_ep_queue()
> >> >>   - fix the same in two places in gadgetfs_setup() too
> >> >
> >> > Never mind. Sent the patch too soon. It gives the following problem.
> >> >
> >> > [  179.810367] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:2724
> >> > _raw_spin_unlock_irq+0x24/0x2c
> >> > [  179.819719] DEBUG_LOCKS_WARN_ON(current->hardirq_context)
> >> 
> >> thanks for letting us know. I'm dropping this from my queue. Please
> >> resend final patch once you have it ;-)
> >
> > It turned out to be a very easy fix ;)
> >
> > I have v3 ready, but checkpatch.pl complains the followings. I don't
> > think this patch should fix them, since the whole driver is coded like
> > that. Any comments?
> 
> Let's fix it with a follow-up patch. Can you do that one too, btw? I can
> do it, if you don't wanna deal with that; but I'm also open to receiving
> free patches heh

I'd like to do it, but I have a pile of other suff to do. (I still
remember I owe you a g-zero test with dwc3...)

Regards,
-Bin.


--
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 v2] usb: gadget: fix spinlock dead lock in gadgetfs

[Felipe Balbi]

>> Bin Liu  writes:
>> > On Wed, May 25, 2016 at 02:18:34PM -0500, Bin Liu wrote:
>> >> [   40.467381] =
>> >> [   40.473013] [ INFO: possible recursive locking detected ]
>> >> [   40.478651] 4.6.0-08691-g7f3db9a #37 Not tainted
>> >> [   40.483466] -
>> >> [   40.489098] usb/733 is trying to acquire lock:
>> >> [   40.493734]  (&(>lock)->rlock){-.}, at: [] 
>> >> ep0_complete+0x18/0xdc [gadgetfs]
>> >> [   40.502882]
>> >> [   40.502882] but task is already holding lock:
>> >> [   40.508967]  (&(>lock)->rlock){-.}, at: [] 
>> >> ep0_read+0x20/0x5e0 [gadgetfs]
>> >> [   40.517811]
>> >> [   40.517811] other info that might help us debug this:
>> >> [   40.524623]  Possible unsafe locking scenario:
>> >> [   40.524623]
>> >> [   40.530798]CPU0
>> >> [   40.533346]
>> >> [   40.535894]   lock(&(>lock)->rlock);
>> >> [   40.540088]   lock(&(>lock)->rlock);
>> >> [   40.544284]
>> >> [   40.544284]  *** DEADLOCK ***
>> >> [   40.544284]
>> >> [   40.550461]  May be due to missing lock nesting notation
>> >> [   40.550461]
>> >> [   40.557544] 2 locks held by usb/733:
>> >> [   40.561271]  #0:  (>f_pos_lock){+.+.+.}, at: [] 
>> >> __fdget_pos+0x40/0x48
>> >> [   40.569219]  #1:  (&(>lock)->rlock){-.}, at: [] 
>> >> ep0_read+0x20/0x5e0 [gadgetfs]
>> >> [   40.578523]
>> >> [   40.578523] stack backtrace:
>> >> [   40.583075] CPU: 0 PID: 733 Comm: usb Not tainted 4.6.0-08691-g7f3db9a 
>> >> #37
>> >> [   40.590246] Hardware name: Generic AM33XX (Flattened Device Tree)
>> >> [   40.596625] [] (unwind_backtrace) from [] 
>> >> (show_stack+0x10/0x14)
>> >> [   40.604718] [] (show_stack) from [] 
>> >> (dump_stack+0xb0/0xe4)
>> >> [   40.612267] [] (dump_stack) from [] 
>> >> (__lock_acquire+0xf68/0x1994)
>> >> [   40.620440] [] (__lock_acquire) from [] 
>> >> (lock_acquire+0xd8/0x238)
>> >> [   40.628621] [] (lock_acquire) from [] 
>> >> (_raw_spin_lock_irqsave+0x38/0x4c)
>> >> [   40.637440] [] (_raw_spin_lock_irqsave) from [] 
>> >> (ep0_complete+0x18/0xdc [gadgetfs])
>> >> [   40.647339] [] (ep0_complete [gadgetfs]) from [] 
>> >> (musb_g_giveback+0x118/0x1b0 [musb_hdrc])
>> >> [   40.657842] [] (musb_g_giveback [musb_hdrc]) from 
>> >> [] (musb_g_ep0_queue+0x16c/0x188 [musb_hdrc])
>> >> [   40.668772] [] (musb_g_ep0_queue [musb_hdrc]) from 
>> >> [] (ep0_read+0x544/0x5e0 [gadgetfs])
>> >> [   40.678963] [] (ep0_read [gadgetfs]) from [] 
>> >> (__vfs_read+0x20/0x110)
>> >> [   40.687414] [] (__vfs_read) from [] 
>> >> (vfs_read+0x88/0x114)
>> >> [   40.694864] [] (vfs_read) from [] 
>> >> (SyS_read+0x44/0x9c)
>> >> [   40.702051] [] (SyS_read) from [] 
>> >> (ret_fast_syscall+0x0/0x1c)
>> >> 
>> >> Cc:  # v3.16+
>> >> Signed-off-by: Bin Liu 
>> >> ---
>> >> v2:
>> >>   - lock protects setup_req(), only unlock for usb_ep_queue()
>> >>   - use GFP_KERNEL in usb_ep_queue()
>> >>   - fix the same in two places in gadgetfs_setup() too
>> >
>> > Never mind. Sent the patch too soon. It gives the following problem.
>> >
>> > [  179.810367] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:2724
>> > _raw_spin_unlock_irq+0x24/0x2c
>> > [  179.819719] DEBUG_LOCKS_WARN_ON(current->hardirq_context)
>> 
>> thanks for letting us know. I'm dropping this from my queue. Please
>> resend final patch once you have it ;-)
>
> It turned out to be a very easy fix ;)
>
> I have v3 ready, but checkpatch.pl complains the followings. I don't
> think this patch should fix them, since the whole driver is coded like
> that. Any comments?

Let's fix it with a follow-up patch. Can you do that one too, btw? I can
do it, if you don't wanna deal with that; but I'm also open to receiving
free patches heh

-- 
balbi


signature.asc
Description: PGP signature

[RFC 3/5] ARM: tegra: Enable UDC on Beaver

[Thierry Reding]

From: Thierry Reding 

Override the compatible string of the first USB controller to enable
device mode.

Signed-off-by: Thierry Reding 
---
 arch/arm/boot/dts/tegra30-beaver.dts | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/tegra30-beaver.dts 
b/arch/arm/boot/dts/tegra30-beaver.dts
index 4ea3ecc10934..5f32bcd96fbc 100644
--- a/arch/arm/boot/dts/tegra30-beaver.dts
+++ b/arch/arm/boot/dts/tegra30-beaver.dts
@@ -1935,6 +1935,17 @@
non-removable;
};
 
+   usb@7d00 {
+   compatible = "nvidia,tegra30-udc";
+   status = "okay";
+   dr_mode = "otg";
+   };
+
+   usb-phy@7d00 {
+   status = "okay";
+   dr_mode = "otg";
+   };
+
usb@7d004000 {
status = "okay";
};
-- 
2.8.3

--
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: SuperH 7760 OHCI

[Martin Townsend]

On Thu, May 26, 2016 at 4:24 PM, Alan Stern  wrote:
> On Thu, 26 May 2016, Martin Townsend wrote:
>
>> Hi,
>>
>> I'm currently trying to get the USB Host working on the SH7760.  I
>> tried the platform driver to start with and get the following error on
>> boot:
>> [3.60] usb 1-1: new full-speed USB device number 2 using 
>> ohci-platform
>> [3.872000] ohci-platform ohci-platform: frame counter not updating; 
>> disabled
>> [3.872000] ohci-platform ohci-platform: HC died; cleaning up
>>
>> So I dug a bit further and see that the SH7760 driver in the 2.6
>> kernel makes use of the 8KB shared memory for HCCA and ED/TD buffers.
>> After looking through the code for the 4.1 Kernel I am currently
>> trying to port to I think I need to write my own platform driver that
>> calls dma_declare_coherent_memory so that the OHCI driver uses this
>> 8KB shared memory.  Then set HCD_LOCAL_MEM in the hc_driver flags to
>> ensure that it uses dma_alloc_coherent.  In other words copy what the
>> ohci-sm501.c file is doing.  I just wanted to confirm that this is
>> what I should be doing or is there a better generic way of telling the
>> OHCI driver to use this 8KB shared memory.
>
> There isn't a generic way of doing it, but you could such a thing to
> the ohci-platform driver.  That would be preferable to adding a new,
> separate platform driver.
>
> Alan Stern
>
Hi Alan,

Thanks for the reply.  So would that entail looking for a second
IORESOURCE_MEM resource and taking this as the device's shared memory
and if present call dma_declare_coherent_memory?
Also how would I or in the HCD_LOCAL_MEM to flags to the hc_driver
struct that's in ohci-hcd.c?

Cheers,
Martin.
--
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

[RFC 0/5] usb: chipidea: Add support for Tegra20 through Tegra124

[Thierry Reding]

From: Thierry Reding 

All Tegra SoC generations from Tegra20 through Tegra124 have a ChipIdea
USB device controller. This set of patches adds very rudimentary support
for it to the existing ChipIdea driver and enables them on the set of
boards that I could easily test on.

I'm sending this out as RFC because I'm not sure yet how to merge this.
While the driver seems to work fine (tested by exporting a USB driver or
eMMC via the mass storage function) I don't yet understand how to make
the driver switch between host and device modes dynamically. It might be
useful to get this merged before, but I'd like to have some feedback on
this, because doing so would mean that we need to use device mode on the
devices where it's enabled and can't use the USBD port in host mode.

Thierry

Thierry Reding (5):
  usb: chipidea: Add support for Tegra20/30/114/124
  ARM: tegra: Enable UDC on TrimSlice
  ARM: tegra: Enable UDC on Beaver
  ARM: tegra: Enable UDC on Dalmore
  ARM: tegra: Enable UDC on Jetson TK1

 arch/arm/boot/dts/tegra114-dalmore.dts|  11 +++
 arch/arm/boot/dts/tegra124-jetson-tk1.dts |  13 +++-
 arch/arm/boot/dts/tegra20-trimslice.dts   |   3 +
 arch/arm/boot/dts/tegra30-beaver.dts  |  11 +++
 drivers/usb/chipidea/Makefile |   1 +
 drivers/usb/chipidea/ci_hdrc_tegra.c  | 109 ++
 6 files changed, 147 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/chipidea/ci_hdrc_tegra.c

-- 
2.8.3

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

[RFC 4/5] ARM: tegra: Enable UDC on Dalmore

[Thierry Reding]

From: Thierry Reding 

Override the compatible string of the first USB controller to enable
device mode.

Signed-off-by: Thierry Reding 
---
 arch/arm/boot/dts/tegra114-dalmore.dts | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts 
b/arch/arm/boot/dts/tegra114-dalmore.dts
index c970bf65c74c..53d664f718ff 100644
--- a/arch/arm/boot/dts/tegra114-dalmore.dts
+++ b/arch/arm/boot/dts/tegra114-dalmore.dts
@@ -1122,6 +1122,17 @@
non-removable;
};
 
+   usb@7d00 {
+   compatible = "nvidia,tegra114-udc";
+   status = "okay";
+   dr_mode = "otg";
+   };
+
+   usb-phy@7d00 {
+   status = "okay";
+   dr_mode = "otg";
+   };
+
usb@7d008000 {
status = "okay";
};
-- 
2.8.3

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

[RFC 1/5] usb: chipidea: Add support for Tegra20/30/114/124

[Thierry Reding]

From: Thierry Reding 

All of these Tegra SoC generations have a ChipIdea UDC IP block that can
be used for device mode communication with a host. Implement rudimentary
support that doesn't allow switching between host and device modes.

Signed-off-by: Thierry Reding 
---
 drivers/usb/chipidea/Makefile|   1 +
 drivers/usb/chipidea/ci_hdrc_tegra.c | 109 +++
 2 files changed, 110 insertions(+)
 create mode 100644 drivers/usb/chipidea/ci_hdrc_tegra.c

diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
index 518e445476c3..3532df6561d9 100644
--- a/drivers/usb/chipidea/Makefile
+++ b/drivers/usb/chipidea/Makefile
@@ -10,6 +10,7 @@ ci_hdrc-$(CONFIG_USB_OTG_FSM) += otg_fsm.o
 obj-$(CONFIG_USB_CHIPIDEA) += ci_hdrc_usb2.o
 obj-$(CONFIG_USB_CHIPIDEA) += ci_hdrc_msm.o
 obj-$(CONFIG_USB_CHIPIDEA) += ci_hdrc_zevio.o
+obj-$(CONFIG_USB_CHIPIDEA) += ci_hdrc_tegra.o
 
 obj-$(CONFIG_USB_CHIPIDEA_PCI) += ci_hdrc_pci.o
 
diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c 
b/drivers/usb/chipidea/ci_hdrc_tegra.c
new file mode 100644
index ..d3cd68441856
--- /dev/null
+++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
@@ -0,0 +1,109 @@
+/*
+ * Copyright (c) 2016, NVIDIA Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#include "ci.h"
+
+struct tegra_udc {
+   struct ci_hdrc_platform_data data;
+   struct platform_device *dev;
+
+   struct usb_phy *phy;
+   struct clk *clk;
+};
+
+static const struct of_device_id tegra_udc_of_match[] = {
+   { .compatible = "nvidia,tegra20-udc" },
+   { .compatible = "nvidia,tegra30-udc" },
+   { .compatible = "nvidia,tegra114-udc" },
+   { .compatible = "nvidia,tegra124-udc" },
+   { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, tegra_udc_of_match);
+
+static int tegra_udc_probe(struct platform_device *pdev)
+{
+   struct tegra_udc *udc;
+   int err;
+
+   udc = devm_kzalloc(>dev, sizeof(*udc), GFP_KERNEL);
+   if (!udc)
+   return -ENOMEM;
+
+   udc->phy = devm_usb_get_phy_by_phandle(>dev, "nvidia,phy", 0);
+   if (IS_ERR(udc->phy)) {
+   err = PTR_ERR(udc->phy);
+   dev_err(>dev, "failed to get PHY: %d\n", err);
+   return err;
+   }
+
+   udc->clk = devm_clk_get(>dev, NULL);
+   if (IS_ERR(udc->clk)) {
+   err = PTR_ERR(udc->clk);
+   dev_err(>dev, "failed to get clock: %d\n", err);
+   return err;
+   }
+
+   err = clk_prepare_enable(udc->clk);
+   if (err < 0) {
+   dev_err(>dev, "failed to enable clock: %d\n", err);
+   return err;
+   }
+
+   /* setup and register ChipIdea HDRC device */
+   udc->data.name = "tegra-udc";
+   udc->data.capoffset = DEF_CAPOFFSET;
+   udc->data.flags = 0;
+   udc->data.usb_phy = udc->phy;
+
+   udc->dev = ci_hdrc_add_device(>dev, pdev->resource,
+ pdev->num_resources, >data);
+   if (IS_ERR(udc->dev)) {
+   err = PTR_ERR(udc->dev);
+   dev_err(>dev, "failed to add HDRC device: %d\n", err);
+   goto disable_clock;
+   }
+
+   platform_set_drvdata(pdev, udc);
+
+   return 0;
+
+disable_clock:
+   clk_disable_unprepare(udc->clk);
+   return err;
+}
+
+static int tegra_udc_remove(struct platform_device *pdev)
+{
+   struct tegra_udc *udc = platform_get_drvdata(pdev);
+
+   clk_disable_unprepare(udc->clk);
+
+   return 0;
+}
+
+static struct platform_driver tegra_udc_driver = {
+   .driver = {
+   .name = "tegra-udc",
+   .of_match_table = tegra_udc_of_match,
+   },
+   .probe = tegra_udc_probe,
+   .remove = tegra_udc_remove,
+};
+module_platform_driver(tegra_udc_driver);
+
+MODULE_DESCRIPTION("NVIDIA Tegra USB device mode driver");
+MODULE_AUTHOR("Thierry Reding ");
+MODULE_ALIAS("platform:tegra-udc");
+MODULE_LICENSE("GPL v2");
-- 
2.8.3

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

[RFC 2/5] ARM: tegra: Enable UDC on TrimSlice

[Thierry Reding]

From: Thierry Reding 

Override the compatible string of the first USB controller to enable
device mode.

Signed-off-by: Thierry Reding 
---
 arch/arm/boot/dts/tegra20-trimslice.dts | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/boot/dts/tegra20-trimslice.dts 
b/arch/arm/boot/dts/tegra20-trimslice.dts
index 4a035f74043a..5496f3dc089c 100644
--- a/arch/arm/boot/dts/tegra20-trimslice.dts
+++ b/arch/arm/boot/dts/tegra20-trimslice.dts
@@ -336,11 +336,14 @@
};
 
usb@c500 {
+   compatible = "nvidia,tegra20-udc";
status = "okay";
+   dr_mode = "otg";
};
 
usb-phy@c500 {
status = "okay";
+   dr_mode = "otg";
vbus-supply = <_reg>;
};
 
-- 
2.8.3

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

[RFC 5/5] ARM: tegra: Enable UDC on Jetson TK1

[Thierry Reding]

From: Thierry Reding 

Override the compatible string of the first USB controller to enable
device mode.

Signed-off-by: Thierry Reding 
---
 arch/arm/boot/dts/tegra124-jetson-tk1.dts | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts 
b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
index 941f36263c8f..30e92cc85b86 100644
--- a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
+++ b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
@@ -1726,7 +1726,7 @@
 
lanes {
usb2-0 {
-   nvidia,function = "xusb";
+   nvidia,function = "snps";
status = "okay";
};
 
@@ -1833,6 +1833,17 @@
};
};
 
+   usb@0,7d00 {
+   compatible = "nvidia,tegra124-udc";
+   status = "okay";
+   dr_mode = "otg";
+   };
+
+   usb-phy@0,7d00 {
+   status = "okay";
+   dr_mode = "otg";
+   };
+
/* mini-PCIe USB */
usb@0,7d004000 {
status = "okay";
-- 
2.8.3

--
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 v2] usb: gadget: fix spinlock dead lock in gadgetfs

[Bin Liu]

Hi,

On Thu, May 26, 2016 at 09:27:26AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Bin Liu  writes:
> > On Wed, May 25, 2016 at 02:18:34PM -0500, Bin Liu wrote:
> >> [   40.467381] =
> >> [   40.473013] [ INFO: possible recursive locking detected ]
> >> [   40.478651] 4.6.0-08691-g7f3db9a #37 Not tainted
> >> [   40.483466] -
> >> [   40.489098] usb/733 is trying to acquire lock:
> >> [   40.493734]  (&(>lock)->rlock){-.}, at: [] 
> >> ep0_complete+0x18/0xdc [gadgetfs]
> >> [   40.502882]
> >> [   40.502882] but task is already holding lock:
> >> [   40.508967]  (&(>lock)->rlock){-.}, at: [] 
> >> ep0_read+0x20/0x5e0 [gadgetfs]
> >> [   40.517811]
> >> [   40.517811] other info that might help us debug this:
> >> [   40.524623]  Possible unsafe locking scenario:
> >> [   40.524623]
> >> [   40.530798]CPU0
> >> [   40.533346]
> >> [   40.535894]   lock(&(>lock)->rlock);
> >> [   40.540088]   lock(&(>lock)->rlock);
> >> [   40.544284]
> >> [   40.544284]  *** DEADLOCK ***
> >> [   40.544284]
> >> [   40.550461]  May be due to missing lock nesting notation
> >> [   40.550461]
> >> [   40.557544] 2 locks held by usb/733:
> >> [   40.561271]  #0:  (>f_pos_lock){+.+.+.}, at: [] 
> >> __fdget_pos+0x40/0x48
> >> [   40.569219]  #1:  (&(>lock)->rlock){-.}, at: [] 
> >> ep0_read+0x20/0x5e0 [gadgetfs]
> >> [   40.578523]
> >> [   40.578523] stack backtrace:
> >> [   40.583075] CPU: 0 PID: 733 Comm: usb Not tainted 4.6.0-08691-g7f3db9a 
> >> #37
> >> [   40.590246] Hardware name: Generic AM33XX (Flattened Device Tree)
> >> [   40.596625] [] (unwind_backtrace) from [] 
> >> (show_stack+0x10/0x14)
> >> [   40.604718] [] (show_stack) from [] 
> >> (dump_stack+0xb0/0xe4)
> >> [   40.612267] [] (dump_stack) from [] 
> >> (__lock_acquire+0xf68/0x1994)
> >> [   40.620440] [] (__lock_acquire) from [] 
> >> (lock_acquire+0xd8/0x238)
> >> [   40.628621] [] (lock_acquire) from [] 
> >> (_raw_spin_lock_irqsave+0x38/0x4c)
> >> [   40.637440] [] (_raw_spin_lock_irqsave) from [] 
> >> (ep0_complete+0x18/0xdc [gadgetfs])
> >> [   40.647339] [] (ep0_complete [gadgetfs]) from [] 
> >> (musb_g_giveback+0x118/0x1b0 [musb_hdrc])
> >> [   40.657842] [] (musb_g_giveback [musb_hdrc]) from 
> >> [] (musb_g_ep0_queue+0x16c/0x188 [musb_hdrc])
> >> [   40.668772] [] (musb_g_ep0_queue [musb_hdrc]) from 
> >> [] (ep0_read+0x544/0x5e0 [gadgetfs])
> >> [   40.678963] [] (ep0_read [gadgetfs]) from [] 
> >> (__vfs_read+0x20/0x110)
> >> [   40.687414] [] (__vfs_read) from [] 
> >> (vfs_read+0x88/0x114)
> >> [   40.694864] [] (vfs_read) from [] 
> >> (SyS_read+0x44/0x9c)
> >> [   40.702051] [] (SyS_read) from [] 
> >> (ret_fast_syscall+0x0/0x1c)
> >> 
> >> Cc:  # v3.16+
> >> Signed-off-by: Bin Liu 
> >> ---
> >> v2:
> >>   - lock protects setup_req(), only unlock for usb_ep_queue()
> >>   - use GFP_KERNEL in usb_ep_queue()
> >>   - fix the same in two places in gadgetfs_setup() too
> >
> > Never mind. Sent the patch too soon. It gives the following problem.
> >
> > [  179.810367] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:2724
> > _raw_spin_unlock_irq+0x24/0x2c
> > [  179.819719] DEBUG_LOCKS_WARN_ON(current->hardirq_context)
> 
> thanks for letting us know. I'm dropping this from my queue. Please
> resend final patch once you have it ;-)

It turned out to be a very easy fix ;)

I have v3 ready, but checkpatch.pl complains the followings. I don't
think this patch should fix them, since the whole driver is coded like
that. Any comments?

WARNING: space prohibited between function name and open parenthesis '('
#68: FILE: drivers/usb/gadget/legacy/inode.c:941:
+   if ((retval = setup_req (ep, req, 0)) == 0) {

ERROR: do not use assignment in if condition
#68: FILE: drivers/usb/gadget/legacy/inode.c:941:
+   if ((retval = setup_req (ep, req, 0)) == 0) {

WARNING: space prohibited between function name and open parenthesis '('
#69: FILE: drivers/usb/gadget/legacy/inode.c:942:
+   spin_unlock_irq (>lock);

WARNING: space prohibited between function name and open parenthesis '('
#70: FILE: drivers/usb/gadget/legacy/inode.c:943:
+   retval = usb_ep_queue (ep, req, GFP_KERNEL);

WARNING: space prohibited between function name and open parenthesis '('
#71: FILE: drivers/usb/gadget/legacy/inode.c:944:
+   spin_lock_irq (>lock);

WARNING: space prohibited between function name and open parenthesis '('
#81: FILE: drivers/usb/gadget/legacy/inode.c:1464:
+   spin_unlock (>lock);

WARNING: space prohibited between function name and open parenthesis '('
#85: FILE: drivers/usb/gadget/legacy/inode.c:1467:
+   spin_lock (>lock);

WARNING: space prohibited between function name and open parenthesis '('
#95: FILE: 

Re: SuperH 7760 OHCI

[Alan Stern]

On Thu, 26 May 2016, Martin Townsend wrote:

> Hi,
> 
> I'm currently trying to get the USB Host working on the SH7760.  I
> tried the platform driver to start with and get the following error on
> boot:
> [3.60] usb 1-1: new full-speed USB device number 2 using ohci-platform
> [3.872000] ohci-platform ohci-platform: frame counter not updating; 
> disabled
> [3.872000] ohci-platform ohci-platform: HC died; cleaning up
> 
> So I dug a bit further and see that the SH7760 driver in the 2.6
> kernel makes use of the 8KB shared memory for HCCA and ED/TD buffers.
> After looking through the code for the 4.1 Kernel I am currently
> trying to port to I think I need to write my own platform driver that
> calls dma_declare_coherent_memory so that the OHCI driver uses this
> 8KB shared memory.  Then set HCD_LOCAL_MEM in the hc_driver flags to
> ensure that it uses dma_alloc_coherent.  In other words copy what the
> ohci-sm501.c file is doing.  I just wanted to confirm that this is
> what I should be doing or is there a better generic way of telling the
> OHCI driver to use this 8KB shared memory.

There isn't a generic way of doing it, but you could such a thing to 
the ohci-platform driver.  That would be preferable to adding a new, 
separate platform driver.

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 v4 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Thierry Reding]

On Thu, May 26, 2016 at 05:23:30PM +0200, Thierry Reding wrote:
> From: Thierry Reding 
> 
> Starting with commit 0b52297f2288 ("reset: Add support for shared reset
> controls") there is a reference count for reset control assertions. The
> goal is to allow resets to be shared by multiple devices and an assert
> will take effect only when all instances have asserted the reset.
> 
> In order to preserve backwards-compatibility, all reset controls become
> exclusive by default. This is to ensure that reset_control_assert() can
> immediately assert in hardware.
> 
> However, this new behaviour triggers the following warning in the EHCI
> driver for Tegra:
> 
> [3.365019] [ cut here ]
> [3.369639] WARNING: CPU: 0 PID: 1 at drivers/reset/core.c:187 
> __of_reset_control_get+0x16c/0x23c
> [3.382151] Modules linked in:
> [3.385214] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 
> 4.6.0-rc6-next-20160503 #140
> [3.392769] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
> [3.399046] [] (unwind_backtrace) from [] 
> (show_stack+0x10/0x14)
> [3.406787] [] (show_stack) from [] 
> (dump_stack+0x90/0xa4)
> [3.414007] [] (dump_stack) from [] (__warn+0xe8/0x100)
> [3.420964] [] (__warn) from [] 
> (warn_slowpath_null+0x20/0x28)
> [3.428525] [] (warn_slowpath_null) from [] 
> (__of_reset_control_get+0x16c/0x23c)
> [3.437648] [] (__of_reset_control_get) from [] 
> (tegra_ehci_probe+0x394/0x518)
> [3.446600] [] (tegra_ehci_probe) from [] 
> (platform_drv_probe+0x4c/0xb0)
> [3.455029] [] (platform_drv_probe) from [] 
> (driver_probe_device+0x1ec/0x330)
> [3.463892] [] (driver_probe_device) from [] 
> (__driver_attach+0xb8/0xbc)
> [3.472320] [] (__driver_attach) from [] 
> (bus_for_each_dev+0x68/0x9c)
> [3.480489] [] (bus_for_each_dev) from [] 
> (bus_add_driver+0x1a0/0x218)
> [3.488743] [] (bus_add_driver) from [] 
> (driver_register+0x78/0xf8)
> [3.496738] [] (driver_register) from [] 
> (do_one_initcall+0x40/0x170)
> [3.504909] [] (do_one_initcall) from [] 
> (kernel_init_freeable+0x158/0x1f8)
> [3.513600] [] (kernel_init_freeable) from [] 
> (kernel_init+0x8/0x114)
> [3.521770] [] (kernel_init) from [] 
> (ret_from_fork+0x14/0x3c)
> [3.529361] ---[ end trace 4bda87dbe4ecef8a ]---
> 
> The reason is that Tegra SoCs have three EHCI controllers, each with a
> separate reset line. However the first controller contains UTMI pads
> configuration registers that are shared with its siblings and that are
> reset as part of the first controller's reset. There is special code in
> the driver to assert and deassert this shared reset at probe time, and
> it does so irrespective of which controller is probed first to ensure
> that these shared registers are reset before any of the controllers are
> initialized. Unfortunately this means that if the first controller gets
> probed first, it will request its own reset line and will subsequently
> request the same reset line again (temporarily) to perform the reset.
> This used to work fine before the above-mentioned commit, but now
> triggers the new WARN.
> 
> Work around this by making sure we reuse the controller's reset if the
> controller happens to be the first controller.
> 
> Cc: Philipp Zabel 
> Cc: Hans de Goede 
> Reviewed-by: Philipp Zabel 
> Signed-off-by: Thierry Reding 
> ---
> Changes in v4:
> - avoid calling reset_control_put() on ERR_PTR()-encoded error codes
> 
> Changes in v3:
> - reword commit message to more accurately describe the hardware design
> 
> Changes in v2:
> - restore has_utmi_pad_registers condition (Alan Stern)
> 
>  drivers/usb/host/ehci-tegra.c | 16 +---
>  1 file changed, 13 insertions(+), 3 deletions(-)

Alan, Greg,

I forgot to mention, but it'd be great if this could go into v4.7
because the reset framework commit that triggers this was merged into
Linus' tree last week.

Thierry


signature.asc
Description: PGP signature

[PATCH v4 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Thierry Reding]

From: Thierry Reding 

Starting with commit 0b52297f2288 ("reset: Add support for shared reset
controls") there is a reference count for reset control assertions. The
goal is to allow resets to be shared by multiple devices and an assert
will take effect only when all instances have asserted the reset.

In order to preserve backwards-compatibility, all reset controls become
exclusive by default. This is to ensure that reset_control_assert() can
immediately assert in hardware.

However, this new behaviour triggers the following warning in the EHCI
driver for Tegra:

[3.365019] [ cut here ]
[3.369639] WARNING: CPU: 0 PID: 1 at drivers/reset/core.c:187 
__of_reset_control_get+0x16c/0x23c
[3.382151] Modules linked in:
[3.385214] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 
4.6.0-rc6-next-20160503 #140
[3.392769] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[3.399046] [] (unwind_backtrace) from [] 
(show_stack+0x10/0x14)
[3.406787] [] (show_stack) from [] 
(dump_stack+0x90/0xa4)
[3.414007] [] (dump_stack) from [] (__warn+0xe8/0x100)
[3.420964] [] (__warn) from [] 
(warn_slowpath_null+0x20/0x28)
[3.428525] [] (warn_slowpath_null) from [] 
(__of_reset_control_get+0x16c/0x23c)
[3.437648] [] (__of_reset_control_get) from [] 
(tegra_ehci_probe+0x394/0x518)
[3.446600] [] (tegra_ehci_probe) from [] 
(platform_drv_probe+0x4c/0xb0)
[3.455029] [] (platform_drv_probe) from [] 
(driver_probe_device+0x1ec/0x330)
[3.463892] [] (driver_probe_device) from [] 
(__driver_attach+0xb8/0xbc)
[3.472320] [] (__driver_attach) from [] 
(bus_for_each_dev+0x68/0x9c)
[3.480489] [] (bus_for_each_dev) from [] 
(bus_add_driver+0x1a0/0x218)
[3.488743] [] (bus_add_driver) from [] 
(driver_register+0x78/0xf8)
[3.496738] [] (driver_register) from [] 
(do_one_initcall+0x40/0x170)
[3.504909] [] (do_one_initcall) from [] 
(kernel_init_freeable+0x158/0x1f8)
[3.513600] [] (kernel_init_freeable) from [] 
(kernel_init+0x8/0x114)
[3.521770] [] (kernel_init) from [] 
(ret_from_fork+0x14/0x3c)
[3.529361] ---[ end trace 4bda87dbe4ecef8a ]---

The reason is that Tegra SoCs have three EHCI controllers, each with a
separate reset line. However the first controller contains UTMI pads
configuration registers that are shared with its siblings and that are
reset as part of the first controller's reset. There is special code in
the driver to assert and deassert this shared reset at probe time, and
it does so irrespective of which controller is probed first to ensure
that these shared registers are reset before any of the controllers are
initialized. Unfortunately this means that if the first controller gets
probed first, it will request its own reset line and will subsequently
request the same reset line again (temporarily) to perform the reset.
This used to work fine before the above-mentioned commit, but now
triggers the new WARN.

Work around this by making sure we reuse the controller's reset if the
controller happens to be the first controller.

Cc: Philipp Zabel 
Cc: Hans de Goede 
Reviewed-by: Philipp Zabel 
Signed-off-by: Thierry Reding 
---
Changes in v4:
- avoid calling reset_control_put() on ERR_PTR()-encoded error codes

Changes in v3:
- reword commit message to more accurately describe the hardware design

Changes in v2:
- restore has_utmi_pad_registers condition (Alan Stern)

 drivers/usb/host/ehci-tegra.c | 16 +---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index c1c1024a054c..9a3d7db5be57 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct 
platform_device *pdev)
struct usb_hcd *hcd = platform_get_drvdata(pdev);
struct tegra_ehci_hcd *tegra =
(struct tegra_ehci_hcd *)hcd_to_ehci(hcd)->priv;
+   bool has_utmi_pad_registers = false;
 
phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
if (!phy_np)
return -ENOENT;
 
+   if (of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"))
+   has_utmi_pad_registers = true;
+
if (!usb1_reset_attempted) {
struct reset_control *usb1_reset;
 
-   usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
+   if (!has_utmi_pad_registers)
+   usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
+   else
+   usb1_reset = tegra->rst;
+
if (IS_ERR(usb1_reset)) {
dev_warn(>dev,
 "can't get utmi-pads reset from the PHY\n");
@@ -99,13 +107,15 @@ static int tegra_reset_usb_controller(struct 
platform_device *pdev)

[PATCH v4 1/2] usb: host: ehci-tegra: Grab the correct UTMI pads reset

[Thierry Reding]

From: Thierry Reding 

There are three EHCI controllers on Tegra SoCs, each with its own reset
line. However, the first controller contains a set of UTMI configuration
registers that are shared with its siblings. These registers will only
be reset as part of the first controller's reset. For proper operation
it must be ensured that the UTMI configuration registers are reset
before any of the EHCI controllers are enabled, irrespective of the
probe order.

Commit a47cc24cd1e5 ("USB: EHCI: tegra: Fix probe order issue leading to
broken USB") introduced code that ensures the first controller is always
reset before setting up any of the controllers, and is never again reset
afterwards.

This code, however, grabs the wrong reset. Each EHCI controller has two
reset controls attached: 1) the USB controller reset and 2) the UTMI
pads reset (really the first controller's reset). In order to reset the
UTMI pads registers the code must grab the second reset, but instead it
grabbing the first.

Fixes: a47cc24cd1e5 ("USB: EHCI: tegra: Fix probe order issue leading to broken 
USB")
Acked-by: Jon Hunter 
Cc: sta...@vger.kernel.org
Signed-off-by: Thierry Reding 
---
Changes in v4:
- add sta...@vger.kernel.org to stable and add Fixes: tag
- add Jon's Acked-by

 drivers/usb/host/ehci-tegra.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index 4031b372008e..c1c1024a054c 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -89,7 +89,7 @@ static int tegra_reset_usb_controller(struct platform_device 
*pdev)
if (!usb1_reset_attempted) {
struct reset_control *usb1_reset;
 
-   usb1_reset = of_reset_control_get(phy_np, "usb");
+   usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
if (IS_ERR(usb1_reset)) {
dev_warn(>dev,
 "can't get utmi-pads reset from the PHY\n");
-- 
2.8.3

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

SuperH 7760 OHCI

[Martin Townsend]

Hi,

I'm currently trying to get the USB Host working on the SH7760.  I
tried the platform driver to start with and get the following error on
boot:
[3.60] usb 1-1: new full-speed USB device number 2 using ohci-platform
[3.872000] ohci-platform ohci-platform: frame counter not updating; disabled
[3.872000] ohci-platform ohci-platform: HC died; cleaning up

So I dug a bit further and see that the SH7760 driver in the 2.6
kernel makes use of the 8KB shared memory for HCCA and ED/TD buffers.
After looking through the code for the 4.1 Kernel I am currently
trying to port to I think I need to write my own platform driver that
calls dma_declare_coherent_memory so that the OHCI driver uses this
8KB shared memory.  Then set HCD_LOCAL_MEM in the hc_driver flags to
ensure that it uses dma_alloc_coherent.  In other words copy what the
ohci-sm501.c file is doing.  I just wanted to confirm that this is
what I should be doing or is there a better generic way of telling the
OHCI driver to use this 8KB shared memory.

Thanks in advance,
Martin.
--
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 v4] input: tablet: add Pegasus Notetaker tablet driver

[Martin Kepplinger]

Am 2016-05-26 um 02:29 schrieb Dmitry Torokhov:
> Hi Martin,
> 
> On Wed, May 25, 2016 at 09:44:34AM +0200, Martin Kepplinger wrote:
>> This adds a driver for the Pegasus Notetaker Pen. When connected,
>> this uses the Pen as an input tablet.
>>
>> This device was sold in various different brandings, for example
>>  "Pegasus Mobile Notetaker M210",
>>  "Genie e-note The Notetaker",
>>  "Staedtler Digital ballpoint pen 990 01",
>>  "IRISnotes Express" or
>>  "NEWLink Digital Note Taker".
>>
>> Here's an example, so that you know what we are talking about:
>> http://www.staedtler.com/en/products/ink-writing-instruments/ballpoint-pens/digital-pen-990-01-digital-ballpoint-pen
>>
>> http://pegatech.blogspot.com/ seems to be a remaining official resource.
>>
>> This device can also transfer saved (offline recorded handwritten) data and
>> there are userspace programs that do this, see https://launchpad.net/m210
>> (Well, alternatively there are really fast scanners out there :)
>>
>> It's *really* fun to use as an input tablet though! So let's support this
>> for everybody.
>>
>> There's no way to disable the device. When the pen is out of range, we just
>> don't get any URBs and don't do anything.
>> Like all other mouses or input tablets, we don't use runtime PM.
>>
>> Signed-off-by: Martin Kepplinger 
>> ---
>>
>> Thanks for having a look. Any more suggestions on this?
>>
>> revision history
>> 
>> v4 use normal work queue instead of a kernel thread (thanks to Oliver Neukum)
>> v3 fix reporting low pen battery and add USB list to CC
>> v2 minor cleanup (remove unnecessary variables)
>> v1 initial release
>>

Hi Dmitry,

thanks for the excellent review! All your suggestions seem valuable so
far and I'll prepare a new version for you.

>> +
>> +static int pegasus_open(struct input_dev *dev)
>> +{
>> +struct pegasus *pegasus = input_get_drvdata(dev);
>> +
>> +pegasus->irq->dev = pegasus->usbdev;
> 
> Is this assignment really needed?
> 

If we set URB_NO_TRANSFER_DMA_MAP and use transfer_dma, yes.


thanks again,

  martin
--
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] Re: Endless "supply vcc not found, using dummy regulator"

[Steinar H. Gunderson]

On Wed, May 25, 2016 at 07:52:36PM +0200, Steinar H. Gunderson wrote:
>> Actually their are some missing patches to tune the usb3 phy.
>> 
>> https://lkml.org/lkml/2014/10/31/266
> This explains why the default networking speed refused to go above
> ~300 Mbit/sec! What happened to the patches, I wonder?

Adding Vivek in case he knows. They certainly don't apply anymore, at least.

/* Steinar */
-- 
Homepage: https://www.sesse.net/
--
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

[RFC PATCH v3 4/6] xhci: don't rely on precalculated value of needed trbs in the enqueue loop

[Mathias Nyman]

Queue trbs until all payload data in the urb is tranferred.

The actual number of trbs might need to change from the pre-calculated
number when the packet alignment restrictions for td fragments in
xhci 4.11.7.1 are taken into account.

Long term plan is to get rid of calculating the needed trbs in advance
all together. It's an unnecessary extra walk through the scatterlist.

This change also allows some bulk queue function simplifications

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 75 +---
 1 file changed, 29 insertions(+), 46 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index f74ac1c..d86da81 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3109,9 +3109,10 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
struct scatterlist *sg = NULL;
bool more_trbs_coming = true;
bool need_zero_pkt = false;
-   unsigned int num_trbs, last_trb_num, i;
+   bool first_trb = true;
+   unsigned int num_trbs;
unsigned int start_cycle, num_sgs = 0;
-   unsigned int running_total, block_len, trb_buff_len, full_len;
+   unsigned int enqd_len, block_len, trb_buff_len, full_len;
int ret;
u32 field, length_field, remainder;
u64 addr;
@@ -3120,14 +3121,19 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
if (!ring)
return -EINVAL;
 
+   full_len = urb->transfer_buffer_length;
/* If we have scatter/gather list, we use it. */
if (urb->num_sgs) {
num_sgs = urb->num_mapped_sgs;
sg = urb->sg;
+   addr = (u64) sg_dma_address(sg);
+   block_len = sg_dma_len(sg);
num_trbs = count_sg_trbs_needed(urb);
-   } else
+   } else {
num_trbs = count_trbs_needed(urb);
-
+   addr = (u64) urb->transfer_dma;
+   block_len = full_len;
+   }
ret = prepare_transfer(xhci, xhci->devs[slot_id],
ep_index, urb->stream_id,
num_trbs, urb, 0, mem_flags);
@@ -3136,8 +3142,6 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
 
urb_priv = urb->hcpriv;
 
-   last_trb_num = num_trbs - 1;
-
/* Deal with URB_ZERO_PACKET - need one more td/trb */
if (urb->transfer_flags & URB_ZERO_PACKET && urb_priv->length > 1)
need_zero_pkt = true;
@@ -3152,40 +3156,20 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
start_trb = >enqueue->generic;
start_cycle = ring->cycle_state;
 
-   full_len = urb->transfer_buffer_length;
-   running_total = 0;
-   block_len = 0;
-
/* Queue the TRBs, even if they are zero-length */
-   for (i = 0; i < num_trbs; i++) {
+   for (enqd_len = 0; enqd_len < full_len; enqd_len += trb_buff_len) {
field = TRB_TYPE(TRB_NORMAL);
 
-   if (block_len == 0) {
-   /* A new contiguous block. */
-   if (sg) {
-   addr = (u64) sg_dma_address(sg);
-   block_len = sg_dma_len(sg);
-   } else {
-   addr = (u64) urb->transfer_dma;
-   block_len = full_len;
-   }
-   /* TRB buffer should not cross 64KB boundaries */
-   trb_buff_len = TRB_BUFF_LEN_UP_TO_BOUNDARY(addr);
-   trb_buff_len = min_t(unsigned int,
-   trb_buff_len,
-   block_len);
-   } else {
-   /* Further through the contiguous block. */
-   trb_buff_len = block_len;
-   if (trb_buff_len > TRB_MAX_BUFF_SIZE)
-   trb_buff_len = TRB_MAX_BUFF_SIZE;
-   }
+   /* TRB buffer should not cross 64KB boundaries */
+   trb_buff_len = TRB_BUFF_LEN_UP_TO_BOUNDARY(addr);
+   trb_buff_len = min_t(unsigned int, trb_buff_len, block_len);
 
-   if (running_total + trb_buff_len > full_len)
-   trb_buff_len = full_len - running_total;
+   if (enqd_len + trb_buff_len > full_len)
+   trb_buff_len = full_len - enqd_len;
 
/* Don't change the cycle bit of the first TRB until later */
-   if (i == 0) {
+   if (first_trb) {
+   first_trb = false;
if (start_cycle == 0)
field |= TRB_CYCLE;
} else
@@ -3194,7 +3178,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
   

[RFC PATCH v3 6/6] xhci: TD-fragment, align the unsplittable case with a bounce buffer

[Mathias Nyman]

If the last trb before a link is not packet size aligned, and is not
splittable then use a bounce buffer for that chunk of max packet size
unalignable data.

Allocate a max packet size bounce buffer for every segment of a bulk
endpoint ring at the same time as allocating the ring.
If we need to align the data before the link trb in that segment then
copy the data to the segment bounce buffer, dma map it, and enqueue it.
Once the td finishes, or is cancelled, unmap it.

For in transfers we need to first map the bounce buffer, then queue it,
after it finishes, copy the bounce buffer to the original sg list, and
finally unmap it

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-mem.c  |  74 ++
 drivers/usb/host/xhci-ring.c | 106 +++
 drivers/usb/host/xhci.c  |   5 +-
 drivers/usb/host/xhci.h  |  10 +++-
 4 files changed, 155 insertions(+), 40 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index bad0d1f..6afe323 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -37,7 +37,9 @@
  * "All components of all Command and Transfer TRBs shall be initialized to 
'0'"
  */
 static struct xhci_segment *xhci_segment_alloc(struct xhci_hcd *xhci,
-   unsigned int cycle_state, gfp_t flags)
+  unsigned int cycle_state,
+  unsigned int max_packet,
+  gfp_t flags)
 {
struct xhci_segment *seg;
dma_addr_t  dma;
@@ -53,6 +55,14 @@ static struct xhci_segment *xhci_segment_alloc(struct 
xhci_hcd *xhci,
return NULL;
}
 
+   if (max_packet) {
+   seg->bounce_buf = kzalloc(max_packet, flags | GFP_DMA);
+   if (!seg->bounce_buf) {
+   dma_pool_free(xhci->segment_pool, seg->trbs, dma);
+   kfree(seg);
+   return NULL;
+   }
+   }
/* If the cycle state is 0, set the cycle bit to 1 for all the TRBs */
if (cycle_state == 0) {
for (i = 0; i < TRBS_PER_SEGMENT; i++)
@@ -70,6 +80,7 @@ static void xhci_segment_free(struct xhci_hcd *xhci, struct 
xhci_segment *seg)
dma_pool_free(xhci->segment_pool, seg->trbs, seg->dma);
seg->trbs = NULL;
}
+   kfree(seg->bounce_buf);
kfree(seg);
 }
 
@@ -317,11 +328,11 @@ static void xhci_initialize_ring_info(struct xhci_ring 
*ring,
 static int xhci_alloc_segments_for_ring(struct xhci_hcd *xhci,
struct xhci_segment **first, struct xhci_segment **last,
unsigned int num_segs, unsigned int cycle_state,
-   enum xhci_ring_type type, gfp_t flags)
+   enum xhci_ring_type type, unsigned int max_packet, gfp_t flags)
 {
struct xhci_segment *prev;
 
-   prev = xhci_segment_alloc(xhci, cycle_state, flags);
+   prev = xhci_segment_alloc(xhci, cycle_state, max_packet, flags);
if (!prev)
return -ENOMEM;
num_segs--;
@@ -330,7 +341,7 @@ static int xhci_alloc_segments_for_ring(struct xhci_hcd 
*xhci,
while (num_segs > 0) {
struct xhci_segment *next;
 
-   next = xhci_segment_alloc(xhci, cycle_state, flags);
+   next = xhci_segment_alloc(xhci, cycle_state, max_packet, flags);
if (!next) {
prev = *first;
while (prev) {
@@ -360,7 +371,7 @@ static int xhci_alloc_segments_for_ring(struct xhci_hcd 
*xhci,
  */
 static struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci,
unsigned int num_segs, unsigned int cycle_state,
-   enum xhci_ring_type type, gfp_t flags)
+   enum xhci_ring_type type, unsigned int max_packet, gfp_t flags)
 {
struct xhci_ring*ring;
int ret;
@@ -370,13 +381,15 @@ static struct xhci_ring *xhci_ring_alloc(struct xhci_hcd 
*xhci,
return NULL;
 
ring->num_segs = num_segs;
+   ring->bounce_buf_len = max_packet;
INIT_LIST_HEAD(>td_list);
ring->type = type;
if (num_segs == 0)
return ring;
 
ret = xhci_alloc_segments_for_ring(xhci, >first_seg,
-   >last_seg, num_segs, cycle_state, type, flags);
+   >last_seg, num_segs, cycle_state, type,
+   max_packet, flags);
if (ret)
goto fail;
 
@@ -470,7 +483,8 @@ int xhci_ring_expansion(struct xhci_hcd *xhci, struct 
xhci_ring *ring,
ring->num_segs : num_segs_needed;
 
ret = xhci_alloc_segments_for_ring(xhci, , ,
-   num_segs, ring->cycle_state, ring->type, flags);
+   num_segs, ring->cycle_state, 

[RFC PATCH v3 5/6] xhci: align the last trb before link if it is easily splittable.

[Mathias Nyman]

TD fragments section 4.11.7.1 in xhci specs have additional requirements
on how trbs in TDs must be organized.

TD fragments shall not span transfer ring segments and TD fragments must
be packet aligned. Normally we don't care about TD fragments, on TD is one
big fragment, but if a TD spans ring segments it will be treated as two
fragments, and we need to comply with the alignment requirements.

For us this means that the payload data must be packet aligned in the
last trb before a link trb.
In most mass storage bulk tranfers we are lucky as the block size aligns
nicely with packet size, and there are no issues.
However, usb network adapters using scatterlists can hit this alignment
issue, and usbtest in kernel triggers this in minutes.

This patch is a partial solution, it solves the easy case when the last
trb before the link trb contains a packet boundary.
If that is the case then just split the trb at the boundary.
If not, then just print a debug message and continue as we have always
done, hoping for the best

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d86da81..bf9ffa4 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3098,6 +3098,27 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, int 
transferred,
return (total_packet_count - ((transferred + trb_buff_len) / maxp));
 }
 
+static int xhci_align_td(struct xhci_hcd *xhci, struct urb *urb, u32 enqd_len,
+u32 *trb_buff_len)
+{
+   unsigned int unalign;
+   unsigned int max_pkt;
+
+   max_pkt = GET_MAX_PACKET(usb_endpoint_maxp(>ep->desc));
+   unalign = (enqd_len + *trb_buff_len) % max_pkt;
+
+   /* we got lucky, last normal TRB data on segment is packet aligned */
+   if (unalign == 0)
+   return 0;
+
+   /* is the last nornal TRB alignable by splitting it */
+   if (*trb_buff_len > unalign) {
+   *trb_buff_len -= unalign;
+   return 0;
+   }
+   return 1;
+}
+
 /* This is very similar to what ehci-q.c qtd_fill() does */
 int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
struct urb *urb, int slot_id, unsigned int ep_index)
@@ -3180,6 +3201,12 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
 */
if (enqd_len + trb_buff_len < full_len) {
field |= TRB_CHAIN;
+   if (last_trb(xhci, ring, ring->enq_seg,
+ring->enqueue + 1)) {
+   if (xhci_align_td(xhci, urb, enqd_len,
+_buff_len))
+   xhci_dbg(xhci, "TRB align fail\n");
+   }
} else {
field |= TRB_IOC;
more_trbs_coming = false;
-- 
1.9.1

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

[RFC PATCH v3 3/6] xhci: use boolean to indicate last trb in td remainder calculation

[Mathias Nyman]

We only need to know if we are queuing the last trb for a TD when
calculating the td remainder field.
The total number of trbs left is not used.

We won't be able to trust the pre-calculated number of trbs used if we
need to align trb data by splitting or merging trbs in order to satisfy
comply with data alignment requirements in xhci specs section 4.11.7.1.

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 15dd226..f74ac1c 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3074,7 +3074,7 @@ int xhci_queue_intr_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
  */
 static u32 xhci_td_remainder(struct xhci_hcd *xhci, int transferred,
  int trb_buff_len, unsigned int td_total_len,
- struct urb *urb, unsigned int num_trbs_left)
+ struct urb *urb, bool more_trbs_coming)
 {
u32 maxp, total_packet_count;
 
@@ -3083,7 +3083,7 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, int 
transferred,
return ((td_total_len - transferred) >> 10);
 
/* One TRB with a zero-length data packet. */
-   if (num_trbs_left == 0 || (transferred == 0 && trb_buff_len == 0) ||
+   if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
trb_buff_len == td_total_len)
return 0;
 
@@ -3198,7 +3198,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
field |= TRB_CHAIN;
} else {
field |= TRB_IOC;
-   more_trbs_coming = need_zero_pkt;
+   more_trbs_coming = false;
td->last_trb = ring->enqueue;
}
 
@@ -3209,13 +3209,12 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
/* Set the TRB length, TD size, and interrupter fields. */
remainder = xhci_td_remainder(xhci, running_total,
trb_buff_len, full_len,
-   urb, num_trbs - i - 1);
-
+   urb, more_trbs_coming);
length_field = TRB_LEN(trb_buff_len) |
TRB_TD_SIZE(remainder) |
TRB_INTR_TARGET(0);
 
-   queue_trb(xhci, ring, more_trbs_coming,
+   queue_trb(xhci, ring, more_trbs_coming | need_zero_pkt,
lower_32_bits(addr),
upper_32_bits(addr),
length_field,
@@ -3639,7 +3638,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, 
gfp_t mem_flags,
/* Set the TRB length, TD size, & interrupter fields. */
remainder = xhci_td_remainder(xhci, running_total,
   trb_buff_len, td_len,
-  urb, trbs_per_td - j - 1);
+  urb, more_trbs_coming);
 
length_field = TRB_LEN(trb_buff_len) |
TRB_INTR_TARGET(0);
-- 
1.9.1

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

[RFC PATCH v3 2/6] xhci: properly prepare zero packet TD after normal bulk TD.

[Mathias Nyman]

If a zero-length packet is needed after a bulk transfer, then an
additional zero length TD was prepared before enqueueing the bulk transfer
This set up the zero packet TD structure with incorrect td->start_seg
and td->first_trb pointers.

Prepare the zero packet TD after the data bulk TD is enqueued instead.
It sets these pointers correctly.

This change also simplifies unnecessary complexity related to keeping
track of the last trb when enqueuing trbs.

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 38 +++---
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index c3e9a60..15dd226 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3107,8 +3107,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
struct xhci_td *td;
struct xhci_generic_trb *start_trb;
struct scatterlist *sg = NULL;
-   bool more_trbs_coming;
-   bool zero_length_needed;
+   bool more_trbs_coming = true;
+   bool need_zero_pkt = false;
unsigned int num_trbs, last_trb_num, i;
unsigned int start_cycle, num_sgs = 0;
unsigned int running_total, block_len, trb_buff_len, full_len;
@@ -3139,17 +3139,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
last_trb_num = num_trbs - 1;
 
/* Deal with URB_ZERO_PACKET - need one more td/trb */
-   zero_length_needed = urb->transfer_flags & URB_ZERO_PACKET &&
-   urb_priv->length == 2;
-   if (zero_length_needed) {
-   num_trbs++;
-   xhci_dbg(xhci, "Creating zero length td.\n");
-   ret = prepare_transfer(xhci, xhci->devs[slot_id],
-   ep_index, urb->stream_id,
-   1, urb, 1, mem_flags);
-   if (unlikely(ret < 0))
-   return ret;
-   }
+   if (urb->transfer_flags & URB_ZERO_PACKET && urb_priv->length > 1)
+   need_zero_pkt = true;
 
td = urb_priv->td[0];
 
@@ -3207,12 +3198,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
field |= TRB_CHAIN;
} else {
field |= TRB_IOC;
-   if (i == last_trb_num)
-   td->last_trb = ring->enqueue;
-   else if (zero_length_needed) {
-   trb_buff_len = 0;
-   urb_priv->td[1]->last_trb = ring->enqueue;
-   }
+   more_trbs_coming = need_zero_pkt;
+   td->last_trb = ring->enqueue;
}
 
/* Only set interrupt on short packet for IN endpoints */
@@ -3228,10 +3215,6 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
TRB_TD_SIZE(remainder) |
TRB_INTR_TARGET(0);
 
-   if (i < num_trbs - 1)
-   more_trbs_coming = true;
-   else
-   more_trbs_coming = false;
queue_trb(xhci, ring, more_trbs_coming,
lower_32_bits(addr),
upper_32_bits(addr),
@@ -3253,6 +3236,15 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
}
}
 
+   if (need_zero_pkt) {
+   ret = prepare_transfer(xhci, xhci->devs[slot_id],
+  ep_index, urb->stream_id,
+  1, urb, 1, mem_flags);
+   urb_priv->td[1]->last_trb = ring->enqueue;
+   field = TRB_TYPE(TRB_NORMAL) | ring->cycle_state | TRB_IOC;
+   queue_trb(xhci, ring, 0, 0, 0, TRB_INTR_TARGET(0), field);
+   }
+
check_trb_math(urb, running_total);
giveback_first_trb(xhci, slot_id, ep_index, urb->stream_id,
start_cycle, start_trb);
-- 
1.9.1

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

[RFC PATCH v3 0/6] xhci: conform with xhci td fragment constraints

[Mathias Nyman]

This patchseries makes sure we follow the td fragmentation constraints
in xhci 4.11.7.1 for bulk tranfers.

The specs say that:
"constraints must be imposed to ensure that the hardware gate count and
validation requirements are minimized for xHC implementations"

The constraint we need to conform to is to make sure the payload data is
packet aligned in the last trb we queue to a segment before a link trb
moves us to the next segment.

In most usecases the payload data is packet aligned, but cases where
scatterlists with several small enrtries are used may hit this issue.

Running "testusb -a -t 7 -s 2048 -v 41 -c 5000" was able to hang the host
after minutes of testing because of unaligned payload data.
After applying this series testusb ran without issues for 4 days.

usb-eth network adapters can potentially hit this issue.

This solution tries to align the data by splitting the trb at a packet
boundary, if that is not possible then a bounce buffer is used.
Initially the plan was to try to insert a link trb mid segment and mid TD
to fulfill the constraints before resorting to a bounce buffer, but
it turned out it requires a lot of driver rewrite. That is an option
that can be implemented later after driver cleanup.

The first 4 patches are more generic cleanups and reworks needed for
the two last patches that actually makes sure data is aligned.

changes since v1:
  cleanup code comments used during development.

changes since v2:
  cleanup the code comments in the correct patches

Mathias Nyman (6):
  xhci: rename ep_ring variable in queue_bulk_tx(), no functional change
  xhci: properly prepare zero packet TD after normal bulk TD.
  xhci: use boolean to indicate last trb in td remainder calculation
  xhci: don't rely on precalculated value of needed trbs in the enqueue
loop
  xhci: align the last trb before link if it is easily splittable.
  xhci: TD-fragment, align the unsplittable case with a bounce buffer

 drivers/usb/host/xhci-mem.c  |  74 +++-
 drivers/usb/host/xhci-ring.c | 263 +--
 drivers/usb/host/xhci.c  |   5 +-
 drivers/usb/host/xhci.h  |  11 +-
 4 files changed, 236 insertions(+), 117 deletions(-)

--
1.9.1

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

[RFC PATCH v3 1/6] xhci: rename ep_ring variable in queue_bulk_tx(), no functional change

[Mathias Nyman]

Tiny change, a bit more readable.
The real reason for this change is that the coming td fragment work
had several over 80 lines character lines split just because of a few
extra characters in variable names.

no functional changes

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 21 ++---
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 52deae4..c3e9a60 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3102,7 +3102,7 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, int 
transferred,
 int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
struct urb *urb, int slot_id, unsigned int ep_index)
 {
-   struct xhci_ring *ep_ring;
+   struct xhci_ring *ring;
struct urb_priv *urb_priv;
struct xhci_td *td;
struct xhci_generic_trb *start_trb;
@@ -3111,14 +3111,13 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
bool zero_length_needed;
unsigned int num_trbs, last_trb_num, i;
unsigned int start_cycle, num_sgs = 0;
-   unsigned int running_total, block_len, trb_buff_len;
-   unsigned int full_len;
+   unsigned int running_total, block_len, trb_buff_len, full_len;
int ret;
u32 field, length_field, remainder;
u64 addr;
 
-   ep_ring = xhci_urb_to_transfer_ring(xhci, urb);
-   if (!ep_ring)
+   ring = xhci_urb_to_transfer_ring(xhci, urb);
+   if (!ring)
return -EINVAL;
 
/* If we have scatter/gather list, we use it. */
@@ -3159,8 +3158,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
 * until we've finished creating all the other TRBs.  The ring's cycle
 * state may change as we enqueue the other TRBs, so save it too.
 */
-   start_trb = _ring->enqueue->generic;
-   start_cycle = ep_ring->cycle_state;
+   start_trb = >enqueue->generic;
+   start_cycle = ring->cycle_state;
 
full_len = urb->transfer_buffer_length;
running_total = 0;
@@ -3199,7 +3198,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
if (start_cycle == 0)
field |= TRB_CYCLE;
} else
-   field |= ep_ring->cycle_state;
+   field |= ring->cycle_state;
 
/* Chain all the TRBs together; clear the chain bit in the last
 * TRB to indicate it's the last TRB in the chain.
@@ -3209,10 +3208,10 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
} else {
field |= TRB_IOC;
if (i == last_trb_num)
-   td->last_trb = ep_ring->enqueue;
+   td->last_trb = ring->enqueue;
else if (zero_length_needed) {
trb_buff_len = 0;
-   urb_priv->td[1]->last_trb = ep_ring->enqueue;
+   urb_priv->td[1]->last_trb = ring->enqueue;
}
}
 
@@ -3233,7 +3232,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
more_trbs_coming = true;
else
more_trbs_coming = false;
-   queue_trb(xhci, ep_ring, more_trbs_coming,
+   queue_trb(xhci, ring, more_trbs_coming,
lower_32_bits(addr),
upper_32_bits(addr),
length_field,
-- 
1.9.1

--
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: [RFC PATCH v2 5/6] xhci: align the last trb before link if it is easily splittable.

[Mathias Nyman]

On 26.05.2016 15:08, Mathias Nyman wrote:

TD fragments section 4.11.7.1 in xhci specs have additional requirements
on how trbs in TDs must be organized.

TD fragments shall not span transfer ring segments and TD fragments must
be packet aligned. Normally we don't care about TD fragments, on TD is one
big fragment, but if a TD spans ring segments it will be treated as two
fragments, and we need to comply with the alignment requirements.

For us this means that the payload data must be packet aligned in the
last trb before a link trb.
In most mass storage bulk tranfers we are lucky as the block size aligns
nicely with packet size, and there are no issues.
However, usb network adapters using scatterlists can hit this alignment
issue, and usbtest in kernel triggers this in minutes.

This patch is a partial solution, it solves the easy case when the last
trb before the link trb contains a packet boundary.
If that is the case then just split the trb at the boundary.
If not, then just print a debug message and continue as we have always
done, hoping for the best

Signed-off-by: Mathias Nyman 
---
  drivers/usb/host/xhci-ring.c | 27 +++
  1 file changed, 27 insertions(+)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d86da81..c7c9521 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3098,6 +3098,27 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, int 
transferred,
return (total_packet_count - ((transferred + trb_buff_len) / maxp));
  }

+static int xhci_align_td(struct xhci_hcd *xhci, struct urb *urb, u32 enqd_len,
+u32 *trb_buff_len)
+{
+   unsigned int unalign;
+   unsigned int max_pkt;
+
+   max_pkt = usb_endpoint_maxp(>ep->desc); /*FIXME MATTU GET_MAX..? */


Sigh.. so I managed to amend the cleanup to the wrong patch, was supposed to
be in 5/6 not 6/6

yet another version on its way

-Mathias

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

[RFC PATCH v2 2/6] xhci: properly prepare zero packet TD after normal bulk TD.

[Mathias Nyman]

If a zero-length packet is needed after a bulk transfer, then an
additional zero length TD was prepared before enqueueing the bulk transfer
This set up the zero packet TD structure with incorrect td->start_seg
and td->first_trb pointers.

Prepare the zero packet TD after the data bulk TD is enqueued instead.
It sets these pointers correctly.

This change also simplifies unnecessary complexity related to keeping
track of the last trb when enqueuing trbs.

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 38 +++---
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index c3e9a60..15dd226 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3107,8 +3107,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
struct xhci_td *td;
struct xhci_generic_trb *start_trb;
struct scatterlist *sg = NULL;
-   bool more_trbs_coming;
-   bool zero_length_needed;
+   bool more_trbs_coming = true;
+   bool need_zero_pkt = false;
unsigned int num_trbs, last_trb_num, i;
unsigned int start_cycle, num_sgs = 0;
unsigned int running_total, block_len, trb_buff_len, full_len;
@@ -3139,17 +3139,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
last_trb_num = num_trbs - 1;
 
/* Deal with URB_ZERO_PACKET - need one more td/trb */
-   zero_length_needed = urb->transfer_flags & URB_ZERO_PACKET &&
-   urb_priv->length == 2;
-   if (zero_length_needed) {
-   num_trbs++;
-   xhci_dbg(xhci, "Creating zero length td.\n");
-   ret = prepare_transfer(xhci, xhci->devs[slot_id],
-   ep_index, urb->stream_id,
-   1, urb, 1, mem_flags);
-   if (unlikely(ret < 0))
-   return ret;
-   }
+   if (urb->transfer_flags & URB_ZERO_PACKET && urb_priv->length > 1)
+   need_zero_pkt = true;
 
td = urb_priv->td[0];
 
@@ -3207,12 +3198,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
field |= TRB_CHAIN;
} else {
field |= TRB_IOC;
-   if (i == last_trb_num)
-   td->last_trb = ring->enqueue;
-   else if (zero_length_needed) {
-   trb_buff_len = 0;
-   urb_priv->td[1]->last_trb = ring->enqueue;
-   }
+   more_trbs_coming = need_zero_pkt;
+   td->last_trb = ring->enqueue;
}
 
/* Only set interrupt on short packet for IN endpoints */
@@ -3228,10 +3215,6 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
TRB_TD_SIZE(remainder) |
TRB_INTR_TARGET(0);
 
-   if (i < num_trbs - 1)
-   more_trbs_coming = true;
-   else
-   more_trbs_coming = false;
queue_trb(xhci, ring, more_trbs_coming,
lower_32_bits(addr),
upper_32_bits(addr),
@@ -3253,6 +3236,15 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
}
}
 
+   if (need_zero_pkt) {
+   ret = prepare_transfer(xhci, xhci->devs[slot_id],
+  ep_index, urb->stream_id,
+  1, urb, 1, mem_flags);
+   urb_priv->td[1]->last_trb = ring->enqueue;
+   field = TRB_TYPE(TRB_NORMAL) | ring->cycle_state | TRB_IOC;
+   queue_trb(xhci, ring, 0, 0, 0, TRB_INTR_TARGET(0), field);
+   }
+
check_trb_math(urb, running_total);
giveback_first_trb(xhci, slot_id, ep_index, urb->stream_id,
start_cycle, start_trb);
-- 
1.9.1

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

[RFC PATCH v2 6/6] xhci: TD-fragment, align the unsplittable case with a bounce buffer

[Mathias Nyman]

If the last trb before a link is not packet size aligned, and is not
splittable then use a bounce buffer for that chunk of max packet size
unalignable data.

Allocate a max packet size bounce buffer for every segment of a bulk
endpoint ring at the same time as allocating the ring.
If we need to align the data before the link trb in that segment then
copy the data to the segment bounce buffer, dma map it, and enqueue it.
Once the td finishes, or is cancelled, unmap it.

For in transfers we need to first map the bounce buffer, then queue it,
after it finishes, copy the bounce buffer to the original sg list, and
finally unmap it

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-mem.c  |  74 ++---
 drivers/usb/host/xhci-ring.c | 111 ++-
 drivers/usb/host/xhci.c  |   5 +-
 drivers/usb/host/xhci.h  |  11 -
 4 files changed, 160 insertions(+), 41 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index bad0d1f..6afe323 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -37,7 +37,9 @@
  * "All components of all Command and Transfer TRBs shall be initialized to 
'0'"
  */
 static struct xhci_segment *xhci_segment_alloc(struct xhci_hcd *xhci,
-   unsigned int cycle_state, gfp_t flags)
+  unsigned int cycle_state,
+  unsigned int max_packet,
+  gfp_t flags)
 {
struct xhci_segment *seg;
dma_addr_t  dma;
@@ -53,6 +55,14 @@ static struct xhci_segment *xhci_segment_alloc(struct 
xhci_hcd *xhci,
return NULL;
}
 
+   if (max_packet) {
+   seg->bounce_buf = kzalloc(max_packet, flags | GFP_DMA);
+   if (!seg->bounce_buf) {
+   dma_pool_free(xhci->segment_pool, seg->trbs, dma);
+   kfree(seg);
+   return NULL;
+   }
+   }
/* If the cycle state is 0, set the cycle bit to 1 for all the TRBs */
if (cycle_state == 0) {
for (i = 0; i < TRBS_PER_SEGMENT; i++)
@@ -70,6 +80,7 @@ static void xhci_segment_free(struct xhci_hcd *xhci, struct 
xhci_segment *seg)
dma_pool_free(xhci->segment_pool, seg->trbs, seg->dma);
seg->trbs = NULL;
}
+   kfree(seg->bounce_buf);
kfree(seg);
 }
 
@@ -317,11 +328,11 @@ static void xhci_initialize_ring_info(struct xhci_ring 
*ring,
 static int xhci_alloc_segments_for_ring(struct xhci_hcd *xhci,
struct xhci_segment **first, struct xhci_segment **last,
unsigned int num_segs, unsigned int cycle_state,
-   enum xhci_ring_type type, gfp_t flags)
+   enum xhci_ring_type type, unsigned int max_packet, gfp_t flags)
 {
struct xhci_segment *prev;
 
-   prev = xhci_segment_alloc(xhci, cycle_state, flags);
+   prev = xhci_segment_alloc(xhci, cycle_state, max_packet, flags);
if (!prev)
return -ENOMEM;
num_segs--;
@@ -330,7 +341,7 @@ static int xhci_alloc_segments_for_ring(struct xhci_hcd 
*xhci,
while (num_segs > 0) {
struct xhci_segment *next;
 
-   next = xhci_segment_alloc(xhci, cycle_state, flags);
+   next = xhci_segment_alloc(xhci, cycle_state, max_packet, flags);
if (!next) {
prev = *first;
while (prev) {
@@ -360,7 +371,7 @@ static int xhci_alloc_segments_for_ring(struct xhci_hcd 
*xhci,
  */
 static struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci,
unsigned int num_segs, unsigned int cycle_state,
-   enum xhci_ring_type type, gfp_t flags)
+   enum xhci_ring_type type, unsigned int max_packet, gfp_t flags)
 {
struct xhci_ring*ring;
int ret;
@@ -370,13 +381,15 @@ static struct xhci_ring *xhci_ring_alloc(struct xhci_hcd 
*xhci,
return NULL;
 
ring->num_segs = num_segs;
+   ring->bounce_buf_len = max_packet;
INIT_LIST_HEAD(>td_list);
ring->type = type;
if (num_segs == 0)
return ring;
 
ret = xhci_alloc_segments_for_ring(xhci, >first_seg,
-   >last_seg, num_segs, cycle_state, type, flags);
+   >last_seg, num_segs, cycle_state, type,
+   max_packet, flags);
if (ret)
goto fail;
 
@@ -470,7 +483,8 @@ int xhci_ring_expansion(struct xhci_hcd *xhci, struct 
xhci_ring *ring,
ring->num_segs : num_segs_needed;
 
ret = xhci_alloc_segments_for_ring(xhci, , ,
-   num_segs, ring->cycle_state, ring->type, flags);
+   num_segs, ring->cycle_state, 

[RFC PATCH v2 5/6] xhci: align the last trb before link if it is easily splittable.

[Mathias Nyman]

TD fragments section 4.11.7.1 in xhci specs have additional requirements
on how trbs in TDs must be organized.

TD fragments shall not span transfer ring segments and TD fragments must
be packet aligned. Normally we don't care about TD fragments, on TD is one
big fragment, but if a TD spans ring segments it will be treated as two
fragments, and we need to comply with the alignment requirements.

For us this means that the payload data must be packet aligned in the
last trb before a link trb.
In most mass storage bulk tranfers we are lucky as the block size aligns
nicely with packet size, and there are no issues.
However, usb network adapters using scatterlists can hit this alignment
issue, and usbtest in kernel triggers this in minutes.

This patch is a partial solution, it solves the easy case when the last
trb before the link trb contains a packet boundary.
If that is the case then just split the trb at the boundary.
If not, then just print a debug message and continue as we have always
done, hoping for the best

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d86da81..c7c9521 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3098,6 +3098,27 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, int 
transferred,
return (total_packet_count - ((transferred + trb_buff_len) / maxp));
 }
 
+static int xhci_align_td(struct xhci_hcd *xhci, struct urb *urb, u32 enqd_len,
+u32 *trb_buff_len)
+{
+   unsigned int unalign;
+   unsigned int max_pkt;
+
+   max_pkt = usb_endpoint_maxp(>ep->desc); /*FIXME MATTU GET_MAX..? */
+   unalign = (enqd_len + *trb_buff_len) % max_pkt;
+
+   /* we got lucky, last normal TRB data on segment is packet aligned */
+   if (unalign == 0)
+   return 0;
+
+   /* is the last nornal TRB alignable by splitting it */
+   if (*trb_buff_len > unalign) {
+   *trb_buff_len -= unalign;
+   return 0;
+   }
+   return 1;
+}
+
 /* This is very similar to what ehci-q.c qtd_fill() does */
 int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
struct urb *urb, int slot_id, unsigned int ep_index)
@@ -3180,6 +3201,12 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
 */
if (enqd_len + trb_buff_len < full_len) {
field |= TRB_CHAIN;
+   if (last_trb(xhci, ring, ring->enq_seg,
+ring->enqueue + 1)) {
+   if (xhci_align_td(xhci, urb, enqd_len,
+_buff_len))
+   xhci_dbg(xhci, "TRB align fail\n");
+   }
} else {
field |= TRB_IOC;
more_trbs_coming = false;
-- 
1.9.1

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

[RFC PATCH v2 3/6] xhci: use boolean to indicate last trb in td remainder calculation

[Mathias Nyman]

We only need to know if we are queuing the last trb for a TD when
calculating the td remainder field.
The total number of trbs left is not used.

We won't be able to trust the pre-calculated number of trbs used if we
need to align trb data by splitting or merging trbs in order to satisfy
comply with data alignment requirements in xhci specs section 4.11.7.1.

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 15dd226..f74ac1c 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3074,7 +3074,7 @@ int xhci_queue_intr_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
  */
 static u32 xhci_td_remainder(struct xhci_hcd *xhci, int transferred,
  int trb_buff_len, unsigned int td_total_len,
- struct urb *urb, unsigned int num_trbs_left)
+ struct urb *urb, bool more_trbs_coming)
 {
u32 maxp, total_packet_count;
 
@@ -3083,7 +3083,7 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, int 
transferred,
return ((td_total_len - transferred) >> 10);
 
/* One TRB with a zero-length data packet. */
-   if (num_trbs_left == 0 || (transferred == 0 && trb_buff_len == 0) ||
+   if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
trb_buff_len == td_total_len)
return 0;
 
@@ -3198,7 +3198,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
field |= TRB_CHAIN;
} else {
field |= TRB_IOC;
-   more_trbs_coming = need_zero_pkt;
+   more_trbs_coming = false;
td->last_trb = ring->enqueue;
}
 
@@ -3209,13 +3209,12 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
/* Set the TRB length, TD size, and interrupter fields. */
remainder = xhci_td_remainder(xhci, running_total,
trb_buff_len, full_len,
-   urb, num_trbs - i - 1);
-
+   urb, more_trbs_coming);
length_field = TRB_LEN(trb_buff_len) |
TRB_TD_SIZE(remainder) |
TRB_INTR_TARGET(0);
 
-   queue_trb(xhci, ring, more_trbs_coming,
+   queue_trb(xhci, ring, more_trbs_coming | need_zero_pkt,
lower_32_bits(addr),
upper_32_bits(addr),
length_field,
@@ -3639,7 +3638,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, 
gfp_t mem_flags,
/* Set the TRB length, TD size, & interrupter fields. */
remainder = xhci_td_remainder(xhci, running_total,
   trb_buff_len, td_len,
-  urb, trbs_per_td - j - 1);
+  urb, more_trbs_coming);
 
length_field = TRB_LEN(trb_buff_len) |
TRB_INTR_TARGET(0);
-- 
1.9.1

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

[RFC PATCH v2 4/6] xhci: don't rely on precalculated value of needed trbs in the enqueue loop

[Mathias Nyman]

Queue trbs until all payload data in the urb is tranferred.

The actual number of trbs might need to change from the pre-calculated
number when the packet alignment restrictions for td fragments in
xhci 4.11.7.1 are taken into account.

Long term plan is to get rid of calculating the needed trbs in advance
all together. It's an unnecessary extra walk through the scatterlist.

This change also allows some bulk queue function simplifications

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 75 +---
 1 file changed, 29 insertions(+), 46 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index f74ac1c..d86da81 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3109,9 +3109,10 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
struct scatterlist *sg = NULL;
bool more_trbs_coming = true;
bool need_zero_pkt = false;
-   unsigned int num_trbs, last_trb_num, i;
+   bool first_trb = true;
+   unsigned int num_trbs;
unsigned int start_cycle, num_sgs = 0;
-   unsigned int running_total, block_len, trb_buff_len, full_len;
+   unsigned int enqd_len, block_len, trb_buff_len, full_len;
int ret;
u32 field, length_field, remainder;
u64 addr;
@@ -3120,14 +3121,19 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
if (!ring)
return -EINVAL;
 
+   full_len = urb->transfer_buffer_length;
/* If we have scatter/gather list, we use it. */
if (urb->num_sgs) {
num_sgs = urb->num_mapped_sgs;
sg = urb->sg;
+   addr = (u64) sg_dma_address(sg);
+   block_len = sg_dma_len(sg);
num_trbs = count_sg_trbs_needed(urb);
-   } else
+   } else {
num_trbs = count_trbs_needed(urb);
-
+   addr = (u64) urb->transfer_dma;
+   block_len = full_len;
+   }
ret = prepare_transfer(xhci, xhci->devs[slot_id],
ep_index, urb->stream_id,
num_trbs, urb, 0, mem_flags);
@@ -3136,8 +3142,6 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
 
urb_priv = urb->hcpriv;
 
-   last_trb_num = num_trbs - 1;
-
/* Deal with URB_ZERO_PACKET - need one more td/trb */
if (urb->transfer_flags & URB_ZERO_PACKET && urb_priv->length > 1)
need_zero_pkt = true;
@@ -3152,40 +3156,20 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
start_trb = >enqueue->generic;
start_cycle = ring->cycle_state;
 
-   full_len = urb->transfer_buffer_length;
-   running_total = 0;
-   block_len = 0;
-
/* Queue the TRBs, even if they are zero-length */
-   for (i = 0; i < num_trbs; i++) {
+   for (enqd_len = 0; enqd_len < full_len; enqd_len += trb_buff_len) {
field = TRB_TYPE(TRB_NORMAL);
 
-   if (block_len == 0) {
-   /* A new contiguous block. */
-   if (sg) {
-   addr = (u64) sg_dma_address(sg);
-   block_len = sg_dma_len(sg);
-   } else {
-   addr = (u64) urb->transfer_dma;
-   block_len = full_len;
-   }
-   /* TRB buffer should not cross 64KB boundaries */
-   trb_buff_len = TRB_BUFF_LEN_UP_TO_BOUNDARY(addr);
-   trb_buff_len = min_t(unsigned int,
-   trb_buff_len,
-   block_len);
-   } else {
-   /* Further through the contiguous block. */
-   trb_buff_len = block_len;
-   if (trb_buff_len > TRB_MAX_BUFF_SIZE)
-   trb_buff_len = TRB_MAX_BUFF_SIZE;
-   }
+   /* TRB buffer should not cross 64KB boundaries */
+   trb_buff_len = TRB_BUFF_LEN_UP_TO_BOUNDARY(addr);
+   trb_buff_len = min_t(unsigned int, trb_buff_len, block_len);
 
-   if (running_total + trb_buff_len > full_len)
-   trb_buff_len = full_len - running_total;
+   if (enqd_len + trb_buff_len > full_len)
+   trb_buff_len = full_len - enqd_len;
 
/* Don't change the cycle bit of the first TRB until later */
-   if (i == 0) {
+   if (first_trb) {
+   first_trb = false;
if (start_cycle == 0)
field |= TRB_CYCLE;
} else
@@ -3194,7 +3178,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
   

[RFC PATCH v2 1/6] xhci: rename ep_ring variable in queue_bulk_tx(), no functional change

[Mathias Nyman]

Tiny change, a bit more readable.
The real reason for this change is that the coming td fragment work
had several over 80 lines character lines split just because of a few
extra characters in variable names.

no functional changes

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 21 ++---
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 52deae4..c3e9a60 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3102,7 +3102,7 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, int 
transferred,
 int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
struct urb *urb, int slot_id, unsigned int ep_index)
 {
-   struct xhci_ring *ep_ring;
+   struct xhci_ring *ring;
struct urb_priv *urb_priv;
struct xhci_td *td;
struct xhci_generic_trb *start_trb;
@@ -3111,14 +3111,13 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
bool zero_length_needed;
unsigned int num_trbs, last_trb_num, i;
unsigned int start_cycle, num_sgs = 0;
-   unsigned int running_total, block_len, trb_buff_len;
-   unsigned int full_len;
+   unsigned int running_total, block_len, trb_buff_len, full_len;
int ret;
u32 field, length_field, remainder;
u64 addr;
 
-   ep_ring = xhci_urb_to_transfer_ring(xhci, urb);
-   if (!ep_ring)
+   ring = xhci_urb_to_transfer_ring(xhci, urb);
+   if (!ring)
return -EINVAL;
 
/* If we have scatter/gather list, we use it. */
@@ -3159,8 +3158,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
 * until we've finished creating all the other TRBs.  The ring's cycle
 * state may change as we enqueue the other TRBs, so save it too.
 */
-   start_trb = _ring->enqueue->generic;
-   start_cycle = ep_ring->cycle_state;
+   start_trb = >enqueue->generic;
+   start_cycle = ring->cycle_state;
 
full_len = urb->transfer_buffer_length;
running_total = 0;
@@ -3199,7 +3198,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
if (start_cycle == 0)
field |= TRB_CYCLE;
} else
-   field |= ep_ring->cycle_state;
+   field |= ring->cycle_state;
 
/* Chain all the TRBs together; clear the chain bit in the last
 * TRB to indicate it's the last TRB in the chain.
@@ -3209,10 +3208,10 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
} else {
field |= TRB_IOC;
if (i == last_trb_num)
-   td->last_trb = ep_ring->enqueue;
+   td->last_trb = ring->enqueue;
else if (zero_length_needed) {
trb_buff_len = 0;
-   urb_priv->td[1]->last_trb = ep_ring->enqueue;
+   urb_priv->td[1]->last_trb = ring->enqueue;
}
}
 
@@ -3233,7 +3232,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
more_trbs_coming = true;
else
more_trbs_coming = false;
-   queue_trb(xhci, ep_ring, more_trbs_coming,
+   queue_trb(xhci, ring, more_trbs_coming,
lower_32_bits(addr),
upper_32_bits(addr),
length_field,
-- 
1.9.1

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

[RFC PATCH v2 0/6] xhci: conform with xhci td fragment constraints

[Mathias Nyman]

This patchseries makes sure we follow the td fragmentation constraints
in xhci 4.11.7.1 for bulk tranfers.

The specs say that:
"constraints must be imposed to ensure that the hardware gate count and
validation requirements are minimized for xHC implementations"

The constraint we need to conform to is to make sure the payload data is
packet aligned in the last trb we queue to a segment before a link trb
moves us to the next segment.

In most usecases the payload data is packet aligned, but cases where
scatterlists with several small enrtries are used may hit this issue.

Running "testusb -a -t 7 -s 2048 -v 41 -c 5000" was able to hang the host
after minutes of testing because of unaligned payload data.
After applying this series testusb ran without issues for 4 days.

usb-eth network adapters can potentially hit this issue.

This solution tries to align the data by splitting the trb at a packet
boundary, if that is not possible then a bounce buffer is used.
Initially the plan was to try to insert a link trb mid segment and mid TD
to fulfill the constraints before resorting to a bounce buffer, but
it turned out it requires a lot of driver rewrite. That is an option
that can be implemented later after driver cleanup.

The first 4 patches are more generic cleanups and reworks needed for
the two last patches that actually makes sure data is aligned.

changes since v1:
  cleanup code comments used during development.

Mathias Nyman (6):
  xhci: rename ep_ring variable in queue_bulk_tx(), no functional change
  xhci: properly prepare zero packet TD after normal bulk TD.
  xhci: use boolean to indicate last trb in td remainder calculation
  xhci: don't rely on precalculated value of needed trbs in the enqueue
loop
  xhci: align the last trb before link if it is easily splittable.
  xhci: TD-fragment, align the unsplittable case with a bounce buffer

 drivers/usb/host/xhci-mem.c  |  74 +++-
 drivers/usb/host/xhci-ring.c | 263 +--
 drivers/usb/host/xhci.c  |   5 +-
 drivers/usb/host/xhci.h  |  11 +-
 4 files changed, 236 insertions(+), 117 deletions(-)

-- 
1.9.1

--
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: [RFC PATCH 6/6] xhci: TD-fragment, align the unsplittable case with a bounce buffer

[Mathias Nyman]

On 26.05.2016 14:32, Felipe Balbi wrote:


Hi,

Mathias Nyman  writes:

@@ -3098,24 +3136,66 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, int 
transferred,
return (total_packet_count - ((transferred + trb_buff_len) / maxp));
  }




+   }
+   *trb_buff_len = new_buff_len;
+   seg->bounce_len = new_buff_len;
+   seg->bounce_offs = enqd_len;
+
+   xhci_dbg(xhci, "Bounce align, new buff len %d\n", *trb_buff_len);
+
+   /* FIXME MATTU make sure memory allocated memory is 64k aligned */

 ^
 do you wanna clean this up ?



Will do, new version coming

-Mathias
--
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: [RFC PATCH 6/6] xhci: TD-fragment, align the unsplittable case with a bounce buffer

[Felipe Balbi]

Hi,

Mathias Nyman  writes:
> @@ -3098,24 +3136,66 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, 
> int transferred,
>   return (total_packet_count - ((transferred + trb_buff_len) / maxp));
>  }
>  
> +
>  static int xhci_align_td(struct xhci_hcd *xhci, struct urb *urb, u32 
> enqd_len,
> -  u32 *trb_buff_len)
> +  u32 *trb_buff_len, struct xhci_segment *seg)
>  {
> + struct device *dev = xhci_to_hcd(xhci)->self.controller;
>   unsigned int unalign;
>   unsigned int max_pkt;
> + u32 new_buff_len;
>  
> - max_pkt = usb_endpoint_maxp(>ep->desc); /*FIXME MATTU GET_MAX..? */
> + max_pkt = GET_MAX_PACKET(usb_endpoint_maxp(>ep->desc));
>   unalign = (enqd_len + *trb_buff_len) % max_pkt;
>  
>   /* we got lucky, last normal TRB data on segment is packet aligned */
>   if (unalign == 0)
>   return 0;
>  
> + xhci_dbg(xhci, "Unaligned %d bytes, buff len %d\n",
> +  unalign, *trb_buff_len);
> +
>   /* is the last nornal TRB alignable by splitting it */
>   if (*trb_buff_len > unalign) {
>   *trb_buff_len -= unalign;
> + xhci_dbg(xhci, "split align, new buff len %d\n", *trb_buff_len);
>   return 0;
>   }
> +
> + /*
> +  * We want enqd_len + trb_buff_len to sum up to a number aligned to
> +  * number which is divisible by the endpoint's wMaxPacketSize. IOW:
> +  * (size of currently enqueued TRBs + remainder) % wMaxPacketSize == 0.
> +  */
> + new_buff_len = max_pkt - (enqd_len % max_pkt);
> +
> + if (new_buff_len > (urb->transfer_buffer_length - enqd_len))
> + new_buff_len = (urb->transfer_buffer_length - enqd_len);
> +
> + /* create a max max_pkt sized bounce buffer pointed to by last trb */
> + if (usb_urb_dir_out(urb)) {
> + sg_pcopy_to_buffer(urb->sg, urb->num_mapped_sgs,
> +seg->bounce_buf, new_buff_len, enqd_len);
> + seg->bounce_dma = dma_map_single(dev, seg->bounce_buf,
> +  max_pkt, DMA_TO_DEVICE);
> + } else {
> + seg->bounce_dma = dma_map_single(dev, seg->bounce_buf,
> +  max_pkt, DMA_FROM_DEVICE);
> + }
> +
> + if (dma_mapping_error(dev, seg->bounce_dma)) {
> + /* try without aligning. Some host controllers survive */
> + xhci_warn(xhci, "Failed mapping bounce buffer, not aligning\n");
> + return 0;
> + }
> + *trb_buff_len = new_buff_len;
> + seg->bounce_len = new_buff_len;
> + seg->bounce_offs = enqd_len;
> +
> + xhci_dbg(xhci, "Bounce align, new buff len %d\n", *trb_buff_len);
> +
> + /* FIXME MATTU make sure memory allocated memory is 64k aligned */
^
do you wanna clean this up ?

-- 
balbi


signature.asc
Description: PGP signature

Re: [RFC PATCH 5/6] xhci: align the last trb before link if it is easily splittable.

[Felipe Balbi]

Hi,

Mathias Nyman  writes:
> TD fragments section 4.11.7.1 in xhci specs have additional requirements
> on how trbs in TDs must be organized.
>
> TD fragments shall not span transfer ring segments and TD fragments must
> be packet aligned. Normally we don't care about TD fragments, on TD is one
> big fragment, but if a TD spans ring segments it will be treated as two
> fragments, and we need to comply with the alignment requirements.
>
> For us this means that the payload data must be packet aligned in the
> last trb before a link trb.
> In most mass storage bulk tranfers we are lucky as the block size aligns
> nicely with packet size, and there are no issues.
> However, usb network adapters using scatterlists can hit this alignment
> issue, and usbtest in kernel triggers this in minutes.
>
> This patch is a partial solution, it solves the easy case when the last
> trb before the link trb contains a packet boundary.
> If that is the case then just split the trb at the boundary.
> If not, then just print a debug message and continue as we have always
> done, hoping for the best
>
> Signed-off-by: Mathias Nyman 
> ---
>  drivers/usb/host/xhci-ring.c | 27 +++
>  1 file changed, 27 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index d86da81..c7c9521 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3098,6 +3098,27 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, 
> int transferred,
>   return (total_packet_count - ((transferred + trb_buff_len) / maxp));
>  }
>  
> +static int xhci_align_td(struct xhci_hcd *xhci, struct urb *urb, u32 
> enqd_len,
> +  u32 *trb_buff_len)
> +{
> + unsigned int unalign;
> + unsigned int max_pkt;
> +
> + max_pkt = usb_endpoint_maxp(>ep->desc); /*FIXME MATTU GET_MAX..? */
 ^^

-- 
balbi


signature.asc
Description: PGP signature

Re: [RFC PATCH 5/6] xhci: align the last trb before link if it is easily splittable.

[Felipe Balbi]

Hi,

Mathias Nyman  writes:
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index d86da81..c7c9521 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3098,6 +3098,27 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, 
> int transferred,
>   return (total_packet_count - ((transferred + trb_buff_len) / maxp));
>  }
>  
> +static int xhci_align_td(struct xhci_hcd *xhci, struct urb *urb, u32 
> enqd_len,
> +  u32 *trb_buff_len)
> +{
> + unsigned int unalign;
> + unsigned int max_pkt;
> +
> + max_pkt = usb_endpoint_maxp(>ep->desc); /*FIXME MATTU GET_MAX..? */
 ^^^
 huh?

-- 
balbi


signature.asc
Description: PGP signature

[RFC PATCH 4/6] xhci: don't rely on precalculated value of needed trbs in the enqueue loop

[Mathias Nyman]

Queue trbs until all payload data in the urb is tranferred.

The actual number of trbs might need to change from the pre-calculated
number when the packet alignment restrictions for td fragments in
xhci 4.11.7.1 are taken into account.

Long term plan is to get rid of calculating the needed trbs in advance
all together. It's an unnecessary extra walk through the scatterlist.

This change also allows some bulk queue function simplifications

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 75 +---
 1 file changed, 29 insertions(+), 46 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index f74ac1c..d86da81 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3109,9 +3109,10 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
struct scatterlist *sg = NULL;
bool more_trbs_coming = true;
bool need_zero_pkt = false;
-   unsigned int num_trbs, last_trb_num, i;
+   bool first_trb = true;
+   unsigned int num_trbs;
unsigned int start_cycle, num_sgs = 0;
-   unsigned int running_total, block_len, trb_buff_len, full_len;
+   unsigned int enqd_len, block_len, trb_buff_len, full_len;
int ret;
u32 field, length_field, remainder;
u64 addr;
@@ -3120,14 +3121,19 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
if (!ring)
return -EINVAL;
 
+   full_len = urb->transfer_buffer_length;
/* If we have scatter/gather list, we use it. */
if (urb->num_sgs) {
num_sgs = urb->num_mapped_sgs;
sg = urb->sg;
+   addr = (u64) sg_dma_address(sg);
+   block_len = sg_dma_len(sg);
num_trbs = count_sg_trbs_needed(urb);
-   } else
+   } else {
num_trbs = count_trbs_needed(urb);
-
+   addr = (u64) urb->transfer_dma;
+   block_len = full_len;
+   }
ret = prepare_transfer(xhci, xhci->devs[slot_id],
ep_index, urb->stream_id,
num_trbs, urb, 0, mem_flags);
@@ -3136,8 +3142,6 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
 
urb_priv = urb->hcpriv;
 
-   last_trb_num = num_trbs - 1;
-
/* Deal with URB_ZERO_PACKET - need one more td/trb */
if (urb->transfer_flags & URB_ZERO_PACKET && urb_priv->length > 1)
need_zero_pkt = true;
@@ -3152,40 +3156,20 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
start_trb = >enqueue->generic;
start_cycle = ring->cycle_state;
 
-   full_len = urb->transfer_buffer_length;
-   running_total = 0;
-   block_len = 0;
-
/* Queue the TRBs, even if they are zero-length */
-   for (i = 0; i < num_trbs; i++) {
+   for (enqd_len = 0; enqd_len < full_len; enqd_len += trb_buff_len) {
field = TRB_TYPE(TRB_NORMAL);
 
-   if (block_len == 0) {
-   /* A new contiguous block. */
-   if (sg) {
-   addr = (u64) sg_dma_address(sg);
-   block_len = sg_dma_len(sg);
-   } else {
-   addr = (u64) urb->transfer_dma;
-   block_len = full_len;
-   }
-   /* TRB buffer should not cross 64KB boundaries */
-   trb_buff_len = TRB_BUFF_LEN_UP_TO_BOUNDARY(addr);
-   trb_buff_len = min_t(unsigned int,
-   trb_buff_len,
-   block_len);
-   } else {
-   /* Further through the contiguous block. */
-   trb_buff_len = block_len;
-   if (trb_buff_len > TRB_MAX_BUFF_SIZE)
-   trb_buff_len = TRB_MAX_BUFF_SIZE;
-   }
+   /* TRB buffer should not cross 64KB boundaries */
+   trb_buff_len = TRB_BUFF_LEN_UP_TO_BOUNDARY(addr);
+   trb_buff_len = min_t(unsigned int, trb_buff_len, block_len);
 
-   if (running_total + trb_buff_len > full_len)
-   trb_buff_len = full_len - running_total;
+   if (enqd_len + trb_buff_len > full_len)
+   trb_buff_len = full_len - enqd_len;
 
/* Don't change the cycle bit of the first TRB until later */
-   if (i == 0) {
+   if (first_trb) {
+   first_trb = false;
if (start_cycle == 0)
field |= TRB_CYCLE;
} else
@@ -3194,7 +3178,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
   

[RFC PATCH 0/6] xhci: conform with xhci td fragment constraints

[Mathias Nyman]

This patchseries makes sure we follow the td fragmentation constraints
in xhci 4.11.7.1 for bulk tranfers.

The specs say that:
"constraints must be imposed to ensure that the hardware gate count and
validation requirements are minimized for xHC implementations"

The constraint we need to conform to is to make sure the payload data is
packet aligned in the last trb we queue to a segment before a link trb
moves us to the next segment.

In most usecases the payload data is packet aligned, but cases where
scatterlists with several small enrtries are used may hit this issue.

Running "testusb -a -t 7 -s 2048 -v 41 -c 5000" was able to hang the host
after minutes of testing because of unaligned payload data.
After applying this series testusb ran without issues for 4 days.

usb-eth network adapters can potentially hit this issue.

This solution tries to align the data by splitting the trb at a packet
boundary, if that is not possible then a bounce buffer is used.
Initially the plan was to try to insert a link trb mid segment and mid TD
to fulfill the constraints before resorting to a bounce buffer, but
it turned out it requires a lot of driver rewrite. That is an option
that can be implemented later after driver cleanup.

The first 4 patches are more generic cleanups and reworks needed for
the two last patches that actually makes sure data is aligned.

Mathias Nyman (6):
  xhci: rename ep_ring variable in queue_bulk_tx(), no functional change
  xhci: properly prepare zero packet TD after normal bulk TD.
  xhci: use boolean to indicate last trb in td remainder calculation
  xhci: don't rely on precalculated value of needed trbs in the enqueue
loop
  xhci: align the last trb before link if it is easily splittable.
  xhci: TD-fragment, align the unsplittable case with a bounce buffer

 drivers/usb/host/xhci-mem.c  |  76 -
 drivers/usb/host/xhci-ring.c | 265 +--
 drivers/usb/host/xhci.c  |   5 +-
 drivers/usb/host/xhci.h  |  11 +-
 4 files changed, 240 insertions(+), 117 deletions(-)

-- 
1.9.1

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

[RFC PATCH 5/6] xhci: align the last trb before link if it is easily splittable.

[Mathias Nyman]

TD fragments section 4.11.7.1 in xhci specs have additional requirements
on how trbs in TDs must be organized.

TD fragments shall not span transfer ring segments and TD fragments must
be packet aligned. Normally we don't care about TD fragments, on TD is one
big fragment, but if a TD spans ring segments it will be treated as two
fragments, and we need to comply with the alignment requirements.

For us this means that the payload data must be packet aligned in the
last trb before a link trb.
In most mass storage bulk tranfers we are lucky as the block size aligns
nicely with packet size, and there are no issues.
However, usb network adapters using scatterlists can hit this alignment
issue, and usbtest in kernel triggers this in minutes.

This patch is a partial solution, it solves the easy case when the last
trb before the link trb contains a packet boundary.
If that is the case then just split the trb at the boundary.
If not, then just print a debug message and continue as we have always
done, hoping for the best

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d86da81..c7c9521 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3098,6 +3098,27 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, int 
transferred,
return (total_packet_count - ((transferred + trb_buff_len) / maxp));
 }
 
+static int xhci_align_td(struct xhci_hcd *xhci, struct urb *urb, u32 enqd_len,
+u32 *trb_buff_len)
+{
+   unsigned int unalign;
+   unsigned int max_pkt;
+
+   max_pkt = usb_endpoint_maxp(>ep->desc); /*FIXME MATTU GET_MAX..? */
+   unalign = (enqd_len + *trb_buff_len) % max_pkt;
+
+   /* we got lucky, last normal TRB data on segment is packet aligned */
+   if (unalign == 0)
+   return 0;
+
+   /* is the last nornal TRB alignable by splitting it */
+   if (*trb_buff_len > unalign) {
+   *trb_buff_len -= unalign;
+   return 0;
+   }
+   return 1;
+}
+
 /* This is very similar to what ehci-q.c qtd_fill() does */
 int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
struct urb *urb, int slot_id, unsigned int ep_index)
@@ -3180,6 +3201,12 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
 */
if (enqd_len + trb_buff_len < full_len) {
field |= TRB_CHAIN;
+   if (last_trb(xhci, ring, ring->enq_seg,
+ring->enqueue + 1)) {
+   if (xhci_align_td(xhci, urb, enqd_len,
+_buff_len))
+   xhci_dbg(xhci, "TRB align fail\n");
+   }
} else {
field |= TRB_IOC;
more_trbs_coming = false;
-- 
1.9.1

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

[RFC PATCH 3/6] xhci: use boolean to indicate last trb in td remainder calculation

[Mathias Nyman]

We only need to know if we are queuing the last trb for a TD when
calculating the td remainder field.
The total number of trbs left is not used.

We won't be able to trust the pre-calculated number of trbs used if we
need to align trb data by splitting or merging trbs in order to satisfy
comply with data alignment requirements in xhci specs section 4.11.7.1.

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 15dd226..f74ac1c 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3074,7 +3074,7 @@ int xhci_queue_intr_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
  */
 static u32 xhci_td_remainder(struct xhci_hcd *xhci, int transferred,
  int trb_buff_len, unsigned int td_total_len,
- struct urb *urb, unsigned int num_trbs_left)
+ struct urb *urb, bool more_trbs_coming)
 {
u32 maxp, total_packet_count;
 
@@ -3083,7 +3083,7 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, int 
transferred,
return ((td_total_len - transferred) >> 10);
 
/* One TRB with a zero-length data packet. */
-   if (num_trbs_left == 0 || (transferred == 0 && trb_buff_len == 0) ||
+   if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
trb_buff_len == td_total_len)
return 0;
 
@@ -3198,7 +3198,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
field |= TRB_CHAIN;
} else {
field |= TRB_IOC;
-   more_trbs_coming = need_zero_pkt;
+   more_trbs_coming = false;
td->last_trb = ring->enqueue;
}
 
@@ -3209,13 +3209,12 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
/* Set the TRB length, TD size, and interrupter fields. */
remainder = xhci_td_remainder(xhci, running_total,
trb_buff_len, full_len,
-   urb, num_trbs - i - 1);
-
+   urb, more_trbs_coming);
length_field = TRB_LEN(trb_buff_len) |
TRB_TD_SIZE(remainder) |
TRB_INTR_TARGET(0);
 
-   queue_trb(xhci, ring, more_trbs_coming,
+   queue_trb(xhci, ring, more_trbs_coming | need_zero_pkt,
lower_32_bits(addr),
upper_32_bits(addr),
length_field,
@@ -3639,7 +3638,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, 
gfp_t mem_flags,
/* Set the TRB length, TD size, & interrupter fields. */
remainder = xhci_td_remainder(xhci, running_total,
   trb_buff_len, td_len,
-  urb, trbs_per_td - j - 1);
+  urb, more_trbs_coming);
 
length_field = TRB_LEN(trb_buff_len) |
TRB_INTR_TARGET(0);
-- 
1.9.1

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

[RFC PATCH 2/6] xhci: properly prepare zero packet TD after normal bulk TD.

[Mathias Nyman]

If a zero-length packet is needed after a bulk transfer, then an
additional zero length TD was prepared before enqueueing the bulk transfer
This set up the zero packet TD structure with incorrect td->start_seg
and td->first_trb pointers.

Prepare the zero packet TD after the data bulk TD is enqueued instead.
It sets these pointers correctly.

This change also simplifies unnecessary complexity related to keeping
track of the last trb when enqueuing trbs.

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 38 +++---
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index c3e9a60..15dd226 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3107,8 +3107,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
struct xhci_td *td;
struct xhci_generic_trb *start_trb;
struct scatterlist *sg = NULL;
-   bool more_trbs_coming;
-   bool zero_length_needed;
+   bool more_trbs_coming = true;
+   bool need_zero_pkt = false;
unsigned int num_trbs, last_trb_num, i;
unsigned int start_cycle, num_sgs = 0;
unsigned int running_total, block_len, trb_buff_len, full_len;
@@ -3139,17 +3139,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
last_trb_num = num_trbs - 1;
 
/* Deal with URB_ZERO_PACKET - need one more td/trb */
-   zero_length_needed = urb->transfer_flags & URB_ZERO_PACKET &&
-   urb_priv->length == 2;
-   if (zero_length_needed) {
-   num_trbs++;
-   xhci_dbg(xhci, "Creating zero length td.\n");
-   ret = prepare_transfer(xhci, xhci->devs[slot_id],
-   ep_index, urb->stream_id,
-   1, urb, 1, mem_flags);
-   if (unlikely(ret < 0))
-   return ret;
-   }
+   if (urb->transfer_flags & URB_ZERO_PACKET && urb_priv->length > 1)
+   need_zero_pkt = true;
 
td = urb_priv->td[0];
 
@@ -3207,12 +3198,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
field |= TRB_CHAIN;
} else {
field |= TRB_IOC;
-   if (i == last_trb_num)
-   td->last_trb = ring->enqueue;
-   else if (zero_length_needed) {
-   trb_buff_len = 0;
-   urb_priv->td[1]->last_trb = ring->enqueue;
-   }
+   more_trbs_coming = need_zero_pkt;
+   td->last_trb = ring->enqueue;
}
 
/* Only set interrupt on short packet for IN endpoints */
@@ -3228,10 +3215,6 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
TRB_TD_SIZE(remainder) |
TRB_INTR_TARGET(0);
 
-   if (i < num_trbs - 1)
-   more_trbs_coming = true;
-   else
-   more_trbs_coming = false;
queue_trb(xhci, ring, more_trbs_coming,
lower_32_bits(addr),
upper_32_bits(addr),
@@ -3253,6 +3236,15 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
}
}
 
+   if (need_zero_pkt) {
+   ret = prepare_transfer(xhci, xhci->devs[slot_id],
+  ep_index, urb->stream_id,
+  1, urb, 1, mem_flags);
+   urb_priv->td[1]->last_trb = ring->enqueue;
+   field = TRB_TYPE(TRB_NORMAL) | ring->cycle_state | TRB_IOC;
+   queue_trb(xhci, ring, 0, 0, 0, TRB_INTR_TARGET(0), field);
+   }
+
check_trb_math(urb, running_total);
giveback_first_trb(xhci, slot_id, ep_index, urb->stream_id,
start_cycle, start_trb);
-- 
1.9.1

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

[RFC PATCH 6/6] xhci: TD-fragment, align the unsplittable case with a bounce buffer

[Mathias Nyman]

If the last trb before a link is not packet size aligned, and is not
splittable then use a bounce buffer for that chunk of max packet size
unalignable data.

Allocate a max packet size bounce buffer for every segment of a bulk
endpoint ring at the same time as allocating the ring.
If we need to align the data before the link trb in that segment then
copy the data to the segment bounce buffer, dma map it, and enqueue it.
Once the td finishes, or is cancelled, unmap it.

For in transfers we need to first map the bounce buffer, then queue it,
after it finishes, copy the bounce buffer to the original sg list, and
finally unmap it

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-mem.c  |  76 ++---
 drivers/usb/host/xhci-ring.c | 113 ++-
 drivers/usb/host/xhci.c  |   5 +-
 drivers/usb/host/xhci.h  |  11 -
 4 files changed, 164 insertions(+), 41 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index bad0d1f..08621b7 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -37,7 +37,9 @@
  * "All components of all Command and Transfer TRBs shall be initialized to 
'0'"
  */
 static struct xhci_segment *xhci_segment_alloc(struct xhci_hcd *xhci,
-   unsigned int cycle_state, gfp_t flags)
+  unsigned int cycle_state,
+  unsigned int max_packet,
+  gfp_t flags)
 {
struct xhci_segment *seg;
dma_addr_t  dma;
@@ -53,6 +55,14 @@ static struct xhci_segment *xhci_segment_alloc(struct 
xhci_hcd *xhci,
return NULL;
}
 
+   if (max_packet) {
+   seg->bounce_buf = kzalloc(max_packet, flags | GFP_DMA);
+   if (!seg->bounce_buf) {
+   dma_pool_free(xhci->segment_pool, seg->trbs, dma);
+   kfree(seg);
+   return NULL;
+   }
+   }
/* If the cycle state is 0, set the cycle bit to 1 for all the TRBs */
if (cycle_state == 0) {
for (i = 0; i < TRBS_PER_SEGMENT; i++)
@@ -70,6 +80,7 @@ static void xhci_segment_free(struct xhci_hcd *xhci, struct 
xhci_segment *seg)
dma_pool_free(xhci->segment_pool, seg->trbs, seg->dma);
seg->trbs = NULL;
}
+   kfree(seg->bounce_buf);
kfree(seg);
 }
 
@@ -317,11 +328,11 @@ static void xhci_initialize_ring_info(struct xhci_ring 
*ring,
 static int xhci_alloc_segments_for_ring(struct xhci_hcd *xhci,
struct xhci_segment **first, struct xhci_segment **last,
unsigned int num_segs, unsigned int cycle_state,
-   enum xhci_ring_type type, gfp_t flags)
+   enum xhci_ring_type type, unsigned int max_packet, gfp_t flags)
 {
struct xhci_segment *prev;
 
-   prev = xhci_segment_alloc(xhci, cycle_state, flags);
+   prev = xhci_segment_alloc(xhci, cycle_state, max_packet, flags);
if (!prev)
return -ENOMEM;
num_segs--;
@@ -330,7 +341,7 @@ static int xhci_alloc_segments_for_ring(struct xhci_hcd 
*xhci,
while (num_segs > 0) {
struct xhci_segment *next;
 
-   next = xhci_segment_alloc(xhci, cycle_state, flags);
+   next = xhci_segment_alloc(xhci, cycle_state, max_packet, flags);
if (!next) {
prev = *first;
while (prev) {
@@ -360,7 +371,7 @@ static int xhci_alloc_segments_for_ring(struct xhci_hcd 
*xhci,
  */
 static struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci,
unsigned int num_segs, unsigned int cycle_state,
-   enum xhci_ring_type type, gfp_t flags)
+   enum xhci_ring_type type, unsigned int max_packet, gfp_t flags)
 {
struct xhci_ring*ring;
int ret;
@@ -370,13 +381,15 @@ static struct xhci_ring *xhci_ring_alloc(struct xhci_hcd 
*xhci,
return NULL;
 
ring->num_segs = num_segs;
+   ring->bounce_buf_len = max_packet;
INIT_LIST_HEAD(>td_list);
ring->type = type;
if (num_segs == 0)
return ring;
 
ret = xhci_alloc_segments_for_ring(xhci, >first_seg,
-   >last_seg, num_segs, cycle_state, type, flags);
+   >last_seg, num_segs, cycle_state, type,
+   max_packet, flags);
if (ret)
goto fail;
 
@@ -470,7 +483,8 @@ int xhci_ring_expansion(struct xhci_hcd *xhci, struct 
xhci_ring *ring,
ring->num_segs : num_segs_needed;
 
ret = xhci_alloc_segments_for_ring(xhci, , ,
-   num_segs, ring->cycle_state, ring->type, flags);
+   num_segs, ring->cycle_state, 

Re: [PATCH v3 08/13] usb: dwc2: gadget: Add dwc2_gadget_read_ep_interrupts function

[Sergei Shtylyov]

Hello.

On 5/26/2016 4:07 AM, John Youn wrote:


From: Vardan Mikayelyan 

Reads and returns interrupts for given endpoint, by masking epint_reg
with corresponding mask.

Tested-by: John Keeping 
Signed-off-by: Vardan Mikayelyan 
Signed-off-by: John Youn 
---
 drivers/usb/dwc2/gadget.c | 30 +-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 2e98241..051eb11 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -1947,6 +1947,34 @@ static void dwc2_hsotg_complete_in(struct dwc2_hsotg 
*hsotg,
 }

 /**
+ * dwc2_gadget_read_ep_interrupts - reads interrupts for given ep
+ * @hsotg: The device state.
+ * @idx: Index of ep.
+ * @dir_in: Endpoint direction 1-in 0-out.
+ *
+ * Reads for endpoint with given index and direction, by masking
+ * epint_reg with coresponding mask.
+ */
+static u32 dwc2_gadget_read_ep_interrupts(struct dwc2_hsotg *hsotg,
+ unsigned int idx, int dir_in)
+{
+   u32 epmsk_reg = dir_in ? DIEPMSK : DOEPMSK;
+   u32 epint_reg = dir_in ? DIEPINT(idx) : DOEPINT(idx);
+   u32 ints;
+   u32 mask;
+   u32 diepempmsk;
+
+   mask = dwc2_readl(hsotg->regs + epmsk_reg);
+   diepempmsk = dwc2_readl(hsotg->regs + DIEPEMPMSK);
+   mask |= ((diepempmsk >> idx) & 0x1) ? DIEPMSK_TXFIFOEMPTY : 0;
+   mask |= DXEPINT_SETUP_RCVD;
+
+   ints = dwc2_readl(hsotg->regs + epint_reg);
+   ints &= mask;
+   return ints;


   Why not just:

return ints & mask;


+}
+
+/**
  * dwc2_hsotg_epint - handle an in/out endpoint interrupt
  * @hsotg: The driver state
  * @idx: The index for the endpoint (0..15)

[...]

MBR, Sergei

--
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] dwc3: gadget: Introduce dwc3_endpoint_xfer_xxx() to check endpoint type

[Baolin Wang]

On 26 May 2016 at 18:27, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>> On 26 May 2016 at 17:45, Felipe Balbi  wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang  writes:
>>>
>>> 
>>>
> Also note that the usb_endpoint_xfer_isoc() call on line 2067 of
> gadget.c (as in my testing/next from today) won't even get executed, so
> we're safe there.

 Never will be executed? then we can remove the
 usb_endpoint_xfer_isoc() (line 2025) at risk?

 2023 clean_busy = dwc3_cleanup_done_reqs(dwc, dep, event, status);
 2024 if (clean_busy && (is_xfer_complete ||
 2025
 usb_endpoint_xfer_isoc(dep->endpoint.desc)))
 2026 dep->flags &= ~DWC3_EP_BUSY;
>>>
>>> hmm, now that I look at this again, in case of XferInProgress, we could
>>> still have a problem.
>>>
>>> I'll fix it up in that commit I pointed you to.
>>
>> Great. Thanks.
>
> fixed now:
>
> https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=testing/next=983b84268656ff2686253b05097d28003bbec52f

OK. I'll test it again with applying your patch. Thanks.

>
> --
> balbi



-- 
Baolin.wang
Best Regards
--
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] dwc3: gadget: Introduce dwc3_endpoint_xfer_xxx() to check endpoint type

[Felipe Balbi]

Hi,

Baolin Wang  writes:
> On 26 May 2016 at 17:45, Felipe Balbi  wrote:
>>
>> Hi,
>>
>> Baolin Wang  writes:
>>
>> 
>>
 Also note that the usb_endpoint_xfer_isoc() call on line 2067 of
 gadget.c (as in my testing/next from today) won't even get executed, so
 we're safe there.
>>>
>>> Never will be executed? then we can remove the
>>> usb_endpoint_xfer_isoc() (line 2025) at risk?
>>>
>>> 2023 clean_busy = dwc3_cleanup_done_reqs(dwc, dep, event, status);
>>> 2024 if (clean_busy && (is_xfer_complete ||
>>> 2025
>>> usb_endpoint_xfer_isoc(dep->endpoint.desc)))
>>> 2026 dep->flags &= ~DWC3_EP_BUSY;
>>
>> hmm, now that I look at this again, in case of XferInProgress, we could
>> still have a problem.
>>
>> I'll fix it up in that commit I pointed you to.
>
> Great. Thanks.

fixed now:

https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=testing/next=983b84268656ff2686253b05097d28003bbec52f

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] dwc3: gadget: Introduce dwc3_endpoint_xfer_xxx() to check endpoint type

[Baolin Wang]

On 26 May 2016 at 17:45, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>
> 
>
>>> Also note that the usb_endpoint_xfer_isoc() call on line 2067 of
>>> gadget.c (as in my testing/next from today) won't even get executed, so
>>> we're safe there.
>>
>> Never will be executed? then we can remove the
>> usb_endpoint_xfer_isoc() (line 2025) at risk?
>>
>> 2023 clean_busy = dwc3_cleanup_done_reqs(dwc, dep, event, status);
>> 2024 if (clean_busy && (is_xfer_complete ||
>> 2025
>> usb_endpoint_xfer_isoc(dep->endpoint.desc)))
>> 2026 dep->flags &= ~DWC3_EP_BUSY;
>
> hmm, now that I look at this again, in case of XferInProgress, we could
> still have a problem.
>
> I'll fix it up in that commit I pointed you to.

Great. Thanks.

>
> --
> balbi



-- 
Baolin.wang
Best Regards
--
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] dwc3: gadget: Introduce dwc3_endpoint_xfer_xxx() to check endpoint type

[Felipe Balbi]

Hi,

Baolin Wang  writes:



>> Also note that the usb_endpoint_xfer_isoc() call on line 2067 of
>> gadget.c (as in my testing/next from today) won't even get executed, so
>> we're safe there.
>
> Never will be executed? then we can remove the
> usb_endpoint_xfer_isoc() (line 2025) at risk?
>
> 2023 clean_busy = dwc3_cleanup_done_reqs(dwc, dep, event, status);
> 2024 if (clean_busy && (is_xfer_complete ||
> 2025
> usb_endpoint_xfer_isoc(dep->endpoint.desc)))
> 2026 dep->flags &= ~DWC3_EP_BUSY;

hmm, now that I look at this again, in case of XferInProgress, we could
still have a problem.

I'll fix it up in that commit I pointed you to.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] drivers: usb: dwc3 : Configure DMA properties and ops from DT

[Felipe Balbi]

Hi,

Leo Li  writes:
>>> On certain platforms (e.g. ARM64) the dma_ops needs to be explicitly set
>>> to be able to do DMA allocations, so use the of_dma_configure() helper
>>> to populate the dma properties and assign an appropriate dma_ops.
>>>
>>> Signed-off-by: Rajesh Bhagat 
>>> Reviewed-by: Yang-Leo Li 
>>
>> Cool, nxp is also using dwc3 :-) C'mon Rajesh, send us a glue layer :)
>>
>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
>>> index c679f63..4d5b783 100644
>>> --- a/drivers/usb/dwc3/host.c
>>> +++ b/drivers/usb/dwc3/host.c
>>> @@ -17,6 +17,7 @@
>>>
>>>  #include 
>>>  #include 
>>> +#include 
>>>
>>>  #include "core.h"
>>>
>>> @@ -32,6 +33,9 @@ int dwc3_host_init(struct dwc3 *dwc)
>>>   return -ENOMEM;
>>>   }
>>>
>>> + if (IS_ENABLED(CONFIG_OF) && dwc->dev->of_node)
>>> + of_dma_configure(>dev, dwc->dev->of_node);
>>
>> okay, so we have a long discussion about this going on. You can catch up
>> with it starting here:
>>
>> http://marc.info/?i=1461612094-30939-1-git-send-email-grygorii.stras...@ti.com
>>
>> At least for now, this patch will be applied. We need to have a better
>> solution for this, one that helps not only DT platforms.
>
> Balbi,
>
> Has the patch from Grygorii been applied?  I don't see it in the
> mainline tree yet.  Without fix, the dwc3 driver will fail for all
> ARM64 SoCs.

right, it's still broken. But we don't want something that fixes only
OF, right? dwc3 is also broken for PCI when IOMMU is enabled. It breaks
for the same reasons.

We really need a way to inherit DMA bits from parent device here.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] dwc3: gadget: Introduce dwc3_endpoint_xfer_xxx() to check endpoint type

[Baolin Wang]

Hi Felipe,

On 26 May 2016 at 14:22, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>> When handling the endpoint interrupt handler, it maybe disable the endpoint
>> from another core user to set the USB endpoint descriptor pointor to be NULL
>> while issuing usb_gadget_giveback_request() function to release lock. So it
>> will be one bug to check the endpoint type by usb_endpoint_xfer_xxx() APIs 
>> with
>> one NULL USB endpoint descriptor.
>
> too complex, Baolin :-) Can you see if this helps:
>
> https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?id=88bf752cfb55e57a78e27c931c9fef63240c739a
>
> The only situation when that can happen is while we drop our lock on
> dwc3_gadget_giveback().

OK, But line 1974 and line 2025 as below may be at risk? So I think
can we have a common place to solve the problem in case
usb_endpoint_xfer_xxx() APIs are issued at this risk? What do you
think? Thanks.

1956 static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
1957 const struct dwc3_event_depevt *event, int status)
1958 {
1959 struct dwc3_request *req;
1960 struct dwc3_trb *trb;
1961 unsigned intslot;
1962 unsigned inti;
1963 int ret;
1964
1965 do {
1966 req = next_request(>req_queued);
1967 if (WARN_ON_ONCE(!req))
1968 return 1;
1969
1970 i = 0;
1971 do {
1972 slot = req->start_slot + i;
1973 if ((slot == DWC3_TRB_NUM - 1) &&
1974 usb_endpoint_xfer_isoc(dep->endpoint.desc))
1975 slot++;
1976 slot %= DWC3_TRB_NUM;
1977 trb = >trb_pool[slot];
1978
1979 ret = __dwc3_cleanup_done_trbs(dwc, dep, req, trb,
1980 event, status);
1981 if (ret)
1982 break;
1983 } while (++i < req->request.num_mapped_sgs);
1984
1985 dwc3_gadget_giveback(dep, req, status);
1986
1987 if (ret)
1988 break;
1989 } while (1);
...

2011 static void dwc3_endpoint_transfer_complete(struct dwc3 *dwc,
2012 struct dwc3_ep *dep, const struct dwc3_event_depevt *event)
2013 {
2014 unsignedstatus = 0;
2015 int clean_busy;
2016 u32 is_xfer_complete;
2017
2018 is_xfer_complete = (event->endpoint_event ==
DWC3_DEPEVT_XFERCOMPLETE);
2019
2020 if (event->status & DEPEVT_STATUS_BUSERR)
2021 status = -ECONNRESET;
2022
2023 clean_busy = dwc3_cleanup_done_reqs(dwc, dep, event, status);
2024 if (clean_busy && (is_xfer_complete ||
2025
usb_endpoint_xfer_isoc(dep->endpoint.desc)))
2026 dep->flags &= ~DWC3_EP_BUSY;

>
> --
> balbi



-- 
Baolin.wang
Best Regards
--
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] usb: host: ehci-msm: Conditionally call ehci suspend/resume

[Pramod Gurav]

On 21 May 2016 at 03:05, Andy Gross  wrote:
> This patch fixes a suspend/resume issue where the driver is blindly
> calling ehci_suspend/resume functions when the ehci hasn't been setup.
> This results in a crash during suspend/resume operations.
>
> Signed-off-by: Andy Gross 

Fixes below crash while doing a system suspend:

root@linaro-alip:~# echo freeze > /sys/power/state
[   22.348960] PM: Syncing filesystems ... done.
[   22.387778] Freezing user space processes ... (elapsed 0.001 seconds) done.
[   22.393614] Freezing remaining freezable tasks ... (elapsed 0.001
seconds) done.
[   22.512736] mmc0: Reset 0x1 never completed.
[   22.514296] Unable to handle kernel NULL pointer dereference at
virtual address 04e8
[   22.516068] pgd = 80003817d000
[   22.524161] [04e8] *pgd=b817e003,
*pud=b817f003, *pmd=
[   22.535414] Internal error: Oops: 9606 [#1] PREEMPT SMP
[   22.535680] Modules linked in:
[   22.544183] CPU: 3 PID: 1499 Comm: bash Not tainted 4.6.0+ #65
[   22.544275] Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
[   22.550006] task: 800039abd780 ti: 80003928c000 task.ti:
80003928c000
[   22.556870] PC is at ehci_suspend+0x34/0xe4
[   22.564240] LR is at ehci_msm_pm_suspend+0x2c/0x34
[   22.568232] pc : [] lr : []
pstate: a145

Tested-by: Pramod Gurav 

> ---
>  drivers/usb/host/ehci-msm.c | 14 --
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
Regards,
Pramod
--
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 v2] usb: gadget: fix spinlock dead lock in gadgetfs

[Felipe Balbi]

Hi,

Bin Liu  writes:
> On Wed, May 25, 2016 at 02:18:34PM -0500, Bin Liu wrote:
>> [   40.467381] =
>> [   40.473013] [ INFO: possible recursive locking detected ]
>> [   40.478651] 4.6.0-08691-g7f3db9a #37 Not tainted
>> [   40.483466] -
>> [   40.489098] usb/733 is trying to acquire lock:
>> [   40.493734]  (&(>lock)->rlock){-.}, at: [] 
>> ep0_complete+0x18/0xdc [gadgetfs]
>> [   40.502882]
>> [   40.502882] but task is already holding lock:
>> [   40.508967]  (&(>lock)->rlock){-.}, at: [] 
>> ep0_read+0x20/0x5e0 [gadgetfs]
>> [   40.517811]
>> [   40.517811] other info that might help us debug this:
>> [   40.524623]  Possible unsafe locking scenario:
>> [   40.524623]
>> [   40.530798]CPU0
>> [   40.533346]
>> [   40.535894]   lock(&(>lock)->rlock);
>> [   40.540088]   lock(&(>lock)->rlock);
>> [   40.544284]
>> [   40.544284]  *** DEADLOCK ***
>> [   40.544284]
>> [   40.550461]  May be due to missing lock nesting notation
>> [   40.550461]
>> [   40.557544] 2 locks held by usb/733:
>> [   40.561271]  #0:  (>f_pos_lock){+.+.+.}, at: [] 
>> __fdget_pos+0x40/0x48
>> [   40.569219]  #1:  (&(>lock)->rlock){-.}, at: [] 
>> ep0_read+0x20/0x5e0 [gadgetfs]
>> [   40.578523]
>> [   40.578523] stack backtrace:
>> [   40.583075] CPU: 0 PID: 733 Comm: usb Not tainted 4.6.0-08691-g7f3db9a #37
>> [   40.590246] Hardware name: Generic AM33XX (Flattened Device Tree)
>> [   40.596625] [] (unwind_backtrace) from [] 
>> (show_stack+0x10/0x14)
>> [   40.604718] [] (show_stack) from [] 
>> (dump_stack+0xb0/0xe4)
>> [   40.612267] [] (dump_stack) from [] 
>> (__lock_acquire+0xf68/0x1994)
>> [   40.620440] [] (__lock_acquire) from [] 
>> (lock_acquire+0xd8/0x238)
>> [   40.628621] [] (lock_acquire) from [] 
>> (_raw_spin_lock_irqsave+0x38/0x4c)
>> [   40.637440] [] (_raw_spin_lock_irqsave) from [] 
>> (ep0_complete+0x18/0xdc [gadgetfs])
>> [   40.647339] [] (ep0_complete [gadgetfs]) from [] 
>> (musb_g_giveback+0x118/0x1b0 [musb_hdrc])
>> [   40.657842] [] (musb_g_giveback [musb_hdrc]) from [] 
>> (musb_g_ep0_queue+0x16c/0x188 [musb_hdrc])
>> [   40.668772] [] (musb_g_ep0_queue [musb_hdrc]) from [] 
>> (ep0_read+0x544/0x5e0 [gadgetfs])
>> [   40.678963] [] (ep0_read [gadgetfs]) from [] 
>> (__vfs_read+0x20/0x110)
>> [   40.687414] [] (__vfs_read) from [] 
>> (vfs_read+0x88/0x114)
>> [   40.694864] [] (vfs_read) from [] (SyS_read+0x44/0x9c)
>> [   40.702051] [] (SyS_read) from [] 
>> (ret_fast_syscall+0x0/0x1c)
>> 
>> Cc:  # v3.16+
>> Signed-off-by: Bin Liu 
>> ---
>> v2:
>>   - lock protects setup_req(), only unlock for usb_ep_queue()
>>   - use GFP_KERNEL in usb_ep_queue()
>>   - fix the same in two places in gadgetfs_setup() too
>
> Never mind. Sent the patch too soon. It gives the following problem.
>
> [  179.810367] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:2724
> _raw_spin_unlock_irq+0x24/0x2c
> [  179.819719] DEBUG_LOCKS_WARN_ON(current->hardirq_context)

thanks for letting us know. I'm dropping this from my queue. Please
resend final patch once you have it ;-)

-- 
balbi


signature.asc
Description: PGP signature