Re: About zero-length packet design for EHCI

[Peter Chen]

On Wed, Jul 01, 2015 at 10:16:12AM -0400, Alan Stern wrote:
> On Wed, 1 Jul 2015, Peter Chen wrote:
> 
> > On Tue, Jun 30, 2015 at 12:03:34PM -0400, Alan Stern wrote:
> > > On Tue, 30 Jun 2015, Peter Chen wrote:
> > > 
> > > > Hi Alan,
> > > > 
> > > > When reading the code (at qh_urb_transaction) about zero-length packet
> > > > for EHCI, would you please help me on below questions:
> > > > 
> > > > - I have not found the zero-length qtd prepared for control read (eg,
> > > > the transfer size is multiple of wMaxPacketSize), Am I missing
> > > > something?
> > > 
> > > The status stage transaction for a control-IN transfer has length 0, 
> > > but I guess that's not what you mean.
> > > 
> > > Control-IN transfers don't have a zero-length QTD in the data stage 
> > > because IN transfers _never_ have a zero-length QTD.
> > 
> > Then, how to cover the ch 8.5.3.2 Variable-length Data Stage:
> > 
> > If the data structure is an exact multiple of wMaxPacketSize for the
> > pipe, the function will return a zero-length packet to indicate the
> > end of the Data stage.
> > 
> > By reading your answers below, does it mean neither host nor device
> > need to prepare qtd and dtd for reading zero-length packet, the hardware
> > can handle it, and knows the data stage is over?
> 
> If a control-IN transfer has a data stage that is shorter than wLength
> and is a multiple of the ep0 maxpacket value, then the peripheral must
> send a zero-length packet to indicate the end of the data stage.  
> Thus, the UDC driver must prepare a zero-length transaction (DTD).
> 

If "one" is "multiple"?
The wLength at setup packet is 64 bytes, Maximum Packet Length at 
dQH is 64 bytes, and the Total Bytes at dTD is 64 bytes too, does device
must prepare a zero-length packet?

I would like to double confirm it since the iPhone (As host after HNP)
can't work well with it if I have a zero-length packet after 64 bytes packet
which I describe above, if without zero-length packet, the iPhone works well.

> The host hardware will recognize when this happens, because the HCD
> will set the appropriate bits in the data-stage qTD.  For example, with
> EHCI the HCD will set the Alternate Next qTD Pointer.  Or with UHCI, 
> the HCD will set the SPD (Short Packet Detect) bit.
> 

I see it in the code, but it is a null pointer (EHCI_LIST_END), does it
mean one qTD (with valid Next qTD Pointer) can handle one 64 byte packet
with or without zero-length packet well both?

Any reasons why iPhone can't handle zero-length packet well?

> But the HCD should never prepare a zero-length qTD for an IN transfer.  
> Zero-length packets are the responsibility of the _sender_, and for IN 
> transfers the sender is the peripheral, not the host.
> 
> Alan Stern
> 

-- 

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: MUSB dual-role on AM335x behaving weirdly

[Gregory CLEMENT]

Hi Felipe,

On 27/05/2015 11:42, Alexandre Belloni wrote:
> Hi,
> 
> On 26/05/2015 at 09:51:18 -0500, Felipe Balbi wrote :
>> On Thu, May 14, 2015 at 04:36:33PM -0500, Bin Liu wrote:
>>> Alexandre,
>>>
>>> On Thu, May 14, 2015 at 4:26 PM, Alexandre Belloni
>>>  wrote:
 On 14/05/2015 at 16:16:12 -0500, Bin Liu wrote :
> I think I found the root cause of the problem: board design issue - I
> bet the custom board has too much cap on VBUS line. It should be <
> 10uF.
>

 We have a custom board that exhibits the issue but it only has a 100nF
 cap on VBUS.
>>>
>>> Have you measured the VBUS discharging? Is there any way to share your
>>> schematics?
>>
>> Alexandre, any further comments ?
>>
> 
> Yeah, I have just got more info.
> 
> This is the relevant part of the schematic:
> http://free-electrons.com/~alexandre/usb.png
> 
> The total VBUS capacitance is 200nF and the USB0 pins are connected
> directly to the AM3358 pins. U1 is actually not fitted.
> 
> We didn't measure VBUS discharging but we observe the OTG pin sensing
> stops when plugging an OTG cable without any device.

Do you have any news about this topic?


Is there something else that we can do to help solving this issue?


Thanks,

Gregory


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
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 v2] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS

[Phil Edworthy]

These changes allow a PHY driver to trigger a VBUS interrupt and
to provide the value of VBUS.

Signed-off-by: Phil Edworthy 

---
v2:
  - vbus variables changed from int to bool.
  - dev_info() changed to dev_err()
---
 drivers/usb/renesas_usbhs/common.h |  2 ++
 drivers/usb/renesas_usbhs/mod.c|  3 +++
 drivers/usb/renesas_usbhs/mod_gadget.c | 38 ++
 3 files changed, 43 insertions(+)

diff --git a/drivers/usb/renesas_usbhs/common.h 
b/drivers/usb/renesas_usbhs/common.h
index 8c5fc12..478700c 100644
--- a/drivers/usb/renesas_usbhs/common.h
+++ b/drivers/usb/renesas_usbhs/common.h
@@ -255,6 +255,8 @@ struct usbhs_priv {
struct renesas_usbhs_driver_param   dparam;
 
struct delayed_work notify_hotplug_work;
+   bool vbus_is_indirect;
+   bool vbus_indirect_value;
struct platform_device *pdev;
 
struct extcon_dev *edev;
diff --git a/drivers/usb/renesas_usbhs/mod.c b/drivers/usb/renesas_usbhs/mod.c
index 9a705b1..8a2d3dd 100644
--- a/drivers/usb/renesas_usbhs/mod.c
+++ b/drivers/usb/renesas_usbhs/mod.c
@@ -42,6 +42,9 @@ static int usbhsm_autonomy_get_vbus(struct platform_device 
*pdev)
 {
struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
 
+   if (priv->vbus_is_indirect)
+   return priv->vbus_indirect_value;
+
return  VBSTS & usbhs_read(priv, INTSTS0);
 }
 
diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c 
b/drivers/usb/renesas_usbhs/mod_gadget.c
index dc2aa32..51b63f9 100644
--- a/drivers/usb/renesas_usbhs/mod_gadget.c
+++ b/drivers/usb/renesas_usbhs/mod_gadget.c
@@ -21,6 +21,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "common.h"
 
 /*
@@ -50,6 +51,7 @@ struct usbhsg_gpriv {
int  uep_size;
 
struct usb_gadget_driver*driver;
+   struct usb_phy  *transceiver;
 
u32 status;
 #define USBHSG_STATUS_STARTED  (1 << 0)
@@ -882,6 +884,8 @@ static int usbhsg_gadget_start(struct usb_gadget *gadget,
 {
struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
+   struct device *dev = usbhs_priv_to_dev(priv);
+   int ret;
 
if (!driver ||
!driver->setup  ||
@@ -891,6 +895,17 @@ static int usbhsg_gadget_start(struct usb_gadget *gadget,
/* first hook up the driver ... */
gpriv->driver = driver;
 
+   /* connect to bus through transceiver */
+   if (!IS_ERR_OR_NULL(gpriv->transceiver)) {
+   ret = otg_set_peripheral(gpriv->transceiver->otg,
+   &gpriv->gadget);
+   if (ret) {
+   dev_err(dev, "%s: can't bind to transceiver\n",
+   gpriv->gadget.name);
+   return ret;
+   }
+   }
+
return usbhsg_try_start(priv, USBHSG_STATUS_REGISTERD);
 }
 
@@ -900,6 +915,10 @@ static int usbhsg_gadget_stop(struct usb_gadget *gadget)
struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
 
usbhsg_try_stop(priv, USBHSG_STATUS_REGISTERD);
+
+   if (!IS_ERR_OR_NULL(gpriv->transceiver))
+   (void) otg_set_peripheral(gpriv->transceiver->otg, NULL);
+
gpriv->driver = NULL;
 
return 0;
@@ -947,12 +966,27 @@ static int usbhsg_set_selfpowered(struct usb_gadget 
*gadget, int is_self)
return 0;
 }
 
+static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active)
+{
+   struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
+   struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
+   struct platform_device *pdev = usbhs_priv_to_pdev(priv);
+
+   priv->vbus_is_indirect = 1;
+   priv->vbus_indirect_value = !!is_active;
+
+   renesas_usbhs_call_notify_hotplug(pdev);
+
+   return 0;
+}
+
 static const struct usb_gadget_ops usbhsg_gadget_ops = {
.get_frame  = usbhsg_get_frame,
.set_selfpowered= usbhsg_set_selfpowered,
.udc_start  = usbhsg_gadget_start,
.udc_stop   = usbhsg_gadget_stop,
.pullup = usbhsg_pullup,
+   .vbus_session   = usbhsg_vbus_session,
 };
 
 static int usbhsg_start(struct usbhs_priv *priv)
@@ -994,6 +1028,10 @@ int usbhs_mod_gadget_probe(struct usbhs_priv *priv)
goto usbhs_mod_gadget_probe_err_gpriv;
}
 
+   gpriv->transceiver = usb_get_phy(USB_PHY_TYPE_UNDEFINED);
+   dev_info(dev, "%s transceiver found\n",
+gpriv->transceiver ? "" : "No");
+
/*
 * CAUTION
 *
-- 
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: [RESEND PATCH 2/2] ARM: at91/dt: update udc compatible strings

[Nicolas Ferre]

Le 01/07/2015 22:35, Kevin Hilman a écrit :
> Nicolas Ferre  writes:
> 
>> From: Boris Brezillon 
>>
>> at91sam9g45, at91sam9x5 and sama5 SoCs should not use
>> "atmel,at91sam9rl-udc" for their USB device compatible property since
>> this compatible is attached to a specific hardware bug fix.
>>
>> Signed-off-by: Boris Brezillon 
>> Acked-by: Alexandre Belloni 
>> Tested-by: Bo Shen 
>> Acked-by: Nicolas Ferre 
>> Cc:   #4.0+
>> ---
>> Hi,
>>
>> This patch was forgotten while dealing with the series "usb: atmel_usba_udc:
>> Rework errata handling". This patch and the previous one should be added to
>> mainline as fixes, the soonest. In fact, the errata handling is now broken
>> because of this desynchronization.
>>
>> We'll try to queue it during 4.2-rc phase for inclusion in arm-soc tree.
> 
> I've picked both of these up for the arm-soc/late branch which I'll try
> to get in before -rc1, if not it will make it for -rc2.

Brilliant! Thanks a lot Kevin.

Bye,
-- 
Nicolas Ferre
--
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 v2] phy: rcar-gen2 usb: Add Host/Function switching for USB0

[Phil Edworthy]

Instead of statically selecting the PHY connection to either the
USBHS (Function) or PCI0 (Host) IP blocks, this change allows the
dts to specifiy gpio pins for the vbus and id signals. Additional
gpio pins are used to control power to an external OTG device and
an override to turn vbus on/off.

Note: the R-Car USB PHY only allows this Host/Function switching
on channel 0.

This has been tested on a r8a7791 based Koelsch board, which uses
a MAX3355 device to supply vbus power when needed.

Signed-off-by: Phil Edworthy 

---
Tested with patch "usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS"
and changes to koelsch dts.

v2:
  - Added -gpio to dts prop names of GPIO pins.
  - Document the new bindings.
---
 .../devicetree/bindings/phy/rcar-gen2-phy.txt  |  10 +
 drivers/phy/phy-rcar-gen2.c| 269 +++--
 2 files changed, 257 insertions(+), 22 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt 
b/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
index 00fc52a..3a501fe 100644
--- a/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
+++ b/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
@@ -30,6 +30,16 @@ the USB channel; see the selector meanings below:
 | 2 | PCI EHCI/OHCI | xHCI  |
 +---+---+---+
 
+Optional properties:
+  - renesas,pwr-gpio:  A gpio specifier that will be active when the
+   PHY is powered on.
+  - renesas,id-gpio:   A gpio specifier that is read to get the USB
+   ID signal.
+  - renesas,vbus-gpio: A gpio specifier that is read to get the USB
+   VBUS signal.
+  - renesas,vbus-pwr-gpio: A gpio specifier that will be active when VBUS
+   is required to be powered.
+
 Example (Lager board):
 
usb-phy@e6590100 {
diff --git a/drivers/phy/phy-rcar-gen2.c b/drivers/phy/phy-rcar-gen2.c
index 97d45f4..1e7a4d9 100644
--- a/drivers/phy/phy-rcar-gen2.c
+++ b/drivers/phy/phy-rcar-gen2.c
@@ -1,5 +1,5 @@
 /*
- * Renesas R-Car Gen2 PHY driver
+ * Renesas R-Car Gen2 USB PHY driver
  *
  * Copyright (C) 2014 Renesas Solutions Corp.
  * Copyright (C) 2014 Cogent Embedded, Inc.
@@ -12,11 +12,16 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
+#include 
+#include 
+#include 
 
 #include 
 
@@ -58,6 +63,18 @@ struct rcar_gen2_channel {
struct rcar_gen2_phy phys[PHYS_PER_CHANNEL];
int selected_phy;
u32 select_mask;
+
+   /* external power enable pin */
+   int gpio_pwr;
+
+   /* Host/Function switching */
+   struct delayed_work work;
+   int use_otg;
+   int gpio_vbus;
+   int gpio_id;
+   int gpio_vbus_pwr;
+   struct usb_phy  *usbphy;
+   struct usb_otg  *otg;
 };
 
 struct rcar_gen2_phy_driver {
@@ -68,31 +85,50 @@ struct rcar_gen2_phy_driver {
struct rcar_gen2_channel *channels;
 };
 
-static int rcar_gen2_phy_init(struct phy *p)
+static void rcar_gen2_phy_switch(struct rcar_gen2_channel *channel,
+   u32 select_value)
 {
-   struct rcar_gen2_phy *phy = phy_get_drvdata(p);
-   struct rcar_gen2_channel *channel = phy->channel;
struct rcar_gen2_phy_driver *drv = channel->drv;
unsigned long flags;
u32 ugctrl2;
 
-   /*
-* Try to acquire exclusive access to PHY.  The first driver calling
-* phy_init()  on a given channel wins, and all attempts  to use another
-* PHY on this channel will fail until phy_exit() is called by the first
-* driver.   Achieving this with cmpxcgh() should be SMP-safe.
-*/
-   if (cmpxchg(&channel->selected_phy, -1, phy->number) != -1)
-   return -EBUSY;
-
-   clk_prepare_enable(drv->clk);
-
spin_lock_irqsave(&drv->lock, flags);
ugctrl2 = readl(drv->base + USBHS_UGCTRL2);
ugctrl2 &= ~channel->select_mask;
-   ugctrl2 |= phy->select_value;
+   ugctrl2 |= select_value;
writel(ugctrl2, drv->base + USBHS_UGCTRL2);
spin_unlock_irqrestore(&drv->lock, flags);
+}
+
+static int rcar_gen2_phy_init(struct phy *p)
+{
+   struct rcar_gen2_phy *phy = phy_get_drvdata(p);
+   struct rcar_gen2_channel *channel = phy->channel;
+   struct rcar_gen2_phy_driver *drv = channel->drv;
+
+   if (!channel->use_otg) {
+   /*
+* Static Host/Function role.
+* Try to acquire exclusive access to PHY. The first driver
+* calling phy_init() on a given channel wins, and all attempts
+* to use another PHY on this channel will fail until
+* phy_exit() is called by the first driver. Achieving this
+* with cmpxc

Re: [PATCH v2] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS

[Laurent Pinchart]

Hi Phil,

(CC'ing Morimoto-san)

Thank you for the patch.

On Thursday 02 July 2015 08:36:42 Phil Edworthy wrote:
> These changes allow a PHY driver to trigger a VBUS interrupt and
> to provide the value of VBUS.
> 
> Signed-off-by: Phil Edworthy 
> 
> ---
> v2:
>   - vbus variables changed from int to bool.
>   - dev_info() changed to dev_err()
> ---
>  drivers/usb/renesas_usbhs/common.h |  2 ++
>  drivers/usb/renesas_usbhs/mod.c|  3 +++
>  drivers/usb/renesas_usbhs/mod_gadget.c | 38 +++
>  3 files changed, 43 insertions(+)
> 
> diff --git a/drivers/usb/renesas_usbhs/common.h
> b/drivers/usb/renesas_usbhs/common.h index 8c5fc12..478700c 100644
> --- a/drivers/usb/renesas_usbhs/common.h
> +++ b/drivers/usb/renesas_usbhs/common.h
> @@ -255,6 +255,8 @@ struct usbhs_priv {
>   struct renesas_usbhs_driver_param   dparam;
> 
>   struct delayed_work notify_hotplug_work;
> + bool vbus_is_indirect;
> + bool vbus_indirect_value;
>   struct platform_device *pdev;
> 
>   struct extcon_dev *edev;
> diff --git a/drivers/usb/renesas_usbhs/mod.c
> b/drivers/usb/renesas_usbhs/mod.c index 9a705b1..8a2d3dd 100644
> --- a/drivers/usb/renesas_usbhs/mod.c
> +++ b/drivers/usb/renesas_usbhs/mod.c
> @@ -42,6 +42,9 @@ static int usbhsm_autonomy_get_vbus(struct platform_device
> *pdev) {
>   struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
> 
> + if (priv->vbus_is_indirect)
> + return priv->vbus_indirect_value;
> +

Something is bothering me. The comment block right before this function states 
that "these functions are used if platform doesn't have external phy". 
However, the mechanism you implement here is aimed at letting a PHY control 
vbus. I have a feeling this isn't the right mechanism.

>   return  VBSTS & usbhs_read(priv, INTSTS0);
>  }
> 
> diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c
> b/drivers/usb/renesas_usbhs/mod_gadget.c index dc2aa32..51b63f9 100644
> --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> @@ -21,6 +21,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "common.h"
> 
>  /*
> @@ -50,6 +51,7 @@ struct usbhsg_gpriv {
>   int  uep_size;
> 
>   struct usb_gadget_driver*driver;
> + struct usb_phy  *transceiver;
> 
>   u32 status;
>  #define USBHSG_STATUS_STARTED(1 << 0)
> @@ -882,6 +884,8 @@ static int usbhsg_gadget_start(struct usb_gadget
> *gadget, {
>   struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
>   struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> + struct device *dev = usbhs_priv_to_dev(priv);
> + int ret;
> 
>   if (!driver ||
>   !driver->setup  ||
> @@ -891,6 +895,17 @@ static int usbhsg_gadget_start(struct usb_gadget
> *gadget, /* first hook up the driver ... */
>   gpriv->driver = driver;
> 
> + /* connect to bus through transceiver */
> + if (!IS_ERR_OR_NULL(gpriv->transceiver)) {
> + ret = otg_set_peripheral(gpriv->transceiver->otg,
> + &gpriv->gadget);
> + if (ret) {
> + dev_err(dev, "%s: can't bind to transceiver\n",
> + gpriv->gadget.name);

Shouldn't gpriv->driver be set to NULL here ? Or possibly better, this code 
block moved before setting gpriv->driver ?

> + return ret;
> + }
> + }
> +
>   return usbhsg_try_start(priv, USBHSG_STATUS_REGISTERD);
>  }
> 
> @@ -900,6 +915,10 @@ static int usbhsg_gadget_stop(struct usb_gadget
> *gadget) struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> 
>   usbhsg_try_stop(priv, USBHSG_STATUS_REGISTERD);
> +
> + if (!IS_ERR_OR_NULL(gpriv->transceiver))
> + (void) otg_set_peripheral(gpriv->transceiver->otg, NULL);

Is there really a need for (void) ?

> +
>   gpriv->driver = NULL;
> 
>   return 0;
> @@ -947,12 +966,27 @@ static int usbhsg_set_selfpowered(struct usb_gadget
> *gadget, int is_self) return 0;
>  }
> 
> +static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active)
> +{
> + struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> + struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> + struct platform_device *pdev = usbhs_priv_to_pdev(priv);
> +
> + priv->vbus_is_indirect = 1;
> + priv->vbus_indirect_value = !!is_active;
> +
> + renesas_usbhs_call_notify_hotplug(pdev);
> +
> + return 0;
> +}
> +
>  static const struct usb_gadget_ops usbhsg_gadget_ops = {
>   .get_frame  = usbhsg_get_frame,
>   .set_selfpowered= usbhsg_set_selfpowered,
>   .udc_start  = usbhsg_gadget_start,
>   .udc_stop   = usbhsg_gadget_stop,
>   .pullup = usbhsg_pullup,
> + .vbus_session   = usbhsg_vbus_session,
>  };
> 
>  static int usbhsg_start(stru

[PATCH v2] arm: koelsch: make USB0 perform Host/Function switching

[Phil Edworthy]

Both USB Host (pci0) and Function (USBHS) drivers are enabled.
The USB PHY driver determines which IP block should be connected
based on vbus and id signals read via gpios.

Note that switch SW5 and SW6 on Koelsch board needs to be set to
position 3 for this to work.

---

Not for upstream until the following patches have been accepted:
"usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS"
"phy: rcar-gen2 usb: Add Host/Function switching for USB0"
Hence, not signed off.

v2:
  - Added -gpio to dts prop names of GPIO pins.
---
 arch/arm/boot/dts/r8a7791-koelsch.dts | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts 
b/arch/arm/boot/dts/r8a7791-koelsch.dts
index cffe33f..1bb34d0 100644
--- a/arch/arm/boot/dts/r8a7791-koelsch.dts
+++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
@@ -615,7 +615,6 @@
 
 &pci0 {
status = "okay";
-   pinctrl-0 = <&usb0_pins>;
pinctrl-names = "default";
 };
 
@@ -627,13 +626,15 @@
 
 &hsusb {
status = "okay";
-   pinctrl-0 = <&usb0_pins>;
pinctrl-names = "default";
-   renesas,enable-gpio = <&gpio5 31 GPIO_ACTIVE_HIGH>;
 };
 
 &usbphy {
status = "okay";
+   renesas,pwr-gpio = <&gpio2 4 GPIO_ACTIVE_HIGH>;
+   renesas,id-gpio = <&gpio5 31 GPIO_ACTIVE_HIGH>;
+   renesas,vbus-gpio = <&gpio7 24 GPIO_ACTIVE_HIGH>;
+   renesas,vbus-pwr-gpio = <&gpio7 23 GPIO_ACTIVE_HIGH>;
 };
 
 &pcie_bus_clk {
-- 
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: [PATCH 2/3] phy: rcar-gen2 usb: Add Host/Function switching for USB0

[Kishon Vijay Abraham I]

Hi,

On Monday 22 June 2015 08:12 PM, Phil Edworthy wrote:
> Instead of statically selecting the PHY connection to either the
> USBHS (Function) or PCI0 (Host) IP blocks, this change allows the
> dts to specifiy gpio pins for the vbus and id signals. Additional
> gpio pins are used to control power to an external OTG device and
> an override to turn vbus on/off.
> 
> Note: the R-Car USB PHY only allows this Host/Function switching
> on channel 0.
> 
> This has been tested on a r8a7791 based Koelsch board, which uses
> a MAX3355 device to supply vbus power when needed.
> 
> Signed-off-by: Phil Edworthy 
> ---
>  drivers/phy/phy-rcar-gen2.c | 269 
> 
>  1 file changed, 247 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/phy/phy-rcar-gen2.c b/drivers/phy/phy-rcar-gen2.c
> index 97d45f4..8564e7d 100644
> --- a/drivers/phy/phy-rcar-gen2.c
> +++ b/drivers/phy/phy-rcar-gen2.c
> @@ -1,5 +1,5 @@
>  /*
> - * Renesas R-Car Gen2 PHY driver
> + * Renesas R-Car Gen2 USB PHY driver
>   *
>   * Copyright (C) 2014 Renesas Solutions Corp.
>   * Copyright (C) 2014 Cogent Embedded, Inc.
> @@ -12,11 +12,16 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 
>  
>  #include 
>  
> @@ -58,6 +63,18 @@ struct rcar_gen2_channel {
>   struct rcar_gen2_phy phys[PHYS_PER_CHANNEL];
>   int selected_phy;
>   u32 select_mask;
> +
> + /* external power enable pin */
> + int gpio_pwr;
> +
> + /* Host/Function switching */
> + struct delayed_work work;
> + int use_otg;
> + int gpio_vbus;
> + int gpio_id;
> + int gpio_vbus_pwr;
> + struct usb_phy  *usbphy;

Using usb_phy is a step backwards IMO. We should rather try to get the missing
functionality adding in Generic PHY.

Thanks
Kishon

> + struct usb_otg  *otg;
>  };
>  
>  struct rcar_gen2_phy_driver {
> @@ -68,31 +85,50 @@ struct rcar_gen2_phy_driver {
>   struct rcar_gen2_channel *channels;
>  };
>  
> -static int rcar_gen2_phy_init(struct phy *p)
> +static void rcar_gen2_phy_switch(struct rcar_gen2_channel *channel,
> + u32 select_value)
>  {
> - struct rcar_gen2_phy *phy = phy_get_drvdata(p);
> - struct rcar_gen2_channel *channel = phy->channel;
>   struct rcar_gen2_phy_driver *drv = channel->drv;
>   unsigned long flags;
>   u32 ugctrl2;
>  
> - /*
> -  * Try to acquire exclusive access to PHY.  The first driver calling
> -  * phy_init()  on a given channel wins, and all attempts  to use another
> -  * PHY on this channel will fail until phy_exit() is called by the first
> -  * driver.   Achieving this with cmpxcgh() should be SMP-safe.
> -  */
> - if (cmpxchg(&channel->selected_phy, -1, phy->number) != -1)
> - return -EBUSY;
> -
> - clk_prepare_enable(drv->clk);
> -
>   spin_lock_irqsave(&drv->lock, flags);
>   ugctrl2 = readl(drv->base + USBHS_UGCTRL2);
>   ugctrl2 &= ~channel->select_mask;
> - ugctrl2 |= phy->select_value;
> + ugctrl2 |= select_value;
>   writel(ugctrl2, drv->base + USBHS_UGCTRL2);
>   spin_unlock_irqrestore(&drv->lock, flags);
> +}
> +
> +static int rcar_gen2_phy_init(struct phy *p)
> +{
> + struct rcar_gen2_phy *phy = phy_get_drvdata(p);
> + struct rcar_gen2_channel *channel = phy->channel;
> + struct rcar_gen2_phy_driver *drv = channel->drv;
> +
> + if (!channel->use_otg) {
> + /*
> +  * Static Host/Function role.
> +  * Try to acquire exclusive access to PHY. The first driver
> +  * calling phy_init() on a given channel wins, and all attempts
> +  * to use another PHY on this channel will fail until
> +  * phy_exit() is called by the first driver. Achieving this
> +  * with cmpxcgh() should be SMP-safe.
> +  */
> + if (cmpxchg(&channel->selected_phy, -1, phy->number) != -1)
> + return -EBUSY;
> +
> + clk_prepare_enable(drv->clk);
> + rcar_gen2_phy_switch(channel, phy->select_value);
> + } else {
> + /*
> +  * Using Host/Function switching, so schedule work to determine
> +  * which role to use.
> +  */
> + clk_prepare_enable(drv->clk);
> + schedule_delayed_work(&channel->work, 100);
> + }
> +
>   return 0;
>  }
>  
> @@ -117,9 +153,9 @@ static int rcar_gen2_phy_power_on(struct phy *p)
>   u32 value;
>   int err = 0, i;
>  
> - /* Skip if it's not USBHS */
> - if (phy->select_value != USBHS_UGCTRL2_USB0SEL_HS_USB)
> - return 0;
> + /* Optional external power pin */
> + if (gpio_is_valid(phy->channel->gpio_pwr))
> + gpio_direction_o

RE: [PATCH v2] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS

[Phil Edworthy]

Hi Laurent,

On 02 July 2015 09:15, Laurent wrote:
> Hi Phil,
> 
> (CC'ing Morimoto-san)
> 
> Thank you for the patch.
> 
> On Thursday 02 July 2015 08:36:42 Phil Edworthy wrote:
> > These changes allow a PHY driver to trigger a VBUS interrupt and
> > to provide the value of VBUS.
> >
> > Signed-off-by: Phil Edworthy 
> >
> > ---
> > v2:
> >   - vbus variables changed from int to bool.
> >   - dev_info() changed to dev_err()
> > ---
> >  drivers/usb/renesas_usbhs/common.h |  2 ++
> >  drivers/usb/renesas_usbhs/mod.c|  3 +++
> >  drivers/usb/renesas_usbhs/mod_gadget.c | 38
> +++
> >  3 files changed, 43 insertions(+)
> >
> > diff --git a/drivers/usb/renesas_usbhs/common.h
> > b/drivers/usb/renesas_usbhs/common.h index 8c5fc12..478700c 100644
> > --- a/drivers/usb/renesas_usbhs/common.h
> > +++ b/drivers/usb/renesas_usbhs/common.h
> > @@ -255,6 +255,8 @@ struct usbhs_priv {
> > struct renesas_usbhs_driver_param   dparam;
> >
> > struct delayed_work notify_hotplug_work;
> > +   bool vbus_is_indirect;
> > +   bool vbus_indirect_value;
> > struct platform_device *pdev;
> >
> > struct extcon_dev *edev;
> > diff --git a/drivers/usb/renesas_usbhs/mod.c
> > b/drivers/usb/renesas_usbhs/mod.c index 9a705b1..8a2d3dd 100644
> > --- a/drivers/usb/renesas_usbhs/mod.c
> > +++ b/drivers/usb/renesas_usbhs/mod.c
> > @@ -42,6 +42,9 @@ static int usbhsm_autonomy_get_vbus(struct
> platform_device
> > *pdev) {
> > struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
> >
> > +   if (priv->vbus_is_indirect)
> > +   return priv->vbus_indirect_value;
> > +
> 
> Something is bothering me. The comment block right before this function states
> that "these functions are used if platform doesn't have external phy".
> However, the mechanism you implement here is aimed at letting a PHY control
> vbus. I have a feeling this isn't the right mechanism.

I took the comment as though it was talking about an physically external PHY, 
but
I suppose it makes no difference from the point of view of this driver. I'll 
have a
look to see if there is a better way to hook this in.


> > return  VBSTS & usbhs_read(priv, INTSTS0);
> >  }
> >
> > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c
> > b/drivers/usb/renesas_usbhs/mod_gadget.c index dc2aa32..51b63f9 100644
> > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> > @@ -21,6 +21,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include "common.h"
> >
> >  /*
> > @@ -50,6 +51,7 @@ struct usbhsg_gpriv {
> > int  uep_size;
> >
> > struct usb_gadget_driver*driver;
> > +   struct usb_phy  *transceiver;
> >
> > u32 status;
> >  #define USBHSG_STATUS_STARTED  (1 << 0)
> > @@ -882,6 +884,8 @@ static int usbhsg_gadget_start(struct usb_gadget
> > *gadget, {
> > struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> > struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> > +   struct device *dev = usbhs_priv_to_dev(priv);
> > +   int ret;
> >
> > if (!driver ||
> > !driver->setup  ||
> > @@ -891,6 +895,17 @@ static int usbhsg_gadget_start(struct usb_gadget
> > *gadget, /* first hook up the driver ... */
> > gpriv->driver = driver;
> >
> > +   /* connect to bus through transceiver */
> > +   if (!IS_ERR_OR_NULL(gpriv->transceiver)) {
> > +   ret = otg_set_peripheral(gpriv->transceiver->otg,
> > +   &gpriv->gadget);
> > +   if (ret) {
> > +   dev_err(dev, "%s: can't bind to transceiver\n",
> > +   gpriv->gadget.name);
> 
> Shouldn't gpriv->driver be set to NULL here ? Or possibly better, this code
> block moved before setting gpriv->driver ?
Makes sense.

> > +   return ret;
> > +   }
> > +   }
> > +
> > return usbhsg_try_start(priv, USBHSG_STATUS_REGISTERD);
> >  }
> >
> > @@ -900,6 +915,10 @@ static int usbhsg_gadget_stop(struct usb_gadget
> > *gadget) struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> >
> > usbhsg_try_stop(priv, USBHSG_STATUS_REGISTERD);
> > +
> > +   if (!IS_ERR_OR_NULL(gpriv->transceiver))
> > +   (void) otg_set_peripheral(gpriv->transceiver->otg, NULL);
> 
> Is there really a need for (void) ?
Oops, none at all.

> > +
> > gpriv->driver = NULL;
> >
> > return 0;
> > @@ -947,12 +966,27 @@ static int usbhsg_set_selfpowered(struct usb_gadget
> > *gadget, int is_self) return 0;
> >  }
> >
> > +static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active)
> > +{
> > +   struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> > +   struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> > +   struct platform_device *pdev = usbhs_priv_to_pdev(priv);
> > +
> > +   priv->vbus_is_indirect = 1;
> > +   priv->vbus_indirect_value = !!is_active;
> > +
> > +   renesas_usbhs_call_notify_hotp

Re: [Spice-devel] [RFC PATCH 1/1] Add a usbredir kernel module to remotely connect USB devices over IP.

[Oliver Neukum]

On Wed, 2015-07-01 at 10:06 +0100, Daniel P. Berrange wrote:

> I don't really think it is sensible to be defining & implementing new
> network services which can't support strong encryption and authentication.
> Rather than passing the file descriptor to the kernel and having it do
> the I/O directly, I think it would be better to dissassociate the kernel
> from the network transport, and thus leave all sockets layer data I/O
> to userspace daemons so they can layer in TLS or SASL or whatever else
> is appropriate for the security need.

Hi,

this hits a fundamental limit. Block IO must be done entirely in kernel
space or the system will deadlock. The USB stack is part of the block
layer and the SCSI error handling. Thus if you involve user space you
cannot honor memory allocation with GFP_NOFS and you break all APIs
where we pass GFP_NOIO in the USB stack.

Supposed you need to reset a storage device for error handling.
Your user space programm does a syscall, which allocates memory
and needs to launder pages. It proceeds to write to the storage device
you wish to reset.

It is the same problem FUSE has with writable mmap. You cannot do
block devices in user space sanely.

Sorry
Oliver


--
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-storage: ignore ZTE MF 823 in mode 0x1225

[Oliver Neukum]

On Wed, 2015-07-01 at 22:50 +0700, Lars Melin wrote:

Hi,

> Then I would like to see an lsusb listing for 19d2:1225 with an SD-card 
> interface, I have only seen 19d2:1225 with a single storage interface 
> which is for the windows install cd-rom.

My dongle indeed has only one interface. But one must not jump to
conclusions from that. Remember that multiple SCSI devices can
be connected to a single storage interface.

> There are almost no 3G dongles having the SD-card interface active in 
> default (non-switched) mode.

Here is a log made with an unfixed kernel:

Jul 02 11:11:02 linux-dtbq.site kernel: usb 3-10: new high-speed USB device 
number 6 using xhci_hcd
Jul 02 11:11:02 linux-dtbq.site kernel: usb 3-10: New USB device found, 
idVendor=19d2, idProduct=1225
Jul 02 11:11:02 linux-dtbq.site kernel: usb 3-10: New USB device strings: 
Mfr=1, Product=2, SerialNumber=3
Jul 02 11:11:02 linux-dtbq.site kernel: usb 3-10: Product: ZTE WCDMA 
Technologies MSM
Jul 02 11:11:02 linux-dtbq.site kernel: usb 3-10: Manufacturer: ZTE,Incorporated
Jul 02 11:11:02 linux-dtbq.site kernel: usb 3-10: SerialNumber: 
MF8230ZTED01CP261718U46EM0SHF3BW707_8C65&&&0
Jul 02 11:11:02 linux-dtbq.site kernel: usb-storage 3-10:1.0: USB Mass Storage 
device detected
Jul 02 11:11:02 linux-dtbq.site kernel: scsi6 : usb-storage 3-10:1.0
Jul 02 11:11:02 linux-dtbq.site kernel: usbcore: registered new interface 
driver usb-storage
Jul 02 11:11:02 linux-dtbq.site kernel: usbcore: registered new interface 
driver uas
Jul 02 11:11:02 linux-dtbq.site mtp-probe[2234]: checking bus 3, device 6: 
"/sys/devices/pci:00/:00:14.0/usb3/3-10"
Jul 02 11:11:02 linux-dtbq.site mtp-probe[2234]: bus: 3, device: 6 was not an 
MTP device
Jul 02 11:11:03 linux-dtbq.site kernel: scsi 6:0:0:0: CD-ROMCWID
 USB SCSI CD-ROM  2.31 PQ: 0 ANSI: 2
Jul 02 11:11:03 linux-dtbq.site kernel: sr2: scsi-1 drive
Jul 02 11:11:03 linux-dtbq.site kernel: sr 6:0:0:0: Attached scsi CD-ROM sr2
Jul 02 11:11:03 linux-dtbq.site kernel: sr 6:0:0:0: Attached scsi generic sg5 
type 5
Jul 02 11:11:03 linux-dtbq.site kernel: scsi 6:0:0:1: Direct-Access ZTE 
 MMC Storage  2.31 PQ: 0 ANSI: 2
Jul 02 11:11:03 linux-dtbq.site kernel: sd 6:0:0:1: Attached scsi generic sg6 
type 0
Jul 02 11:11:03 linux-dtbq.site kernel: sd 6:0:0:1: [sdd] Attached SCSI 
removable disk

As you can see, it has a CD and a card reader on different LUNs.

Regards
Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/2] usb: chipidea: Reduce ULPI PHY reset pulse to datasheet spec of 1us

[David Laight]

From: Peter Chen
> Sent: 30 June 2015 03:06
> On Fri, Jun 26, 2015 at 03:47:03PM +0200, Mike Looijmans wrote:
> > The datasheet for the 334x PHY mentions that a reset can be performed:
> > "... by bringing the pin low for a minimum of 1 microsecond and
> > then high."
> > A delay of 5ms to implement that seems overly long, so reduce it to
> > just 1us.
> > As for the delay after reset, the datasheet only mentioned that the
> > chip will assert the DIR output. 1ms seems like a safe time to wait
> > for that to happen, so no change there.
> >
> > Signed-off-by: Mike Looijmans 
> > ---
> >  drivers/usb/chipidea/core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > index e970863..c865abe 100644
> > --- a/drivers/usb/chipidea/core.c
> > +++ b/drivers/usb/chipidea/core.c
> > @@ -664,7 +664,7 @@ static int ci_hdrc_create_ulpi_phy(struct device *dev, 
> > struct ci_hdrc *ci)
> > dev_err(dev, "Failed to request ULPI reset gpio: %d\n", 
> > ret);
> > return ret;
> > }
> > -   msleep(5);
> > +   udelay(1);

If the spec says 1us I'd delay for longer just to make sure.
And add a comment saying that the reset needs to be 1us long.

David

--
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] Revert "usb: chipidea: usbmisc_imx: delete clock information"

[Peter Chen]

On Wed, Jul 01, 2015 at 12:06:19PM -0300, Fabio Estevam wrote:
> From: Fabio Estevam 
> 
> This reverts commit 73dea4a912b2bfe955305de4891018f9e71e399d.
> 
> Since commit 73dea4a912b2("usb: chipidea: usbmisc_imx: delete clock
> information") it is not possible to boot a kernel on a mx27pdk:
> 
> usbcore: registered new interface driver usb-storage
> Unhandled fault: external abort on non-linefetch (0x008) at 0xf4424600
> pgd = c0004000
> [f4424600] *pgd=1452(bad)
> Internal error: : 8 [#1] PREEMPT ARM
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper Not tainted 4.1.0-next-20150701-dirty #3089
> Hardware name: Freescale i.MX27 (Device Tree Support)
> task: c7832b60 ti: c783e000 task.ti: c783e000
> PC is at usbmisc_imx27_init+0x4c/0xbc
> LR is at usbmisc_imx27_init+0x40/0xbc
> pc : []lr : []psr: 6093
> sp : c783fe08  ip :   fp : 
> r10: c0576434  r9 : 009c  r8 : c7a773a0
> r7 : 0100  r6 : 6013  r5 : c7a776f0  r4 : c7a773f0
> r3 : f4424600  r2 :   r1 : 0001  r0 : 0001
> Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
> Control: 0005317f  Table: a0004000  DAC: 0017
> Process swapper (pid: 1, stack limit = 0xc783e190)
> Stack: (0xc783fe08 to 0xc784)
> 
> This commit also breaks the booting of imx6q-udoo board [1], so revert
> it to avoid these regressions.
> 
> [1] http://www.spinics.net/lists/linux-usb/msg125597.html
> 
> Signed-off-by: Fabio Estevam 
> ---
>  drivers/usb/chipidea/usbmisc_imx.c | 19 +++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/usb/chipidea/usbmisc_imx.c 
> b/drivers/usb/chipidea/usbmisc_imx.c
> index 140945c..8f4cd92 100644
> --- a/drivers/usb/chipidea/usbmisc_imx.c
> +++ b/drivers/usb/chipidea/usbmisc_imx.c
> @@ -11,6 +11,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -83,6 +84,7 @@ struct usbmisc_ops {
>  struct imx_usbmisc {
>   void __iomem *base;
>   spinlock_t lock;
> + struct clk *clk;
>   const struct usbmisc_ops *ops;
>  };
>  
> @@ -426,6 +428,7 @@ static int usbmisc_imx_probe(struct platform_device *pdev)
>  {
>   struct resource *res;
>   struct imx_usbmisc *data;
> + int ret;
>   struct of_device_id *tmp_dev;
>  
>   data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> @@ -439,6 +442,20 @@ static int usbmisc_imx_probe(struct platform_device 
> *pdev)
>   if (IS_ERR(data->base))
>   return PTR_ERR(data->base);
>  
> + data->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(data->clk)) {
> + dev_err(&pdev->dev,
> + "failed to get clock, err=%ld\n", PTR_ERR(data->clk));
> + return PTR_ERR(data->clk);
> + }
> +
> + ret = clk_prepare_enable(data->clk);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "clk_prepare_enable failed, err=%d\n", ret);
> + return ret;
> + }
> +
>   tmp_dev = (struct of_device_id *)
>   of_match_device(usbmisc_imx_dt_ids, &pdev->dev);
>   data->ops = (const struct usbmisc_ops *)tmp_dev->data;
> @@ -449,6 +466,8 @@ static int usbmisc_imx_probe(struct platform_device *pdev)
>  
>  static int usbmisc_imx_remove(struct platform_device *pdev)
>  {
> + struct imx_usbmisc *usbmisc = dev_get_drvdata(&pdev->dev);
> + clk_disable_unprepare(usbmisc->clk);
>   return 0;
>  }
>  
> -- 
> 1.9.1
> 

Sorry, I can't accept it, and it will remain the clock on, and break
runtime pm feature.

In fact, the core and non-core use the same clocks, and at imx27, it
may need two or three clocks to let the controller work for imx27.
I can accept handle three clocks like below at ci_hdrc_imx.c to fix
this issue.

arch/arm/boot/dts/imx25.dtsi
clocks = <&clks 9>, <&clks 70>, <&clks 8>;
clock-names = "ipg", "ahb", "per";

-- 

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

[PATCH v3] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS

[Phil Edworthy]

These changes allow a PHY driver to trigger a VBUS interrupt and
to provide the value of VBUS.

Signed-off-by: Phil Edworthy 

---
v3:
  - Changed how indirect vbus is plumbed in.
  - Removed unnecessary (void) on call to otg_set_peripheral.
  - Moved code that connects to bus through transceiver so it
is before setting gpriv->driver.

v2:
  - vbus variables changed from int to bool.
  - dev_info() changed to dev_err()
---
 drivers/usb/renesas_usbhs/common.c |  8 +--
 drivers/usb/renesas_usbhs/common.h |  2 ++
 drivers/usb/renesas_usbhs/mod_gadget.c | 38 ++
 3 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/renesas_usbhs/common.c 
b/drivers/usb/renesas_usbhs/common.c
index 0f7e850..15e67d8 100644
--- a/drivers/usb/renesas_usbhs/common.c
+++ b/drivers/usb/renesas_usbhs/common.c
@@ -379,7 +379,10 @@ static void usbhsc_hotplug(struct usbhs_priv *priv)
/*
 * get vbus status from platform
 */
-   enable = usbhs_platform_call(priv, get_vbus, pdev);
+   if (priv->vbus_is_indirect)
+   enable = priv->vbus_indirect_value;
+   else
+   enable = usbhs_platform_call(priv, get_vbus, pdev);
 
/*
 * get id from platform
@@ -662,7 +665,8 @@ static int usbhs_probe(struct platform_device *pdev)
pm_runtime_enable(&pdev->dev);
if (!usbhsc_flags_has(priv, USBHSF_RUNTIME_PWCTRL)) {
usbhsc_power_ctrl(priv, 1);
-   usbhs_mod_autonomy_mode(priv);
+   if (!priv->vbus_is_indirect)
+   usbhs_mod_autonomy_mode(priv);
}
 
/*
diff --git a/drivers/usb/renesas_usbhs/common.h 
b/drivers/usb/renesas_usbhs/common.h
index 8c5fc12..478700c 100644
--- a/drivers/usb/renesas_usbhs/common.h
+++ b/drivers/usb/renesas_usbhs/common.h
@@ -255,6 +255,8 @@ struct usbhs_priv {
struct renesas_usbhs_driver_param   dparam;
 
struct delayed_work notify_hotplug_work;
+   bool vbus_is_indirect;
+   bool vbus_indirect_value;
struct platform_device *pdev;
 
struct extcon_dev *edev;
diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c 
b/drivers/usb/renesas_usbhs/mod_gadget.c
index dc2aa32..93ad4ea 100644
--- a/drivers/usb/renesas_usbhs/mod_gadget.c
+++ b/drivers/usb/renesas_usbhs/mod_gadget.c
@@ -21,6 +21,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "common.h"
 
 /*
@@ -50,6 +51,7 @@ struct usbhsg_gpriv {
int  uep_size;
 
struct usb_gadget_driver*driver;
+   struct usb_phy  *transceiver;
 
u32 status;
 #define USBHSG_STATUS_STARTED  (1 << 0)
@@ -882,12 +884,25 @@ static int usbhsg_gadget_start(struct usb_gadget *gadget,
 {
struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
+   struct device *dev = usbhs_priv_to_dev(priv);
+   int ret;
 
if (!driver ||
!driver->setup  ||
driver->max_speed < USB_SPEED_FULL)
return -EINVAL;
 
+   /* connect to bus through transceiver */
+   if (!IS_ERR_OR_NULL(gpriv->transceiver)) {
+   ret = otg_set_peripheral(gpriv->transceiver->otg,
+   &gpriv->gadget);
+   if (ret) {
+   dev_err(dev, "%s: can't bind to transceiver\n",
+   gpriv->gadget.name);
+   return ret;
+   }
+   }
+
/* first hook up the driver ... */
gpriv->driver = driver;
 
@@ -900,6 +915,10 @@ static int usbhsg_gadget_stop(struct usb_gadget *gadget)
struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
 
usbhsg_try_stop(priv, USBHSG_STATUS_REGISTERD);
+
+   if (!IS_ERR_OR_NULL(gpriv->transceiver))
+   otg_set_peripheral(gpriv->transceiver->otg, NULL);
+
gpriv->driver = NULL;
 
return 0;
@@ -947,12 +966,27 @@ static int usbhsg_set_selfpowered(struct usb_gadget 
*gadget, int is_self)
return 0;
 }
 
+static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active)
+{
+   struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
+   struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
+   struct platform_device *pdev = usbhs_priv_to_pdev(priv);
+
+   priv->vbus_is_indirect = 1;
+   priv->vbus_indirect_value = !!is_active;
+
+   renesas_usbhs_call_notify_hotplug(pdev);
+
+   return 0;
+}
+
 static const struct usb_gadget_ops usbhsg_gadget_ops = {
.get_frame  = usbhsg_get_frame,
.set_selfpowered= usbhsg_set_selfpowered,
.udc_start  = usbhsg_gadget_start,
.udc_stop   = usbhsg_gadget_stop,
.pullup = usbhsg_pullup,
+   .vbus_session   = usbhsg_vbus_session,
 };
 
 static int usbhsg_start(s

Re: Re: [PATCH] xHCI: FSE handled cleanly by incrementing event dequeue pointer

[SAUROV KANTI SHYAM]

Some undefined values, mostly null are coming in event->buffer in my case for 
"stop,-invalid length” transfer event.
However, in the portion of the code you suggested does not increment the 
dequeue pointer of transfer event ring, for trb_comp_code = COMP_STOP_INVAL.
In handle_tx_event() cleanup: does not increment the dequeue pointer of 
transfer event ring.
In xhci_handle_event():
…
case TRB_TYPE(TRB_TRANSFER):
ret = handle_tx_event(xhci, &event->trans_event);
if (ret < 0)
xhci->error_bitmask |= 1 << 9;
else
update_ptrs = 0;
…
if (update_ptrs)
/* Update SW event ring dequeue pointer */
inc_deq(xhci, xhci->event_ring);
…
As update_ptrs=0, inc_deq(xhci, xhci->event_ring) won't run.
Regards,
-Saurov

--- Original Message ---
Sender : Mathias Nyman
Date : Jul 01, 2015 17:35 (GMT+05:30)
Title : Re: [PATCH] xHCI: FSE handled cleanly by incrementing event dequeue 
pointer

On 01.07.2015 13:13, SAUROV KANTI SHYAM wrote:
> When a transfer is in progress and a STOP command is issued to xHC, 
> xHC will generate 2 event TRBs on event ring:
> (1) Force Stopped Event TRB on successful completion of STOP cmd.
> (2) Transfer Event TRB  for the TD which was in progress and 
> stopped in the middle (with its Completion Code set to Stopped). 
> Since the value of event->buffer is undefined and should be ignored, 
> at this point the code should return after incrementing the dequeue 
> pointer of event ring.
> 

The first event should be a "stop, invalid length" transfer event in case we 
stop
in between TD's.
The  event-> buffer should be valid and contain the current dequeue pointer 
value
of the ring, see xhci 1.0 section 4.6.9.
It might still be part of the previous TD, but this is is already taken care of 
in handle_tx_event():

/*  
   
 * Skip the Force Stopped Event. The event_trb(event_dma) of FSE
   
 * is not in the current TD pointed by ep_ring->dequeue because 
   
 * that the hardware dequeue pointer still at the previous TRB  
   
 * of the current TD. The previous TRB maybe a Link TD or the   
   
 * last TRB of the previous TD. The command completion handle   
   
 * will take care the rest. 
   
 */
if (!event_seg && (trb_comp_code == COMP_STOP ||
   trb_comp_code == COMP_STOP_INVAL)) {
ret = 0;
goto cleanup;
}

The "stop,-invalid length"  transfer event is followed by a command completion 
event, which 
we handle by calling xhci_handle_cmd_stop_ep(), so no point in calling it from 
handle_tx_event()

Does this patch solve any issue in your case? there might be something else 
going on.

If you have some custom solution using event data TRBs on the transfer ring 
then it is
possible that it stops on an event data TRB, and the event->buffer is not a 
pointer to the ring.

In that case the ED (Event Data) bit in the transfer event should first be 
checked.

-Mathias

Viewing many gadgets in error under Windows with mass storage function and configfs

[Gregory CLEMENT]

Hello,

When I use configs to configure the mass storage function for the
gadget, and when the device is plugged under Windows, then the
partition that I expose is well managed, but I also see 7 other gadget
in the device manager with error.

This seven bogus gadget seems to be the 7 other LUN that are not
used. Indeed if I apply this dirty patch:

diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
b/drivers/usb/gadget/function/f_mass_storage.c
index 3cc109f..2b4ae98 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -3511,7 +3511,8 @@ static struct usb_function_instance *fsg_alloc_inst(void)
rc = PTR_ERR(opts->common);
goto release_opts;
}
-   rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
+// rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
+   rc = fsg_common_set_nluns(opts->common, 1);
if (rc)
goto release_opts;

Then there is no more gadget error under Windows. As the value of
FSG_MAX_LUNS is 8 and in my configuration I only use one partition,
then it makes sens that I see 7 bogus gadgets.

I also saw that in the legacy driver, it was possible to modify the
number of LUN using the module parameter file_count.

There is no such parameter when using configfs.

What would be the correct way to fix it?

I thought about enumerate how many LUN where configured through
configfs and then setting the exact number using fsg_common_set_nluns.

There is no problem at all under Linux so maybe it is jut the way how
the gadget is exposed through USB.

I tested it on the kernel 3.17 and 4.1 using the musb of the SoC
AM335x.

Thanks for your feedback.

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
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: renesas_usbhs: Allow an OTG PHY driver to provide VBUS

[Sergei Shtylyov]

Hello.

On 7/2/2015 10:36 AM, Phil Edworthy wrote:


These changes allow a PHY driver to trigger a VBUS interrupt and
to provide the value of VBUS.



Signed-off-by: Phil Edworthy 


[...]


diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c 
b/drivers/usb/renesas_usbhs/mod_gadget.c
index dc2aa32..51b63f9 100644
--- a/drivers/usb/renesas_usbhs/mod_gadget.c
+++ b/drivers/usb/renesas_usbhs/mod_gadget.c

[...]

@@ -947,12 +966,27 @@ static int usbhsg_set_selfpowered(struct usb_gadget 
*gadget, int is_self)
return 0;
  }

+static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active)
+{
+   struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
+   struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
+   struct platform_device *pdev = usbhs_priv_to_pdev(priv);
+
+   priv->vbus_is_indirect = 1;


   s/1/true/.


+   priv->vbus_indirect_value = !!is_active;
+
+   renesas_usbhs_call_notify_hotplug(pdev);
+
+   return 0;
+}
+

[...]

@@ -994,6 +1028,10 @@ int usbhs_mod_gadget_probe(struct usbhs_priv *priv)
goto usbhs_mod_gadget_probe_err_gpriv;
}

+   gpriv->transceiver = usb_get_phy(USB_PHY_TYPE_UNDEFINED);
+   dev_info(dev, "%s transceiver found\n",
+gpriv->transceiver ? "" : "No");


   Hm, I told you to make this message cleaner, and you haven't. :-(

[...]

WBR, 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 v2] arm: koelsch: make USB0 perform Host/Function switching

[Sergei Shtylyov]

Hello.

On 7/2/2015 11:14 AM, Phil Edworthy wrote:


Both USB Host (pci0) and Function (USBHS) drivers are enabled.
The USB PHY driver determines which IP block should be connected
based on vbus and id signals read via gpios.



Note that switch SW5 and SW6 on Koelsch board needs to be set to
position 3 for this to work.


[...]


diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts 
b/arch/arm/boot/dts/r8a7791-koelsch.dts
index cffe33f..1bb34d0 100644
--- a/arch/arm/boot/dts/r8a7791-koelsch.dts
+++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
@@ -615,7 +615,6 @@

  &pci0 {
status = "okay";
-   pinctrl-0 = <&usb0_pins>;
pinctrl-names = "default";


   As you're removing "pinctrl-0" prop, you also should remove "pinctrl-names".


  };

@@ -627,13 +626,15 @@

  &hsusb {
status = "okay";
-   pinctrl-0 = <&usb0_pins>;


   Likewise.

[...]

WBR, 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: [Spice-devel] [RFC PATCH 1/1] Add a usbredir kernel module to remotely connect USB devices over IP.

[Hans de Goede]

Hi,

On 02-07-15 10:45, Oliver Neukum wrote:

On Wed, 2015-07-01 at 10:06 +0100, Daniel P. Berrange wrote:


I don't really think it is sensible to be defining & implementing new
network services which can't support strong encryption and authentication.
Rather than passing the file descriptor to the kernel and having it do
the I/O directly, I think it would be better to dissassociate the kernel
from the network transport, and thus leave all sockets layer data I/O
to userspace daemons so they can layer in TLS or SASL or whatever else
is appropriate for the security need.


Hi,

this hits a fundamental limit. Block IO must be done entirely in kernel
space or the system will deadlock. The USB stack is part of the block
layer and the SCSI error handling. Thus if you involve user space you
cannot honor memory allocation with GFP_NOFS and you break all APIs
where we pass GFP_NOIO in the USB stack.

Supposed you need to reset a storage device for error handling.
Your user space programm does a syscall, which allocates memory
and needs to launder pages. It proceeds to write to the storage device
you wish to reset.

It is the same problem FUSE has with writable mmap. You cannot do
block devices in user space sanely.


So how is this dealt with for usbip ?

Regards,

Hans
--
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: Viewing many gadgets in error under Windows with mass storage function and configfs

[Felipe Balbi]

Hi,

On Thu, Jul 02, 2015 at 12:51:54PM +0200, Gregory CLEMENT wrote:
> Hello,
> 
> When I use configs to configure the mass storage function for the
> gadget, and when the device is plugged under Windows, then the
> partition that I expose is well managed, but I also see 7 other gadget
> in the device manager with error.
> 
> This seven bogus gadget seems to be the 7 other LUN that are not
> used. Indeed if I apply this dirty patch:
> 
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
> b/drivers/usb/gadget/function/f_mass_storage.c
> index 3cc109f..2b4ae98 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -3511,7 +3511,8 @@ static struct usb_function_instance 
> *fsg_alloc_inst(void)
> rc = PTR_ERR(opts->common);
> goto release_opts;
> }
> -   rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
> +// rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
> +   rc = fsg_common_set_nluns(opts->common, 1);
> if (rc)
> goto release_opts;
> 
> Then there is no more gadget error under Windows. As the value of
> FSG_MAX_LUNS is 8 and in my configuration I only use one partition,
> then it makes sens that I see 7 bogus gadgets.
> 
> I also saw that in the legacy driver, it was possible to modify the
> number of LUN using the module parameter file_count.

This has been reported. Michal was working on a fix, but the patch
hasn't been applied yet.

> There is no such parameter when using configfs.
> 
> What would be the correct way to fix it?

Making sure you don't lie to the other side of the cable :-)

> I thought about enumerate how many LUN where configured through
> configfs and then setting the exact number using fsg_common_set_nluns.

yeah, that should be the way.

> There is no problem at all under Linux so maybe it is jut the way how
> the gadget is exposed through USB.

no, we shouldn't really lie, this really needs fixing.

> 
> I tested it on the kernel 3.17 and 4.1 using the musb of the SoC
> AM335x.
> 
> Thanks for your feedback.
> 
> Gregory
> 
> -- 
> Gregory Clement, Free Electrons
> Kernel, drivers, real-time and embedded Linux
> development, consulting, training and support.
> http://free-electrons.com

-- 
balbi


signature.asc
Description: Digital signature

Re: Viewing many gadgets in error under Windows with mass storage function and configfs

[Gregory CLEMENT]

Hi Felipe,

thanks for you prompt feedback.

On 02/07/2015 13:45, Felipe Balbi wrote:
> Hi,
> 
> On Thu, Jul 02, 2015 at 12:51:54PM +0200, Gregory CLEMENT wrote:
>> Hello,
>>
>> When I use configs to configure the mass storage function for the
>> gadget, and when the device is plugged under Windows, then the
>> partition that I expose is well managed, but I also see 7 other gadget
>> in the device manager with error.
>>
>> This seven bogus gadget seems to be the 7 other LUN that are not
>> used. Indeed if I apply this dirty patch:
>>
>> diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
>> b/drivers/usb/gadget/function/f_mass_storage.c
>> index 3cc109f..2b4ae98 100644
>> --- a/drivers/usb/gadget/function/f_mass_storage.c
>> +++ b/drivers/usb/gadget/function/f_mass_storage.c
>> @@ -3511,7 +3511,8 @@ static struct usb_function_instance 
>> *fsg_alloc_inst(void)
>> rc = PTR_ERR(opts->common);
>> goto release_opts;
>> }
>> -   rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
>> +// rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
>> +   rc = fsg_common_set_nluns(opts->common, 1);
>> if (rc)
>> goto release_opts;
>>
>> Then there is no more gadget error under Windows. As the value of
>> FSG_MAX_LUNS is 8 and in my configuration I only use one partition,
>> then it makes sens that I see 7 bogus gadgets.
>>
>> I also saw that in the legacy driver, it was possible to modify the
>> number of LUN using the module parameter file_count.
> 
> This has been reported. Michal was working on a fix, but the patch
> hasn't been applied yet.

Oh sorry to not have finding the thread about it.

> 
>> There is no such parameter when using configfs.
>>
>> What would be the correct way to fix it?
> 
> Making sure you don't lie to the other side of the cable :-)
> 
>> I thought about enumerate how many LUN where configured through
>> configfs and then setting the exact number using fsg_common_set_nluns.
> 
> yeah, that should be the way.
> 
>> There is no problem at all under Linux so maybe it is jut the way how
>> the gadget is exposed through USB.
> 
> no, we shouldn't really lie, this really needs fixing.

Ok so don't hesitate to ask me to test or if you need any help.

Thanks,

Gregory


> 
>>
>> I tested it on the kernel 3.17 and 4.1 using the musb of the SoC
>> AM335x.
>>
>> Thanks for your feedback.
>>
>> Gregory
>>
>> -- 
>> Gregory Clement, Free Electrons
>> Kernel, drivers, real-time and embedded Linux
>> development, consulting, training and support.
>> http://free-electrons.com
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
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: renesas_usbhs: Allow an OTG PHY driver to provide VBUS

[Phil Edworthy]

Hi Sergei,

On 02 July 2015 12:17, Sergei wrote:
> To: Phil Edworthy; Yoshihiro Shimoda
> Hello.
> 
> On 7/2/2015 10:36 AM, Phil Edworthy wrote:
> 
> > These changes allow a PHY driver to trigger a VBUS interrupt and
> > to provide the value of VBUS.
> 
> > Signed-off-by: Phil Edworthy 
> 
> [...]
> 
> > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c
> b/drivers/usb/renesas_usbhs/mod_gadget.c
> > index dc2aa32..51b63f9 100644
> > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> [...]
> > @@ -947,12 +966,27 @@ static int usbhsg_set_selfpowered(struct usb_gadget
> *gadget, int is_self)
> > return 0;
> >   }
> >
> > +static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active)
> > +{
> > +   struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> > +   struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> > +   struct platform_device *pdev = usbhs_priv_to_pdev(priv);
> > +
> > +   priv->vbus_is_indirect = 1;
> 
> s/1/true/.
Ok.
 
> > +   priv->vbus_indirect_value = !!is_active;
> > +
> > +   renesas_usbhs_call_notify_hotplug(pdev);
> > +
> > +   return 0;
> > +}
> > +
> [...]
> > @@ -994,6 +1028,10 @@ int usbhs_mod_gadget_probe(struct usbhs_priv
> *priv)
> > goto usbhs_mod_gadget_probe_err_gpriv;
> > }
> >
> > +   gpriv->transceiver = usb_get_phy(USB_PHY_TYPE_UNDEFINED);
> > +   dev_info(dev, "%s transceiver found\n",
> > +gpriv->transceiver ? "" : "No");
> 
> Hm, I told you to make this message cleaner, and you haven't. :-(
Ah, sorry, I missed that comment.

> [...]
> 
> WBR, Sergei

Thanks
Phil
--
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] arm: koelsch: make USB0 perform Host/Function switching

[Phil Edworthy]

Hi Sergei.

On 02 July 2015 12:32, Sergei wrote:
> Hello.
> 
> On 7/2/2015 11:14 AM, Phil Edworthy wrote:
> 
> > Both USB Host (pci0) and Function (USBHS) drivers are enabled.
> > The USB PHY driver determines which IP block should be connected
> > based on vbus and id signals read via gpios.
> 
> > Note that switch SW5 and SW6 on Koelsch board needs to be set to
> > position 3 for this to work.
> 
> [...]
> 
> > diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts
> b/arch/arm/boot/dts/r8a7791-koelsch.dts
> > index cffe33f..1bb34d0 100644
> > --- a/arch/arm/boot/dts/r8a7791-koelsch.dts
> > +++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
> > @@ -615,7 +615,6 @@
> >
> >   &pci0 {
> > status = "okay";
> > -   pinctrl-0 = <&usb0_pins>;
> > pinctrl-names = "default";
> 
> As you're removing "pinctrl-0" prop, you also should remove 
> "pinctrl-names".
Ok.

> >   };
> >
> > @@ -627,13 +626,15 @@
> >
> >   &hsusb {
> > status = "okay";
> > -   pinctrl-0 = <&usb0_pins>;
> 
> Likewise.
Sure.

> [...]
> 
> WBR, Sergei

Thanks
Phil
--
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: [Spice-devel] [RFC PATCH 1/1] Add a usbredir kernel module to remotely connect USB devices over IP.

[Oliver Neukum]

On Thu, 2015-07-02 at 13:35 +0200, Hans de Goede wrote:
> Hi,
> 
> On 02-07-15 10:45, Oliver Neukum wrote:
> > On Wed, 2015-07-01 at 10:06 +0100, Daniel P. Berrange wrote:
> >
> >> I don't really think it is sensible to be defining & implementing new
> >> network services which can't support strong encryption and authentication.
> >> Rather than passing the file descriptor to the kernel and having it do
> >> the I/O directly, I think it would be better to dissassociate the kernel
> >> from the network transport, and thus leave all sockets layer data I/O
> >> to userspace daemons so they can layer in TLS or SASL or whatever else
> >> is appropriate for the security need.
> >
> > Hi,
> >
> > this hits a fundamental limit. Block IO must be done entirely in kernel
> > space or the system will deadlock. The USB stack is part of the block
> > layer and the SCSI error handling. Thus if you involve user space you
> > cannot honor memory allocation with GFP_NOFS and you break all APIs
> > where we pass GFP_NOIO in the USB stack.
> >
> > Supposed you need to reset a storage device for error handling.
> > Your user space programm does a syscall, which allocates memory
> > and needs to launder pages. It proceeds to write to the storage device
> > you wish to reset.
> >
> > It is the same problem FUSE has with writable mmap. You cannot do
> > block devices in user space sanely.
> 
> So how is this dealt with for usbip ?

As far as I can tell, it isn't. Running a storage device over usbip
is a bit dangerous.

Regards
Oliver


--
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] xHCI: FSE handled cleanly by incrementing event dequeue pointer

[Mathias Nyman]

Hi

On 02.07.2015 13:45, SAUROV KANTI SHYAM wrote:
> Some undefined values, mostly null are coming in event->buffer in my case for 
> "stop,-invalid length” transfer event.

That doesn't sound good, what xhci vendor and version are you using?
Have you seen/tried this on other xhci hw as well? 

What kernel version are you using? are there any custom xhci driver 
modification in it?

> However, in the portion of the code you suggested does not increment the 
> dequeue pointer of transfer event ring, for trb_comp_code = COMP_STOP_INVAL.
> In handle_tx_event() cleanup: does not increment the dequeue pointer of 
> transfer event ring.
> In xhci_handle_event():
> …
> case TRB_TYPE(TRB_TRANSFER):
> ret = handle_tx_event(xhci, &event->trans_event);
> if (ret < 0)
> xhci->error_bitmask |= 1 << 9;
> else
> update_ptrs = 0;
> …
> if (update_ptrs)
> /* Update SW event ring dequeue pointer */
> inc_deq(xhci, xhci->event_ring);
> …
> As update_ptrs=0, inc_deq(xhci, xhci->event_ring) won't run.
> Regards,

Event ring dequeue is increased either in handle_tx_event(), cleanup:

if (trb_comp_code == COMP_MISSED_INT || !ep->skip) {
inc_deq(xhci, xhci->event_ring);

Or then in xhci_handle_event() if handle_tx_event() returns error.

For example in handle_tx_event() if event->buffer is null, then
ep_ring = xhci_dma_to_transfer_ring(ep, le64_to_cpu(event->buffer));  is NULL
and the check immediately after would return with -ENODEV 

-Mathias

P.S. The potion of code was not a suggestion, it was copypasted from the 
upstream xhci driver, and has
looked like that since 3.17 kernel

--
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: Viewing many gadgets in error under Windows with mass storage function and configfs

[Felipe Balbi]

Hi,

On Thu, Jul 02, 2015 at 01:58:44PM +0200, Gregory CLEMENT wrote:
> Hi Felipe,
> 
> thanks for you prompt feedback.
> 
> On 02/07/2015 13:45, Felipe Balbi wrote:
> > Hi,
> > 
> > On Thu, Jul 02, 2015 at 12:51:54PM +0200, Gregory CLEMENT wrote:
> >> Hello,
> >>
> >> When I use configs to configure the mass storage function for the
> >> gadget, and when the device is plugged under Windows, then the
> >> partition that I expose is well managed, but I also see 7 other gadget
> >> in the device manager with error.
> >>
> >> This seven bogus gadget seems to be the 7 other LUN that are not
> >> used. Indeed if I apply this dirty patch:
> >>
> >> diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
> >> b/drivers/usb/gadget/function/f_mass_storage.c
> >> index 3cc109f..2b4ae98 100644
> >> --- a/drivers/usb/gadget/function/f_mass_storage.c
> >> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> >> @@ -3511,7 +3511,8 @@ static struct usb_function_instance 
> >> *fsg_alloc_inst(void)
> >> rc = PTR_ERR(opts->common);
> >> goto release_opts;
> >> }
> >> -   rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
> >> +// rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
> >> +   rc = fsg_common_set_nluns(opts->common, 1);
> >> if (rc)
> >> goto release_opts;
> >>
> >> Then there is no more gadget error under Windows. As the value of
> >> FSG_MAX_LUNS is 8 and in my configuration I only use one partition,
> >> then it makes sens that I see 7 bogus gadgets.
> >>
> >> I also saw that in the legacy driver, it was possible to modify the
> >> number of LUN using the module parameter file_count.
> > 
> > This has been reported. Michal was working on a fix, but the patch
> > hasn't been applied yet.
> 
> Oh sorry to not have finding the thread about it.
> 
> > 
> >> There is no such parameter when using configfs.
> >>
> >> What would be the correct way to fix it?
> > 
> > Making sure you don't lie to the other side of the cable :-)
> > 
> >> I thought about enumerate how many LUN where configured through
> >> configfs and then setting the exact number using fsg_common_set_nluns.
> > 
> > yeah, that should be the way.
> > 
> >> There is no problem at all under Linux so maybe it is jut the way how
> >> the gadget is exposed through USB.
> > 
> > no, we shouldn't really lie, this really needs fixing.
> 
> Ok so don't hesitate to ask me to test or if you need any help.

sure thing, here's the thread if you want to chip in:

http://www.spinics.net/lists/linux-usb/msg126284.html

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH] usb-storage: ignore ZTE MF 823 in mode 0x1225

[Oliver Neukum]

On Wed, 2015-07-01 at 22:50 +0700, Lars Melin wrote:

> Then I would like to see an lsusb listing for 19d2:1225 with an SD-card 
> interface, I have only seen 19d2:1225 with a single storage interface 
> which is for the windows install cd-rom.
> There are almost no 3G dongles having the SD-card interface active in 
> default (non-switched) mode.

Though I see your point. Is the switching desirable at all for this
device or do I need to block the device on the SCSI level?

Regards
Oliver


--
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: Viewing many gadgets in error under Windows with mass storage function and configfs

[Michal Nazarewicz]

> On Thu, Jul 02, 2015 at 12:51:54PM +0200, Gregory CLEMENT wrote:
>> When I use configs to configure the mass storage function for the
>> gadget, and when the device is plugged under Windows, then the
>> partition that I expose is well managed, but I also see 7 other gadget
>> in the device manager with error.
>> 
>> This seven bogus gadget seems to be the 7 other LUN that are not
>> used. Indeed if I apply this dirty patch:
>> 
>> diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
>> b/drivers/usb/gadget/function/f_mass_storage.c
>> index 3cc109f..2b4ae98 100644
>> --- a/drivers/usb/gadget/function/f_mass_storage.c
>> +++ b/drivers/usb/gadget/function/f_mass_storage.c
>> @@ -3511,7 +3511,8 @@ static struct usb_function_instance 
>> *fsg_alloc_inst(void)
>> rc = PTR_ERR(opts->common);
>> goto release_opts;
>> }
>> -   rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
>> +// rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
>> +   rc = fsg_common_set_nluns(opts->common, 1);
>> if (rc)
>> goto release_opts;
>> 
>> Then there is no more gadget error under Windows. As the value of
>> FSG_MAX_LUNS is 8 and in my configuration I only use one partition,
>> then it makes sens that I see 7 bogus gadgets.
>> 
>> I also saw that in the legacy driver, it was possible to modify the
>> number of LUN using the module parameter file_count.

On Thu, Jul 02 2015, Felipe Balbi wrote:
> This has been reported. Michal was working on a fix, but the patch
> hasn't been applied yet.

I’ve came up with [1], which you should feel free to test, but then
Krzysztof came along with [2], which among other things addressed the
LUN count issue, and I kind of stopped working on the issue waiting for
his follow up.

Since merge window is about to close, perhaps the solution is to get [1]
merged quickly so it shows up in 4.2 and then get approaches described
in [3] for the future.

[1]
,
patch also attached at the end of the message
[2] 

[3] 


- >8 ---
>From 59b6ec98aa9c638f7d140ad0b6b5a30bb2ba4145 Mon Sep 17 00:00:00 2001
From: Michal Nazarewicz 
Date: Fri, 19 Jun 2015 23:56:34 +0200
Subject: [PATCH] usb: f_mass_storage: limit number of reported LUNs

Mass storage function created via configfs always reports eight LUNs
to the hosts even if only one LUN has been configured.  Adjust the
number when the USB function is allocated based on LUNs that user
has created.

Signed-off-by: Michal Nazarewicz 
Cc: sta...@vger.kernel.org
---
 drivers/usb/gadget/function/f_mass_storage.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
b/drivers/usb/gadget/function/f_mass_storage.c
index 3cc109f..e1beb14 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -2796,8 +2796,6 @@ int fsg_common_set_nluns(struct fsg_common *common, int 
nluns)
common->luns = curlun;
common->nluns = nluns;
 
-   pr_info("Number of LUNs=%d\n", common->nluns);
-
return 0;
 }
 EXPORT_SYMBOL_GPL(fsg_common_set_nluns);
@@ -3563,14 +3561,26 @@ static struct usb_function *fsg_alloc(struct 
usb_function_instance *fi)
struct fsg_opts *opts = fsg_opts_from_func_inst(fi);
struct fsg_common *common = opts->common;
struct fsg_dev *fsg;
+   unsigned nluns, i;
 
fsg = kzalloc(sizeof(*fsg), GFP_KERNEL);
if (unlikely(!fsg))
return ERR_PTR(-ENOMEM);
 
mutex_lock(&opts->lock);
+   if (!opts->refcnt) {
+   for (nluns = i = 0; i < FSG_MAX_LUNS; ++i)
+   if (common->luns[i])
+   nluns = i + 1;
+   if (!nluns)
+   pr_warn("No LUNS defined, continuing anyway\n");
+   else
+   common->nluns = nluns;
+   pr_info("Number of LUNs=%u\n", common->nluns);
+   }
opts->refcnt++;
mutex_unlock(&opts->lock);
+
fsg->function.name  = FSG_DRIVER_DESC;
fsg->function.bind  = fsg_bind;
fsg->function.unbind= fsg_unbind;

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--
--
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 2/3] phy: rcar-gen2 usb: Add Host/Function switching for USB0

[Phil Edworthy]

Hi Kishon,

On 02 July 2015 09:22, Kishon wrote:
> Hi,
> 
> On Monday 22 June 2015 08:12 PM, Phil Edworthy wrote:
> > Instead of statically selecting the PHY connection to either the
> > USBHS (Function) or PCI0 (Host) IP blocks, this change allows the
> > dts to specifiy gpio pins for the vbus and id signals. Additional
> > gpio pins are used to control power to an external OTG device and
> > an override to turn vbus on/off.
> >
> > Note: the R-Car USB PHY only allows this Host/Function switching
> > on channel 0.
> >
> > This has been tested on a r8a7791 based Koelsch board, which uses
> > a MAX3355 device to supply vbus power when needed.
> >
> > Signed-off-by: Phil Edworthy 
> > ---
> >  drivers/phy/phy-rcar-gen2.c | 269
> 
> >  1 file changed, 247 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/phy/phy-rcar-gen2.c b/drivers/phy/phy-rcar-gen2.c
> > index 97d45f4..8564e7d 100644
> > --- a/drivers/phy/phy-rcar-gen2.c
> > +++ b/drivers/phy/phy-rcar-gen2.c
> > @@ -1,5 +1,5 @@
> >  /*
> > - * Renesas R-Car Gen2 PHY driver
> > + * Renesas R-Car Gen2 USB PHY driver
> >   *
> >   * Copyright (C) 2014 Renesas Solutions Corp.
> >   * Copyright (C) 2014 Cogent Embedded, Inc.
> > @@ -12,11 +12,16 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> > +#include 
> >
> >  #include 
> >
> > @@ -58,6 +63,18 @@ struct rcar_gen2_channel {
> > struct rcar_gen2_phy phys[PHYS_PER_CHANNEL];
> > int selected_phy;
> > u32 select_mask;
> > +
> > +   /* external power enable pin */
> > +   int gpio_pwr;
> > +
> > +   /* Host/Function switching */
> > +   struct delayed_work work;
> > +   int use_otg;
> > +   int gpio_vbus;
> > +   int gpio_id;
> > +   int gpio_vbus_pwr;
> > +   struct usb_phy  *usbphy;
> 
> Using usb_phy is a step backwards IMO. We should rather try to get the missing
> functionality adding in Generic PHY.
Ok, that will take quite a bit more work. Do you know if anyone is working on 
this?

> Thanks
> Kishon
> 
> > +   struct usb_otg  *otg;
> >  };
> >
> >  struct rcar_gen2_phy_driver {
> > @@ -68,31 +85,50 @@ struct rcar_gen2_phy_driver {
> > struct rcar_gen2_channel *channels;
> >  };
> >
> > -static int rcar_gen2_phy_init(struct phy *p)
> > +static void rcar_gen2_phy_switch(struct rcar_gen2_channel *channel,
> > +   u32 select_value)
> >  {
> > -   struct rcar_gen2_phy *phy = phy_get_drvdata(p);
> > -   struct rcar_gen2_channel *channel = phy->channel;
> > struct rcar_gen2_phy_driver *drv = channel->drv;
> > unsigned long flags;
> > u32 ugctrl2;
> >
> > -   /*
> > -* Try to acquire exclusive access to PHY.  The first driver calling
> > -* phy_init()  on a given channel wins, and all attempts  to use another
> > -* PHY on this channel will fail until phy_exit() is called by the first
> > -* driver.   Achieving this with cmpxcgh() should be SMP-safe.
> > -*/
> > -   if (cmpxchg(&channel->selected_phy, -1, phy->number) != -1)
> > -   return -EBUSY;
> > -
> > -   clk_prepare_enable(drv->clk);
> > -
> > spin_lock_irqsave(&drv->lock, flags);
> > ugctrl2 = readl(drv->base + USBHS_UGCTRL2);
> > ugctrl2 &= ~channel->select_mask;
> > -   ugctrl2 |= phy->select_value;
> > +   ugctrl2 |= select_value;
> > writel(ugctrl2, drv->base + USBHS_UGCTRL2);
> > spin_unlock_irqrestore(&drv->lock, flags);
> > +}
> > +
> > +static int rcar_gen2_phy_init(struct phy *p)
> > +{
> > +   struct rcar_gen2_phy *phy = phy_get_drvdata(p);
> > +   struct rcar_gen2_channel *channel = phy->channel;
> > +   struct rcar_gen2_phy_driver *drv = channel->drv;
> > +
> > +   if (!channel->use_otg) {
> > +   /*
> > +* Static Host/Function role.
> > +* Try to acquire exclusive access to PHY. The first driver
> > +* calling phy_init() on a given channel wins, and all attempts
> > +* to use another PHY on this channel will fail until
> > +* phy_exit() is called by the first driver. Achieving this
> > +* with cmpxcgh() should be SMP-safe.
> > +*/
> > +   if (cmpxchg(&channel->selected_phy, -1, phy->number) != -1)
> > +   return -EBUSY;
> > +
> > +   clk_prepare_enable(drv->clk);
> > +   rcar_gen2_phy_switch(channel, phy->select_value);
> > +   } else {
> > +   /*
> > +* Using Host/Function switching, so schedule work to determine
> > +* which role to use.
> > +*/
> > +   clk_prepare_enable(drv->clk);
> > +   schedule_delayed_work(&channel->work, 100);
> > +   }
> > +
> > return 0;
> >  }
> >
> > @@ -117,9 +153,9 @@ static int rcar_gen2_phy_power_on(struct phy *p)
> > u32 value;
> > 

Re: Viewing many gadgets in error under Windows with mass storage function and configfs

[Krzysztof Opasiak]

On 07/02/2015 02:45 PM, Michal Nazarewicz wrote:

On Thu, Jul 02, 2015 at 12:51:54PM +0200, Gregory CLEMENT wrote:

When I use configs to configure the mass storage function for the
gadget, and when the device is plugged under Windows, then the
partition that I expose is well managed, but I also see 7 other gadget
in the device manager with error.

This seven bogus gadget seems to be the 7 other LUN that are not
used. Indeed if I apply this dirty patch:

diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
b/drivers/usb/gadget/function/f_mass_storage.c
index 3cc109f..2b4ae98 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -3511,7 +3511,8 @@ static struct usb_function_instance *fsg_alloc_inst(void)
 rc = PTR_ERR(opts->common);
 goto release_opts;
 }
-   rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
+// rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
+   rc = fsg_common_set_nluns(opts->common, 1);
 if (rc)
 goto release_opts;

Then there is no more gadget error under Windows. As the value of
FSG_MAX_LUNS is 8 and in my configuration I only use one partition,
then it makes sens that I see 7 bogus gadgets.

I also saw that in the legacy driver, it was possible to modify the
number of LUN using the module parameter file_count.


On Thu, Jul 02 2015, Felipe Balbi wrote:

This has been reported. Michal was working on a fix, but the patch
hasn't been applied yet.


I’ve came up with [1], which you should feel free to test, but then
Krzysztof came along with [2], which among other things addressed the
LUN count issue, and I kind of stopped working on the issue waiting for
his follow up.


Sorry that it took so much time. I have been quite busy with some other 
things. I will send fixed version of that series in a few hours.



--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
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-storage: ignore ZTE MF 823 in mode 0x1225

[Oliver Neukum]

On Thu, 2015-07-02 at 14:43 +0200, Oliver Neukum wrote:
> On Wed, 2015-07-01 at 22:50 +0700, Lars Melin wrote:
> 
> > Then I would like to see an lsusb listing for 19d2:1225 with an SD-card 
> > interface, I have only seen 19d2:1225 with a single storage interface 
> > which is for the windows install cd-rom.
> > There are almost no 3G dongles having the SD-card interface active in 
> > default (non-switched) mode.
> 
> Though I see your point. Is the switching desirable at all for this
> device or do I need to block the device on the SCSI level?
> 
>   Regards
>   Oliver

However, when I reinstall usb_modeswitch I see that the storage
functionality is unnecessary to switch this device.

Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: new high-speed USB device 
number 8 using xhci_hcd
Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: New USB device found, 
idVendor=19d2, idProduct=1225
Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: New USB device strings: 
Mfr=1, Product=2, SerialNumber=3
Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: Product: ZTE WCDMA 
Technologies MSM
Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: Manufacturer: ZTE,Incorporated
Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: SerialNumber: 
MF8230ZTED01CP261718U46EM0SHF3BW707_8C65&&&0
Jul 02 14:56:10 linux-dtbq.site kernel: usb-storage 3-10:1.0: USB Mass Storage 
device detected
Jul 02 14:56:10 linux-dtbq.site kernel: Vendor: 0x19d2, Product: 0x1225, 
Revision: 0xf0f1
Jul 02 14:56:10 linux-dtbq.site kernel: Interface Subclass: 0x06, Protocol: 0x50
Jul 02 14:56:10 linux-dtbq.site kernel: usb-storage 3-10:1.0: device ignored
Jul 02 14:56:10 linux-dtbq.site kernel: storage_probe() failed
Jul 02 14:56:10 linux-dtbq.site kernel: -- sending exit command to thread
Jul 02 14:56:10 linux-dtbq.site mtp-probe[2730]: checking bus 3, device 8: 
"/sys/devices/pci:00/:00:14.0/usb3/3-10"
Jul 02 14:56:10 linux-dtbq.site mtp-probe[2730]: bus: 3, device: 8 was not an 
MTP device
Jul 02 14:56:10 linux-dtbq.site usb_modeswitch[2740]: switch device 19d2:1225 
on 003/008
Jul 02 14:56:15 linux-dtbq.site kernel: usb 3-10: USB disconnect, device number 
8
Jul 02 14:56:16 linux-dtbq.site kernel: usb 3-10: new high-speed USB device 
number 9 using xhci_hcd

Regards
Oliver


--
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: Viewing many gadgets in error under Windows with mass storage function and configfs

[Felipe Balbi]

On Thu, Jul 02, 2015 at 02:55:34PM +0200, Krzysztof Opasiak wrote:
> 
> 
> On 07/02/2015 02:45 PM, Michal Nazarewicz wrote:
> >>On Thu, Jul 02, 2015 at 12:51:54PM +0200, Gregory CLEMENT wrote:
> >>>When I use configs to configure the mass storage function for the
> >>>gadget, and when the device is plugged under Windows, then the
> >>>partition that I expose is well managed, but I also see 7 other gadget
> >>>in the device manager with error.
> >>>
> >>>This seven bogus gadget seems to be the 7 other LUN that are not
> >>>used. Indeed if I apply this dirty patch:
> >>>
> >>>diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
> >>>b/drivers/usb/gadget/function/f_mass_storage.c
> >>>index 3cc109f..2b4ae98 100644
> >>>--- a/drivers/usb/gadget/function/f_mass_storage.c
> >>>+++ b/drivers/usb/gadget/function/f_mass_storage.c
> >>>@@ -3511,7 +3511,8 @@ static struct usb_function_instance 
> >>>*fsg_alloc_inst(void)
> >>> rc = PTR_ERR(opts->common);
> >>> goto release_opts;
> >>> }
> >>>-   rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
> >>>+// rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
> >>>+   rc = fsg_common_set_nluns(opts->common, 1);
> >>> if (rc)
> >>> goto release_opts;
> >>>
> >>>Then there is no more gadget error under Windows. As the value of
> >>>FSG_MAX_LUNS is 8 and in my configuration I only use one partition,
> >>>then it makes sens that I see 7 bogus gadgets.
> >>>
> >>>I also saw that in the legacy driver, it was possible to modify the
> >>>number of LUN using the module parameter file_count.
> >
> >On Thu, Jul 02 2015, Felipe Balbi wrote:
> >>This has been reported. Michal was working on a fix, but the patch
> >>hasn't been applied yet.
> >
> >I’ve came up with [1], which you should feel free to test, but then
> >Krzysztof came along with [2], which among other things addressed the
> >LUN count issue, and I kind of stopped working on the issue waiting for
> >his follow up.
> 
> Sorry that it took so much time. I have been quite busy with some other
> things. I will send fixed version of that series in a few hours.

I'm taking Michal's patch as a quick fix for the -rc though. That patch
is simple enough to get in and solves the issue at hand.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH] usb-storage: ignore ZTE MF 823 in mode 0x1225

[Lars Melin]

On 2015-07-02 20:01, Oliver Neukum wrote:

On Thu, 2015-07-02 at 14:43 +0200, Oliver Neukum wrote:

On Wed, 2015-07-01 at 22:50 +0700, Lars Melin wrote:


Then I would like to see an lsusb listing for 19d2:1225 with an SD-card
interface, I have only seen 19d2:1225 with a single storage interface
which is for the windows install cd-rom.
There are almost no 3G dongles having the SD-card interface active in
default (non-switched) mode.


Though I see your point. Is the switching desirable at all for this
device or do I need to block the device on the SCSI level?

Regards
Oliver


However, when I reinstall usb_modeswitch I see that the storage
functionality is unnecessary to switch this device.

Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: new high-speed USB device 
number 8 using xhci_hcd
Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: New USB device found, 
idVendor=19d2, idProduct=1225
Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: New USB device strings: 
Mfr=1, Product=2, SerialNumber=3
Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: Product: ZTE WCDMA 
Technologies MSM
Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: Manufacturer: ZTE,Incorporated
Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: SerialNumber: 
MF8230ZTED01CP261718U46EM0SHF3BW707_8C65&&&0
Jul 02 14:56:10 linux-dtbq.site kernel: usb-storage 3-10:1.0: USB Mass Storage 
device detected
Jul 02 14:56:10 linux-dtbq.site kernel: Vendor: 0x19d2, Product: 0x1225, 
Revision: 0xf0f1
Jul 02 14:56:10 linux-dtbq.site kernel: Interface Subclass: 0x06, Protocol: 0x50
Jul 02 14:56:10 linux-dtbq.site kernel: usb-storage 3-10:1.0: device ignored
Jul 02 14:56:10 linux-dtbq.site kernel: storage_probe() failed
Jul 02 14:56:10 linux-dtbq.site kernel: -- sending exit command to thread
Jul 02 14:56:10 linux-dtbq.site mtp-probe[2730]: checking bus 3, device 8: 
"/sys/devices/pci:00/:00:14.0/usb3/3-10"
Jul 02 14:56:10 linux-dtbq.site mtp-probe[2730]: bus: 3, device: 8 was not an 
MTP device
Jul 02 14:56:10 linux-dtbq.site usb_modeswitch[2740]: switch device 19d2:1225 
on 003/008
Jul 02 14:56:15 linux-dtbq.site kernel: usb 3-10: USB disconnect, device number 
8
Jul 02 14:56:16 linux-dtbq.site kernel: usb 3-10: new high-speed USB device 
number 9 using xhci_hcd

Regards
Oliver



Yes but there are other 19d2:1225 devices which don't have dual-mode 
(19d2:1403 RNDIS or 19d2:1405 ECM) they do only have one of the modes 
and they don't auto-flip so they need usb_modeswitch.


This is the first 3G dongle I have seen with multiple LUN on the same 
USB interface, there has always been one interface for each storage 
function in the past. So yes I jumped to conclusion..


Anyway, it would be good if you could block the MMC interface on LUN 
level while still having the virtual cd-rom working.


cheers
/Lars

--
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-storage: ignore ZTE MF 823 in mode 0x1225

[Oliver Neukum]

On Thu, 2015-07-02 at 20:33 +0700, Lars Melin wrote:
> On 2015-07-02 20:01, Oliver Neukum wrote:
> > On Thu, 2015-07-02 at 14:43 +0200, Oliver Neukum wrote:
> >> On Wed, 2015-07-01 at 22:50 +0700, Lars Melin wrote:
> >>
> >>> Then I would like to see an lsusb listing for 19d2:1225 with an SD-card
> >>> interface, I have only seen 19d2:1225 with a single storage interface
> >>> which is for the windows install cd-rom.
> >>> There are almost no 3G dongles having the SD-card interface active in
> >>> default (non-switched) mode.
> >>
> >> Though I see your point. Is the switching desirable at all for this
> >> device or do I need to block the device on the SCSI level?
> >>
> >>Regards
> >>Oliver
> >
> > However, when I reinstall usb_modeswitch I see that the storage
> > functionality is unnecessary to switch this device.
> >
> > Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: new high-speed USB device 
> > number 8 using xhci_hcd
> > Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: New USB device found, 
> > idVendor=19d2, idProduct=1225
> > Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: New USB device strings: 
> > Mfr=1, Product=2, SerialNumber=3
> > Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: Product: ZTE WCDMA 
> > Technologies MSM
> > Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: Manufacturer: 
> > ZTE,Incorporated
> > Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: SerialNumber: 
> > MF8230ZTED01CP261718U46EM0SHF3BW707_8C65&&&0
> > Jul 02 14:56:10 linux-dtbq.site kernel: usb-storage 3-10:1.0: USB Mass 
> > Storage device detected
> > Jul 02 14:56:10 linux-dtbq.site kernel: Vendor: 0x19d2, Product: 0x1225, 
> > Revision: 0xf0f1
> > Jul 02 14:56:10 linux-dtbq.site kernel: Interface Subclass: 0x06, Protocol: 
> > 0x50
> > Jul 02 14:56:10 linux-dtbq.site kernel: usb-storage 3-10:1.0: device ignored
> > Jul 02 14:56:10 linux-dtbq.site kernel: storage_probe() failed
> > Jul 02 14:56:10 linux-dtbq.site kernel: -- sending exit command to thread
> > Jul 02 14:56:10 linux-dtbq.site mtp-probe[2730]: checking bus 3, device 8: 
> > "/sys/devices/pci:00/:00:14.0/usb3/3-10"
> > Jul 02 14:56:10 linux-dtbq.site mtp-probe[2730]: bus: 3, device: 8 was not 
> > an MTP device
> > Jul 02 14:56:10 linux-dtbq.site usb_modeswitch[2740]: switch device 
> > 19d2:1225 on 003/008
> > Jul 02 14:56:15 linux-dtbq.site kernel: usb 3-10: USB disconnect, device 
> > number 8
> > Jul 02 14:56:16 linux-dtbq.site kernel: usb 3-10: new high-speed USB device 
> > number 9 using xhci_hcd
> >
> > Regards
> > Oliver
> >
> 
> Yes but there are other 19d2:1225 devices which don't have dual-mode

So they reused the ID. Oh just great.

> (19d2:1403 RNDIS or 19d2:1405 ECM) they do only have one of the modes 
> and they don't auto-flip so they need usb_modeswitch.

Understood. And I just found out that the auto flip is unreliable
without storage. But do they need storage interfaces to switch
to the other modes? As the virtual CD does not go away I don't
really see much value in retaining the original (storage only) mode,
as all it offers is an opportunity to lose data.

> This is the first 3G dongle I have seen with multiple LUN on the same 
> USB interface, there has always been one interface for each storage 
> function in the past. So yes I jumped to conclusion..
> 
> Anyway, it would be good if you could block the MMC interface on LUN 
> level while still having the virtual cd-rom working.

This device is total crap. Do you know about alternative commands
that allow selecting the mode the device switches to?

Regards
Oliver 

--
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: Viewing many gadgets in error under Windows with mass storage function and configfs

[Gregory CLEMENT]

Hi Felipe,

On 02/07/2015 15:14, Felipe Balbi wrote:
> On Thu, Jul 02, 2015 at 02:55:34PM +0200, Krzysztof Opasiak wrote:
>>
>>
>> On 07/02/2015 02:45 PM, Michal Nazarewicz wrote:
 On Thu, Jul 02, 2015 at 12:51:54PM +0200, Gregory CLEMENT wrote:
> When I use configs to configure the mass storage function for the
> gadget, and when the device is plugged under Windows, then the
> partition that I expose is well managed, but I also see 7 other gadget
> in the device manager with error.
>
> This seven bogus gadget seems to be the 7 other LUN that are not
> used. Indeed if I apply this dirty patch:
>
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
> b/drivers/usb/gadget/function/f_mass_storage.c
> index 3cc109f..2b4ae98 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -3511,7 +3511,8 @@ static struct usb_function_instance 
> *fsg_alloc_inst(void)
> rc = PTR_ERR(opts->common);
> goto release_opts;
> }
> -   rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
> +// rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
> +   rc = fsg_common_set_nluns(opts->common, 1);
> if (rc)
> goto release_opts;
>
> Then there is no more gadget error under Windows. As the value of
> FSG_MAX_LUNS is 8 and in my configuration I only use one partition,
> then it makes sens that I see 7 bogus gadgets.
>
> I also saw that in the legacy driver, it was possible to modify the
> number of LUN using the module parameter file_count.
>>>
>>> On Thu, Jul 02 2015, Felipe Balbi wrote:
 This has been reported. Michal was working on a fix, but the patch
 hasn't been applied yet.
>>>
>>> I’ve came up with [1], which you should feel free to test, but then
>>> Krzysztof came along with [2], which among other things addressed the
>>> LUN count issue, and I kind of stopped working on the issue waiting for
>>> his follow up.
>>
>> Sorry that it took so much time. I have been quite busy with some other
>> things. I will send fixed version of that series in a few hours.
> 
> I'm taking Michal's patch as a quick fix for the -rc though. That patch
> is simple enough to get in and solves the issue at hand.

I would like to test the patch, is it the one included in the following email ?

http://www.spinics.net/lists/linux-usb/msg126292.html


Thanks,

Gregory


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
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: Viewing many gadgets in error under Windows with mass storage function and configfs

[Felipe Balbi]

On Thu, Jul 02, 2015 at 03:52:32PM +0200, Gregory CLEMENT wrote:
> Hi Felipe,
> 
> On 02/07/2015 15:14, Felipe Balbi wrote:
> > On Thu, Jul 02, 2015 at 02:55:34PM +0200, Krzysztof Opasiak wrote:
> >>
> >>
> >> On 07/02/2015 02:45 PM, Michal Nazarewicz wrote:
>  On Thu, Jul 02, 2015 at 12:51:54PM +0200, Gregory CLEMENT wrote:
> > When I use configs to configure the mass storage function for the
> > gadget, and when the device is plugged under Windows, then the
> > partition that I expose is well managed, but I also see 7 other gadget
> > in the device manager with error.
> >
> > This seven bogus gadget seems to be the 7 other LUN that are not
> > used. Indeed if I apply this dirty patch:
> >
> > diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
> > b/drivers/usb/gadget/function/f_mass_storage.c
> > index 3cc109f..2b4ae98 100644
> > --- a/drivers/usb/gadget/function/f_mass_storage.c
> > +++ b/drivers/usb/gadget/function/f_mass_storage.c
> > @@ -3511,7 +3511,8 @@ static struct usb_function_instance 
> > *fsg_alloc_inst(void)
> > rc = PTR_ERR(opts->common);
> > goto release_opts;
> > }
> > -   rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
> > +// rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
> > +   rc = fsg_common_set_nluns(opts->common, 1);
> > if (rc)
> > goto release_opts;
> >
> > Then there is no more gadget error under Windows. As the value of
> > FSG_MAX_LUNS is 8 and in my configuration I only use one partition,
> > then it makes sens that I see 7 bogus gadgets.
> >
> > I also saw that in the legacy driver, it was possible to modify the
> > number of LUN using the module parameter file_count.
> >>>
> >>> On Thu, Jul 02 2015, Felipe Balbi wrote:
>  This has been reported. Michal was working on a fix, but the patch
>  hasn't been applied yet.
> >>>
> >>> I’ve came up with [1], which you should feel free to test, but then
> >>> Krzysztof came along with [2], which among other things addressed the
> >>> LUN count issue, and I kind of stopped working on the issue waiting for
> >>> his follow up.
> >>
> >> Sorry that it took so much time. I have been quite busy with some other
> >> things. I will send fixed version of that series in a few hours.
> > 
> > I'm taking Michal's patch as a quick fix for the -rc though. That patch
> > is simple enough to get in and solves the issue at hand.
> 
> I would like to test the patch, is it the one included in the following email 
> ?
> 
> http://www.spinics.net/lists/linux-usb/msg126292.html

I just pushed it to my testing/fixes, if you test that, I can still add
your Tested-by, thanks

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH] Revert "usb: chipidea: usbmisc_imx: delete clock information"

[Maciej S. Szmigiero]

On 02.07.2015 10:40, Peter Chen wrote:
> On Wed, Jul 01, 2015 at 12:06:19PM -0300, Fabio Estevam wrote:
>> From: Fabio Estevam 
>>
>> This reverts commit 73dea4a912b2bfe955305de4891018f9e71e399d.
>>
>> Since commit 73dea4a912b2("usb: chipidea: usbmisc_imx: delete clock
>> information") it is not possible to boot a kernel on a mx27pdk:
>>
>> usbcore: registered new interface driver usb-storage
>> Unhandled fault: external abort on non-linefetch (0x008) at 0xf4424600
>> pgd = c0004000
>> [f4424600] *pgd=1452(bad)
>> Internal error: : 8 [#1] PREEMPT ARM
>> Modules linked in:
>> CPU: 0 PID: 1 Comm: swapper Not tainted 4.1.0-next-20150701-dirty #3089
>> Hardware name: Freescale i.MX27 (Device Tree Support)
>> task: c7832b60 ti: c783e000 task.ti: c783e000
>> PC is at usbmisc_imx27_init+0x4c/0xbc
>> LR is at usbmisc_imx27_init+0x40/0xbc
>> pc : []lr : []psr: 6093
>> sp : c783fe08  ip :   fp : 
>> r10: c0576434  r9 : 009c  r8 : c7a773a0
>> r7 : 0100  r6 : 6013  r5 : c7a776f0  r4 : c7a773f0
>> r3 : f4424600  r2 :   r1 : 0001  r0 : 0001
>> Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
>> Control: 0005317f  Table: a0004000  DAC: 0017
>> Process swapper (pid: 1, stack limit = 0xc783e190)
>> Stack: (0xc783fe08 to 0xc784)
>>
>> This commit also breaks the booting of imx6q-udoo board [1], so revert
>> it to avoid these regressions.
>>
>> [1] http://www.spinics.net/lists/linux-usb/msg125597.html
>>
>> Signed-off-by: Fabio Estevam 
>> ---
>>  drivers/usb/chipidea/usbmisc_imx.c | 19 +++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/usb/chipidea/usbmisc_imx.c 
>> b/drivers/usb/chipidea/usbmisc_imx.c
>> index 140945c..8f4cd92 100644
>> --- a/drivers/usb/chipidea/usbmisc_imx.c
>> +++ b/drivers/usb/chipidea/usbmisc_imx.c
>> @@ -11,6 +11,7 @@
>>  
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -83,6 +84,7 @@ struct usbmisc_ops {
>>  struct imx_usbmisc {
>>  void __iomem *base;
>>  spinlock_t lock;
>> +struct clk *clk;
>>  const struct usbmisc_ops *ops;
>>  };
>>  
>> @@ -426,6 +428,7 @@ static int usbmisc_imx_probe(struct platform_device 
>> *pdev)
>>  {
>>  struct resource *res;
>>  struct imx_usbmisc *data;
>> +int ret;
>>  struct of_device_id *tmp_dev;
>>  
>>  data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> @@ -439,6 +442,20 @@ static int usbmisc_imx_probe(struct platform_device 
>> *pdev)
>>  if (IS_ERR(data->base))
>>  return PTR_ERR(data->base);
>>  
>> +data->clk = devm_clk_get(&pdev->dev, NULL);
>> +if (IS_ERR(data->clk)) {
>> +dev_err(&pdev->dev,
>> +"failed to get clock, err=%ld\n", PTR_ERR(data->clk));
>> +return PTR_ERR(data->clk);
>> +}
>> +
>> +ret = clk_prepare_enable(data->clk);
>> +if (ret) {
>> +dev_err(&pdev->dev,
>> +"clk_prepare_enable failed, err=%d\n", ret);
>> +return ret;
>> +}
>> +
>>  tmp_dev = (struct of_device_id *)
>>  of_match_device(usbmisc_imx_dt_ids, &pdev->dev);
>>  data->ops = (const struct usbmisc_ops *)tmp_dev->data;
>> @@ -449,6 +466,8 @@ static int usbmisc_imx_probe(struct platform_device 
>> *pdev)
>>  
>>  static int usbmisc_imx_remove(struct platform_device *pdev)
>>  {
>> +struct imx_usbmisc *usbmisc = dev_get_drvdata(&pdev->dev);
>> +clk_disable_unprepare(usbmisc->clk);
>>  return 0;
>>  }
>>  
>> -- 
>> 1.9.1
>>
> 
> Sorry, I can't accept it, and it will remain the clock on, and break
> runtime pm feature.
> 
> In fact, the core and non-core use the same clocks, and at imx27, it
> may need two or three clocks to let the controller work for imx27.
> I can accept handle three clocks like below at ci_hdrc_imx.c to fix
> this issue.
> 
> arch/arm/boot/dts/imx25.dtsi
>   clocks = <&clks 9>, <&clks 70>, <&clks 8>;
>   clock-names = "ipg", "ahb", "per";
> 

This would also work for UDOO board (one clock then can be used for hub),
but there might be a problem if such clock would be disabled by
runtime PM.

The hub chip (USB2514) datasheet is silent whether the clock could
be stopped, for example during suspend, so one has to assume that
it shouldn't be done.

Best regards,
Maciej Szmigiero

--
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: Viewing many gadgets in error under Windows with mass storage function and configfs

[Krzysztof Opasiak]

On 07/02/2015 03:14 PM, Felipe Balbi wrote:

On Thu, Jul 02, 2015 at 02:55:34PM +0200, Krzysztof Opasiak wrote:



On 07/02/2015 02:45 PM, Michal Nazarewicz wrote:

On Thu, Jul 02, 2015 at 12:51:54PM +0200, Gregory CLEMENT wrote:

When I use configs to configure the mass storage function for the
gadget, and when the device is plugged under Windows, then the
partition that I expose is well managed, but I also see 7 other gadget
in the device manager with error.

This seven bogus gadget seems to be the 7 other LUN that are not
used. Indeed if I apply this dirty patch:

diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
b/drivers/usb/gadget/function/f_mass_storage.c
index 3cc109f..2b4ae98 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -3511,7 +3511,8 @@ static struct usb_function_instance *fsg_alloc_inst(void)
 rc = PTR_ERR(opts->common);
 goto release_opts;
 }
-   rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
+// rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
+   rc = fsg_common_set_nluns(opts->common, 1);
 if (rc)
 goto release_opts;

Then there is no more gadget error under Windows. As the value of
FSG_MAX_LUNS is 8 and in my configuration I only use one partition,
then it makes sens that I see 7 bogus gadgets.

I also saw that in the legacy driver, it was possible to modify the
number of LUN using the module parameter file_count.


On Thu, Jul 02 2015, Felipe Balbi wrote:

This has been reported. Michal was working on a fix, but the patch
hasn't been applied yet.


I’ve came up with [1], which you should feel free to test, but then
Krzysztof came along with [2], which among other things addressed the
LUN count issue, and I kind of stopped working on the issue waiting for
his follow up.


Sorry that it took so much time. I have been quite busy with some other
things. I will send fixed version of that series in a few hours.


I'm taking Michal's patch as a quick fix for the -rc though. That patch
is simple enough to get in and solves the issue at hand.



Nope it doesn't. It may read past buffer in case of legacy gadgets 
please see discussion in [1]


1 - http://marc.info/?l=linux-usb&m=143498343720763&w=2

BR's
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
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: Viewing many gadgets in error under Windows with mass storage function and configfs

[Gregory CLEMENT]

Hi Felipe,

On 02/07/2015 16:01, Felipe Balbi wrote:
> On Thu, Jul 02, 2015 at 03:52:32PM +0200, Gregory CLEMENT wrote:
>> Hi Felipe,
>>
>> On 02/07/2015 15:14, Felipe Balbi wrote:
>>> On Thu, Jul 02, 2015 at 02:55:34PM +0200, Krzysztof Opasiak wrote:


 On 07/02/2015 02:45 PM, Michal Nazarewicz wrote:
>> On Thu, Jul 02, 2015 at 12:51:54PM +0200, Gregory CLEMENT wrote:
>>> When I use configs to configure the mass storage function for the
>>> gadget, and when the device is plugged under Windows, then the
>>> partition that I expose is well managed, but I also see 7 other gadget
>>> in the device manager with error.
>>>
>>> This seven bogus gadget seems to be the 7 other LUN that are not
>>> used. Indeed if I apply this dirty patch:
>>>
>>> diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
>>> b/drivers/usb/gadget/function/f_mass_storage.c
>>> index 3cc109f..2b4ae98 100644
>>> --- a/drivers/usb/gadget/function/f_mass_storage.c
>>> +++ b/drivers/usb/gadget/function/f_mass_storage.c
>>> @@ -3511,7 +3511,8 @@ static struct usb_function_instance 
>>> *fsg_alloc_inst(void)
>>> rc = PTR_ERR(opts->common);
>>> goto release_opts;
>>> }
>>> -   rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
>>> +// rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
>>> +   rc = fsg_common_set_nluns(opts->common, 1);
>>> if (rc)
>>> goto release_opts;
>>>
>>> Then there is no more gadget error under Windows. As the value of
>>> FSG_MAX_LUNS is 8 and in my configuration I only use one partition,
>>> then it makes sens that I see 7 bogus gadgets.
>>>
>>> I also saw that in the legacy driver, it was possible to modify the
>>> number of LUN using the module parameter file_count.
>
> On Thu, Jul 02 2015, Felipe Balbi wrote:
>> This has been reported. Michal was working on a fix, but the patch
>> hasn't been applied yet.
>
> I’ve came up with [1], which you should feel free to test, but then
> Krzysztof came along with [2], which among other things addressed the
> LUN count issue, and I kind of stopped working on the issue waiting for
> his follow up.

 Sorry that it took so much time. I have been quite busy with some other
 things. I will send fixed version of that series in a few hours.
>>>
>>> I'm taking Michal's patch as a quick fix for the -rc though. That patch
>>> is simple enough to get in and solves the issue at hand.
>>
>> I would like to test the patch, is it the one included in the following 
>> email ?
>>
>> http://www.spinics.net/lists/linux-usb/msg126292.html
> 
> I just pushed it to my testing/fixes, if you test that, I can still add
> your Tested-by, thanks

I've tested the patch from your testing/fixes, and I have no more the 7 others
gadget seen as bogus under Windows. You can add my

Tested-by: Gregory CLEMENT 

on a AM335x based board.

Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
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: About zero-length packet design for EHCI

[Alan Stern]

On Thu, 2 Jul 2015, Peter Chen wrote:

> > If a control-IN transfer has a data stage that is shorter than wLength
> > and is a multiple of the ep0 maxpacket value, then the peripheral must
> > send a zero-length packet to indicate the end of the data stage.  
> > Thus, the UDC driver must prepare a zero-length transaction (DTD).
> > 
> 
> If "one" is "multiple"?
> The wLength at setup packet is 64 bytes, Maximum Packet Length at 
> dQH is 64 bytes, and the Total Bytes at dTD is 64 bytes too, does device
> must prepare a zero-length packet?

No.  Here is the exact rule for when a device must prepare a 0-length 
packet for a control-IN data stage.

Let wlength = le16_to_cpu(setup->wLength) and
let act_length = req->length be the actual data length.
Then an extra 0-length packet is needed if:

act_length > 0 && act_length < wlength &&
(act_length % maxpacket) == 0.

(Note that if act_length == 0 then req already gives rise to a 0-length 
packet; no additional packets are needed.)

Here's another way to describe the same rule:

 1. Every packet before the final packet must have size
equal to maxpacket.

 2. If act_length < wlength then the final packet must have
size smaller than maxpacket.

 3. If act_length == wlength and act_length > 0 then the final
packet must have size > 0.

You can check that this is equivalent to the rule above.

In the example you gave, act_length == wlength so a 0-length packet
isn't needed.

> I would like to double confirm it since the iPhone (As host after HNP)
> can't work well with it if I have a zero-length packet after 64 bytes packet
> which I describe above, if without zero-length packet, the iPhone works well.

That means the iPhone is behaving properly.

> > The host hardware will recognize when this happens, because the HCD
> > will set the appropriate bits in the data-stage qTD.  For example, with
> > EHCI the HCD will set the Alternate Next qTD Pointer.  Or with UHCI, 
> > the HCD will set the SPD (Short Packet Detect) bit.
> > 
> 
> I see it in the code, but it is a null pointer (EHCI_LIST_END), does it
> mean one qTD (with valid Next qTD Pointer) can handle one 64 byte packet
> with or without zero-length packet well both?

That is a tricky question, and the full answer involves the complicated
details of how EHCI works.

Here is a simplified answer.  If a transfer requires more than one qTD, 
then all the qTDs but the last one will have their Alternate Next qTD 
Pointers set to a special value.  The last qTD doesn't need this.

Since ehci-hcd never uses more than one qTD for a control transfer data
stage, the issue doesn't comes up.  But if you look at how ehci-hcd
handles large bulk transfers, you will see what I mean.

> Any reasons why iPhone can't handle zero-length packet well?

When the iPhone receives the 64-byte packet, it thinks the data stage 
is finished.  It moves on to the status stage, where it sends a 
0-length packet to the gadget and waits for the acknowledgment.  If the 
gadget is still trying to send a 0-length packet, it won't acknowledge 
the 0-length packet sent by the iPhone.  The status stage will time out 
and the control transfer will fail.

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: About zero-length packet design for EHCI

[Steve Calfee]

On Wed, Jul 1, 2015 at 11:00 PM, Peter Chen  wrote:
> If "one" is "multiple"?
> The wLength at setup packet is 64 bytes, Maximum Packet Length at
> dQH is 64 bytes, and the Total Bytes at dTD is 64 bytes too, does device
> must prepare a zero-length packet?
>
> I would like to double confirm it since the iPhone (As host after HNP)
> can't work well with it if I have a zero-length packet after 64 bytes packet
> which I describe above, if without zero-length packet, the iPhone works well.
>
>> The host hardware will recognize when this happens, because the HCD
>> will set the appropriate bits in the data-stage qTD.  For example, with
>> EHCI the HCD will set the Alternate Next qTD Pointer.  Or with UHCI,
>> the HCD will set the SPD (Short Packet Detect) bit.
>>
>
> I see it in the code, but it is a null pointer (EHCI_LIST_END), does it
> mean one qTD (with valid Next qTD Pointer) can handle one 64 byte packet
> with or without zero-length packet well both?
>
> Any reasons why iPhone can't handle zero-length packet well?
>
>> But the HCD should never prepare a zero-length qTD for an IN transfer.
>> Zero-length packets are the responsibility of the _sender_, and for IN
>> transfers the sender is the peripheral, not the host.

I can't speak about the details of the ehci driver. But for this issue
we can speak about sender and receiver, Host and gadget both send and
receive during transfers.

If the protocol does not specify a transfer length, then the sender
MUST send a zlt if the transfer is less than expected and a multiple
(1*, 2*, 3*) of maxpacketsize.

If the protocol does specify a transfer length, then exactly that
length should be sent and received with NO ZLT. For example USB mass
storage sends multiples of maxpacketsize but does not send ZLTs.

If you do an analyzer trace between windows and your gadget, then you
can see what those guys think the rules for your protocol are.


Regards, Steve
--
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: Configfs usb mass_storage device always reports 8 LUNs ?

[Michal Nazarewicz]

Sorry, I didn’t notice this reply before.

> On 06/22/2015 04:24 PM, Michal Nazarewicz wrote:
>> common->nluns is set to FSG_MAX_LUNS in fsg_alloc_inst (which is called
>> prior to fsg_alloc) so this is not an issue.

On Mon, Jun 22 2015, Krzysztof Opasiak wrote:
> True, but all legacy gadgets which use mass storage call 
> fsg_common_set_nluns() after reading module parameters and after 
> allocating function instance. As argument they pass number luns received 
> in module params.

Right.  As a quick fix I would propose doing:

--- >8 -
diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
b/drivers/usb/gadget/function/f_mass_storage.c
index e1beb14..15c3071 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -2786,7 +2786,7 @@ int fsg_common_set_nluns(struct fsg_common *common, int 
nluns)
return -EINVAL;
}
 
-   curlun = kcalloc(nluns, sizeof(*curlun), GFP_KERNEL);
+   curlun = kcalloc(FSG_MAX_LUNS, sizeof(*curlun), GFP_KERNEL);
if (unlikely(!curlun))
return -ENOMEM;
 
--- >8 -

Since I think we agreed that luns should just be turned into an array
with static length the above won’t be too far from that goal.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--
--
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: Viewing many gadgets in error under Windows with mass storage function and configfs

[Michal Nazarewicz]

> On 07/02/2015 03:14 PM, Felipe Balbi wrote:
>> I'm taking Michal's patch as a quick fix for the -rc though. That patch
>> is simple enough to get in and solves the issue at hand.

On Thu, Jul 02 2015, Krzysztof Opasiak wrote:
> Nope it doesn't. It may read past buffer in case of legacy gadgets 
> please see discussion in [1]
>
> 1 - http://marc.info/?l=linux-usb&m=143498343720763&w=2

Sorry, I’ve missed that reply before.  Let’s bring the discussion back
to the original thread.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--
--
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-storage: ignore ZTE MF 823 in mode 0x1225

[Dan Williams]

On Thu, 2015-07-02 at 15:43 +0200, Oliver Neukum wrote:
> On Thu, 2015-07-02 at 20:33 +0700, Lars Melin wrote:
> > On 2015-07-02 20:01, Oliver Neukum wrote:
> > > On Thu, 2015-07-02 at 14:43 +0200, Oliver Neukum wrote:
> > >> On Wed, 2015-07-01 at 22:50 +0700, Lars Melin wrote:
> > >>
> > >>> Then I would like to see an lsusb listing for 19d2:1225 with an SD-card
> > >>> interface, I have only seen 19d2:1225 with a single storage interface
> > >>> which is for the windows install cd-rom.
> > >>> There are almost no 3G dongles having the SD-card interface active in
> > >>> default (non-switched) mode.
> > >>
> > >> Though I see your point. Is the switching desirable at all for this
> > >> device or do I need to block the device on the SCSI level?
> > >>
> > >>  Regards
> > >>  Oliver
> > >
> > > However, when I reinstall usb_modeswitch I see that the storage
> > > functionality is unnecessary to switch this device.
> > >
> > > Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: new high-speed USB 
> > > device number 8 using xhci_hcd
> > > Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: New USB device found, 
> > > idVendor=19d2, idProduct=1225
> > > Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: New USB device strings: 
> > > Mfr=1, Product=2, SerialNumber=3
> > > Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: Product: ZTE WCDMA 
> > > Technologies MSM
> > > Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: Manufacturer: 
> > > ZTE,Incorporated
> > > Jul 02 14:56:10 linux-dtbq.site kernel: usb 3-10: SerialNumber: 
> > > MF8230ZTED01CP261718U46EM0SHF3BW707_8C65&&&0
> > > Jul 02 14:56:10 linux-dtbq.site kernel: usb-storage 3-10:1.0: USB Mass 
> > > Storage device detected
> > > Jul 02 14:56:10 linux-dtbq.site kernel: Vendor: 0x19d2, Product: 0x1225, 
> > > Revision: 0xf0f1
> > > Jul 02 14:56:10 linux-dtbq.site kernel: Interface Subclass: 0x06, 
> > > Protocol: 0x50
> > > Jul 02 14:56:10 linux-dtbq.site kernel: usb-storage 3-10:1.0: device 
> > > ignored
> > > Jul 02 14:56:10 linux-dtbq.site kernel: storage_probe() failed
> > > Jul 02 14:56:10 linux-dtbq.site kernel: -- sending exit command to thread
> > > Jul 02 14:56:10 linux-dtbq.site mtp-probe[2730]: checking bus 3, device 
> > > 8: "/sys/devices/pci:00/:00:14.0/usb3/3-10"
> > > Jul 02 14:56:10 linux-dtbq.site mtp-probe[2730]: bus: 3, device: 8 was 
> > > not an MTP device
> > > Jul 02 14:56:10 linux-dtbq.site usb_modeswitch[2740]: switch device 
> > > 19d2:1225 on 003/008
> > > Jul 02 14:56:15 linux-dtbq.site kernel: usb 3-10: USB disconnect, device 
> > > number 8
> > > Jul 02 14:56:16 linux-dtbq.site kernel: usb 3-10: new high-speed USB 
> > > device number 9 using xhci_hcd
> > >
> > >   Regards
> > >   Oliver
> > >
> > 
> > Yes but there are other 19d2:1225 devices which don't have dual-mode
> 
> So they reused the ID. Oh just great.

Welcome to the world of USB WWAN dongles.  This happens *all* the
time...  The worst offenders I know of are Huawei and ZTE, probably
because they make so many.  For example 12d1:1001 is used by like 50
devices before modeswitch (I jest, but not by much).

Dan

> > (19d2:1403 RNDIS or 19d2:1405 ECM) they do only have one of the modes 
> > and they don't auto-flip so they need usb_modeswitch.
> 
> Understood. And I just found out that the auto flip is unreliable
> without storage. But do they need storage interfaces to switch
> to the other modes? As the virtual CD does not go away I don't
> really see much value in retaining the original (storage only) mode,
> as all it offers is an opportunity to lose data.
> 
> > This is the first 3G dongle I have seen with multiple LUN on the same 
> > USB interface, there has always been one interface for each storage 
> > function in the past. So yes I jumped to conclusion..
> > 
> > Anyway, it would be good if you could block the MMC interface on LUN 
> > level while still having the virtual cd-rom working.
> 
> This device is total crap. Do you know about alternative commands
> that allow selecting the mode the device switches to?
> 
>   Regards
>   Oliver 
> 
> --
> 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


--
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: [Spice-devel] [RFC PATCH 1/1] Add a usbredir kernel module to remotely connect USB devices over IP.

[Jeremy White]

On 07/02/2015 07:10 AM, Oliver Neukum wrote:
> On Thu, 2015-07-02 at 13:35 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 02-07-15 10:45, Oliver Neukum wrote:
>>> On Wed, 2015-07-01 at 10:06 +0100, Daniel P. Berrange wrote:
>>>
 I don't really think it is sensible to be defining & implementing new
 network services which can't support strong encryption and authentication.
 Rather than passing the file descriptor to the kernel and having it do
 the I/O directly, I think it would be better to dissassociate the kernel
 from the network transport, and thus leave all sockets layer data I/O
 to userspace daemons so they can layer in TLS or SASL or whatever else
 is appropriate for the security need.
>>>
>>> Hi,
>>>
>>> this hits a fundamental limit. Block IO must be done entirely in kernel
>>> space or the system will deadlock. The USB stack is part of the block
>>> layer and the SCSI error handling. Thus if you involve user space you
>>> cannot honor memory allocation with GFP_NOFS and you break all APIs
>>> where we pass GFP_NOIO in the USB stack.
>>>
>>> Supposed you need to reset a storage device for error handling.
>>> Your user space programm does a syscall, which allocates memory
>>> and needs to launder pages. It proceeds to write to the storage device
>>> you wish to reset.
>>>
>>> It is the same problem FUSE has with writable mmap. You cannot do
>>> block devices in user space sanely.
>>
>> So how is this dealt with for usbip ?
> 
> As far as I can tell, it isn't. Running a storage device over usbip
> is a bit dangerous.

I don't follow that analysis.  The usbip interactions with the usb stack
all seem to be atomic, and never trigger a syscall, as far as I can
tell.  A port reset will flip a few bits and return.  A urb enqueue
queues and wakes a different thread, and returns.  The alternate thread
performs the sendmsg.

I'm not suggesting that running a storage device over usbip is
especially safe, but I don't see the limit on the design.

Cheers,

Jeremy
--
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: Configfs usb mass_storage device always reports 8 LUNs ?

[Krzysztof Opasiak]

On 07/02/2015 05:25 PM, Michal Nazarewicz wrote:

Sorry, I didn’t notice this reply before.


On 06/22/2015 04:24 PM, Michal Nazarewicz wrote:

common->nluns is set to FSG_MAX_LUNS in fsg_alloc_inst (which is called
prior to fsg_alloc) so this is not an issue.


On Mon, Jun 22 2015, Krzysztof Opasiak wrote:

True, but all legacy gadgets which use mass storage call
fsg_common_set_nluns() after reading module parameters and after
allocating function instance. As argument they pass number luns received
in module params.


Right.  As a quick fix I would propose doing:

--- >8 -
diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
b/drivers/usb/gadget/function/f_mass_storage.c
index e1beb14..15c3071 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -2786,7 +2786,7 @@ int fsg_common_set_nluns(struct fsg_common *common, int 
nluns)
 return -EINVAL;
 }

-   curlun = kcalloc(nluns, sizeof(*curlun), GFP_KERNEL);
+   curlun = kcalloc(FSG_MAX_LUNS, sizeof(*curlun), GFP_KERNEL);
 if (unlikely(!curlun))
 return -ENOMEM;

--- >8 -



Now it should be fine as a temporary solution.


Since I think we agreed that luns should just be turned into an array
with static length the above won’t be too far from that goal.



True, just started working on it.

--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
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: MUSB dual-role on AM335x behaving weirdly

[Bin Liu]

Hi,

On Thu, Jul 2, 2015 at 2:16 AM, Gregory CLEMENT
 wrote:
> Hi Felipe,
>
> On 27/05/2015 11:42, Alexandre Belloni wrote:
>> Hi,
>>
>> On 26/05/2015 at 09:51:18 -0500, Felipe Balbi wrote :
>>> On Thu, May 14, 2015 at 04:36:33PM -0500, Bin Liu wrote:
 Alexandre,

 On Thu, May 14, 2015 at 4:26 PM, Alexandre Belloni
  wrote:
> On 14/05/2015 at 16:16:12 -0500, Bin Liu wrote :
>> I think I found the root cause of the problem: board design issue - I
>> bet the custom board has too much cap on VBUS line. It should be <
>> 10uF.
>>
>
> We have a custom board that exhibits the issue but it only has a 100nF
> cap on VBUS.

 Have you measured the VBUS discharging? Is there any way to share your
 schematics?
>>>
>>> Alexandre, any further comments ?
>>>
>>
>> Yeah, I have just got more info.
>>
>> This is the relevant part of the schematic:
>> http://free-electrons.com/~alexandre/usb.png
>>
>> The total VBUS capacitance is 200nF and the USB0 pins are connected
>> directly to the AM3358 pins. U1 is actually not fitted.
>>
>> We didn't measure VBUS discharging but we observe the OTG pin sensing
>> stops when plugging an OTG cable without any device.
>
> Do you have any news about this topic?
>
>
> Is there something else that we can do to help solving this issue?

In the case of CONFIG_USB_MUSB_DUAL_ROLE=y and dr_mode=otg, how is the
gadget driver configured? It has to be a module not built-in.

Regards,
-Bin.

>
>
> Thanks,
>
> Gregory
>
>
> --
> Gregory Clement, Free Electrons
> Kernel, drivers, real-time and embedded Linux
> development, consulting, training and support.
> http://free-electrons.com
--
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: Configfs usb mass_storage device always reports 8 LUNs ?

[Michal Nazarewicz]

> On 07/02/2015 05:25 PM, Michal Nazarewicz wrote:
>> Since I think we agreed that luns should just be turned into an array
>> with static length the above won’t be too far from that goal.

On Thu, Jul 02 2015, Krzysztof Opasiak wrote:
> True, just started working on it.

I just got this to compile but haven’t tested it past that.  Feel free
to grab it or whatever:


>From 7fd36d82c7d190224961cbdd8b3ee365fe5f5d18 Mon Sep 17 00:00:00 2001
From: Michal Nazarewicz 
Date: Thu, 2 Jul 2015 19:14:59 +0200
Subject: [PATCH] usb: f_mass_storage: convert luns pointer into an array

Simplify the code by turning fsg_commen::luns into an array of pointers
with a static size rather than having it a pointer to dynamically
allocated array.  For legacy gadgets this will waste some memory, but
in configfs-based gadgets the array is allocated with maximum size
anyway.

Signed-off-by: Michal Nazarewicz 
---
 drivers/usb/gadget/function/f_mass_storage.c | 52 ++--
 drivers/usb/gadget/function/f_mass_storage.h |  2 --
 drivers/usb/gadget/legacy/acm_ms.c   |  6 ++--
 drivers/usb/gadget/legacy/mass_storage.c |  6 ++--
 drivers/usb/gadget/legacy/multi.c|  6 ++--
 5 files changed, 17 insertions(+), 55 deletions(-)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
b/drivers/usb/gadget/function/f_mass_storage.c
index 15c3071..e34c6b2 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -281,7 +281,7 @@ struct fsg_common {
 
unsigned intnluns;
unsigned intlun;
-   struct fsg_lun  **luns;
+   struct fsg_lun  *luns[FSG_MAX_LUNS];
struct fsg_lun  *curlun;
 
unsigned intbulk_out_maxpacket;
@@ -2751,11 +2751,11 @@ void fsg_common_remove_lun(struct fsg_lun *lun, bool 
sysfs)
 }
 EXPORT_SYMBOL_GPL(fsg_common_remove_lun);
 
-static void _fsg_common_remove_luns(struct fsg_common *common, int n)
+void fsg_common_remove_luns(struct fsg_common *common)
 {
int i;
 
-   for (i = 0; i < n; ++i)
+   for (i = 0; i < FSG_MAX_LUNS; ++i)
if (common->luns[i]) {
fsg_common_remove_lun(common->luns[i], common->sysfs);
common->luns[i] = NULL;
@@ -2763,37 +2763,16 @@ static void _fsg_common_remove_luns(struct fsg_common 
*common, int n)
 }
 EXPORT_SYMBOL_GPL(fsg_common_remove_luns);
 
-void fsg_common_remove_luns(struct fsg_common *common)
-{
-   _fsg_common_remove_luns(common, common->nluns);
-}
-
-void fsg_common_free_luns(struct fsg_common *common)
-{
-   fsg_common_remove_luns(common);
-   kfree(common->luns);
-   common->luns = NULL;
-}
-EXPORT_SYMBOL_GPL(fsg_common_free_luns);
-
 int fsg_common_set_nluns(struct fsg_common *common, int nluns)
 {
-   struct fsg_lun **curlun;
-
/* Find out how many LUNs there should be */
if (nluns < 1 || nluns > FSG_MAX_LUNS) {
pr_err("invalid number of LUNs: %u\n", nluns);
return -EINVAL;
}
 
-   curlun = kcalloc(FSG_MAX_LUNS, sizeof(*curlun), GFP_KERNEL);
-   if (unlikely(!curlun))
-   return -ENOMEM;
-
-   if (common->luns)
-   fsg_common_free_luns(common);
+   fsg_common_remove_luns(common);
 
-   common->luns = curlun;
common->nluns = nluns;
 
return 0;
@@ -2880,7 +2859,7 @@ int fsg_common_create_lun(struct fsg_common *common, 
struct fsg_lun_config *cfg,
char *pathbuf, *p;
int rc = -ENOMEM;
 
-   if (!common->nluns || !common->luns)
+   if (!common->nluns)
return -ENODEV;
 
if (common->luns[id])
@@ -2976,7 +2955,7 @@ int fsg_common_create_luns(struct fsg_common *common, 
struct fsg_config *cfg)
return 0;
 
 fail:
-   _fsg_common_remove_luns(common, i);
+   fsg_common_remove_luns(common);
return rc;
 }
 EXPORT_SYMBOL_GPL(fsg_common_create_luns);
@@ -3020,6 +2999,7 @@ EXPORT_SYMBOL_GPL(fsg_common_run_thread);
 static void fsg_common_release(struct kref *ref)
 {
struct fsg_common *common = container_of(ref, struct fsg_common, ref);
+   unsigned i;
 
/* If the thread isn't already dead, tell it to exit now */
if (common->state != FSG_STATE_TERMINATED) {
@@ -3027,22 +3007,14 @@ static void fsg_common_release(struct kref *ref)
wait_for_completion(&common->thread_notifier);
}
 
-   if (likely(common->luns)) {
-   struct fsg_lun **lun_it = common->luns;
-   unsigned i = common->nluns;
-
-   /* In error recovery common->nluns may be zero. */
-   for (; i; --i, ++lun_it) {
-   struct fsg_lun *lun = *lun_it;
-   if (!lun)
-   continue;
+   for (i = 0; i < FSG_MAX_LUNS; ++i) {
+   struct fsg_lun *lun = common->luns[i];
+   if (lun) {
 

Re: usb:gadget:hid:Add BOOT mode support

[Alan Stern]

On Wed, 27 May 2015, Golmer Palmer wrote:

> Hi Alan,
> 
> Thank you for the new patch!
> However, a comment:
> 
> * At page 74 of the specifications, 
> section F.3 Boot Keyboard Requirements,
> (http://www.usb.org/developers/hidpage/HID1_11.pdf)
> is defined that:
> "The Boot Keyboard shall, upon reset, return to the non- boot protocol 
> which is
> described in its Report descriptor. That is, the Report descriptor for a 
> Boot
> Keyboard does not necessarily match the boot protocol. The Report 
> descriptor
> for a Boot Keyboard is the non-boot protocol descriptor. "
> 
> This enforces that the default mode (protocol) is REPORT, not BOOT.
> So why initialize in the patch to "1" for devices without boot support
> and "0" for devices with boot support?
> 
> I suggest to always initialize to "1" (=REPORT mode).
> 
> Remember that the assumption is that the DESCRIPTOR of the BOOT protocol
> is IDENTICAL to the descriptor of the REPORT protocol, so they are the
> same. This is the key point!
> 
> Moreover, if a device don't supports BOOT mode, then the function
> Get_Protocol() needs to be unimplemented. See page 54, section
> 7.2.5 Get_Protocol Request. So, I suggest to maintain the old behaviour
> ("goto stall" for returning ERROR) in the Get_Protocol() in case of
> bInterfaceSubClass != USB_INTERFACE_SUBCLASS_BOOT. This can be in the
> same way as you done in the Set_Protocol function.

Golmer:

I sent you a revised patch incorporating this correction more 
than a month ago, and I haven't heard back.  Have you tested the newest 
patch?  Does it work correctly?

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: [Spice-devel] [RFC PATCH 1/1] Add a usbredir kernel module to remotely connect USB devices over IP.

[Oliver Neukum]

On Thu, 2015-07-02 at 10:57 -0500, Jeremy White wrote:
> On 07/02/2015 07:10 AM, Oliver Neukum wrote:
> > On Thu, 2015-07-02 at 13:35 +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 02-07-15 10:45, Oliver Neukum wrote:
> >>> On Wed, 2015-07-01 at 10:06 +0100, Daniel P. Berrange wrote:
> >>>
>  I don't really think it is sensible to be defining & implementing new
>  network services which can't support strong encryption and 
>  authentication.
>  Rather than passing the file descriptor to the kernel and having it do
>  the I/O directly, I think it would be better to dissassociate the kernel
>  from the network transport, and thus leave all sockets layer data I/O
>  to userspace daemons so they can layer in TLS or SASL or whatever else
>  is appropriate for the security need.
> >>>
> >>> Hi,
> >>>
> >>> this hits a fundamental limit. Block IO must be done entirely in kernel
> >>> space or the system will deadlock. The USB stack is part of the block
> >>> layer and the SCSI error handling. Thus if you involve user space you
> >>> cannot honor memory allocation with GFP_NOFS and you break all APIs
> >>> where we pass GFP_NOIO in the USB stack.
> >>>
> >>> Supposed you need to reset a storage device for error handling.
> >>> Your user space programm does a syscall, which allocates memory
> >>> and needs to launder pages. It proceeds to write to the storage device
> >>> you wish to reset.
> >>>
> >>> It is the same problem FUSE has with writable mmap. You cannot do
> >>> block devices in user space sanely.
> >>
> >> So how is this dealt with for usbip ?
> > 
> > As far as I can tell, it isn't. Running a storage device over usbip
> > is a bit dangerous.
> 
> I don't follow that analysis.  The usbip interactions with the usb stack
> all seem to be atomic, and never trigger a syscall, as far as I can
> tell.  A port reset will flip a few bits and return.  A urb enqueue
> queues and wakes a different thread, and returns.  The alternate thread
> performs the sendmsg.
> 
> I'm not suggesting that running a storage device over usbip is
> especially safe, but I don't see the limit on the design.

Are you referring to the current code or the proposed user space pipe?

Regards
Oliver



--
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 v3] usb: xhci: Add support for URB_ZERO_PACKET to bulk/sg transfers

[Reyad Attiyat]

This commit checks for the URB_ZERO_PACKET flag and creates an extra
zero-length td if the urb transfer length is a multiple of the endpoint's
max packet length.

Signed-off-by: Reyad Attiyat 
---
 drivers/usb/host/xhci-ring.c | 66 ++--
 drivers/usb/host/xhci.c  |  5 
 2 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 7d34cbf..31ea1b9 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3038,9 +3038,11 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
struct xhci_td *td;
struct scatterlist *sg;
int num_sgs;
-   int trb_buff_len, this_sg_len, running_total;
+   int trb_buff_len, this_sg_len, running_total, ret;
unsigned int total_packet_count;
+   bool zero_length_needed;
bool first_trb;
+   int last_trb_num;
u64 addr;
bool more_trbs_coming;
 
@@ -3056,13 +3058,27 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, 
gfp_t mem_flags,
total_packet_count = DIV_ROUND_UP(urb->transfer_buffer_length,
usb_endpoint_maxp(&urb->ep->desc));
 
-   trb_buff_len = prepare_transfer(xhci, xhci->devs[slot_id],
+   ret = prepare_transfer(xhci, xhci->devs[slot_id],
ep_index, urb->stream_id,
num_trbs, urb, 0, mem_flags);
-   if (trb_buff_len < 0)
-   return trb_buff_len;
+   if (ret < 0)
+   return ret;
 
urb_priv = urb->hcpriv;
+
+   /* 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 (ret < 0)
+   return ret;
+   }
+
td = urb_priv->td[0];
 
/*
@@ -3092,6 +3108,7 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
trb_buff_len = urb->transfer_buffer_length;
 
first_trb = true;
+   last_trb_num = zero_length_needed ? 2 : 1;
/* Queue the first TRB, even if it's zero-length */
do {
u32 field = 0;
@@ -3109,12 +3126,15 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, 
gfp_t mem_flags,
/* Chain all the TRBs together; clear the chain bit in the last
 * TRB to indicate it's the last TRB in the chain.
 */
-   if (num_trbs > 1) {
+   if (num_trbs > last_trb_num) {
field |= TRB_CHAIN;
-   } else {
-   /* FIXME - add check for ZERO_PACKET flag before this */
+   } else if (num_trbs == last_trb_num) {
td->last_trb = ep_ring->enqueue;
field |= TRB_IOC;
+   } else if (zero_length_needed && num_trbs == 1) {
+   trb_buff_len = 0;
+   urb_priv->td[1]->last_trb = ep_ring->enqueue;
+   field |= TRB_IOC;
}
 
/* Only set interrupt on short packet for IN endpoints */
@@ -3176,7 +3196,7 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
if (running_total + trb_buff_len > urb->transfer_buffer_length)
trb_buff_len =
urb->transfer_buffer_length - running_total;
-   } while (running_total < urb->transfer_buffer_length);
+   } while (num_trbs > 0);
 
check_trb_math(urb, num_trbs, running_total);
giveback_first_trb(xhci, slot_id, ep_index, urb->stream_id,
@@ -3194,7 +3214,9 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
int num_trbs;
struct xhci_generic_trb *start_trb;
bool first_trb;
+   int last_trb_num;
bool more_trbs_coming;
+   bool zero_length_needed;
int start_cycle;
u32 field, length_field;
 
@@ -3225,7 +3247,6 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
num_trbs++;
running_total += TRB_MAX_BUFF_SIZE;
}
-   /* FIXME: this doesn't deal with URB_ZERO_PACKET - need one more */
 
ret = prepare_transfer(xhci, xhci->devs[slot_id],
ep_index, urb->stream_id,
@@ -3234,6 +3255,20 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
return ret;
 
urb_priv = urb->hcpriv;
+
+   /* Deal with URB_ZERO_PACKET - need one more td/trb */
+   zero_length_needed = urb->transfer_flags & URB_ZERO_PACKET &&
+   urb_

[PATCH v2 0/5] Mass storage fixes and improvements

[Krzysztof Opasiak]

Hello,

This series fix a few bugs in mass storage function, adds a warning
message when binding function with not contiguous LUN ids and replace
dynamically allocated luns array with static one what simplifies the
code.

This series also fix GET_MAX_LUNS request to return max id of valid
lun, not some number of slots in luns array.

Best regards,

-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics 

Krzysztof Opasiak (5):
  usb: gadget: mass_storage: Free buffers if create lun fails
  usb: gadget: mass_storage: Place EXPORT_SYMBOL_GPL() after func
definition
  usb: gadget: storage-common: Set FSG_MAX_LUNS to 16
  usb: gadget: mass_storage: Use static array for luns
  usb: gadget: mass_storage: Warn if LUNs ids are not contiguous

 drivers/usb/gadget/function/f_mass_storage.c |  140 +++---
 drivers/usb/gadget/function/f_mass_storage.h |4 -
 drivers/usb/gadget/function/storage_common.h |2 +-
 drivers/usb/gadget/legacy/acm_ms.c   |6 --
 drivers/usb/gadget/legacy/mass_storage.c |6 --
 drivers/usb/gadget/legacy/multi.c|6 --
 6 files changed, 62 insertions(+), 102 deletions(-)

-- 
1.7.9.5

--
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 v2 4/5] usb: gadget: mass_storage: Use static array for luns

[Krzysztof Opasiak]

This patch replace dynamicly allocated luns array with static one.
This simplifies the code of mass storage function and modules.
It also fix issue with reporting wrong number of LUNs in GET_MAX_LUN
request.

Also change the nluns to max_luns which is better name for value
stored in that place as we no longer need to store size of luns
array.

Reported-by: David Fisher 
Signed-off-by: Krzysztof Opasiak 
---
 drivers/usb/gadget/function/f_mass_storage.c |  125 ++
 drivers/usb/gadget/function/f_mass_storage.h |4 -
 drivers/usb/gadget/legacy/acm_ms.c   |6 --
 drivers/usb/gadget/legacy/mass_storage.c |6 --
 drivers/usb/gadget/legacy/multi.c|6 --
 5 files changed, 48 insertions(+), 99 deletions(-)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
b/drivers/usb/gadget/function/f_mass_storage.c
index ad0f69b..befe251 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -279,9 +279,9 @@ struct fsg_common {
int cmnd_size;
u8  cmnd[MAX_COMMAND_SIZE];
 
-   unsigned intnluns;
+   int max_lun;
unsigned intlun;
-   struct fsg_lun  **luns;
+   struct fsg_lun  *luns[FSG_MAX_LUNS];
struct fsg_lun  *curlun;
 
unsigned intbulk_out_maxpacket;
@@ -533,7 +533,7 @@ static int fsg_setup(struct usb_function *f,
w_length != 1)
return -EDOM;
VDBG(fsg, "get max LUN\n");
-   *(u8 *)req->buf = fsg->common->nluns - 1;
+   *(u8 *)req->buf = fsg->common->max_lun;
 
/* Respond with data/status */
req->length = min((u16)1, w_length);
@@ -2131,7 +2131,7 @@ static int received_cbw(struct fsg_dev *fsg, struct 
fsg_buffhd *bh)
}
 
/* Is the CBW meaningful? */
-   if (cbw->Lun >= FSG_MAX_LUNS || cbw->Flags & ~US_BULK_FLAG_IN ||
+   if (cbw->Lun > common->max_lun || cbw->Flags & ~US_BULK_FLAG_IN ||
cbw->Length <= 0 || cbw->Length > MAX_COMMAND_SIZE) {
DBG(fsg, "non-meaningful CBW: lun = %u, flags = 0x%x, "
"cmdlen %u\n",
@@ -2159,7 +2159,7 @@ static int received_cbw(struct fsg_dev *fsg, struct 
fsg_buffhd *bh)
if (common->data_size == 0)
common->data_dir = DATA_DIR_NONE;
common->lun = cbw->Lun;
-   if (common->lun < common->nluns)
+   if (common->lun < ARRAY_SIZE(common->luns))
common->curlun = common->luns[common->lun];
else
common->curlun = NULL;
@@ -2307,7 +2307,7 @@ reset:
}
 
common->running = 1;
-   for (i = 0; i < common->nluns; ++i)
+   for (i = 0; i <= common->max_lun; ++i)
if (common->luns[i])
common->luns[i]->unit_attention_data =
SS_RESET_OCCURRED;
@@ -2409,7 +2409,7 @@ static void handle_exception(struct fsg_common *common)
if (old_state == FSG_STATE_ABORT_BULK_OUT)
common->state = FSG_STATE_STATUS_PHASE;
else {
-   for (i = 0; i < common->nluns; ++i) {
+   for (i = 0; i <= common->max_lun; ++i) {
curlun = common->luns[i];
if (!curlun)
continue;
@@ -2453,7 +2453,7 @@ static void handle_exception(struct fsg_common *common)
 * a waste of time.  Ditto for the INTERFACE_CHANGE and
 * CONFIG_CHANGE cases.
 */
-   /* for (i = 0; i < common->nluns; ++i) */
+   /* for (i = 0; i <= common->max_lun; ++i) */
/*  if (common->luns[i]) */
/*  common->luns[i]->unit_attention_data = */
/*  SS_RESET_OCCURRED;  */
@@ -2552,12 +2552,11 @@ static int fsg_main_thread(void *common_)
 
if (!common->ops || !common->ops->thread_exits
 || common->ops->thread_exits(common) < 0) {
-   struct fsg_lun **curlun_it = common->luns;
-   unsigned i = common->nluns;
+   int i = 0;
 
down_write(&common->filesem);
-   for (; i--; ++curlun_it) {
-   struct fsg_lun *curlun = *curlun_it;
+   for (; i <= common->max_lun; ++i) {
+   struct fsg_lun *curlun = common->luns[i];
if (!curlun || !fsg_lun_is_open(curlun))
continue;
 
@@ -2676,6 +2675,8 @@ static struct fsg_common *fsg_common_setup(struct 
fsg_common *common)
init_completion(&common->thread_notifier);
init_waitqueue_head(&common->fsg_wait);
common->state = FSG_STATE_TERMINATED;
+   memset(common->luns, 0, sizeof(common->lun

[PATCH v2 5/5] usb: gadget: mass_storage: Warn if LUNs ids are not contiguous

[Krzysztof Opasiak]

According to mass storage specification:

"Logical Unit Numbers on the device shall be numbered contiguously
 starting from LUN 0 to a maximum LUN of 15 (Fh)"

So let's at least print a warning message that LUNs ids should be
contiguous.

Signed-off-by: Krzysztof Opasiak 
---
 drivers/usb/gadget/function/f_mass_storage.c |6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
b/drivers/usb/gadget/function/f_mass_storage.c
index befe251..08a4fdb 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -3042,6 +3042,12 @@ static int fsg_bind(struct usb_configuration *c, struct 
usb_function *f)
return -EINVAL;
}
 
+   for (i = 0; i < ARRAY_SIZE(common->luns); ++i)
+   if (!common->luns[i])
+   break;
+   if (common->max_lun != i - 1)
+   pr_err("LUN ids should be contiguous.\n");
+
opts = fsg_opts_from_func_inst(f->fi);
if (!opts->no_configfs) {
ret = fsg_common_set_cdev(fsg->common, c->cdev,
-- 
1.7.9.5

--
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 v2 3/5] usb: gadget: storage-common: Set FSG_MAX_LUNS to 16

[Krzysztof Opasiak]

Mass storage spec allows up to 16 LUNs, so let's don't
add some more restrictive limits.

Signed-off-by: Krzysztof Opasiak 
Acked-by: Michal Nazarewicz 
---
 drivers/usb/gadget/function/f_mass_storage.c |2 +-
 drivers/usb/gadget/function/storage_common.h |2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
b/drivers/usb/gadget/function/f_mass_storage.c
index 48af0a6..ad0f69b 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -54,7 +54,7 @@
  * following fields:
  *
  * nluns   Number of LUNs function have (anywhere from 1
- * to FSG_MAX_LUNS which is 8).
+ * to FSG_MAX_LUNS).
  * lunsAn array of LUN configuration values.  This
  * should be filled for each LUN that
  * function will include (ie. for "nluns"
diff --git a/drivers/usb/gadget/function/storage_common.h 
b/drivers/usb/gadget/function/storage_common.h
index 70c8914..c3544e6 100644
--- a/drivers/usb/gadget/function/storage_common.h
+++ b/drivers/usb/gadget/function/storage_common.h
@@ -123,7 +123,7 @@ static inline bool fsg_lun_is_open(struct fsg_lun *curlun)
 #define FSG_BUFLEN ((u32)16384)
 
 /* Maximal number of LUNs supported in mass storage function */
-#define FSG_MAX_LUNS   8
+#define FSG_MAX_LUNS   16
 
 enum fsg_buffer_state {
BUF_STATE_EMPTY = 0,
-- 
1.7.9.5

--
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 v2 2/5] usb: gadget: mass_storage: Place EXPORT_SYMBOL_GPL() after func definition

[Krzysztof Opasiak]

EXPORT_SYMBOL_GPL() is usually placed after function definition
not before.

Signed-off-by: Krzysztof Opasiak 
Acked-by: Michal Nazarewicz 
---
 drivers/usb/gadget/function/f_mass_storage.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
b/drivers/usb/gadget/function/f_mass_storage.c
index 2e8f37e..48af0a6 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -2761,12 +2761,12 @@ static void _fsg_common_remove_luns(struct fsg_common 
*common, int n)
common->luns[i] = NULL;
}
 }
-EXPORT_SYMBOL_GPL(fsg_common_remove_luns);
 
 void fsg_common_remove_luns(struct fsg_common *common)
 {
_fsg_common_remove_luns(common, common->nluns);
 }
+EXPORT_SYMBOL_GPL(fsg_common_remove_luns);
 
 void fsg_common_free_luns(struct fsg_common *common)
 {
-- 
1.7.9.5

--
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 v2 1/5] usb: gadget: mass_storage: Free buffers if create lun fails

[Krzysztof Opasiak]

Creation of LUN 0 may fail (for example due to ENOMEM).
As fsg_common_set_num_buffers() does some memory allocation
we should free it before it becomes unavailable.

Signed-off-by: Krzysztof Opasiak 
Acked-by: Michal Nazarewicz 
---
 drivers/usb/gadget/function/f_mass_storage.c |5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
b/drivers/usb/gadget/function/f_mass_storage.c
index 3cc109f..2e8f37e 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -3526,6 +3526,9 @@ static struct usb_function_instance *fsg_alloc_inst(void)
config.removable = true;
rc = fsg_common_create_lun(opts->common, &config, 0, "lun.0",
(const char **)&opts->func_inst.group.cg_item.ci_name);
+   if (rc)
+   goto release_buffers;
+
opts->lun0.lun = opts->common->luns[0];
opts->lun0.lun_id = 0;
config_group_init_type_name(&opts->lun0.group, "lun.0", &fsg_lun_type);
@@ -3536,6 +3539,8 @@ static struct usb_function_instance *fsg_alloc_inst(void)
 
return &opts->func_inst;
 
+release_buffers:
+   fsg_common_free_buffers(opts->common);
 release_luns:
kfree(opts->common->luns);
 release_opts:
-- 
1.7.9.5

--
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 v3] usb: xhci: Add support for URB_ZERO_PACKET to bulk/sg transfers

[Reyad Attiyat]

This version of the patch changes last_trb varible name to
last_trb_num and fixes code style. I have also added a td to the urb
td array. This now gets prepared properl,y with prepare_transfer(),
and is handled correctly when transferred and completed. It only calls
the urb completion callback once as there is a check in finish_td() to
ensure that all td's have been received.

If you think I should change anything else please let me know.

Thank you,
Reyad Attiyat

On Thu, Jul 2, 2015 at 1:54 PM, Reyad Attiyat  wrote:
> This commit checks for the URB_ZERO_PACKET flag and creates an extra
> zero-length td if the urb transfer length is a multiple of the endpoint's
> max packet length.
>
> Signed-off-by: Reyad Attiyat 
> ---
>  drivers/usb/host/xhci-ring.c | 66 
> ++--
>  drivers/usb/host/xhci.c  |  5 
>  2 files changed, 57 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 7d34cbf..31ea1b9 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3038,9 +3038,11 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, 
> gfp_t mem_flags,
> struct xhci_td *td;
> struct scatterlist *sg;
> int num_sgs;
> -   int trb_buff_len, this_sg_len, running_total;
> +   int trb_buff_len, this_sg_len, running_total, ret;
> unsigned int total_packet_count;
> +   bool zero_length_needed;
> bool first_trb;
> +   int last_trb_num;
> u64 addr;
> bool more_trbs_coming;
>
> @@ -3056,13 +3058,27 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, 
> gfp_t mem_flags,
> total_packet_count = DIV_ROUND_UP(urb->transfer_buffer_length,
> usb_endpoint_maxp(&urb->ep->desc));
>
> -   trb_buff_len = prepare_transfer(xhci, xhci->devs[slot_id],
> +   ret = prepare_transfer(xhci, xhci->devs[slot_id],
> ep_index, urb->stream_id,
> num_trbs, urb, 0, mem_flags);
> -   if (trb_buff_len < 0)
> -   return trb_buff_len;
> +   if (ret < 0)
> +   return ret;
>
> urb_priv = urb->hcpriv;
> +
> +   /* 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 (ret < 0)
> +   return ret;
> +   }
> +
> td = urb_priv->td[0];
>
> /*
> @@ -3092,6 +3108,7 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, 
> gfp_t mem_flags,
> trb_buff_len = urb->transfer_buffer_length;
>
> first_trb = true;
> +   last_trb_num = zero_length_needed ? 2 : 1;
> /* Queue the first TRB, even if it's zero-length */
> do {
> u32 field = 0;
> @@ -3109,12 +3126,15 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, 
> gfp_t mem_flags,
> /* Chain all the TRBs together; clear the chain bit in the 
> last
>  * TRB to indicate it's the last TRB in the chain.
>  */
> -   if (num_trbs > 1) {
> +   if (num_trbs > last_trb_num) {
> field |= TRB_CHAIN;
> -   } else {
> -   /* FIXME - add check for ZERO_PACKET flag before this 
> */
> +   } else if (num_trbs == last_trb_num) {
> td->last_trb = ep_ring->enqueue;
> field |= TRB_IOC;
> +   } else if (zero_length_needed && num_trbs == 1) {
> +   trb_buff_len = 0;
> +   urb_priv->td[1]->last_trb = ep_ring->enqueue;
> +   field |= TRB_IOC;
> }
>
> /* Only set interrupt on short packet for IN endpoints */
> @@ -3176,7 +3196,7 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, 
> gfp_t mem_flags,
> if (running_total + trb_buff_len > 
> urb->transfer_buffer_length)
> trb_buff_len =
> urb->transfer_buffer_length - running_total;
> -   } while (running_total < urb->transfer_buffer_length);
> +   } while (num_trbs > 0);
>
> check_trb_math(urb, num_trbs, running_total);
> giveback_first_trb(xhci, slot_id, ep_index, urb->stream_id,
> @@ -3194,7 +3214,9 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
> mem_flags,
> int num_trbs;
> struct xhci_generic_trb *start_trb;
> bool first_trb;
> +   int last_trb_num;
> bool more_trbs_coming;
> +   bool zero_length_

Re: [Spice-devel] [RFC PATCH 1/1] Add a usbredir kernel module to remotely connect USB devices over IP.

[Jeremy White]

On 07/02/2015 01:46 PM, Oliver Neukum wrote:
> On Thu, 2015-07-02 at 10:57 -0500, Jeremy White wrote:
>> On 07/02/2015 07:10 AM, Oliver Neukum wrote:
>>> On Thu, 2015-07-02 at 13:35 +0200, Hans de Goede wrote:
 Hi,

 On 02-07-15 10:45, Oliver Neukum wrote:
> On Wed, 2015-07-01 at 10:06 +0100, Daniel P. Berrange wrote:
>
>> I don't really think it is sensible to be defining & implementing new
>> network services which can't support strong encryption and 
>> authentication.
>> Rather than passing the file descriptor to the kernel and having it do
>> the I/O directly, I think it would be better to dissassociate the kernel
>> from the network transport, and thus leave all sockets layer data I/O
>> to userspace daemons so they can layer in TLS or SASL or whatever else
>> is appropriate for the security need.
>
> Hi,
>
> this hits a fundamental limit. Block IO must be done entirely in kernel
> space or the system will deadlock. The USB stack is part of the block
> layer and the SCSI error handling. Thus if you involve user space you
> cannot honor memory allocation with GFP_NOFS and you break all APIs
> where we pass GFP_NOIO in the USB stack.
>
> Supposed you need to reset a storage device for error handling.
> Your user space programm does a syscall, which allocates memory
> and needs to launder pages. It proceeds to write to the storage device
> you wish to reset.
>
> It is the same problem FUSE has with writable mmap. You cannot do
> block devices in user space sanely.

 So how is this dealt with for usbip ?
>>>
>>> As far as I can tell, it isn't. Running a storage device over usbip
>>> is a bit dangerous.
>>
>> I don't follow that analysis.  The usbip interactions with the usb stack
>> all seem to be atomic, and never trigger a syscall, as far as I can
>> tell.  A port reset will flip a few bits and return.  A urb enqueue
>> queues and wakes a different thread, and returns.  The alternate thread
>> performs the sendmsg.
>>
>> I'm not suggesting that running a storage device over usbip is
>> especially safe, but I don't see the limit on the design.
> 
> Are you referring to the current code or the proposed user space pipe?

I'm referring to current usbip code.  But the proposed driver would have
the same behavior.

To be clear, I think the only tangible new proposal is the one Hans put
forth, which would modify the driver I originally posted to use a
netlink socket instead of a passing a file descriptor in via sysfs.
That would allow the user space application responsible for initiating
the request to provide TLS as desired.  It comes with the expense of an
extra memcpy, but I suspect Hans is right in saying the network
latencies make that an irrelevant cost.

Cheers,

Jeremy
--
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: Configfs usb mass_storage device always reports 8 LUNs ?

[Krzysztof Opasiak]

On 07/02/2015 07:16 PM, Michal Nazarewicz wrote:

On 07/02/2015 05:25 PM, Michal Nazarewicz wrote:

Since I think we agreed that luns should just be turned into an array
with static length the above won’t be too far from that goal.


On Thu, Jul 02 2015, Krzysztof Opasiak wrote:

True, just started working on it.


I just got this to compile but haven’t tested it past that.  Feel free
to grab it or whatever:



Ohh, I was quite busy with testing (and debugging) my patches and didn't 
notice this message, sorry...


I have run through this patch and didn't find any serious issues.
But in my opinion instead of editing set_nluns() we could simply drop it 
and simplify legacy gadgets code as we do almost nothing in this function.


--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
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: Configfs usb mass_storage device always reports 8 LUNs ?

[Felipe Balbi]

On Thu, Jul 02, 2015 at 09:12:30PM +0200, Krzysztof Opasiak wrote:
> 
> 
> On 07/02/2015 07:16 PM, Michal Nazarewicz wrote:
> >>On 07/02/2015 05:25 PM, Michal Nazarewicz wrote:
> >>>Since I think we agreed that luns should just be turned into an array
> >>>with static length the above won’t be too far from that goal.
> >
> >On Thu, Jul 02 2015, Krzysztof Opasiak wrote:
> >>True, just started working on it.
> >
> >I just got this to compile but haven’t tested it past that.  Feel free
> >to grab it or whatever:
> >
> 
> Ohh, I was quite busy with testing (and debugging) my patches and didn't
> notice this message, sorry...
> 
> I have run through this patch and didn't find any serious issues.
> But in my opinion instead of editing set_nluns() we could simply drop it and
> simplify legacy gadgets code as we do almost nothing in this function.

for the -rc cycle we need a minimal fix, though. Simplification,
refactoring and whatever else, needs to be merged during merge window.

-- 
balbi


signature.asc
Description: Digital signature

Re: Configfs usb mass_storage device always reports 8 LUNs ?

[Krzysztof Opasiak]

On 07/02/2015 09:14 PM, Felipe Balbi wrote:

On Thu, Jul 02, 2015 at 09:12:30PM +0200, Krzysztof Opasiak wrote:



On 07/02/2015 07:16 PM, Michal Nazarewicz wrote:

On 07/02/2015 05:25 PM, Michal Nazarewicz wrote:

Since I think we agreed that luns should just be turned into an array
with static length the above won’t be too far from that goal.


On Thu, Jul 02 2015, Krzysztof Opasiak wrote:

True, just started working on it.


I just got this to compile but haven’t tested it past that.  Feel free
to grab it or whatever:



Ohh, I was quite busy with testing (and debugging) my patches and didn't
notice this message, sorry...

I have run through this patch and didn't find any serious issues.
But in my opinion instead of editing set_nluns() we could simply drop it and
simplify legacy gadgets code as we do almost nothing in this function.


for the -rc cycle we need a minimal fix, though. Simplification,
refactoring and whatever else, needs to be merged during merge window.



Ok, sure. So the easiest way to fix problems in previously applied patch 
would be as Michal suggested here: [1]. Putting 1 and 2 together should 
be fine as a minimal fix for GET_MAX_LUNS issue.


1 - http://marc.info/?l=linux-usb&m=143585072403772&w=2
2 - http://marc.info/?l=linux-usb&m=143584114100542&w=2

Best regards,

--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
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 1/5] usb: gadget: mass_storage: Free buffers if create lun fails

[Krzysztof Opasiak]

Felippe,

Could you please take this patch as a fix now and leave the rest of this 
series to merge window or should I resend it separately?


On 07/02/2015 08:56 PM, Krzysztof Opasiak wrote:

Creation of LUN 0 may fail (for example due to ENOMEM).
As fsg_common_set_num_buffers() does some memory allocation
we should free it before it becomes unavailable.

Signed-off-by: Krzysztof Opasiak 
Acked-by: Michal Nazarewicz 
---
  drivers/usb/gadget/function/f_mass_storage.c |5 +
  1 file changed, 5 insertions(+)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
b/drivers/usb/gadget/function/f_mass_storage.c
index 3cc109f..2e8f37e 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -3526,6 +3526,9 @@ static struct usb_function_instance *fsg_alloc_inst(void)
config.removable = true;
rc = fsg_common_create_lun(opts->common, &config, 0, "lun.0",
(const char **)&opts->func_inst.group.cg_item.ci_name);
+   if (rc)
+   goto release_buffers;
+
opts->lun0.lun = opts->common->luns[0];
opts->lun0.lun_id = 0;
config_group_init_type_name(&opts->lun0.group, "lun.0", &fsg_lun_type);
@@ -3536,6 +3539,8 @@ static struct usb_function_instance *fsg_alloc_inst(void)

return &opts->func_inst;

+release_buffers:
+   fsg_common_free_buffers(opts->common);
  release_luns:
kfree(opts->common->luns);
  release_opts:



--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
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 5/5] usb: gadget: mass_storage: Warn if LUNs ids are not contiguous

[Michal Nazarewicz]

On Thu, Jul 02 2015, Krzysztof Opasiak wrote:
> According to mass storage specification:
>
> "Logical Unit Numbers on the device shall be numbered contiguously
>  starting from LUN 0 to a maximum LUN of 15 (Fh)"
>
> So let's at least print a warning message that LUNs ids should be
> contiguous.
>
> Signed-off-by: Krzysztof Opasiak 

Acked-by: Michal Nazarewicz 

> ---
>  drivers/usb/gadget/function/f_mass_storage.c |6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
> b/drivers/usb/gadget/function/f_mass_storage.c
> index befe251..08a4fdb 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -3042,6 +3042,12 @@ static int fsg_bind(struct usb_configuration *c, 
> struct usb_function *f)
>   return -EINVAL;
>   }
>  
> + for (i = 0; i < ARRAY_SIZE(common->luns); ++i)
> + if (!common->luns[i])
> + break;
> + if (common->max_lun != i - 1)
> + pr_err("LUN ids should be contiguous.\n");
> +
>   opts = fsg_opts_from_func_inst(f->fi);
>   if (!opts->no_configfs) {
>   ret = fsg_common_set_cdev(fsg->common, c->cdev,
> -- 
> 1.7.9.5
>

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--
--
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 4/5] usb: gadget: mass_storage: Use static array for luns

[Michal Nazarewicz]

On Thu, Jul 02 2015, Krzysztof Opasiak wrote:
> This patch replace dynamicly allocated luns array with static one.
> This simplifies the code of mass storage function and modules.
> It also fix issue with reporting wrong number of LUNs in GET_MAX_LUN
> request.
>
> Also change the nluns to max_luns which is better name for value
> stored in that place as we no longer need to store size of luns
> array.
>
> Reported-by: David Fisher 
> Signed-off-by: Krzysztof Opasiak 
> ---
>  drivers/usb/gadget/function/f_mass_storage.c |  125 
> ++
>  drivers/usb/gadget/function/f_mass_storage.h |4 -
>  drivers/usb/gadget/legacy/acm_ms.c   |6 --
>  drivers/usb/gadget/legacy/mass_storage.c |6 --
>  drivers/usb/gadget/legacy/multi.c|6 --
>  5 files changed, 48 insertions(+), 99 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
> b/drivers/usb/gadget/function/f_mass_storage.c
> index ad0f69b..befe251 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -279,9 +279,9 @@ struct fsg_common {
>   int cmnd_size;
>   u8  cmnd[MAX_COMMAND_SIZE];
>  
> - unsigned intnluns;
> + int max_lun;

To be honest, I don’t like this change.  It is more idiomatic to store
number of elements in an array rather than index of the last element.
Sooner or later someone will do for (i = 0; i < common->max_lun; ++i)
out of habit.

>   unsigned intlun;
> - struct fsg_lun  **luns;
> + struct fsg_lun  *luns[FSG_MAX_LUNS];
>   struct fsg_lun  *curlun;
>  
>   unsigned intbulk_out_maxpacket;

> @@ -2760,48 +2767,16 @@ static void _fsg_common_remove_luns(struct fsg_common 
> *common, int n)
>   fsg_common_remove_lun(common->luns[i], common->sysfs);
>   common->luns[i] = NULL;
>   }
> +
> + _fsg_common_reduce_max_lun(common);
>  }
>  
>  void fsg_common_remove_luns(struct fsg_common *common)
>  {
> - _fsg_common_remove_luns(common, common->nluns);
> + _fsg_common_remove_luns(common, common->max_lun);

Shouldn’t this be:

+   _fsg_common_remove_luns(common, common->max_lun + 1);

>  }
>  EXPORT_SYMBOL_GPL(fsg_common_remove_luns);
>  

> @@ -2954,7 +2931,7 @@ error_lun:
>   if (common->sysfs)
>   device_unregister(&lun->dev);
>   fsg_lun_close(lun);
> - common->luns[id] = NULL;

You need to keep this line.

> + _fsg_common_reduce_max_lun(common);
>  error_sysfs:
>   kfree(lun);
>   return rc;

> @@ -3305,11 +3282,9 @@ static struct config_group *fsg_lun_make(struct 
> config_group *group,
>   return ERR_PTR(ret);
>  
>   fsg_opts = to_fsg_opts(&group->cg_item);
> - if (num >= FSG_MAX_LUNS)
> - return ERR_PTR(-ERANGE);

Going through all the memory allocation and mutex locking just to find
out that num is to big seems wasteful.  I’d keep this check here.

>  
>   mutex_lock(&fsg_opts->lock);
> - if (fsg_opts->refcnt || fsg_opts->common->luns[num]) {
> + if (fsg_opts->refcnt) {
>   ret = -EBUSY;
>   goto out;
>   }

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--
--
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 4/5] usb: gadget: mass_storage: Use static array for luns

[Krzysztof Opasiak]

On 07/02/2015 09:29 PM, Michal Nazarewicz wrote:

On Thu, Jul 02 2015, Krzysztof Opasiak wrote:

This patch replace dynamicly allocated luns array with static one.
This simplifies the code of mass storage function and modules.
It also fix issue with reporting wrong number of LUNs in GET_MAX_LUN
request.

Also change the nluns to max_luns which is better name for value
stored in that place as we no longer need to store size of luns
array.

Reported-by: David Fisher 
Signed-off-by: Krzysztof Opasiak 
---
  drivers/usb/gadget/function/f_mass_storage.c |  125 ++
  drivers/usb/gadget/function/f_mass_storage.h |4 -
  drivers/usb/gadget/legacy/acm_ms.c   |6 --
  drivers/usb/gadget/legacy/mass_storage.c |6 --
  drivers/usb/gadget/legacy/multi.c|6 --
  5 files changed, 48 insertions(+), 99 deletions(-)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
b/drivers/usb/gadget/function/f_mass_storage.c
index ad0f69b..befe251 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -279,9 +279,9 @@ struct fsg_common {
int cmnd_size;
u8  cmnd[MAX_COMMAND_SIZE];

-   unsigned intnluns;
+   int max_lun;


To be honest, I don’t like this change.  It is more idiomatic to store
number of elements in an array rather than index of the last element.
Sooner or later someone will do for (i = 0; i < common->max_lun; ++i)
out of habit.



The reason why I did this was allowing not contiguous LUNs ids. As you 
suggested in earlier discussion [1] we should allow user space shoot 
themselves in the foot and tis is one of the consequence. What we should 
really return in GET_MAX_LUN is last valid LUN id in our array, not 
number of LUNs. Windows is paying attention to this value and for 
examnple for luns: 0 5 6 it makes a different if you reply with nluns -1 
or max_lun. In first option windows will see only one LUN while in 
second all three of them.


As we increment max_lun only when adding a LUN currently it is not 
possible to get out of bounds.



unsigned intlun;
-   struct fsg_lun  **luns;
+   struct fsg_lun  *luns[FSG_MAX_LUNS];
struct fsg_lun  *curlun;

unsigned intbulk_out_maxpacket;



@@ -2760,48 +2767,16 @@ static void _fsg_common_remove_luns(struct fsg_common 
*common, int n)
fsg_common_remove_lun(common->luns[i], common->sysfs);
common->luns[i] = NULL;
}
+
+   _fsg_common_reduce_max_lun(common);
  }

  void fsg_common_remove_luns(struct fsg_common *common)
  {
-   _fsg_common_remove_luns(common, common->nluns);
+   _fsg_common_remove_luns(common, common->max_lun);


Shouldn’t this be:

+   _fsg_common_remove_luns(common, common->max_lun + 1);


agree, should be fixed.




  }
  EXPORT_SYMBOL_GPL(fsg_common_remove_luns);




@@ -2954,7 +2931,7 @@ error_lun:
if (common->sysfs)
device_unregister(&lun->dev);
fsg_lun_close(lun);
-   common->luns[id] = NULL;


You need to keep this line.


This one is also true. Sorry, my mistake...




+   _fsg_common_reduce_max_lun(common);
  error_sysfs:
kfree(lun);
return rc;



@@ -3305,11 +3282,9 @@ static struct config_group *fsg_lun_make(struct 
config_group *group,
return ERR_PTR(ret);

fsg_opts = to_fsg_opts(&group->cg_item);
-   if (num >= FSG_MAX_LUNS)
-   return ERR_PTR(-ERANGE);


Going through all the memory allocation and mutex locking just to find
out that num is to big seems wasteful.  I’d keep this check here.


Ok I will leave it as it was before
.




mutex_lock(&fsg_opts->lock);
-   if (fsg_opts->refcnt || fsg_opts->common->luns[num]) {
+   if (fsg_opts->refcnt) {
ret = -EBUSY;
goto out;
}




1 - http://marc.info/?l=linux-usb&m=143498348620786&w=2
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
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: [Spice-devel] [RFC PATCH 1/1] Add a usbredir kernel module to remotely connect USB devices over IP.

[Alan Stern]

On Thu, 2 Jul 2015, Jeremy White wrote:

> >> I don't follow that analysis.  The usbip interactions with the usb stack
> >> all seem to be atomic, and never trigger a syscall, as far as I can
> >> tell.  A port reset will flip a few bits and return.  A urb enqueue
> >> queues and wakes a different thread, and returns.  The alternate thread
> >> performs the sendmsg.
> >>
> >> I'm not suggesting that running a storage device over usbip is
> >> especially safe, but I don't see the limit on the design.
> > 
> > Are you referring to the current code or the proposed user space pipe?
> 
> I'm referring to current usbip code.  But the proposed driver would have
> the same behavior.
> 
> To be clear, I think the only tangible new proposal is the one Hans put
> forth, which would modify the driver I originally posted to use a
> netlink socket instead of a passing a file descriptor in via sysfs.
> That would allow the user space application responsible for initiating
> the request to provide TLS as desired.  It comes with the expense of an
> extra memcpy, but I suspect Hans is right in saying the network
> latencies make that an irrelevant cost.

Oliver is talking about the danger of having part of the communication 
path for a block device run through userspace.

Imagine a situation where the client uses a USB storage device provided
by the server as a swap device.  And suppose a userspace daemon on the
client has to process USB packets as they pass between the client and
the server.  If the daemon is idle for some time, parts of its address
space may get stored in the swap area on the server and paged out.

Now consider what happens when those parts of memory need to be paged
back in.  The client submits a request to read from the swap area.  
The request is transformed into USB packets and sent through the
userspace daemon for transmission to the server.  But the daemon can't
process the packets because it is waiting for its missing parts to be
paged back!  Result: deadlock.

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: [Spice-devel] [RFC PATCH 1/1] Add a usbredir kernel module to remotely connect USB devices over IP.

[Jeremy White]

On 07/02/2015 02:59 PM, Alan Stern wrote:
> On Thu, 2 Jul 2015, Jeremy White wrote:
> 
 I don't follow that analysis.  The usbip interactions with the usb stack
 all seem to be atomic, and never trigger a syscall, as far as I can
 tell.  A port reset will flip a few bits and return.  A urb enqueue
 queues and wakes a different thread, and returns.  The alternate thread
 performs the sendmsg.

 I'm not suggesting that running a storage device over usbip is
 especially safe, but I don't see the limit on the design.
>>>
>>> Are you referring to the current code or the proposed user space pipe?
>>
>> I'm referring to current usbip code.  But the proposed driver would have
>> the same behavior.
>>
>> To be clear, I think the only tangible new proposal is the one Hans put
>> forth, which would modify the driver I originally posted to use a
>> netlink socket instead of a passing a file descriptor in via sysfs.
>> That would allow the user space application responsible for initiating
>> the request to provide TLS as desired.  It comes with the expense of an
>> extra memcpy, but I suspect Hans is right in saying the network
>> latencies make that an irrelevant cost.
> 
> Oliver is talking about the danger of having part of the communication 
> path for a block device run through userspace.
> 
> Imagine a situation where the client uses a USB storage device provided
> by the server as a swap device.  And suppose a userspace daemon on the
> client has to process USB packets as they pass between the client and
> the server.  If the daemon is idle for some time, parts of its address
> space may get stored in the swap area on the server and paged out.
> 
> Now consider what happens when those parts of memory need to be paged
> back in.  The client submits a request to read from the swap area.  
> The request is transformed into USB packets and sent through the
> userspace daemon for transmission to the server.  But the daemon can't
> process the packets because it is waiting for its missing parts to be
> paged back!  Result: deadlock.

Right.  I followed that.  Oliver also asserted that he believed that the
current usbip implementation has this flaw; I do not follow that.  The
concept is that the usbip device driver virtualizes the device behavior;
isolating the running kernel from the vagaries of the network transport.
 All proposed usbredir implementations, even if they move the network
transport to user space, would retain that behavior.

Cheers,

Jeremy
--
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: [Spice-devel] [RFC PATCH 1/1] Add a usbredir kernel module to remotely connect USB devices over IP.

[Alan Stern]

On Thu, 2 Jul 2015, Jeremy White wrote:

> > Oliver is talking about the danger of having part of the communication 
> > path for a block device run through userspace.
> > 
> > Imagine a situation where the client uses a USB storage device provided
> > by the server as a swap device.  And suppose a userspace daemon on the
> > client has to process USB packets as they pass between the client and
> > the server.  If the daemon is idle for some time, parts of its address
> > space may get stored in the swap area on the server and paged out.
> > 
> > Now consider what happens when those parts of memory need to be paged
> > back in.  The client submits a request to read from the swap area.  
> > The request is transformed into USB packets and sent through the
> > userspace daemon for transmission to the server.  But the daemon can't
> > process the packets because it is waiting for its missing parts to be
> > paged back!  Result: deadlock.
> 
> Right.  I followed that.  Oliver also asserted that he believed that the
> current usbip implementation has this flaw; I do not follow that.  The
> concept is that the usbip device driver virtualizes the device behavior;
> isolating the running kernel from the vagaries of the network transport.
>  All proposed usbredir implementations, even if they move the network
> transport to user space, would retain that behavior.

The point is that a device driver like usbip _cannot_ isolate the 
running kernel from the vagaries of the network transport if part of 
that transport occurs in userspace.

If any part of the transport passes through userspace, you can end up 
in a situation like what I outlined above, where a message can't be 
transported until after its reply has been received.  There's no way 
for a device driver to prevent a deadlock when this occurs, no matter 
what it virtualizes.

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 v2] usb: move assignment out of if condition

[Kris Borer]

Fix four occurrences of checkpatch.pl error:

ERROR: do not use assignment in if condition

Signed-off-by: Kris Borer 
---
 drivers/usb/core/hcd.c | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index be5b207..3204a4d 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2683,12 +2683,14 @@ int usb_add_hcd(struct usb_hcd *hcd,
 * bottom up so that hcds can customize the root hubs before hub_wq
 * starts talking to them.  (Note, bus id is assigned early too.)
 */
-   if ((retval = hcd_buffer_create(hcd)) != 0) {
+   retval = hcd_buffer_create(hcd);
+   if (retval != 0) {
dev_dbg(hcd->self.controller, "pool alloc failed\n");
goto err_create_buf;
}
 
-   if ((retval = usb_register_bus(&hcd->self)) < 0)
+   retval = usb_register_bus(&hcd->self);
+   if (retval < 0)
goto err_register_bus;
 
rhdev = usb_alloc_dev(NULL, &hcd->self, 0);
@@ -2734,9 +2736,13 @@ int usb_add_hcd(struct usb_hcd *hcd,
/* "reset" is misnamed; its role is now one-time init. the controller
 * should already have been reset (and boot firmware kicked off etc).
 */
-   if (hcd->driver->reset && (retval = hcd->driver->reset(hcd)) < 0) {
-   dev_err(hcd->self.controller, "can't setup: %d\n", retval);
-   goto err_hcd_driver_setup;
+   if (hcd->driver->reset) {
+   retval = hcd->driver->reset(hcd);
+   if (retval < 0) {
+   dev_err(hcd->self.controller, "can't setup: %d\n",
+   retval);
+   goto err_hcd_driver_setup;
+   }
}
hcd->rh_pollable = 1;
 
@@ -2766,7 +2772,8 @@ int usb_add_hcd(struct usb_hcd *hcd,
}
 
/* starting here, usbcore will pay attention to this root hub */
-   if ((retval = register_root_hub(hcd)) != 0)
+   retval = register_root_hub(hcd);
+   if (retval != 0)
goto err_register_root_hub;
 
retval = sysfs_create_group(&rhdev->dev.kobj, &usb_bus_attr_group);
-- 
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

[PATCH] usb: dwc3: core: avoid NULL pointer dereference

[Felipe Balbi]

commit 3e10a2ce98d1 ("usb: dwc3: add hsphy_interface
property") introduced a possible NULL pointer
dereference because dwc->hsphy_interface can be
NULL.

In order to fix it, all we have to do is guard
strncmp() against a NULL argument.

Fixes: 3e10a2ce98d1 ("usb: dwc3: add hsphy_interface property")
Tested-by: Murali Karicheri 
Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/core.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 5c110d8e293b..ff5773c66b84 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -446,10 +446,12 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
/* Select the HS PHY interface */
switch (DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3)) {
case DWC3_GHWPARAMS3_HSPHY_IFC_UTMI_ULPI:
-   if (!strncmp(dwc->hsphy_interface, "utmi", 4)) {
+   if (dwc->hsphy_interface &&
+   !strncmp(dwc->hsphy_interface, "utmi", 4)) {
reg &= ~DWC3_GUSB2PHYCFG_ULPI_UTMI;
break;
-   } else if (!strncmp(dwc->hsphy_interface, "ulpi", 4)) {
+   } else if (dwc->hsphy_interface &&
+   !strncmp(dwc->hsphy_interface, "ulpi", 4)) {
reg |= DWC3_GUSB2PHYCFG_ULPI_UTMI;
dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
} else {
-- 
2.4.4

--
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] arm: koelsch: make USB0 perform Host/Function switching

[Simon Horman]

Hi Phil,

when you re-spin this patch could you change the prefix to the following?

ARM: shmobile: koelsch:

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

musb gadget HUB traffic sniffing/forwarding

[Y. Zhang]

Hi all,

I'm new to linux usb and would like some guidance on achieving some of
the following goals.
I have a beaglebone black (BBB - currently running kernel v3.12) with
OTG in peripheral mode on usb bus#1 and a host port to connect devices
on usb bus#2.

1. usb bus#1: connect OTG on BBB as a musb gadget (via gadgetfs) to
another PC host (e.g. linux, windows, etc); usb bus#2: connect a hub
to BBB host
+ using libusb to control the hub
+ relay all traffics from host->musb gadget->libusb->hub->(applicable
multiple devices hot-plugged to the hub), and vice versa

So far I'm able to see traffics between the PC host and the hub before
plugging any device into the hub. That is, usb traffics for endpoints
of the hub itself. Once I plug in a device, after certain actions on
the hub port, the host will send requests (get_descriptor/set_address)
to address 0, which are not forwarded to the gadhetfs user space. I'd
like to know:
* Are traffics for address 0 visible to musb gadget? What code in
usb/musb and/or usb/gadget can be used/revised to retrieve/handle
them?
* If it is feasible to accomplish the above, then the PC host would
enumerate devices plugged into the hub. What code usage/change would
be necessary to further retrieve messages addressed to endpoints of
individual devices and relay them to the HUB (so the HUB can broadcast
to individual devices), and also forward messages from devices
received via hub (with libusb) back to host via musb gadget?

2. usb bus#1: connect the BBB OTG to a hub attached to the PC host
   + using musb gadget if needed: host-hub-OTG-(gadget)
   + when the host sends any messages to the hub, it would then
broadcast (at least for usb2.0) to all ports on the hub.
   + sniff broadcast traffic on the OTG usb port (on bus#1)

I don't necessarily need the OTG gadget to respond anything to the hub
or PC host, but hope to sniff all usb messages broadcast by the hub,
which means I will be able to see messages sent by the PC host to
other devices plugged into the hub. What code in usb/musb and/or
usb/gadget can be used/revised to sniff such traffics? I've tried
usbmon but it only monitors devices controlled by hcd (on bus#2) not
the OTG in peripheral mode (on bus#1)

Thanks a lot in advance,
Gordon
--
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: About zero-length packet design for EHCI

[Peter Chen]

On Thu, Jul 02, 2015 at 10:46:36AM -0400, Alan Stern wrote:
> On Thu, 2 Jul 2015, Peter Chen wrote:
> 
> > > If a control-IN transfer has a data stage that is shorter than wLength
> > > and is a multiple of the ep0 maxpacket value, then the peripheral must
> > > send a zero-length packet to indicate the end of the data stage.  
> > > Thus, the UDC driver must prepare a zero-length transaction (DTD).
> > > 
> > 
> > If "one" is "multiple"?
> > The wLength at setup packet is 64 bytes, Maximum Packet Length at 
> > dQH is 64 bytes, and the Total Bytes at dTD is 64 bytes too, does device
> > must prepare a zero-length packet?
> 
> No.  Here is the exact rule for when a device must prepare a 0-length 
> packet for a control-IN data stage.
> 
>   Let wlength = le16_to_cpu(setup->wLength) and
>   let act_length = req->length be the actual data length.
>   Then an extra 0-length packet is needed if:
> 
>   act_length > 0 && act_length < wlength &&
>   (act_length % maxpacket) == 0.
> 
> (Note that if act_length == 0 then req already gives rise to a 0-length 
> packet; no additional packets are needed.)
> 
> Here's another way to describe the same rule:
> 
>  1. Every packet before the final packet must have size
>   equal to maxpacket.
> 
>  2. If act_length < wlength then the final packet must have
>   size smaller than maxpacket.
> 
>  3. If act_length == wlength and act_length > 0 then the final
>   packet must have size > 0.
> 
> You can check that this is equivalent to the rule above.
> 
> In the example you gave, act_length == wlength so a 0-length packet
> isn't needed.

Thanks, I think I understand it now.

Maybe only the case A like below commit needs zero-length packet.

usb: chipidea: udc: Disable auto ZLP generation on ep0
953c66469735aed8d2ada639a72b150f01dae605

> > > The host hardware will recognize when this happens, because the HCD
> > > will set the appropriate bits in the data-stage qTD.  For example, with
> > > EHCI the HCD will set the Alternate Next qTD Pointer.  Or with UHCI, 
> > > the HCD will set the SPD (Short Packet Detect) bit.
> > > 
> > 
> > I see it in the code, but it is a null pointer (EHCI_LIST_END), does it
> > mean one qTD (with valid Next qTD Pointer) can handle one 64 byte packet
> > with or without zero-length packet well both?
> 
> That is a tricky question, and the full answer involves the complicated
> details of how EHCI works.
> 
> Here is a simplified answer.  If a transfer requires more than one qTD, 
> then all the qTDs but the last one will have their Alternate Next qTD 
> Pointers set to a special value.  The last qTD doesn't need this.
> 
> Since ehci-hcd never uses more than one qTD for a control transfer data
> stage, the issue doesn't comes up.  But if you look at how ehci-hcd
> handles large bulk transfers, you will see what I mean.

The case A I list above, it should uses more than one qTD, right?

> 
> > Any reasons why iPhone can't handle zero-length packet well?
> 
> When the iPhone receives the 64-byte packet, it thinks the data stage 
> is finished.  It moves on to the status stage, where it sends a 
> 0-length packet to the gadget and waits for the acknowledgment.  If the 
> gadget is still trying to send a 0-length packet, it won't acknowledge 
> the 0-length packet sent by the iPhone.  The status stage will time out 
> and the control transfer will fail.
> 
> Alan Stern
> 

-- 

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

[PATCH 1/1] usb: gadget: composite: only control read may need zero length packet

[Peter Chen]

According to the USB 2.0 Spec CH5.5.3 and CH8.5.3.2, only control
read (IN) needs zero length packet.

Cc: Alan Stern 
Signed-off-by: Peter Chen 
---
 drivers/usb/gadget/composite.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 1c0c3da..c3898b5 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1744,7 +1744,8 @@ unknown:
}
req->length = value;
req->context = cdev;
-   req->zero = value < w_length;
+   if (ctrl->bRequestType == USB_DIR_IN)
+   req->zero = value < w_length;
value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
if (value < 0) {
DBG(cdev, "ep_queue --> %d\n", value);
@@ -1820,7 +1821,8 @@ try_fun_setup:
if (value >= 0 && value != USB_GADGET_DELAYED_STATUS) {
req->length = value;
req->context = cdev;
-   req->zero = value < w_length;
+   if (ctrl->bRequestType == USB_DIR_IN)
+   req->zero = value < w_length;
value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
if (value < 0) {
DBG(cdev, "ep_queue --> %d\n", value);
-- 
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: musb gadget HUB traffic sniffing/forwarding

[Felipe Balbi]

Hi,

On Thu, Jul 02, 2015 at 06:02:50PM -0700, Y. Zhang wrote:
> Hi all,
> 
> I'm new to linux usb and would like some guidance on achieving some of
> the following goals.
> I have a beaglebone black (BBB - currently running kernel v3.12) with

you need to ask for support from whoever gave you that v3.12 kernel. If
you're using TI's releases, then you need to get in touch with TI's
official support channels. This forum, can't really help you out unless
you move to v4.1 kernel.

> OTG in peripheral mode on usb bus#1 and a host port to connect devices
> on usb bus#2.
> 
> 1. usb bus#1: connect OTG on BBB as a musb gadget (via gadgetfs) to
> another PC host (e.g. linux, windows, etc); usb bus#2: connect a hub
> to BBB host
> + using libusb to control the hub
> + relay all traffics from host->musb gadget->libusb->hub->(applicable
> multiple devices hot-plugged to the hub), and vice versa
> 
> So far I'm able to see traffics between the PC host and the hub before
> plugging any device into the hub. That is, usb traffics for endpoints
> of the hub itself. Once I plug in a device, after certain actions on
> the hub port, the host will send requests (get_descriptor/set_address)
> to address 0, which are not forwarded to the gadhetfs user space. I'd
> like to know:
> * Are traffics for address 0 visible to musb gadget? What code in
> usb/musb and/or usb/gadget can be used/revised to retrieve/handle
> them?
> * If it is feasible to accomplish the above, then the PC host would
> enumerate devices plugged into the hub. What code usage/change would
> be necessary to further retrieve messages addressed to endpoints of
> individual devices and relay them to the HUB (so the HUB can broadcast
> to individual devices), and also forward messages from devices
> received via hub (with libusb) back to host via musb gadget?
> 
> 2. usb bus#1: connect the BBB OTG to a hub attached to the PC host
>+ using musb gadget if needed: host-hub-OTG-(gadget)
>+ when the host sends any messages to the hub, it would then
> broadcast (at least for usb2.0) to all ports on the hub.
>+ sniff broadcast traffic on the OTG usb port (on bus#1)
> 
> I don't necessarily need the OTG gadget to respond anything to the hub
> or PC host, but hope to sniff all usb messages broadcast by the hub,
> which means I will be able to see messages sent by the PC host to
> other devices plugged into the hub. What code in usb/musb and/or
> usb/gadget can be used/revised to sniff such traffics? I've tried
> usbmon but it only monitors devices controlled by hcd (on bus#2) not
> the OTG in peripheral mode (on bus#1)

you really want a HW sniffer. Another option is to mess around with
tracepoints and add support for tracing usb_requests and dump out the
necessary data.

cheers

-- 
balbi


signature.asc
Description: Digital signature

Re: Re: [PATCH] xHCI: FSE handled cleanly by incrementing event dequeue pointer

[SAUROV KANTI SHYAM]

Hi Mathias
The kernel version is 3.8.2, but patches of 3.17 are back ported into it.

> Event ring dequeue is increased either in handle_tx_event(), cleanup:
> if (trb_comp_code == COMP_MISSED_INT || !ep->skip) {
>inc_deq(xhci, xhci->event_ring);

Event ring dequeue should be incremented for "stop,-invalid length” transfer 
event as well (as described in the patch below):
Signed-off-by: Saurov Shyam 
---
 drivers/usb/host/xhci-ring.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 7d34cbf..4d2817d 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2500,7 +2500,9 @@ cleanup:
 * Do not update event ring dequeue pointer if ep->skip is set.
 * Will roll back to continue process missed tds.
 */
-   if (trb_comp_code == COMP_MISSED_INT || !ep->skip) {
+   if (trb_comp_code == COMP_MISSED_INT ||
+   trb_comp_code == COMP_STOP_INVAL ||
+   !ep->skip) {
inc_deq(xhci, xhci->event_ring);
}
--
1.7.9.5 

> Or then in xhci_handle_event() if handle_tx_event() returns error.
> For example in handle_tx_event() if event->buffer is null, then
> ep_ring = xhci_dma_to_transfer_ring(ep, le64_to_cpu(event->buffer));  is NULL
> and the check immediately after would return with -ENODEV 
If event-> buffer is valid for "stop,-invalid length” transfer event, hence 
error condition will not happen.
So the above patch is applicable in this sense.

> P.S. The potion of code was not a suggestion, it was copypasted from the 
> upstream xhci driver, and has
> looked like that since 3.17 kernel
By the phrase “code you suggested” I meant is “code what you referred”. Sorry 
for miscommunication!

--- Original Message ---
Sender : Mathias Nyman
Date : Jul 02, 2015 17:59 (GMT+05:30)
Title : Re: [PATCH] xHCI: FSE handled cleanly by incrementing event dequeue 
pointer

Hi

On 02.07.2015 13:45, SAUROV KANTI SHYAM wrote:
> Some undefined values, mostly null are coming in event->buffer in my case for 
> "stop,-invalid length” transfer event.

That doesn't sound good, what xhci vendor and version are you using?
Have you seen/tried this on other xhci hw as well? 

What kernel version are you using? are there any custom xhci driver 
modification in it?

> However, in the portion of the code you suggested does not increment the 
> dequeue pointer of transfer event ring, for trb_comp_code = COMP_STOP_INVAL.
> In handle_tx_event() cleanup: does not increment the dequeue pointer of 
> transfer event ring.
> In xhci_handle_event():
> …
> case TRB_TYPE(TRB_TRANSFER):
> ret = handle_tx_event(xhci, &event->trans_event);
> if (ret < 0)
> xhci->error_bitmask |= 1 << 9;
> else
> update_ptrs = 0;
> …
> if (update_ptrs)
> /* Update SW event ring dequeue pointer */
> inc_deq(xhci, xhci->event_ring);
> …
> As update_ptrs=0, inc_deq(xhci, xhci->event_ring) won't run.
> Regards,

Event ring dequeue is increased either in handle_tx_event(), cleanup:

if (trb_comp_code == COMP_MISSED_INT || !ep->skip) {
inc_deq(xhci, xhci->event_ring);

Or then in xhci_handle_event() if handle_tx_event() returns error.

For example in handle_tx_event() if event->buffer is null, then
ep_ring = xhci_dma_to_transfer_ring(ep, le64_to_cpu(event->buffer));  is NULL
and the check immediately after would return with -ENODEV 

-Mathias

P.S. The potion of code was not a suggestion, it was copypasted from the 
upstream xhci driver, and has
looked like that since 3.17 kernel