[PATCH V1 2/4] usb: serial: f81534: add auto RTS direction support

[Ji-Ze Hong (Peter Hong)]

The F81532/534 had auto RTS direction support for RS485 mode.
We'll read it from internal Flash with address 0x2f01~0x2f04 for 4 ports.
There are 4 conditions below:
0: F81534_PORT_CONF_RS232.
1: F81534_PORT_CONF_RS485.
2: value error, default to F81534_PORT_CONF_RS232.
3: F81534_PORT_CONF_RS485_INVERT.

F81532/534 Clock register (offset +08h)

Bit0:   UART Enable (always on)
Bit2-1: Clock source selector
00: 1.846MHz.
01: 18.46MHz.
10: 24MHz.
11: 14.77MHz.
Bit4:   Auto direction(RTS) control (RTS pin Low when TX)
Bit5:   Invert direction(RTS) when Bit4 enabled (RTS pin high when TX)

Signed-off-by: Ji-Ze Hong (Peter Hong) 
---
 drivers/usb/serial/f81534.c | 54 +++--
 1 file changed, 52 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
index 76c676ef5f0d..b2d10309c335 100644
--- a/drivers/usb/serial/f81534.c
+++ b/drivers/usb/serial/f81534.c
@@ -102,11 +102,16 @@
 
 #define F81534_DEFAULT_BAUD_RATE   9600
 
+#define F81534_PORT_CONF_RS232 0
+#define F81534_PORT_CONF_RS485 BIT(0)
+#define F81534_PORT_CONF_RS485_INVERT  (BIT(0) | BIT(1))
 #define F81534_PORT_CONF_DISABLE_PORT  BIT(3)
 #define F81534_PORT_CONF_NOT_EXIST_PORTBIT(7)
 #define F81534_PORT_UNAVAILABLE\
(F81534_PORT_CONF_DISABLE_PORT | F81534_PORT_CONF_NOT_EXIST_PORT)
 
+#define F81534_UART_MODE_MASK  (BIT(0) | BIT(1))
+
 #define F81534_1X_RXTRIGGER0xc3
 #define F81534_8X_RXTRIGGER0xcf
 
@@ -119,6 +124,8 @@
  * 01: 18.46MHz.
  * 10: 24MHz.
  * 11: 14.77MHz.
+ * Bit4:   Auto direction(RTS) control (RTS pin Low when TX)
+ * Bit5:   Invert direction(RTS) when Bit4 enabled (RTS pin high when TX)
  */
 
 #define F81534_CLK_1_846_MHZ   BIT(0)
@@ -126,6 +133,9 @@
 #define F81534_CLK_24_MHZ  (BIT(0) | BIT(2))
 #define F81534_CLK_14_77_MHZ   (BIT(0) | BIT(1) | BIT(2))
 
+#define F81534_CLK_RS485_MODE  BIT(4)
+#define F81534_CLK_RS485_INVERTBIT(5)
+
 static const struct usb_device_id f81534_id_table[] = {
{ USB_DEVICE(FINTEK_VENDOR_ID_1, FINTEK_DEVICE_ID) },
{ USB_DEVICE(FINTEK_VENDOR_ID_2, FINTEK_DEVICE_ID) },
@@ -485,16 +495,20 @@ static int f81534_set_port_config(struct usb_serial_port 
*port,
struct tty_struct *tty, u32 baudrate, u32 old_baudrate, u8 lcr)
 {
struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
+   struct f81534_serial_private *serial_priv;
u32 divisor;
int status;
int idx;
u8 value;
+   u8 tmp;
static u32 const baudrate_table[] = {115200, 921600, 1152000,
150};
static u8 const clock_table[] = {F81534_CLK_1_846_MHZ,
F81534_CLK_14_77_MHZ, F81534_CLK_18_46_MHZ,
F81534_CLK_24_MHZ};
 
+   serial_priv = usb_get_serial_data(port->serial);
+
do {
for (idx = 0; idx < ARRAY_SIZE(baudrate_table); ++idx) {
if (baudrate <= baudrate_table[idx] &&
@@ -520,8 +534,25 @@ static int f81534_set_port_config(struct usb_serial_port 
*port,
} while (1);
 
port_priv->baud_base = baudrate_table[idx];
-   status = f81534_set_port_register(port, F81534_CLOCK_REG,
-   clock_table[idx]);
+   tmp = serial_priv->conf_data[port_priv->phy_num];
+
+   switch (tmp & F81534_UART_MODE_MASK) {
+   case F81534_PORT_CONF_RS485_INVERT:
+   value = F81534_CLK_RS485_MODE | F81534_CLK_RS485_INVERT;
+   break;
+   case F81534_PORT_CONF_RS485:
+   value = F81534_CLK_RS485_MODE;
+   break;
+
+   default:
+   /* fall through, default RS232 Mode */
+   case F81534_PORT_CONF_RS232:
+   value = 0;
+   break;
+   }
+
+   value |= clock_table[idx];
+   status = f81534_set_port_register(port, F81534_CLOCK_REG, value);
if (status) {
dev_err(>dev, "CLOCK_REG setting failed\n");
return status;
@@ -1270,9 +1301,12 @@ static void f81534_lsr_worker(struct work_struct *work)
 
 static int f81534_port_probe(struct usb_serial_port *port)
 {
+   struct f81534_serial_private *serial_priv;
struct f81534_port_private *port_priv;
int ret;
+   u8 value;
 
+   serial_priv = usb_get_serial_data(port->serial);
port_priv = devm_kzalloc(>dev, sizeof(*port_priv), GFP_KERNEL);
if (!port_priv)
return -ENOMEM;
@@ -1304,6 +1338,22 @@ static int f81534_port_probe(struct usb_serial_port 
*port)
if (ret)
return ret;
 
+   value = 

[PATCH V1 4/4] usb: serial: f81534: add H/W disable port support

[Ji-Ze Hong (Peter Hong)]

The F81532/534 can be disable port by manufacturer with
following H/W design.
1: Connect DCD/DSR/CTS/RI pin to ground.
2: Connect RX pin to ground.

In driver, we'll implements some detect method likes following:
1: Read MSR.
2: Turn MCR LOOP bit on, off and read LSR after delay with 60ms.
   It'll contain BREAK status in LSR.

Signed-off-by: Ji-Ze Hong (Peter Hong) 
---
 drivers/usb/serial/f81534.c | 74 +
 1 file changed, 74 insertions(+)

diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
index 30b966d71ae8..18bd2a478199 100644
--- a/drivers/usb/serial/f81534.c
+++ b/drivers/usb/serial/f81534.c
@@ -751,6 +751,74 @@ static int f81534_find_config_idx(struct usb_serial 
*serial, u8 *index)
 }
 
 /*
+ * The F81532/534 will not report serial port to USB serial subsystem when
+ * H/W DCD/DSR/CTS/RI/RX pin connected to ground.
+ *
+ * To detect RX pin status, we'll enable MCR interal loopback, disable it and
+ * delayed for 60ms. It connected to ground If LSR register report UART_LSR_BI.
+ */
+static int f81534_check_port_hw_disabled(struct usb_serial *serial, int phy)
+{
+   int status;
+   u8 old_mcr;
+   u8 msr;
+   u8 lsr;
+   u8 msr_mask;
+
+   msr_mask = UART_MSR_DCD | UART_MSR_RI | UART_MSR_DSR | UART_MSR_CTS;
+
+   status = f81534_get_register(serial,
+   F81534_MODEM_STATUS_REG + phy * 0x10, );
+   if (status)
+   return status;
+
+   if ((msr & msr_mask) != msr_mask)
+   return 0;
+
+   status = f81534_set_register(serial,
+   F81534_FIFO_CONTROL_REG + phy * 0x10,
+   UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR |
+   UART_FCR_CLEAR_XMIT);
+   if (status)
+   return status;
+
+   status = f81534_get_register(serial,
+   F81534_MODEM_CONTROL_REG + phy * 0x10,
+   _mcr);
+   if (status)
+   return status;
+
+   status = f81534_set_register(serial,
+   F81534_MODEM_CONTROL_REG + phy * 0x10,
+   UART_MCR_LOOP);
+   if (status)
+   return status;
+
+   status = f81534_set_register(serial,
+   F81534_MODEM_CONTROL_REG + phy * 0x10, 0x0);
+   if (status)
+   return status;
+
+   msleep(60);
+
+   status = f81534_get_register(serial,
+   F81534_LINE_STATUS_REG + phy * 0x10, );
+   if (status)
+   return status;
+
+   status = f81534_set_register(serial,
+   F81534_MODEM_CONTROL_REG + phy * 0x10,
+   old_mcr);
+   if (status)
+   return status;
+
+   if ((lsr & UART_LSR_BI) == UART_LSR_BI)
+   return -ENODEV;
+
+   return 0;
+}
+
+/*
  * We had 2 generation of F81532/534 IC. All has an internal storage.
  *
  * 1st is pure USB-to-TTL RS232 IC and designed for 4 ports only, no any
@@ -832,6 +900,9 @@ static int f81534_calc_num_ports(struct usb_serial *serial,
 
/* New style, find all possible ports */
for (i = 0; i < F81534_NUM_PORT; ++i) {
+   if (f81534_check_port_hw_disabled(serial, i))
+   continue;
+
if (setting[i] & F81534_PORT_UNAVAILABLE)
continue;
 
@@ -1306,6 +1377,9 @@ static int f81534_attach(struct usb_serial *serial)
 
/* Assign phy-to-logic mapping */
for (i = 0; i < F81534_NUM_PORT; ++i) {
+   if (f81534_check_port_hw_disabled(serial, i))
+   serial_priv->conf_data[i] |= F81534_PORT_UNAVAILABLE;
+
if (serial_priv->conf_data[i] & F81534_PORT_UNAVAILABLE)
continue;
 
-- 
2.7.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

[PATCH V1 1/4] usb: serial: f81534: add high baud rate support

[Ji-Ze Hong (Peter Hong)]

The F81532/534 had 4 clocksource 1.846/18.46/14.77/24MHz and baud rates can
be up to 1.5Mbits with 24MHz.

F81532/534 Clock register (offset +08h)

Bit0:   UART Enable (always on)
Bit2-1: Clock source selector
00: 1.846MHz.
01: 18.46MHz.
10: 24MHz.
11: 14.77MHz.

Signed-off-by: Ji-Ze Hong (Peter Hong) 
---
 drivers/usb/serial/f81534.c | 84 -
 1 file changed, 68 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
index cb8214860192..76c676ef5f0d 100644
--- a/drivers/usb/serial/f81534.c
+++ b/drivers/usb/serial/f81534.c
@@ -45,6 +45,7 @@
 #define F81534_MODEM_CONTROL_REG   (0x04 + F81534_UART_BASE_ADDRESS)
 #define F81534_LINE_STATUS_REG (0x05 + F81534_UART_BASE_ADDRESS)
 #define F81534_MODEM_STATUS_REG(0x06 + 
F81534_UART_BASE_ADDRESS)
+#define F81534_CLOCK_REG   (0x08 + F81534_UART_BASE_ADDRESS)
 #define F81534_CONFIG1_REG (0x09 + F81534_UART_BASE_ADDRESS)
 
 #define F81534_DEF_CONF_ADDRESS_START  0x3000
@@ -61,7 +62,7 @@
 
 /* Default URB timeout for USB operations */
 #define F81534_USB_MAX_RETRY   10
-#define F81534_USB_TIMEOUT 1000
+#define F81534_USB_TIMEOUT 2000
 #define F81534_SET_GET_REGISTER0xA0
 
 #define F81534_NUM_PORT4
@@ -100,7 +101,6 @@
 #define F81534_CMD_READ0x03
 
 #define F81534_DEFAULT_BAUD_RATE   9600
-#define F81534_MAX_BAUDRATE115200
 
 #define F81534_PORT_CONF_DISABLE_PORT  BIT(3)
 #define F81534_PORT_CONF_NOT_EXIST_PORTBIT(7)
@@ -110,6 +110,22 @@
 #define F81534_1X_RXTRIGGER0xc3
 #define F81534_8X_RXTRIGGER0xcf
 
+/*
+ * F81532/534 Clock registers (offset +08h)
+ *
+ * Bit0:   UART Enable (always on)
+ * Bit2-1: Clock source selector
+ * 00: 1.846MHz.
+ * 01: 18.46MHz.
+ * 10: 24MHz.
+ * 11: 14.77MHz.
+ */
+
+#define F81534_CLK_1_846_MHZ   BIT(0)
+#define F81534_CLK_18_46_MHZ   (BIT(0) | BIT(1))
+#define F81534_CLK_24_MHZ  (BIT(0) | BIT(2))
+#define F81534_CLK_14_77_MHZ   (BIT(0) | BIT(1) | BIT(2))
+
 static const struct usb_device_id f81534_id_table[] = {
{ USB_DEVICE(FINTEK_VENDOR_ID_1, FINTEK_DEVICE_ID) },
{ USB_DEVICE(FINTEK_VENDOR_ID_2, FINTEK_DEVICE_ID) },
@@ -133,6 +149,7 @@ struct f81534_port_private {
struct usb_serial_port *port;
unsigned long tx_empty;
spinlock_t msr_lock;
+   u32 baud_base;
u8 shadow_mcr;
u8 shadow_lcr;
u8 shadow_msr;
@@ -464,13 +481,51 @@ static u32 f81534_calc_baud_divisor(u32 baudrate, u32 
clockrate)
return DIV_ROUND_CLOSEST(clockrate, baudrate);
 }
 
-static int f81534_set_port_config(struct usb_serial_port *port, u32 baudrate,
-   u8 lcr)
+static int f81534_set_port_config(struct usb_serial_port *port,
+   struct tty_struct *tty, u32 baudrate, u32 old_baudrate, u8 lcr)
 {
struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
u32 divisor;
int status;
+   int idx;
u8 value;
+   static u32 const baudrate_table[] = {115200, 921600, 1152000,
+   150};
+   static u8 const clock_table[] = {F81534_CLK_1_846_MHZ,
+   F81534_CLK_14_77_MHZ, F81534_CLK_18_46_MHZ,
+   F81534_CLK_24_MHZ};
+
+   do {
+   for (idx = 0; idx < ARRAY_SIZE(baudrate_table); ++idx) {
+   if (baudrate <= baudrate_table[idx] &&
+   baudrate_table[idx] % baudrate == 0)
+   break;
+   }
+
+   /* Found acceptable baud rate */
+   if (idx != ARRAY_SIZE(baudrate_table))
+   break;
+
+   if (baudrate == old_baudrate &&
+   old_baudrate != F81534_DEFAULT_BAUD_RATE)
+   old_baudrate = F81534_DEFAULT_BAUD_RATE;
+
+   dev_warn(>dev,
+   "baudrate: %d not supported, change to: %d\n",
+   baudrate, old_baudrate);
+
+   baudrate = old_baudrate;
+   tty_encode_baud_rate(tty, baudrate, baudrate);
+
+   } while (1);
+
+   port_priv->baud_base = baudrate_table[idx];
+   status = f81534_set_port_register(port, F81534_CLOCK_REG,
+   clock_table[idx]);
+   if (status) {
+   dev_err(>dev, "CLOCK_REG setting failed\n");
+   return status;
+   }
 
if (baudrate <= 1200)
value = F81534_1X_RXTRIGGER;/* 128 FIFO & TL: 1x */
@@ -486,7 +541,7 @@ 

[PATCH V1 3/4] usb: serial: f81534: add output pin control

[Ji-Ze Hong (Peter Hong)]

The F81532/534 had 3 output pin (M0/SD, M1, M2) with open-drain mode to
control transceiver. We'll read it from internal Flash with address
0x2f05~0x2f08 for 4 ports. The value is range from 0 to 7. The M0/SD is
MSB of this value. For a examples, If read value is 6, we'll write M0/SD,
M1, M2 as 1, 1, 0.

Register mapping for output value:
Port 0:
M2: 0x2ae8 bit7, M1: 0x2a90 bit5, M0/SD: 0x2a90 bit4
Port 1:
M2: 0x2ae8 bit6, M1: 0x2ae8 bit0, M0/SD: 0x2ae8 bit3
Port 2:
M2: 0x2a90 bit0, M1: 0x2ae8 bit2, M0/SD: 0x2a80 bit6
Port 3:
M2: 0x2a90 bit3, M1: 0x2a90 bit2, M0/SD: 0x2a90 bit1

Signed-off-by: Ji-Ze Hong (Peter Hong) 
---
 drivers/usb/serial/f81534.c | 67 -
 1 file changed, 66 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
index b2d10309c335..30b966d71ae8 100644
--- a/drivers/usb/serial/f81534.c
+++ b/drivers/usb/serial/f81534.c
@@ -56,6 +56,7 @@
 #define F81534_CUSTOM_NO_CUSTOM_DATA   0xff
 #define F81534_CUSTOM_VALID_TOKEN  0xf0
 #define F81534_CONF_OFFSET 1
+#define F81534_CONF_GPIO_OFFSET4
 
 #define F81534_MAX_DATA_BLOCK  64
 #define F81534_MAX_BUS_RETRY   20
@@ -166,6 +167,23 @@ struct f81534_port_private {
u8 phy_num;
 };
 
+struct f81534_pin_data {
+   const u16 reg_addr;
+   const u16 reg_mask;
+};
+
+struct f81534_port_out_pin {
+   struct f81534_pin_data pin[3];
+};
+
+/* Pin output value for M2/M1/M0(SD) */
+static const struct f81534_port_out_pin f81534_port_out_pins[] = {
+{{{0x2ae8, BIT(7)}, {0x2a90, BIT(5)}, {0x2a90, BIT(4) } } },
+{{{0x2ae8, BIT(6)}, {0x2ae8, BIT(0)}, {0x2ae8, BIT(3) } } },
+{{{0x2a90, BIT(0)}, {0x2ae8, BIT(2)}, {0x2a80, BIT(6) } } },
+{{{0x2a90, BIT(3)}, {0x2a90, BIT(2)}, {0x2a90, BIT(1) } } },
+};
+
 static int f81534_logic_to_phy_port(struct usb_serial *serial,
struct usb_serial_port *port)
 {
@@ -271,6 +289,22 @@ static int f81534_get_register(struct usb_serial *serial, 
u16 reg, u8 *data)
return status;
 }
 
+static int f81534_set_mask_register(struct usb_serial *serial, u16 reg,
+   u8 mask, u8 data)
+{
+   int status;
+   u8 tmp;
+
+   status = f81534_get_register(serial, reg, );
+   if (status)
+   return status;
+
+   tmp &= ~mask;
+   tmp |= (mask & data);
+
+   return f81534_set_register(serial, reg, tmp);
+}
+
 static int f81534_set_port_register(struct usb_serial_port *port, u16 reg,
u8 data)
 {
@@ -1299,6 +1333,37 @@ static void f81534_lsr_worker(struct work_struct *work)
dev_warn(>dev, "read LSR failed: %d\n", status);
 }
 
+static int f81534_set_port_output_pin(struct usb_serial_port *port)
+{
+   struct f81534_serial_private *serial_priv;
+   struct f81534_port_private *port_priv;
+   struct usb_serial *serial;
+   const struct f81534_port_out_pin *pins;
+   int status;
+   int i;
+   u8 value;
+   u8 idx;
+
+   serial = port->serial;
+   serial_priv = usb_get_serial_data(serial);
+   port_priv = usb_get_serial_port_data(port);
+
+   idx = F81534_CONF_GPIO_OFFSET + port_priv->phy_num;
+   value = serial_priv->conf_data[idx];
+   pins = _port_out_pins[port_priv->phy_num];
+
+   for (i = 0; i < ARRAY_SIZE(pins->pin); ++i) {
+   status = f81534_set_mask_register(serial,
+   pins->pin[i].reg_addr, pins->pin[i].reg_mask,
+   value & BIT(i) ? pins->pin[i].reg_mask : 0);
+   if (status)
+   return status;
+   }
+
+   dev_info(>dev, "Output pin (M0/M1/M2): %d\n", value);
+   return 0;
+}
+
 static int f81534_port_probe(struct usb_serial_port *port)
 {
struct f81534_serial_private *serial_priv;
@@ -1354,7 +1419,7 @@ static int f81534_port_probe(struct usb_serial_port *port)
break;
}
 
-   return 0;
+   return f81534_set_port_output_pin(port);
 }
 
 static int f81534_port_remove(struct usb_serial_port *port)
-- 
2.7.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: Seagate External SMR drive USB resets (XHCI transfer error, not timeout)

[Jérôme Carretero]

Hi,

On Wed, 15 Nov 2017 18:17:08 -0500
Jérôme Carretero  wrote:

> Hi,
> 
> 
> On Thu, 16 Nov 2017 07:40:08 +0900
> James Bottomley  wrote:
> 
> > On Wed, 2017-11-15 at 17:02 -0500, Alan Stern wrote:  
> > > On Wed, 15 Nov 2017, Jérôme Carretero wrote:
> > > > >   Because with several of these drives / lots of
> > > > >activity /
> > > occasional
> > > > >   issues, it looks like it will be hard to catch (yes I can
> > > > >use
> > > > > usbmon).
> > > > > 
> > > > > - It looks like there is no configurable timeout for USB
> > > > > MSC
> > > requests.
> > > > >   Perhaps the device is not responding in time and this is
> > > > >why
> > > it's
> > > > >   reset?
> > > 
> > > Timeouts are set by the SCSI layer.  I believe they are rather
> > > long (30 seconds, by default).  Presumably they are configurable,
> > > although I would have to do some digging to figure out how.
> > 
> > They're in /sys/class/scsi_device//device/timeout  
> 
> 
> I'll use wireshark to check the cause: for sure, the drives are not
> "timing out" after 30 seconds (indeed the reported value
> in /sys/class/scsi_device/.../timeout or /sys/block/*/device/timeout),
> because I see (in dstat) that a disk is busy until the right about the
> moment where its reset message appears.
> 
> Is it possible that the SCSI timeout doesn't get set into an USB URB
> timeout (I'll check by myself, but asking doesn't hurt) ?

I performed an usbmon capture extract, centered around the event
(there was a few hundred MBs written for this to happen):

 Nov 15 22:16:33 Bidule kernel: usb 6-4.3.2.1: reset SuperSpeed USB
  device number 8 using xhci_hcd

I can see that the computer is sending a write request, and sees a
-EPROTO in answer (capture in attachment), so scratch the timeout issue
(and actually when thinking about it, this matches what UAS was saying,
except that UAS was taking ages to recover).

Looked for EPROTO in the usb code, and found a dynamic debug printf in
XHCI; after enabling it:

 Nov 15 22:45:03 Bidule kernel: xhci_hcd :07:00.0: Transfer error for slot 
13 ep 3 on endpoint
 Nov 15 22:45:03 Bidule kernel: xhci_hcd :07:00.0: Transfer error for slot 
12 ep 3 on endpoint
 Nov 15 22:45:03 Bidule kernel: usb 6-4.3.3.1: reset SuperSpeed USB device 
number 9 using xhci_hcd
 Nov 15 22:45:03 Bidule kernel: usb 6-4.3.2.1: reset SuperSpeed USB device 
number 8 using xhci_hcd

First, I understand that a bad USB device could poison the kernel log,
but shouldn't that xhci_dbg() (and others eg. babble) be at least an
xhci_info() (I saw 2a9227a5)?

Then... I don't know enough to attribute the issue the upstream USB hub(s)
or the drive endpoint not behaving properly, or the kernel... what
should I do with these messages?

I'm still filling the drives, will perform a scrub after, to see if
the issue causes data loss...


-- 
Jérôme

PS: BTW, thanks a lot for the help so far.
PPS: It would be so nice if someone from Seagate was reading this.


smr-reset-excerpt.pcapng.gz
Description: application/gzip

Re: [PATCH 2/2 v6] typec: tcpm: Only request matching pdos

[Guenter Roeck]

On 11/15/2017 05:01 PM, Badhri Jagan Sridharan wrote:

On Tue, Nov 14, 2017 at 9:05 AM, Guenter Roeck  wrote:

[ ... ]


Reviewed-by: Guenter Roeck 

/* Though I really dislike this new-world comment style.
  * It hurts my eyes and distracts me from the code.
  */


Did I somehow introduce this ? I dont think I introduced new comments.



[ ... ]


+ /* There could only be one fixed pdo
+  * at a specific voltage level.
+  * So breaking here.
+  */


Guenter
--
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 2/2 v7] typec: tcpm: Only request matching pdos

[Badhri Jagan Sridharan]

At present, TCPM code assumes that local device supports
variable/batt pdos and always selects the pdo with highest
possible power within the board limit. This assumption
might not hold good for all devices. To overcome this,
this patch makes TCPM only accept a source_pdo when there is
a matching sink pdo.

For Fixed pdos: The voltage should match between the
incoming source_cap and the registered snk_pdo
For Variable/Batt pdos: The incoming source_cap voltage
range should fall within the registered snk_pdo's voltage
range.

Also, when the cap_mismatch bit is set, the max_power/current
should be set to the max_current/power of the sink_pdo.
This is according to:

"If the Capability Mismatch bit is set to one
The Maximum Operating Current/Power field may contain a value
larger than the maximum current/power offered in the Source
Capabilities message’s PDO as referenced by the Object position field.
This enables the Sink to indicate that it requires more current/power
than is being offered. If the Sink requires a different voltage this
will be indicated by its Sink Capabilities message.

Signed-off-by: Badhri Jagan Sridharan 
Acked-by: Heikki Krogerus 
Reviewed-by: Guenter Roeck 

---
Changelog since v1:
- Rebased the patch on top of drivers/usb/type/tcpm.c
- Added duplicate pdo check for variable/batt pdo.
- Fixed tabs as suggested by dan.carpen...@oracle.com

Changelog since v2:
- Rebase

Changelog since v3:
- Refactored code fixed formatting issues as suggested
  by heikki.kroge...@linux.intel.com

Changelog since v4:
- Rebase

Changelog since v5:
- handling the sink pdo index as pointer argument in tcpm_pd_select_pdo
- ACK by heikki.kroge...@linux.intel.com

Changelog since v6:
- Added Reviewed-by: Guenter Roeck 

 drivers/usb/typec/tcpm.c | 163 +++
 1 file changed, 121 insertions(+), 42 deletions(-)

diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index 8b637a4b474b..f4d563ee7690 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -252,6 +252,9 @@ struct tcpm_port {
unsigned int nr_src_pdo;
u32 snk_pdo[PDO_MAX_OBJECTS];
unsigned int nr_snk_pdo;
+   unsigned int nr_fixed; /* number of fixed sink PDOs */
+   unsigned int nr_var; /* number of variable sink PDOs */
+   unsigned int nr_batt; /* number of battery sink PDOs */
u32 snk_vdo[VDO_MAX_OBJECTS];
unsigned int nr_snk_vdo;
 
@@ -1767,39 +1770,90 @@ static int tcpm_pd_check_request(struct tcpm_port *port)
return 0;
 }
 
-static int tcpm_pd_select_pdo(struct tcpm_port *port)
+#define min_power(x, y) min(pdo_max_power(x), pdo_max_power(y))
+#define min_current(x, y) min(pdo_max_current(x), pdo_max_current(y))
+
+static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo,
+ int *src_pdo)
 {
-   unsigned int i, max_mw = 0, max_mv = 0;
+   unsigned int i, j, max_mw = 0, max_mv = 0, mw = 0, mv = 0, ma = 0;
int ret = -EINVAL;
 
/*
-* Select the source PDO providing the most power while staying within
-* the board's voltage limits. Prefer PDO providing exp
+* Select the source PDO providing the most power which has a
+* matchig sink cap.
 */
for (i = 0; i < port->nr_source_caps; i++) {
u32 pdo = port->source_caps[i];
enum pd_pdo_type type = pdo_type(pdo);
-   unsigned int mv, ma, mw;
 
-   if (type == PDO_TYPE_FIXED)
-   mv = pdo_fixed_voltage(pdo);
-   else
-   mv = pdo_min_voltage(pdo);
-
-   if (type == PDO_TYPE_BATT) {
-   mw = pdo_max_power(pdo);
-   } else {
-   ma = min(pdo_max_current(pdo),
-port->max_snk_ma);
-   mw = ma * mv / 1000;
-   }
-
-   /* Perfer higher voltages if available */
-   if ((mw > max_mw || (mw == max_mw && mv > max_mv)) &&
-   mv <= port->max_snk_mv) {
-   ret = i;
-   max_mw = mw;
-   max_mv = mv;
+   if (type == PDO_TYPE_FIXED) {
+   for (j = 0; j < port->nr_fixed; j++) {
+   if (pdo_fixed_voltage(pdo) ==
+   pdo_fixed_voltage(port->snk_pdo[j])) {
+   ma = min_current(pdo, port->snk_pdo[j]);
+   mv = pdo_fixed_voltage(pdo);
+   mw = ma * mv / 1000;
+   if (mw > max_mw ||
+   (mw == max_mw && mv > max_mv)) {
+   ret = 0;
+  

[PATCH 1/2 v7] typec: tcpm: Validate source and sink caps

[Badhri Jagan Sridharan]

The source and sink caps should follow the following rules.
This patch validates whether the src_caps/snk_caps adheres
to it.

6.4.1 Capabilities Message
A Capabilities message (Source Capabilities message or Sink
Capabilities message) shall have at least one Power
Data Object for vSafe5V. The Capabilities message shall also
contain the sending Port’s information followed by up to
6 additional Power Data Objects. Power Data Objects in a
Capabilities message shall be sent in the following order:

1. The vSafe5V Fixed Supply Object shall always be the first object.
2. The remaining Fixed Supply Objects, if present, shall be sent
   in voltage order; lowest to highest.
3. The Battery Supply Objects, if present shall be sent in Minimum
   Voltage order; lowest to highest.
4. The Variable Supply (non-battery) Objects, if present, shall be
   sent in Minimum Voltage order; lowest to highest.

Errors in source/sink_caps of the local port will prevent
the port registration. Whereas, errors in source caps of partner
device would only log them.

Signed-off-by: Badhri Jagan Sridharan 
Acked-by: Heikki Krogerus 
Reviewed-by: Guenter Roeck 

---
Changelog since v1:
- rebased on top drivers/usb/typec/tcpm.c as suggested by
  gre...@linuxfoundation.org
- renamed nr_snk_*_pdo as suggested by dan.carpen...@oracle.com
- removed stale comment as suggested by li...@roeck-us.net
- removed the tests for nr_snk_*_pdo as suggested by
  dan.carpen...@oracle.com
- Fixed sytling as suggested by dan.carpen...@oracle.com
- renamed tcpm_get_nr_type_pdos to nr_type_pdos as suggested by
  dan.carpen...@oracle.com
- fixed nr_type_pdos as suggested by dan.carpen...@oracle.com
- tcpm_pd_select_pdo now checks for all matching variable/batt pdos
  instead of the first matching one.

Changelog since v2:
- refactored error messages as suggested by
  heikki.kroge...@linux.intel.com

Changelog since v3:
- fixed formatting errors as suggested by
  heikki.kroge...@linux.intel.com

Changelog since v4:
- Reusing the macro

Changelog since v5:
- Moved to enum for pdo_cap_err messages as suggested by
 heikki.kroge...@linux.intel.com
- Ack by heikki.kroge...@linux.intel.com

Changelog since v6:
- Added Reviewed-by: Guenter Roeck 

 drivers/usb/typec/tcpm.c | 130 +++
 include/linux/usb/pd.h   |   2 +
 include/linux/usb/tcpm.h |  16 +++---
 3 files changed, 131 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index c166fc77dfb8..8b637a4b474b 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -1247,6 +1247,100 @@ static void vdm_state_machine_work(struct work_struct 
*work)
mutex_unlock(>lock);
 }
 
+enum pdo_err {
+   PDO_NO_ERR,
+   PDO_ERR_NO_VSAFE5V,
+   PDO_ERR_VSAFE5V_NOT_FIRST,
+   PDO_ERR_PDO_TYPE_NOT_IN_ORDER,
+   PDO_ERR_FIXED_NOT_SORTED,
+   PDO_ERR_VARIABLE_BATT_NOT_SORTED,
+   PDO_ERR_DUPE_PDO,
+};
+
+static const char * const pdo_err_msg[] = {
+   [PDO_ERR_NO_VSAFE5V] =
+   " err: source/sink caps should atleast have vSafe5V",
+   [PDO_ERR_VSAFE5V_NOT_FIRST] =
+   " err: vSafe5V Fixed Supply Object Shall always be the first object",
+   [PDO_ERR_PDO_TYPE_NOT_IN_ORDER] =
+   " err: PDOs should be in the following order: Fixed; Battery; Variable",
+   [PDO_ERR_FIXED_NOT_SORTED] =
+   " err: Fixed supply pdos should be in increasing order of their fixed 
voltage",
+   [PDO_ERR_VARIABLE_BATT_NOT_SORTED] =
+   " err: Variable/Battery supply pdos should be in increasing order of 
their minimum voltage",
+   [PDO_ERR_DUPE_PDO] =
+   " err: Variable/Batt supply pdos cannot have same min/max voltage",
+};
+
+static enum pdo_err tcpm_caps_err(struct tcpm_port *port, const u32 *pdo,
+ unsigned int nr_pdo)
+{
+   unsigned int i;
+
+   /* Should at least contain vSafe5v */
+   if (nr_pdo < 1)
+   return PDO_ERR_NO_VSAFE5V;
+
+   /* The vSafe5V Fixed Supply Object Shall always be the first object */
+   if (pdo_type(pdo[0]) != PDO_TYPE_FIXED ||
+   pdo_fixed_voltage(pdo[0]) != VSAFE5V)
+   return PDO_ERR_VSAFE5V_NOT_FIRST;
+
+   for (i = 1; i < nr_pdo; i++) {
+   if (pdo_type(pdo[i]) < pdo_type(pdo[i - 1])) {
+   return PDO_ERR_PDO_TYPE_NOT_IN_ORDER;
+   } else if (pdo_type(pdo[i]) == pdo_type(pdo[i - 1])) {
+   enum pd_pdo_type type = pdo_type(pdo[i]);
+
+   switch (type) {
+   /*
+* The remaining Fixed Supply Objects, if
+* present, shall be sent in voltage order;
+* lowest to highest.
+*/
+   case PDO_TYPE_FIXED:
+   if 

Re: [PATCH 2/2 v6] typec: tcpm: Only request matching pdos

[Badhri Jagan Sridharan]

On Tue, Nov 14, 2017 at 9:05 AM, Guenter Roeck  wrote:
> On Sun, Nov 12, 2017 at 04:23:17PM -0800, Badhri Jagan Sridharan wrote:
>> At present, TCPM code assumes that local device supports
>> variable/batt pdos and always selects the pdo with highest
>> possible power within the board limit. This assumption
>> might not hold good for all devices. To overcome this,
>> this patch makes TCPM only accept a source_pdo when there is
>> a matching sink pdo.
>>
>> For Fixed pdos: The voltage should match between the
>> incoming source_cap and the registered snk_pdo
>> For Variable/Batt pdos: The incoming source_cap voltage
>> range should fall within the registered snk_pdo's voltage
>> range.
>>
>> Also, when the cap_mismatch bit is set, the max_power/current
>> should be set to the max_current/power of the sink_pdo.
>> This is according to:
>>
>> "If the Capability Mismatch bit is set to one
>> The Maximum Operating Current/Power field may contain a value
>> larger than the maximum current/power offered in the Source
>> Capabilities message’s PDO as referenced by the Object position field.
>> This enables the Sink to indicate that it requires more current/power
>> than is being offered. If the Sink requires a different voltage this
>> will be indicated by its Sink Capabilities message.
>>
>> Signed-off-by: Badhri Jagan Sridharan 
>> Acked-by: Heikki Krogerus 
>
> Reviewed-by: Guenter Roeck 
>
> /* Though I really dislike this new-world comment style.
>  * It hurts my eyes and distracts me from the code.
>  */

Did I somehow introduce this ? I dont think I introduced new comments.

>
>> ---
>> Changelog since v1:
>> - Rebased the patch on top of drivers/usb/type/tcpm.c
>> - Added duplicate pdo check for variable/batt pdo.
>> - Fixed tabs as suggested by dan.carpen...@oracle.com
>>
>> Changelog since v2:
>> - Rebase
>>
>> Changelog since v3:
>> - Refactored code fixed formatting issues as suggested
>>   by heikki.kroge...@linux.intel.com
>>
>> Changelog since v4:
>> - Rebase
>>
>> Changelog since v5:
>> - handling the sink pdo index as pointer argument in tcpm_pd_select_pdo
>> - ACK by heikki.kroge...@linux.intel.com
>>
>>  drivers/usb/typec/tcpm.c | 163 
>> +++
>>  1 file changed, 121 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
>> index 8b637a4b474b..f4d563ee7690 100644
>> --- a/drivers/usb/typec/tcpm.c
>> +++ b/drivers/usb/typec/tcpm.c
>> @@ -252,6 +252,9 @@ struct tcpm_port {
>>   unsigned int nr_src_pdo;
>>   u32 snk_pdo[PDO_MAX_OBJECTS];
>>   unsigned int nr_snk_pdo;
>> + unsigned int nr_fixed; /* number of fixed sink PDOs */
>> + unsigned int nr_var; /* number of variable sink PDOs */
>> + unsigned int nr_batt; /* number of battery sink PDOs */
>>   u32 snk_vdo[VDO_MAX_OBJECTS];
>>   unsigned int nr_snk_vdo;
>>
>> @@ -1767,39 +1770,90 @@ static int tcpm_pd_check_request(struct tcpm_port 
>> *port)
>>   return 0;
>>  }
>>
>> -static int tcpm_pd_select_pdo(struct tcpm_port *port)
>> +#define min_power(x, y) min(pdo_max_power(x), pdo_max_power(y))
>> +#define min_current(x, y) min(pdo_max_current(x), pdo_max_current(y))
>> +
>> +static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo,
>> +   int *src_pdo)
>>  {
>> - unsigned int i, max_mw = 0, max_mv = 0;
>> + unsigned int i, j, max_mw = 0, max_mv = 0, mw = 0, mv = 0, ma = 0;
>>   int ret = -EINVAL;
>>
>>   /*
>> -  * Select the source PDO providing the most power while staying within
>> -  * the board's voltage limits. Prefer PDO providing exp
>> +  * Select the source PDO providing the most power which has a
>> +  * matchig sink cap.
>>*/
>>   for (i = 0; i < port->nr_source_caps; i++) {
>>   u32 pdo = port->source_caps[i];
>>   enum pd_pdo_type type = pdo_type(pdo);
>> - unsigned int mv, ma, mw;
>>
>> - if (type == PDO_TYPE_FIXED)
>> - mv = pdo_fixed_voltage(pdo);
>> - else
>> - mv = pdo_min_voltage(pdo);
>> -
>> - if (type == PDO_TYPE_BATT) {
>> - mw = pdo_max_power(pdo);
>> - } else {
>> - ma = min(pdo_max_current(pdo),
>> -  port->max_snk_ma);
>> - mw = ma * mv / 1000;
>> - }
>> -
>> - /* Perfer higher voltages if available */
>> - if ((mw > max_mw || (mw == max_mw && mv > max_mv)) &&
>> - mv <= port->max_snk_mv) {
>> - ret = i;
>> - max_mw = mw;
>> - max_mv = mv;
>> + if (type == PDO_TYPE_FIXED) {
>> + for (j = 0; j < port->nr_fixed; j++) {
>> + if (pdo_fixed_voltage(pdo) 

Re: Seagate External SMR drive USB resets... why? / USB storage debugging

[Bart Van Assche]

On Wed, 2017-11-15 at 18:27 -0500, Jérôme Carretero wrote:
> OK but I find that a "reset" message without any reason is not
> as helpful as it could have been. At the minimum I'll try to scratch my
> own itch and see if I can go at the bottom of my issue.

If you want more information about SCSI error handler decisions, observing
the output sent by the following command to the system log will probably help:

echo 63 > /sys/module/scsi_mod/parameters/scsi_logging_level

See also
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/scsi_logging.h
Bart.

Re: Seagate External SMR drive USB resets... why? / USB storage debugging

[Jérôme Carretero]

On Wed, 15 Nov 2017 17:02:39 -0500 (EST)
Alan Stern  wrote:

> > > - It looks like (haven't tested it yet) the CONFIG_DYNAMIC_DEBUG
> > > isn't used with the USB mass storage debugging infrastructure,
> > > please confirm? If unused, are we interested to have a patch that
> > > would go back to regular pr_debug() that can work with dynamic
> > > debugging?  
> 
> dev_dbg() please, not pr_debug().  But yes, that would be worthwhile.

Yes obviously.

Adding Joe Perches to the loop as he did a refactor of USB mass storage
a while ago.


> Note, however, that usb-storage debugging is meant only for debugging
> problems in the usb-storage driver itself, not for debugging problems
> in attached devices.  We use usbmon or wireshark for the latter.

OK but I find that a "reset" message without any reason is not
as helpful as it could have been. At the minimum I'll try to scratch my
own itch and see if I can go at the bottom of my issue.


Regards,

-- 
Jérôme
--
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: Seagate External SMR drive USB resets (was: Re: [PATCH] uas: Add US_FL_NO_ATA_1X quirk for one more Seagate device)

[Jérôme Carretero]

Hi,


On Thu, 16 Nov 2017 07:40:08 +0900
James Bottomley  wrote:

> On Wed, 2017-11-15 at 17:02 -0500, Alan Stern wrote:
> > On Wed, 15 Nov 2017, Jérôme Carretero wrote:  
> > > >   Because with several of these drives / lots of activity /  
> > occasional  
> > > >   issues, it looks like it will be hard to catch (yes I can use
> > > > usbmon).
> > > > 
> > > > - It looks like there is no configurable timeout for USB MSC  
> > requests.  
> > > >   Perhaps the device is not responding in time and this is why  
> > it's  
> > > >   reset?  
> > 
> > Timeouts are set by the SCSI layer.  I believe they are rather long
> > (30 seconds, by default).  Presumably they are configurable,
> > although I would have to do some digging to figure out how.  
> 
> They're in /sys/class/scsi_device//device/timeout


I'll use wireshark to check the cause: for sure, the drives are not
"timing out" after 30 seconds (indeed the reported value
in /sys/class/scsi_device/.../timeout or /sys/block/*/device/timeout),
because I see (in dstat) that a disk is busy until the right about the
moment where its reset message appears.

Is it possible that the SCSI timeout doesn't get set into an USB URB
timeout (I'll check by myself, but asking doesn't hurt) ?


Thank you,

-- 
Jérôme
--
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: Seagate External SMR drive USB resets (was: Re: [PATCH] uas: Add US_FL_NO_ATA_1X quirk for one more Seagate device)

[James Bottomley]

On Wed, 2017-11-15 at 17:02 -0500, Alan Stern wrote:
> On Wed, 15 Nov 2017, Jérôme Carretero wrote:
> > >   Because with several of these drives / lots of activity /
> occasional
> > >   issues, it looks like it will be hard to catch (yes I can use
> > > usbmon).
> > > 
> > > - It looks like there is no configurable timeout for USB MSC
> requests.
> > >   Perhaps the device is not responding in time and this is why
> it's
> > >   reset?
> 
> Timeouts are set by the SCSI layer.  I believe they are rather long
> (30 seconds, by default).  Presumably they are configurable, although
> I would have to do some digging to figure out how.

They're in /sys/class/scsi_device//device/timeout

jejb@bedivere:~> ls -l /sys/class/scsi_device/0\:0\:0\:0/device/timeout
-rw-r--r-- 1 root root 4096 Nov 15 14:37 
/sys/class/scsi_device/0:0:0:0/device/timeout
jejb@bedivere:~> cat /sys/class/scsi_device/0\:0\:0\:0/device/timeout
30

You can actually have a udev rule adjust them on a per device id basis.

James



--
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: Seagate External SMR drive USB resets (was: Re: [PATCH] uas: Add US_FL_NO_ATA_1X quirk for one more Seagate device)

[Alan Stern]

On Wed, 15 Nov 2017, Jérôme Carretero wrote:

> (Adding Tejun Heo who was assigned on still-open bugzilla #93581 which
> is about SATA but seems terribly related.)
> 
> On Wed, 15 Nov 2017 16:43:14 -0500
> Jérôme Carretero  wrote:
> 
> > Hi Hans,
> > 
> > 
> > Tests are currently undergoing with drives operating in plain USB mass
> > storage class. In a first time, I'm filling drives with data
> > (uncontrolled corpus, just TBs that I have on hand). It looks like the
> > drives with most usage history are the ones that drop most often.
> > 
> > kernel: usb 3-4.1.1: reset SuperSpeed USB device number 6 using
> > xhci_hcd kernel: usb 3-4.2.1: reset SuperSpeed USB device number 7
> > using xhci_hcd kernel: usb 3-4.3.1.1: reset SuperSpeed USB device
> > number 13 using xhci_hcd kernel: usb 3-4.3.2.1: reset SuperSpeed USB
> > device number 14 using xhci_hcd kernel: usb 3-4.4: reset SuperSpeed
> > USB device number 8 using xhci_hcd kernel: usb 6-4.3.2.1: reset
> > SuperSpeed USB device number 8 using xhci_hcd kernel: usb 6-4.3.3.1:
> > reset SuperSpeed USB device number 9 using xhci_hcd kernel: usb
> > 6-4.4.1: reset SuperSpeed USB device number 6 using xhci_hcd
> > 
> > Will provide some more interesting/visual data later.
> > 
> > 
> > I'm surprised that the message "reset SuperSpeed USB device ..." is
> > displayed without prior information about why.
> > Someone with more background could give hints?

usb-storage resets a drive whenever the SCSI layer tells it to or when
a protocol error occurs.  As far as I know, the SCSI layer issues these
resets only when a command has timed out.

> > I took a look at the USB MSC code and have few questions /
> > observations:
> > 
> > - It looks like (haven't tested it yet) the CONFIG_DYNAMIC_DEBUG isn't
> >   used with the USB mass storage debugging infrastructure, please
> >   confirm? If unused, are we interested to have a patch that would go
> >   back to regular pr_debug() that can work with dynamic debugging?

dev_dbg() please, not pr_debug().  But yes, that would be worthwhile.

Note, however, that usb-storage debugging is meant only for debugging
problems in the usb-storage driver itself, not for debugging problems
in attached devices.  We use usbmon or wireshark for the latter.

> >   Because with several of these drives / lots of activity / occasional
> >   issues, it looks like it will be hard to catch (yes I can use
> > usbmon).
> > 
> > - It looks like there is no configurable timeout for USB MSC requests.
> >   Perhaps the device is not responding in time and this is why it's
> >   reset?

Timeouts are set by the SCSI layer.  I believe they are rather long (30 
seconds, by default).  Presumably they are configurable, although I 
would have to do some digging to figure out how.

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: Seagate External SMR drive USB resets (was: Re: [PATCH] uas: Add US_FL_NO_ATA_1X quirk for one more Seagate device)

[Jérôme Carretero]

(Adding Tejun Heo who was assigned on still-open bugzilla #93581 which
is about SATA but seems terribly related.)

On Wed, 15 Nov 2017 16:43:14 -0500
Jérôme Carretero  wrote:

> Hi Hans,
> 
> 
> Tests are currently undergoing with drives operating in plain USB mass
> storage class. In a first time, I'm filling drives with data
> (uncontrolled corpus, just TBs that I have on hand). It looks like the
> drives with most usage history are the ones that drop most often.
> 
> kernel: usb 3-4.1.1: reset SuperSpeed USB device number 6 using
> xhci_hcd kernel: usb 3-4.2.1: reset SuperSpeed USB device number 7
> using xhci_hcd kernel: usb 3-4.3.1.1: reset SuperSpeed USB device
> number 13 using xhci_hcd kernel: usb 3-4.3.2.1: reset SuperSpeed USB
> device number 14 using xhci_hcd kernel: usb 3-4.4: reset SuperSpeed
> USB device number 8 using xhci_hcd kernel: usb 6-4.3.2.1: reset
> SuperSpeed USB device number 8 using xhci_hcd kernel: usb 6-4.3.3.1:
> reset SuperSpeed USB device number 9 using xhci_hcd kernel: usb
> 6-4.4.1: reset SuperSpeed USB device number 6 using xhci_hcd
> 
> Will provide some more interesting/visual data later.
> 
> 
> I'm surprised that the message "reset SuperSpeed USB device ..." is
> displayed without prior information about why.
> Someone with more background could give hints?
> 
> 
> I took a look at the USB MSC code and have few questions /
> observations:
> 
> - It looks like (haven't tested it yet) the CONFIG_DYNAMIC_DEBUG isn't
>   used with the USB mass storage debugging infrastructure, please
>   confirm? If unused, are we interested to have a patch that would go
>   back to regular pr_debug() that can work with dynamic debugging?
> 
>   Because with several of these drives / lots of activity / occasional
>   issues, it looks like it will be hard to catch (yes I can use
> usbmon).
> 
> - It looks like there is no configurable timeout for USB MSC requests.
>   Perhaps the device is not responding in time and this is why it's
>   reset?
> 
> 
> Best regards,
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Seagate External SMR drive USB resets (was: Re: [PATCH] uas: Add US_FL_NO_ATA_1X quirk for one more Seagate device)

[Jérôme Carretero]

Hi Hans,


Tests are currently undergoing with drives operating in plain USB mass
storage class. In a first time, I'm filling drives with data
(uncontrolled corpus, just TBs that I have on hand). It looks like the
drives with most usage history are the ones that drop most often.

kernel: usb 3-4.1.1: reset SuperSpeed USB device number 6 using xhci_hcd
kernel: usb 3-4.2.1: reset SuperSpeed USB device number 7 using xhci_hcd
kernel: usb 3-4.3.1.1: reset SuperSpeed USB device number 13 using xhci_hcd
kernel: usb 3-4.3.2.1: reset SuperSpeed USB device number 14 using xhci_hcd
kernel: usb 3-4.4: reset SuperSpeed USB device number 8 using xhci_hcd
kernel: usb 6-4.3.2.1: reset SuperSpeed USB device number 8 using xhci_hcd
kernel: usb 6-4.3.3.1: reset SuperSpeed USB device number 9 using xhci_hcd
kernel: usb 6-4.4.1: reset SuperSpeed USB device number 6 using xhci_hcd

Will provide some more interesting/visual data later.


I'm surprised that the message "reset SuperSpeed USB device ..." is
displayed without prior information about why.
Someone with more background could give hints?


I took a look at the USB MSC code and have few questions / observations:

- It looks like (haven't tested it yet) the CONFIG_DYNAMIC_DEBUG isn't
  used with the USB mass storage debugging infrastructure, please
  confirm? If unused, are we interested to have a patch that would go
  back to regular pr_debug() that can work with dynamic debugging?

  Because with several of these drives / lots of activity / occasional
  issues, it looks like it will be hard to catch (yes I can use usbmon).

- It looks like there is no configurable timeout for USB MSC requests.
  Perhaps the device is not responding in time and this is why it's
  reset?


Best regards,

-- 
Jérôme


On Mon, 13 Nov 2017 12:38:14 -0500
Jérôme Carretero  wrote:

> Hi Hans,
> 
> On Mon, 13 Nov 2017 10:04:53 +0100
> Hans de Goede  wrote:
> 
> > Hi,
> > 
> > On 13-11-17 07:14, Jérôme Carretero wrote:  
> > > On Mon, 13 Nov 2017 07:01:30 +0300
> > > Andrey Astafyev <1...@246060.ru> wrote:
> > > 
> > >> 13.11.2017 00:42, Jérôme Carretero пишет:
> > >>> Nov 12 16:20:59 Bidule kernel: sd 22:0:0:0: [sdaa] tag#2
> > >>> uas_eh_abort_handler 0 uas-tag 3 inflight: CMD OUT
> > >>> [...]
> > >>> Do you see such things?  
> 
> > > For my devices, adding US_FL_NO_ATA_1X to unusual_uas.h didn't
> > > change anything, and while adding US_FL_IGNORE_UAS (using
> > > quirks=0bc2:ab34:u,0bc2:ab38:u) there are still device resets,
> > > but they cause shorter hangs in system activity (~1 second when
> > > UAS was more like ~20).
> > 
> > The errors you are seeing are write errors. If you're seeing these
> > errors with both the usb-storage and uas drivers then there likely
> > is something wrong with your setup / hardware.  
> 
> My latest drives are Seagate Backup+ Hub 8TB and have ~ 50 hours of
> uptime. I have connected them to different controllers and they do the
> same as the first generation of the same capacity from 2015.
> 
> SMART says that everything is OK on these disks (I have another that
> was RMA'ed and the symptoms of failure are something else), and if
> there were USB errors, the messages wouldn't be at the higher SCSI
> level, I guess I would see "xact failed" USB errors... no?
> 
> > Does the drive in question use an external power-supply or is it
> > USB bus-powered? If it is the latter then that is likely the
> > problem.  
> 
> External power supply & ~2-ft cable provided by Seagate.
> 
> > Anyways things I would check and try to swap are both the cable
> > used, the power-supply used (if any), the USB-port used as well
> > as trying the disk on a completely different computer.  
> 
> I did that. The same thing happens.
> 
> > I've the feeling something is busted with your hardware, it
> > could be the disk itself. Did you mention that this was the first
> > release of a new higher capacity ? Those often have some kinks
> > which are worked out in later revisions.  
> 
> No, that's about the 3rd release I think.
> 
> 
> I really suspect this has to do with GC activity of these SMR drives,
> as if the write activity is throttled or in more spaced bursts (same
> USB-level intensity), then there is no problem.
> 
> I will do longer tests and see if only some of them do that, after
> they have been subjected to similar usage history.
> 
> 
> Best regards,
> 

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

[PATCH 2/2] musb: remove unused pipe variable

[Corentin Labbe]

This patch fix the following build warning:
drivers/usb/musb/musb_host.c:1809:8: warning: variable 'pipe' set but not used 
[-Wunused-but-set-variable]
by removing the pipe variable in musb_host_rx()

Signed-off-by: Corentin Labbe 
---
 drivers/usb/musb/musb_host.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index aa573ab99384..394b4ac86161 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -1779,7 +1779,6 @@ void musb_host_rx(struct musb *musb, u8 epnum)
struct musb_qh  *qh = hw_ep->in_qh;
size_t  xfer_len;
void __iomem*mbase = musb->mregs;
-   int pipe;
u16 rx_csr, val;
booliso_err = false;
booldone = false;
@@ -1808,8 +1807,6 @@ void musb_host_rx(struct musb *musb, u8 epnum)
return;
}
 
-   pipe = urb->pipe;
-
trace_musb_urb_rx(musb, urb);
 
/* check for errors, concurrent stall & unlink is not really
-- 
2.13.6

--
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/2] musb: remove unused frame variable

[Corentin Labbe]

This patch fix the following warning
drivers/usb/musb/musb_host.c:223:8: warning: variable 'frame' set but not used 
[-Wunused-but-set-variable]
by remove the frame variable in musb_start_urb().

Signed-off-by: Corentin Labbe 
---
 drivers/usb/musb/musb_host.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 2627363fb4fe..aa573ab99384 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -195,7 +195,6 @@ static struct musb_qh *musb_ep_get_qh(struct musb_hw_ep 
*ep, int is_in)
 static void
 musb_start_urb(struct musb *musb, int is_in, struct musb_qh *qh)
 {
-   u16 frame;
u32 len;
void __iomem*mbase =  musb->mregs;
struct urb  *urb = next_urb(qh);
@@ -244,7 +243,6 @@ musb_start_urb(struct musb *musb, int is_in, struct musb_qh 
*qh)
case USB_ENDPOINT_XFER_ISOC:
case USB_ENDPOINT_XFER_INT:
musb_dbg(musb, "check whether there's still time for periodic 
Tx");
-   frame = musb_readw(mbase, MUSB_FRAME);
/* FIXME this doesn't implement that scheduling policy ...
 * or handle framecounter wrapping
 */
-- 
2.13.6

--
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: usbip port number limits

[Shuah Khan]

On 11/15/2017 01:52 AM, Juan Zea wrote:
>> On 11/14/2017 09:25 AM, Juan Zea wrote: 
>>> Hi, 
>>>
>>> I've been working on the issue. This is what I found about multi-controller 
>>> setup: 
>>>
>>> The problem comes from the usbip tool trying to connect usb2 devices to 
>>> usb3 ports, like this: 
>>>
>>> - usbip tool asks vhci driver for free port. 
>>> - If the first port (usb2) is already occupied, vhci answers with usb3 port 
>>> (instead of next controller's usb2 port) 
>>> - usbip tool tries to connect usb2 device to usb3 port and fails 
>>
>> It would be interesting to see how this fails. Can you send me complete 
>> dmesg from client and server for this. 
> 
> I'm attaching two files for this: dmesg-usbip-server.txt and 
> dmesg-usbip-client.txt
> Server is my own machine, and dmesg is too long, so I have selected latest 
> lines. Hope it's enough.
> Client side is a kvm machine I'm using for these tests, so I made a clear 
> boot just for this and its complete.
> 
> 
>>> - usbip tool asks for the next free port, which is still the same usb3 
>>> port. 
>>> - Loop the above forever 
>>
>> That doesn't sound right. 
> 
> I'm also attaching usbip attach output with debug option 
> (usbip-attach-debug.txt). I think it's interesting to see this for the 
> forever loop.
> 
>>>
>>> the code at tools/usb/usbip/libsrc/vhci_driver.c : 
>>> 329 int usbip_vhci_get_free_port(uint32_t speed) 
>>> 330 { 
>>> 331 for (int i = 0; i < vhci_driver->nports; i++) { 
>>> 332 if (speed == USB_SPEED_SUPER && 
>>> 333 vhci_driver->idev[i].hub != HUB_SPEED_SUPER) 
>>> 334 continue; 
>>> 335 
>>> 336 if (vhci_driver->idev[i].status == VDEV_ST_NULL) 
>>> 337 return vhci_driver->idev[i].port; 
>>> 338 } 
>>> 339 
>>> 340 return -1; 
>>> 341 } 
>>>
>>> prevents usb3 devices connecting to usb2 ports, but not the other way 
>>> around. 
>>
>> Connecting usb2 device to usb3 port should work. It would make sense 
>> to prevent usb3 devices connecting to usb2 ports, and not the other 
>> way round. 
>>
> 
> Ok, that sounds right, but then... why having usb2 ports at all? Isn't it a 
> nuisance to maintain code that duplicates the hubs if usb3 ports can do the 
> job?> 

Some systems have both usb2 and usb3 ports. You can't get rid of
usb2 or usb support. More specifically LOW_SPEED, and HIGH_SPEED
support can't be removed. usb2 devices work in usb3 port due to
backwards protocol compatibility. You will see xhci, ehci drivers
in the for this very same reason.

thanks,
-- Shuah
--
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: 8k interrupts/sec with USB hub

[Greg KH]

On Wed, Nov 15, 2017 at 02:58:00PM +, Billy Araujo wrote:
> Hi Greg,
> 
> 1. What kernel version are you using here?
> 
> Sorry about the lack of info. The kernel is: SOCKit: Angstrom v2016.12
> - Kernel 4.1.22-ltsi-altera

Oh wow that is old and obsolete.  Please go get support from the vendor
that is forcing you to use such a thing, you are already paying them for
the support, might as well use it :)

> 2. Why is this an "issue"?  Is the device not working properly?
> 
> It is an issue because it makes the CPU busier and when I have another
> application which needs to run with low latency, these interrupts get
> in the way because I think they have higher priority.

Try the patches that Johan pointed you at, that might help out.  If not,
again, go ask the vendor, they are the best ones to know.

good luck!

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

Re: [PATCH 4/8] dt-bindings: usb: document hub and host-controller properties

[Rob Herring]

On Thu, Nov 09, 2017 at 06:07:19PM +0100, Johan Hovold wrote:
> Hub nodes and host-controller nodes with child nodes must specify values
> for #address-cells (1) and #size-cells (0).
> 
> Also make the definition of the related reg property a bit more
> stringent, and add comments to the example source.
> 
> Signed-off-by: Johan Hovold 
> ---
>  Documentation/devicetree/bindings/usb/usb-device.txt | 20 
> 
>  1 file changed, 16 insertions(+), 4 deletions(-)

I'm happy to apply patches 1-4 for 4.15 if you want.

Rob
--
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/8] dt-bindings: usb: fix example hub node name

[Rob Herring]

On Thu, Nov 09, 2017 at 06:07:16PM +0100, Johan Hovold wrote:
> According to the OF Recommended Practice for USB, hub nodes shall be
> named "hub", but the example had mixed up the label and node names. Fix
> the node name and drop the redundant label.
> 
> While at it, remove a newline and add a missing semicolon to the example
> source.
> 
> Signed-off-by: Johan Hovold 
> ---
>  Documentation/devicetree/bindings/usb/usb-device.txt | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Acked-by: Rob Herring 

--
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: musb: musb_host: Introduce postponed URB giveback

[Matwey V. Kornilov]

The issue is also present in 4.9.60-ti-r75

2017-11-04 17:05 GMT+03:00 Matwey V. Kornilov :
> Hi Bin,
>
> I've just checked that the issue is still present in 4.13.10.
>
> 2017-04-27 13:20 GMT+03:00 Matwey V. Kornilov :
>> This commit changes the order of actions undertaken in
>> musb_advance_schedule() in order to overcome issue with broken
>> isochronous transfer [1].
>>
>> There is no harm to split musb_giveback into two pieces.  The first
>> unlinks finished urb, the second givebacks it. The issue here that
>> givebacking may be quite time-consuming due to urb->complete() call.
>> As it happens in case of pwc-driven web cameras. It may take about 0.5
>> ms to call __musb_giveback() that calls urb->callback() internally.
>> Under specific circumstances setting MUSB_RXCSR_H_REQPKT in subsequent
>> musb_start_urb() for the next urb will be too late to produce physical
>> IN packet. Since auto req is not used by this module the exchange
>> would be as the following:
>>
>> []   7.220456 d=  0.000997 [182   +  3.667] [  3] IN   : 4.5
>> [T   ]   7.220459 d=  0.03 [182   +  7.000] [800] DATA0: [skipped]
>> []   7.222456 d=  0.001997 [184   +  3.667] [  3] IN   : 4.5
>> []   7.222459 d=  0.03 [184   +  7.000] [  3] DATA0: 00 00
>>
>> It is known that missed IN in isochronous mode makes some
>> perepherial broken. For instance, pwc-driven or uvc-driven
>> web cameras.
>> In order to workaround this issue we postpone calling
>> urb->callback() after setting MUSB_RXCSR_H_REQPKT for the
>> next urb if there is the next urb pending in queue.
>>
>> [1] https://www.spinics.net/lists/linux-usb/msg145747.html
>>
>> Fixes: f551e1352983 ("Revert "usb: musb: musb_host: Enable HCD_BH flag to 
>> handle urb return in bottom half"")
>> Signed-off-by: Matwey V. Kornilov 
>> ---
>>  drivers/usb/musb/musb_host.c | 54 
>> +---
>>  1 file changed, 46 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
>> index ac3a4952abb4..b590c2555dab 100644
>> --- a/drivers/usb/musb/musb_host.c
>> +++ b/drivers/usb/musb/musb_host.c
>> @@ -299,19 +299,24 @@ musb_start_urb(struct musb *musb, int is_in, struct 
>> musb_qh *qh)
>> }
>>  }
>>
>> -/* Context: caller owns controller lock, IRQs are blocked */
>> -static void musb_giveback(struct musb *musb, struct urb *urb, int status)
>> +static void __musb_giveback(struct musb *musb, struct urb *urb, int status)
>>  __releases(musb->lock)
>>  __acquires(musb->lock)
>>  {
>> -   trace_musb_urb_gb(musb, urb);
>> -
>> -   usb_hcd_unlink_urb_from_ep(musb->hcd, urb);
>> spin_unlock(>lock);
>> usb_hcd_giveback_urb(musb->hcd, urb, status);
>> spin_lock(>lock);
>>  }
>>
>> +/* Context: caller owns controller lock, IRQs are blocked */
>> +static void musb_giveback(struct musb *musb, struct urb *urb, int status)
>> +{
>> +   trace_musb_urb_gb(musb, urb);
>> +
>> +   usb_hcd_unlink_urb_from_ep(musb->hcd, urb);
>> +   __musb_giveback(musb, urb, status);
>> +}
>> +
>>  /* For bulk/interrupt endpoints only */
>>  static inline void musb_save_toggle(struct musb_qh *qh, int is_in,
>> struct urb *urb)
>> @@ -346,6 +351,7 @@ static void musb_advance_schedule(struct musb *musb, 
>> struct urb *urb,
>> struct musb_hw_ep   *ep = qh->hw_ep;
>> int ready = qh->is_ready;
>> int status;
>> +   int postponed_giveback = 0;
>>
>> status = (urb->status == -EINPROGRESS) ? 0 : urb->status;
>>
>> @@ -361,9 +367,35 @@ static void musb_advance_schedule(struct musb *musb, 
>> struct urb *urb,
>> break;
>> }
>>
>> -   qh->is_ready = 0;
>> -   musb_giveback(musb, urb, status);
>> -   qh->is_ready = ready;
>> +   usb_hcd_unlink_urb_from_ep(musb->hcd, urb);
>> +
>> +   /* It may take about 0.5 ms to call __musb_giveback() that
>> +* calls urb->callback() internally. Under specific circumstances
>> +* setting MUSB_RXCSR_H_REQPKT in subsequent musb_start_urb() for the
>> +* next urb will be too late to produce physical IN packet. Since
>> +* auto req is not used by this module the exchange would be as the
>> +* following:
>> +*
>> +* []   7.220456 d=  0.000997 [182   +  3.667] [  3] IN   : 
>> 4.5
>> +* [T   ]   7.220459 d=  0.03 [182   +  7.000] [800] DATA0: 
>> [skipped]
>> +* []   7.222456 d=  0.001997 [184   +  3.667] [  3] IN   : 
>> 4.5
>> +* []   7.222459 d=  0.03 [184   +  7.000] [  3] DATA0: 
>> 00 00
>> +*
>> +* It is known that missed IN in isochronous mode makes some
>> +* perepherial broken. For instance, pwc-driven or uvc-driven
>> +* web cameras.
>> +* In 

Re: 8k interrupts/sec with USB hub

[Billy Araujo]

Hi Johan,


Thanks for your answer. That seems a lot like what I am seeing because
I had a USB tracer and it showed many NAKed transactions.


Regards,

Billy.










On Wed, Nov 15, 2017 at 2:45 PM, Johan Hovold  wrote:
> On Wed, Nov 15, 2017 at 12:12:16PM +, Billy Araujo wrote:
>> Hi all,
>>
>> I built a kernel/rootfs for altera SoCkit amd uses the default SoCkit
>> device tree and I plug when in plug in a USB stick it uses the dwc2
>> driver.
>> When doing cat /proc/interrupts I get normal amount of interrupts -
>> everything seems ok.
>>
>> However, when I connect the USB stick to a USB hub and then to the
>> same USB port I get a great amount of interrupts approx. 8000 per
>> second. Anyone know why this different behaviour?
>
>> I have seen several threads with stating this issue but haven't found
>> a clear answer.
>
> This sounds familiar, take a look at:
>
> https://lkml.kernel.org/r/20171030170802.14489-1-diand...@chromium.org
>
> Johan
--
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: usbip port number limits

[Shuah Khan]

Hi Juan,

On 11/15/2017 07:43 AM, Juan Zea wrote:
> 
>>> Also, will you be able to revert the usb3 commit 
>>> 1c9de5bf428612458427943b724bea51abde520a 
>>>
>>> and see if any of the problems go away. 
>>>
>>> thanks, 
>>> -- Shuah 
>>>
> 
>> I'm on it and will send results later.
> 
>> Thanks,
>> Juan
> 
> Ok, I'm back. The revert was quite complex, with several conflicts I was not 
> able to resolve. So I started testing full checkouts around that series of 
> changes by Yuyang.

I was hoping it will be easier. I was apprehensive it could be a com[ex revert. 
:(

> That led me to bisecting the problem with the fingerprint reader, and the 
> culprit is here: 
> 
> 03cd00d538a6feb0492cd153edf256ef7d7bd95e is the first bad commit
> commit 03cd00d538a6feb0492cd153edf256ef7d7bd95e
> Author: Yuyang Du 
> Date:   Thu Jun 8 13:04:09 2017 +0800
> 
> usbip: vhci-hcd: Set the vhci structure up to work
> 
> This patch enables the new vhci structure. Its lock protects
> both the USB2 hub and the shared USB3 hub.
> 
> Signed-off-by: Yuyang Du 
> Acked-by: Shuah Khan 
> Signed-off-by: Greg Kroah-Hartman 
> 
> :04 04 b7b5c6b16db801c74354bb0d0a247855f64b0829 
> 72524e78d281ebc8d36fa32cb93ed0278e99f880 M  drivers
> 
> The commit before, fingerprint reader works. Also, multicontroller works (no 
> usb3 ports). 
> In this bad commit, I get the errors I sent in the previous message.
> 

Thanks for reporting and debugging the problem to isolate the commit. I was
suspecting one of these commit based on the messages you are seeing.

I will see if I can fix this without doing a huge reverts.

thanks,
-- Shuah
--
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: 8k interrupts/sec with USB hub

[Billy Araujo]

Hi Greg,

1. What kernel version are you using here?

Sorry about the lack of info. The kernel is: SOCKit: Angstrom v2016.12
- Kernel 4.1.22-ltsi-altera

2. Why is this an "issue"?  Is the device not working properly?

It is an issue because it makes the CPU busier and when I have another
application which needs to run with low latency, these interrupts get
in the way because I think they have higher priority.

I don't know whose fault it is, hardware, driver or kernel, I just was
wondering why it behaves different with the USB hub connected.

Regards,

Billy.





On Wed, Nov 15, 2017 at 1:58 PM, Greg KH  wrote:
> On Wed, Nov 15, 2017 at 12:12:16PM +, Billy Araujo wrote:
>> Hi all,
>>
>> I built a kernel/rootfs for altera SoCkit amd uses the default SoCkit
>> device tree and I plug when in plug in a USB stick it uses the dwc2
>> driver.
>
> What kernel version are you using here?
>
>> When doing cat /proc/interrupts I get normal amount of interrupts -
>> everything seems ok.
>>
>> However, when I connect the USB stick to a USB hub and then to the
>> same USB port I get a great amount of interrupts approx. 8000 per
>> second. Anyone know why this different behaviour?
>>
>> root@cyclone5:~# cat /proc/interrupts
>>
>>CPU0
>>
>> 16:  53922   GIC  29 Edge  twd
>>
>> 17:  0   GIC 199 Level timer0
>>
>> 18:  0   GIC 136 Level ffe01000.pdma
>>
>> 26:  0   GIC 190 Level ffc04000.i2c
>>
>> 27:  0   GIC 191 Level ffc05000.i2c
>>
>> 29:  24718   GIC 171 Level dw-mci
>>
>> 40:   1138   GIC 194 Level serial
>>
>> 41:3946968   GIC 160 Level ffb4.usb, ffb4.usb,
>> dwc2_hsotg:usb1
>>
>>
>>
>> root@cyclone5:~# [  651.854021] usb 1-1: new high-speed USB device
>> number 7 using dwc2
>>
>> [  652.065190] usb 1-1: New USB device found, idVendor=05e3, idProduct=0608
>>
>> [  652.071869] usb 1-1: New USB device strings: Mfr=0, Product=1, 
>> SerialNumber=0
>>
>> [  652.079007] usb 1-1: Product: USB2.0 Hub
>>
>> [  652.088468] hub 1-1:1.0: USB hub found
>>
>> [  652.092830] hub 1-1:1.0: 4 ports detected
>>
>> [  652.374026] usb 1-1.1: new high-speed USB device number 8 using dwc2
>>
>> [  652.475473] usb 1-1.1: New USB device found, idVendor=05e3, idProduct=0608
>>
>> [  652.482331] usb 1-1.1: New USB device strings: Mfr=0, Product=1,
>> SerialNumber=0
>>
>> [  652.489639] usb 1-1.1: Product: USB2.0 Hub
>>
>> [  652.499214] hub 1-1.1:1.0: USB hub found
>>
>> [  652.503762] hub 1-1.1:1.0: 4 ports detected
>>
>>
>> I have seen several threads with stating this issue but haven't found
>> a clear answer.
>
> Why is this an "issue"?  Is the device not working properly?  Is the
> device not seen correctly?  USB is a "constantly asking the device for
> data" type of protocol, lots of interrupts is a normal thing.
>
> Heck, we know of some machines where you plug a USB keyboard into them
> and it starts to take 30% of the CPU time up just to handle the
> interrupts.  That's not USB's fault, or the kernels, it's the horrible
> hardware implementation that the board has on it, where it is up to the
> CPU to do most of the work.  Odds are that is what is happening here
> with your platform.
>
> thanks,
>
> greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 8k interrupts/sec with USB hub

[Johan Hovold]

On Wed, Nov 15, 2017 at 12:12:16PM +, Billy Araujo wrote:
> Hi all,
> 
> I built a kernel/rootfs for altera SoCkit amd uses the default SoCkit
> device tree and I plug when in plug in a USB stick it uses the dwc2
> driver.
> When doing cat /proc/interrupts I get normal amount of interrupts -
> everything seems ok.
> 
> However, when I connect the USB stick to a USB hub and then to the
> same USB port I get a great amount of interrupts approx. 8000 per
> second. Anyone know why this different behaviour?

> I have seen several threads with stating this issue but haven't found
> a clear answer.

This sounds familiar, take a look at:

https://lkml.kernel.org/r/20171030170802.14489-1-diand...@chromium.org

Johan
--
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: usbip port number limits

[Juan Zea]

>>Also, will you be able to revert the usb3 commit 
>>1c9de5bf428612458427943b724bea51abde520a 
>>
>>and see if any of the problems go away. 
>>
>>thanks, 
>>-- Shuah 
>>

>I'm on it and will send results later.

>Thanks,
>Juan

Ok, I'm back. The revert was quite complex, with several conflicts I was not 
able to resolve. So I started testing full checkouts around that series of 
changes by Yuyang.
That led me to bisecting the problem with the fingerprint reader, and the 
culprit is here: 

03cd00d538a6feb0492cd153edf256ef7d7bd95e is the first bad commit
commit 03cd00d538a6feb0492cd153edf256ef7d7bd95e
Author: Yuyang Du 
Date:   Thu Jun 8 13:04:09 2017 +0800

usbip: vhci-hcd: Set the vhci structure up to work

This patch enables the new vhci structure. Its lock protects
both the USB2 hub and the shared USB3 hub.

Signed-off-by: Yuyang Du 
Acked-by: Shuah Khan 
Signed-off-by: Greg Kroah-Hartman 

:04 04 b7b5c6b16db801c74354bb0d0a247855f64b0829 
72524e78d281ebc8d36fa32cb93ed0278e99f880 M  drivers

The commit before, fingerprint reader works. Also, multicontroller works (no 
usb3 ports). 
In this bad commit, I get the errors I sent in the previous message.

Regards,
Juan

Juan Antonio Zea Herranz 
Proyectos y consultoría | Qindel Group 
E: juan@qindel.com 
T: +34 91 766 24 21 
M: +34 637 74 63 09 
W: qindel.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] Re: xhci bandwidth problem with isochronous endpoints

[Mathias Nyman]

On 14.11.2017 21:16, Curt Meyers wrote:
...


Hi Mathias,

The manufacture hired a Linux developer to solve the problem with this 
isochronous camera device and it turns out to be related to LPM that only shows 
up in newer Intel hardware, according to the developer. He says that there is 
something wrong with the MEL (Max Exit Latency) and skipping it in the call to 
xhci_configure_endpoint() fixes the problem, or works around it.



...



I just received this updated patch, it adds a call to spin_unlock_irqrestore() 
and a comment for the code.

The comment says that LPM with isochronous endpoints causes config_EP to fail.

---

diff -Naur host.ori/xhci.c host/xhci.c
--- host.ori/xhci.c 2017-11-02 01:50:04.544937000 +0800
+++ host/xhci.c 2017-11-08 01:40:38.929681000 +0800
@@ -4001,6 +4001,20 @@
    return 0;
  }

+ // Workaround: Enable LPM when a SS ISOC EP is opened will cause all 
subsequent config_EP fail
+ if ((xhci->quirks & XHCI_SET_MEL_STUCK) && (USB_SPEED_SUPER == udev->speed) && 
(max_exit_latency > 0))
+ {
+   int i;
+   for (i = 0; i < 31; i++) {
+ struct xhci_ep_ctx *ep_ctx = xhci_get_ep_ctx(xhci, virt_dev->out_ctx, i);
+ int ep_type = CTX_TO_EP_TYPE(le32_to_cpu(ep_ctx->ep_info2));
+ if ((ISOC_IN_EP == ep_type) || (ISOC_OUT_EP == ep_type)) {
+   spin_unlock_irqrestore(>lock, flags);
+   return 0;
+ }
+   }
+ }
+
  /* Attempt to issue an Evaluate Context command to change the MEL. */
  command = xhci->lpm_command;
  ctrl_ctx = xhci_get_input_control_ctx(command->in_ctx);
diff -Naur host.ori/xhci.h host/xhci.h
--- host.ori/xhci.h 2017-11-02 01:50:04.544937000 +0800
+++ host/xhci.h 2017-11-02 23:41:17.0 +0800
@@ -1650,6 +1650,8 @@
 #define XHCI_SSIC_PORT_UNUSED  (1 << 22)
 #define XHCI_NO_64BIT_SUPPORT  (1 << 23)
 #define XHCI_MISSING_CAS (1 << 24)
+#define XHCI_SET_MEL_STUCK (1 << 31)
+
  unsigned int    num_active_eps;
  unsigned int    limit_active_eps;
  /* There are two roothubs to keep track of bus suspend info for */
diff -Naur host.ori/xhci-pci.c host/xhci-pci.c
--- host.ori/xhci-pci.c 2017-11-02 01:50:04.536937000 +0800
+++ host/xhci-pci.c 2017-11-08 01:40:38.929681000 +0800
@@ -168,6 +168,7 @@
 pdev->device == PCI_DEVICE_ID_INTEL_BROXTON_B_XHCI ||
 pdev->device == PCI_DEVICE_ID_INTEL_APL_XHCI)) {
    xhci->quirks |= XHCI_PME_STUCK_QUIRK;
+   xhci->quirks |= XHCI_SET_MEL_STUCK;
  }
  if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
 pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI) {

---


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

Hi Mathias,

Not sure if you saw my second diff, it is slightly different than the first. 
Since I am going between the developer and the maintainers is there something I 
can do to move this investigation along?

This patch provides a fix for the serious problem we have found with USB3 
isochronous devices running on newer Intel hardware.

Perhaps you or someone else at Intel that is very familiar with the USB 
hardware can better understand the implications of this patch.

Thanks,
Curt



Hi,

This code basically prevents the "evaluate context" from setting any MEL value 
(other than 0).
But it still enables LPM for these devices, which increases the "wakeup" 
latency time for the link.
Now the code just doesn't tell the xHC controller that the Maximum Exit Latency 
(MEL) is increased

Any chance I could get a direct contact to this developer?

Thanks
Mathias  
--

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

Re: 8k interrupts/sec with USB hub

[Greg KH]

On Wed, Nov 15, 2017 at 12:12:16PM +, Billy Araujo wrote:
> Hi all,
> 
> I built a kernel/rootfs for altera SoCkit amd uses the default SoCkit
> device tree and I plug when in plug in a USB stick it uses the dwc2
> driver.

What kernel version are you using here?

> When doing cat /proc/interrupts I get normal amount of interrupts -
> everything seems ok.
> 
> However, when I connect the USB stick to a USB hub and then to the
> same USB port I get a great amount of interrupts approx. 8000 per
> second. Anyone know why this different behaviour?
> 
> root@cyclone5:~# cat /proc/interrupts
> 
>CPU0
> 
> 16:  53922   GIC  29 Edge  twd
> 
> 17:  0   GIC 199 Level timer0
> 
> 18:  0   GIC 136 Level ffe01000.pdma
> 
> 26:  0   GIC 190 Level ffc04000.i2c
> 
> 27:  0   GIC 191 Level ffc05000.i2c
> 
> 29:  24718   GIC 171 Level dw-mci
> 
> 40:   1138   GIC 194 Level serial
> 
> 41:3946968   GIC 160 Level ffb4.usb, ffb4.usb,
> dwc2_hsotg:usb1
> 
> 
> 
> root@cyclone5:~# [  651.854021] usb 1-1: new high-speed USB device
> number 7 using dwc2
> 
> [  652.065190] usb 1-1: New USB device found, idVendor=05e3, idProduct=0608
> 
> [  652.071869] usb 1-1: New USB device strings: Mfr=0, Product=1, 
> SerialNumber=0
> 
> [  652.079007] usb 1-1: Product: USB2.0 Hub
> 
> [  652.088468] hub 1-1:1.0: USB hub found
> 
> [  652.092830] hub 1-1:1.0: 4 ports detected
> 
> [  652.374026] usb 1-1.1: new high-speed USB device number 8 using dwc2
> 
> [  652.475473] usb 1-1.1: New USB device found, idVendor=05e3, idProduct=0608
> 
> [  652.482331] usb 1-1.1: New USB device strings: Mfr=0, Product=1,
> SerialNumber=0
> 
> [  652.489639] usb 1-1.1: Product: USB2.0 Hub
> 
> [  652.499214] hub 1-1.1:1.0: USB hub found
> 
> [  652.503762] hub 1-1.1:1.0: 4 ports detected
> 
> 
> I have seen several threads with stating this issue but haven't found
> a clear answer.

Why is this an "issue"?  Is the device not working properly?  Is the
device not seen correctly?  USB is a "constantly asking the device for
data" type of protocol, lots of interrupts is a normal thing.

Heck, we know of some machines where you plug a USB keyboard into them
and it starts to take 30% of the CPU time up just to handle the
interrupts.  That's not USB's fault, or the kernels, it's the horrible
hardware implementation that the board has on it, where it is up to the
CPU to do most of the work.  Odds are that is what is happening here
with your platform.

thanks,

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

8k interrupts/sec with USB hub

[Billy Araujo]

Hi all,

I built a kernel/rootfs for altera SoCkit amd uses the default SoCkit
device tree and I plug when in plug in a USB stick it uses the dwc2
driver.
When doing cat /proc/interrupts I get normal amount of interrupts -
everything seems ok.

However, when I connect the USB stick to a USB hub and then to the
same USB port I get a great amount of interrupts approx. 8000 per
second. Anyone know why this different behaviour?

root@cyclone5:~# cat /proc/interrupts

   CPU0

16:  53922   GIC  29 Edge  twd

17:  0   GIC 199 Level timer0

18:  0   GIC 136 Level ffe01000.pdma

26:  0   GIC 190 Level ffc04000.i2c

27:  0   GIC 191 Level ffc05000.i2c

29:  24718   GIC 171 Level dw-mci

40:   1138   GIC 194 Level serial

41:3946968   GIC 160 Level ffb4.usb, ffb4.usb,
dwc2_hsotg:usb1



root@cyclone5:~# [  651.854021] usb 1-1: new high-speed USB device
number 7 using dwc2

[  652.065190] usb 1-1: New USB device found, idVendor=05e3, idProduct=0608

[  652.071869] usb 1-1: New USB device strings: Mfr=0, Product=1, SerialNumber=0

[  652.079007] usb 1-1: Product: USB2.0 Hub

[  652.088468] hub 1-1:1.0: USB hub found

[  652.092830] hub 1-1:1.0: 4 ports detected

[  652.374026] usb 1-1.1: new high-speed USB device number 8 using dwc2

[  652.475473] usb 1-1.1: New USB device found, idVendor=05e3, idProduct=0608

[  652.482331] usb 1-1.1: New USB device strings: Mfr=0, Product=1,
SerialNumber=0

[  652.489639] usb 1-1.1: Product: USB2.0 Hub

[  652.499214] hub 1-1.1:1.0: USB hub found

[  652.503762] hub 1-1.1:1.0: 4 ports detected


I have seen several threads with stating this issue but haven't found
a clear answer.


Regards,

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

Hello Dear...

[M,Shakour Rosarita]

Hello Dear...

I know that this message will come to you as a surprise. I hoped that
you will not expose or betray this trust and confident that I am about
to repose on you, my name is M, Shakour Rosarita. I am 19 years old
Girl, female, from Tartu Syria, (never married) 61 kg, white in
complexion, and I am currently living in the refugee camp here in
Abidjan Ivory Coast, My late beloved father M,Shakour Hassin was a
renowned businessman and owner of Natour Petrol Station in Syria, he
was killed in a stampede riot in Tartu, Syria.
When I got the news about my parents. I fled to a nearby country
Jordan before I joined a ferry to Africa and came to Abidjan capital
city Ivory Coast West Africa find safety here.
I came in 2015 to Abidjan and I now live in refugee camps here as
refugees are allowed freely to enter here without, My late father
deposited (US$6.200.000.00m) My late father kept the money at the bank
of Africa, I want you to help me transfer the money to your hand so
that you will help me bring me into your country for my continue
education.

I sent my pictures here for you to see. Who I am seriously looking for
a good-person in my life, so I want to hear from you soon and learn
more about you.

I am an open-minded and friendly girl to share a good time with you
and have fun and enjoy on the go, bird watching, the rest of our
lives. My Hobbies, tourism books, dance, music, soccer, tennis,
swimming and cinema.
I would just think about you, including your dose and doesn’t .I
believe we will do well together, and live like one family.
Thank you and God bless you, for you in your promise to help me here,
looking forward to your reply by the grace of God and have a good day.
I hope you send me your photos as well? I await your next reply in
faith please reply through my private email at
(mshakourrosarit...@gmail.com):
Thanks.
Ms Rosarita
--
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 net,stable] net: cdc_ncm: GetNtbFormat endian fix

[David Miller]

From: Bjørn Mork 
Date: Wed, 15 Nov 2017 09:35:02 +0100

> The GetNtbFormat and SetNtbFormat requests operate on 16 bit little
> endian values. We get away with ignoring this most of the time, because
> we only care about USB_CDC_NCM_NTB16_FORMAT which is 0x.  This
> fails for USB_CDC_NCM_NTB32_FORMAT.
> 
> Fix comparison between LE value from device and constant by converting
> the constant to LE.
> 
> Reported-by: Ben Hutchings 
> Fixes: 2b02c20ce0c2 ("cdc_ncm: Set NTB format again after altsetting switch 
> for Huawei devices")
> Cc: Enrico Mioso 
> Cc: Christian Panton 
> Signed-off-by: Bjørn Mork 

Applied and queued up for -stable, 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

Re: [PATCH net,stable] net: cdc_ncm: GetNtbFormat endian fix

[Enrico Mioso]

Thank you very much Ben and bjorn, for the spot, the fix, and overall helping 
me learn new things.

Acked-By: Enrico Mioso 



On Wed, 15 Nov 2017, Bjørn Mork wrote:


Date: Wed, 15 Nov 2017 09:35:02
From: Bjørn Mork 
To: net...@vger.kernel.org
Cc: Oliver Neukum ,
Ben Hutchings , linux-usb@vger.kernel.org,
Bjørn Mork , Enrico Mioso ,
Christian Panton 
Subject: [PATCH net,stable] net: cdc_ncm: GetNtbFormat endian fix

The GetNtbFormat and SetNtbFormat requests operate on 16 bit little
endian values. We get away with ignoring this most of the time, because
we only care about USB_CDC_NCM_NTB16_FORMAT which is 0x.  This
fails for USB_CDC_NCM_NTB32_FORMAT.

Fix comparison between LE value from device and constant by converting
the constant to LE.

Reported-by: Ben Hutchings 
Fixes: 2b02c20ce0c2 ("cdc_ncm: Set NTB format again after altsetting switch for 
Huawei devices")
Cc: Enrico Mioso 
Cc: Christian Panton 
Signed-off-by: Bjørn Mork 
---
drivers/net/usb/cdc_ncm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 47cab1bde065..9e1b74590682 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -771,7 +771,7 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct 
usb_interface *intf, u8 data_
int err;
u8 iface_no;
struct usb_cdc_parsed_header hdr;
-   u16 curr_ntb_format;
+   __le16 curr_ntb_format;

ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
if (!ctx)
@@ -889,7 +889,7 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct 
usb_interface *intf, u8 data_
goto error2;
}

-   if (curr_ntb_format == USB_CDC_NCM_NTB32_FORMAT) {
+   if (curr_ntb_format == cpu_to_le16(USB_CDC_NCM_NTB32_FORMAT)) {
dev_info(>dev, "resetting NTB format to 16-bit");
err = usbnet_write_cmd(dev, USB_CDC_SET_NTB_FORMAT,
   USB_TYPE_CLASS | USB_DIR_OUT
--
2.11.0

RE: [PATCH] usb: dwc3: Enable the USB snooping

[Felipe Balbi]

Hi,

Ran Wang  writes:
>> Ran Wang  writes:
>> > Add support for USB3 snooping by asserting bits in register
>> > DWC3_GSBUSCFG0 for data and descriptor.
>> 
>> we know *how* to enable a feature :-) It's always the same, you fiddle with
>> some registers and it works. What you failed to tell us is:
>> 
>> a) WHY do you need this?
>> b) WHY do we need another DT property for this?
>> c) WHAT does this mean for PCI devices?
>
> So far I cannot have the answer for you, will get you back after some 
> discussion
> with my colleagues.

IOW, you have no idea why you need this, right? We're not patching
things for the sake of patching things. We need to understand what these
changes mean to the HW before we send out a patch publicly.

Remember that the moment a patch like this is accepted, it has the
potential of changing behavior for *ALL* users.

>> > +  }
>> > +
>> > +  dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, cfg);
>> 
>> this will *always* read and write GSBUSCFG0 even for those platforms which
>> don't need to change anything on this register. You should just bail out 
>> early
>> if !dwc->dma_coherent
>> 
>> Also, I think dma_coherent is likely not the best name for this property.
>> 
>> Another question is: Why wasn't this setup properly during coreConsultant
>> instantiation of the RTL? Do you have devices on the market already that
>> need this or is this some early FPGA model or test-only ASIC?
>
> Yes, you are right. Actually I thought that all dwc3 IP  will have this 
> register, and
> it can be controlled by DTS property. 

they all *have* the register, however, it's sort of expected that RTL
engineer will setup good defaults when instantiating the RTL using SNPS'
coreConsultant tool.

Does your platform work without this patch?

>> > +
>> >  /* Global Debug Queue/FIFO Space Available Register */
>> >  #define DWC3_GDBGFIFOSPACE_NUM(n) ((n) & 0x1f)
>> >  #define DWC3_GDBGFIFOSPACE_TYPE(n)(((n) << 5) & 0x1e0)
>> > @@ -859,6 +867,7 @@ struct dwc3_scratchpad_array {
>> >   *3   - Reserved
>> >   * @imod_interval: set the interrupt moderation interval in 250ns
>> >   * increments or 0 to disable.
>> > + * @dma_coherent: set if enable dma-coherent.
>> 
>> you're not enabling dma coherency, you're enabling cache snooping. And
>> this property should describe that. Also, keep in mind that different devices
>> may want different cache types for each of those fields, so your property
>> would have to be a lot more complex. Something like:
>> 
>>  snps,cache-type = , , ...
>> 
>> Then driver would have to parse this properly to setup GSBUSCFG0.
>
> Got it, learn a lot, need more time to digest and test, thanks for
> your patiently explanation.

no problem, please figure out the answers to my previous questions,
without which I can't accept your patch.

>> In any
>> case, I still want to know why do you really need this? What's the reason?
>> What happens if you don't fix GSBUSCFG0? What's the value you have there
>> by default? Why isn't that default good enough?
>
> So far the Layerscape SoC (such as LS1088A) has enabled this feature and I
> have tested it. Once we add dma-coherent on DTS without this Patch, dwc3
> will fail on device enumeration as below:
> [   15.124031] xhci-hcd xhci-hcd.0.auto: Error while assigning device slot ID
> [   15.130912] xhci-hcd xhci-hcd.0.auto: Max number of devices this xHCI host 
> supports is 127.
> [   15.139268] usb usb1-port1: couldn't allocate usb_device

okay, so without these changes, your host doesn't work. What is the
default value on your platform without these changes? (revert patch,
boot platform, let it fail, get register output from our regdump in debugfs)

-- 
balbi
--
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: "hub doesn't have any ports" error message on the disabled USB addressable root hub port

[Thang Q. Nguyen]

On Wed, Nov 1, 2017 at 4:17 PM, Thang Q. Nguyen  wrote:
> Hi,
>
> On Mon, Oct 23, 2017 at 5:27 PM, Mathias Nyman
>  wrote:
>> On 23.10.2017 12:37, Thang Q. Nguyen wrote:
>>>
>>> Hi,
>>> In our latest ARM64-based CPU, we use the DesignWare USB which is
>>> xHCI-compatible. For some reasons, we disable USB3.0 support so remove the
>>> unnecessary USB3.0 capability structure information as specified in the
>>> xHCI specification 1.1, section 7.2:
>>> "At least one of these capability structures is required for all xHCI
>>> implementations. More than one may be defined for implementations that
>>> support more that one bus protocol."
>>>
>>
>> So there is no supported protocol capability with revision major == 3, in
>> your case.
>>
>>
>>> When booting kernel, linux kernel displays an error message to claim about
>>> no USB port on the USB3.0 roothub:
>>> [3.497767] hub 2-0:1.0: config failed, hub doesn't have any ports!
>>> (err -19)
>>> Although the error message does not affect USB functionality, error
>>> message will make users confused.
>>>
>>> Looking into the XHCI driver, there are always two USB hub ports
>>> implemented. This implements the xHCI specification as in 4.19.7:
>>> "In a USB3 hub, two independently addressable hub ports exist for each
>>> physical down stream connector; a USB2 compatible port accessed through
>>> the USB2 connection and a USB3 compatible port accessed through the
>>> SuperSpeed connection. The Root Hub of the xHCI emulates this operation by
>>> defining a Root Hub PORTSC register for each connection type; USB2
>>> (Low-/Full-/High-Speed) or USB3 (SuperSpeed)."
>>>
>>> But the problem is that, xhci-hcd always pass both USB hub ports to
>>> usb-core driver. In case any of USB2 or USB3 compatible port is missing,
>>> there is no port on the corresponding hub and kernel will displays the
>>> error message.
>>>
>>> Below are some approaches we suggest to overcome the issue:
>>> Approach 1: Allow roothub has no port. The change will look like:
>>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>>> index b5c7336..995d62d 100644
>>> --- a/drivers/usb/core/hub.c
>>> +++ b/drivers/usb/core/hub.c
>>> @@ -1347,6 +1347,9 @@ static int hub_configure(struct usb_hub
>>> *hub,
>>> ret = -ENODEV;
>>> goto fail;
>>> } else if (hub->descriptor->bNbrPorts == 0) {
>>> +   if (hdev->parent)
>>> +   return -ENODEV;
>>> +
>>> message = "hub doesn't have any
>>> ports!";
>>> ret = -ENODEV;
>>> goto fail;
>>> Approach 2: do not pass a USB hub port to usb-core if it has no port.
>>> Don't know if this approach is feasible or not.
>>> Approach 3: implement maximum-speed attribute support and this will be set
>>> to high-speed in case of missing USB3 support. But this works only for the
>>> case of USB3.0 disabled.
>>>
>>> Can you help review and suggest what approach should be used to fix this
>>> issue?
>>
>>
>> Current xhci driver design assumes there are two hcds available.
>> Several functions assume a shared hcd exists.
>>
>> Creating a hcd with a roothub with zero ports is probably a faster way to
>> get
>> things running, but I think we should redesign parts of the xhci driver to
>> work with only one hcd.
>>
>> So best would be to never add the secodary hcd if there are no USB 3 ports,
>> meaning no:
>>   xhci->shared_hcd = usb_create_shared_hcd(..);
>>   usb_add_hcd(xhci->shared_hcd, dev->irq,
> Below code is to not add shared_hcd if no USB 3 port:
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 1cb6eae..0881fb5 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -285,12 +285,14 @@ static int xhci_plat_probe(struct platform_device *pdev)
> if (ret)
> goto disable_usb_phy;
>
> -   if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
> -   xhci->shared_hcd->can_do_streams = 1;
> +   if (xhci->num_usb3_ports > 0) {
> +   if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
> +   xhci->shared_hcd->can_do_streams = 1;
>
> -   ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
> -   if (ret)
> -   goto dealloc_usb2_hcd;
> +   ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
> +   if (ret)
> +   goto dealloc_usb2_hcd;
> +   }
>
> device_enable_async_suspend(>dev);
> pm_runtime_put_noidle(>dev);
>
> Then, the xhci_run should execute xhci_start() in case no USB 3 port
> (xhci->shared_hcd->run() will execute xhci_run_finished() which run
> xhci_start().
> diff --git a/drivers/usb/host/xhci.c 

RE: [PATCH] usb: dwc3: Enable the USB snooping

[Ran Wang]

Hi Balbi,

> -Original Message-
> From: Felipe Balbi [mailto:ba...@kernel.org]
> Sent: Wednesday, November 15, 2017 4:52 PM
> To: Ran Wang 
> Cc: Greg Kroah-Hartman ; open
> list:DESIGNWARE USB3 DRD IP DRIVER ; open
> list ; Jerry Huang ;
> Rajesh Bhagat ; Leo Li ; Ran
> Wang ; Rob Herring ;
> devicet...@vger.kernel.org
> Subject: Re: [PATCH] usb: dwc3: Enable the USB snooping
> 
> 
> Hi,
> 
> Ran Wang  writes:
> > Add support for USB3 snooping by asserting bits in register
> > DWC3_GSBUSCFG0 for data and descriptor.
> 
> we know *how* to enable a feature :-) It's always the same, you fiddle with
> some registers and it works. What you failed to tell us is:
> 
> a) WHY do you need this?
> b) WHY do we need another DT property for this?
> c) WHAT does this mean for PCI devices?

So far I cannot have the answer for you, will get you back after some discussion
with my colleagues.

> > Signed-off-by: Changming Huang 
> > Signed-off-by: Rajesh Bhagat 
> > Signed-off-by: Ran Wang 
> > ---
> >  drivers/usb/dwc3/core.c | 24 
> > drivers/usb/dwc3/core.h | 10 ++
> >  2 files changed, 34 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index
> > 07832509584f..ffc078ab4a1c 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -236,6 +236,26 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
> > return -ETIMEDOUT;
> >  }
> >
> > +/*
> > + * dwc3_enable_snooping - Enable snooping feature
> > + * @dwc3: Pointer to our controller context structure  */ static void
> > +dwc3_enable_snooping(struct dwc3 *dwc) {
> > +   u32 cfg;
> > +
> > +   cfg = dwc3_readl(dwc->regs, DWC3_GSBUSCFG0);
> > +   if (dwc->dma_coherent) {
> > +   cfg &= ~DWC3_GSBUSCFG0_SNP_MASK;
> > +   cfg |= (AXI3_CACHE_TYPE_SNP <<
> DWC3_GSBUSCFG0_DATARD_SHIFT) |
> > +   (AXI3_CACHE_TYPE_SNP <<
> DWC3_GSBUSCFG0_DESCRD_SHIFT) |
> > +   (AXI3_CACHE_TYPE_SNP <<
> DWC3_GSBUSCFG0_DATAWR_SHIFT) |
> > +   (AXI3_CACHE_TYPE_SNP <<
> DWC3_GSBUSCFG0_DESCWR_SHIFT);
> 
> This "value << shift" looks super clumsy. I would rather have something akin
> to:
> 
>   cfg |= DWC3_GSBUSCFG0_DATARD_CACHEABLE |
>   DWC3_GSBUSCFG0_DESCRD_CACHEABLE ...
> 
> and so on.

Got it. 

> > +   }
> > +
> > +   dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, cfg);
> 
> this will *always* read and write GSBUSCFG0 even for those platforms which
> don't need to change anything on this register. You should just bail out early
> if !dwc->dma_coherent
> 
> Also, I think dma_coherent is likely not the best name for this property.
> 
> Another question is: Why wasn't this setup properly during coreConsultant
> instantiation of the RTL? Do you have devices on the market already that
> need this or is this some early FPGA model or test-only ASIC?

Yes, you are right. Actually I thought that all dwc3 IP  will have this 
register, and
it can be controlled by DTS property. 

> > @@ -776,6 +796,8 @@ static int dwc3_core_init(struct dwc3 *dwc)
> > /* Adjust Frame Length */
> > dwc3_frame_length_adjustment(dwc);
> >
> > +   dwc3_enable_snooping(dwc);
> > +
> > usb_phy_set_suspend(dwc->usb2_phy, 0);
> > usb_phy_set_suspend(dwc->usb3_phy, 0);
> > ret = phy_power_on(dwc->usb2_generic_phy);
> > @@ -1021,6 +1043,8 @@ static void dwc3_get_properties(struct dwc3
> *dwc)
> > _threshold);
> > dwc->usb3_lpm_capable = device_property_read_bool(dev,
> > "snps,usb3_lpm_capable");
> > +   dwc->dma_coherent = device_property_read_bool(dev,
> > +   "dma-coherent");
> >
> > dwc->disable_scramble_quirk = device_property_read_bool(dev,
> > "snps,disable_scramble_quirk");
> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index
> > 4a4a4c98508c..6e6a66650e53 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -153,6 +153,14 @@
> >
> >  /* Bit fields */
> >
> > +/* Global SoC Bus Configuration Register 0 */
> > +#define AXI3_CACHE_TYPE_SNP0x2 /* cacheable */
> > +#define DWC3_GSBUSCFG0_DATARD_SHIFT28
> > +#define DWC3_GSBUSCFG0_DESCRD_SHIFT24
> > +#define DWC3_GSBUSCFG0_DATAWR_SHIFT20
> > +#define DWC3_GSBUSCFG0_DESCWR_SHIFT16
> > +#define DWC3_GSBUSCFG0_SNP_MASK0x
> 
> 
> 
> > +
> >  /* Global Debug Queue/FIFO Space Available Register */
> >  #define DWC3_GDBGFIFOSPACE_NUM(n)  ((n) & 0x1f)
> >  #define DWC3_GDBGFIFOSPACE_TYPE(n) (((n) << 5) & 0x1e0)
> > @@ -859,6 +867,7 @@ struct dwc3_scratchpad_array {
> >   * 

Re: [PATCH] uas: Add US_FL_NO_ATA_1X quirk for one more Seagate device

[Hans de Goede]

Hi,

On 15-11-17 10:06, Oliver Neukum wrote:

Am Dienstag, den 14.11.2017, 18:44 +0100 schrieb Hans de Goede:


Greg, please do no merge the 2 recent uas seagate quirks I send
then. I will submit a patch with the new approach right away.


Hi,

I am afraid in that case we will need a way to override a
quirk in the other direction, that is, to not apply it.


We already have that usb_stor_adjust_quirks when a match to the
device's vend:prod ids is found in usb_storage.quirks,
usb_stor_adjust_quirks will clear all quirks it allows to be set
through the usb_storage.quirks module option.

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: [PATCH] uas: Add US_FL_NO_ATA_1X quirk for one more Seagate device

[Oliver Neukum]

Am Dienstag, den 14.11.2017, 18:44 +0100 schrieb Hans de Goede:
> 
> Greg, please do no merge the 2 recent uas seagate quirks I send
> then. I will submit a patch with the new approach right away.

Hi,

I am afraid in that case we will need a way to override a
quirk in the other direction, that is, to not apply it.

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: usbip port number limits

[Juan Zea]

>On 11/14/2017 09:25 AM, Juan Zea wrote: 
>> Hi, 
>> 
>> I've been working on the issue. This is what I found about multi-controller 
>> setup: 
>> 
>> The problem comes from the usbip tool trying to connect usb2 devices to usb3 
>> ports, like this: 
>> 
>> - usbip tool asks vhci driver for free port. 
>> - If the first port (usb2) is already occupied, vhci answers with usb3 port 
>> (instead of next controller's usb2 port) 
>> - usbip tool tries to connect usb2 device to usb3 port and fails 
>
>It would be interesting to see how this fails. Can you send me complete 
>dmesg from client and server for this. 

I'm attaching two files for this: dmesg-usbip-server.txt and 
dmesg-usbip-client.txt
Server is my own machine, and dmesg is too long, so I have selected latest 
lines. Hope it's enough.
Client side is a kvm machine I'm using for these tests, so I made a clear boot 
just for this and its complete.


>> - usbip tool asks for the next free port, which is still the same usb3 port. 
>> - Loop the above forever 
>
>That doesn't sound right. 

I'm also attaching usbip attach output with debug option 
(usbip-attach-debug.txt). I think it's interesting to see this for the forever 
loop.

>> 
>> the code at tools/usb/usbip/libsrc/vhci_driver.c : 
>> 329 int usbip_vhci_get_free_port(uint32_t speed) 
>> 330 { 
>> 331 for (int i = 0; i < vhci_driver->nports; i++) { 
>> 332 if (speed == USB_SPEED_SUPER && 
>> 333 vhci_driver->idev[i].hub != HUB_SPEED_SUPER) 
>> 334 continue; 
>> 335 
>> 336 if (vhci_driver->idev[i].status == VDEV_ST_NULL) 
>> 337 return vhci_driver->idev[i].port; 
>> 338 } 
>> 339 
>> 340 return -1; 
>> 341 } 
>> 
>> prevents usb3 devices connecting to usb2 ports, but not the other way 
>> around. 
>
>Connecting usb2 device to usb3 port should work. It would make sense 
>to prevent usb3 devices connecting to usb2 ports, and not the other 
>way round. 
>

Ok, that sounds right, but then... why having usb2 ports at all? Isn't it a 
nuisance to maintain code that duplicates the hubs if usb3 ports can do the job?


>> 
>> I've patched this preventing the opposite, and multi-controller starts 
>> working the way I think it should: 
>> - usb2 devices connect exclusively to usb2 ports 
>> - The above is also true for usb3 devices and ports 
>> - When there is no compatible port for the device in the present controller, 
>> go for the next> 
>> At least, for connecting devices this seems to work, and I can get several 
>> usb2 and usb3 connected to the same machine in different orders. 
>> The question is... is this the way it is supposed to work? 
>
>Unless there is some exception in the usb3 support in the usbip driver 
>that prevent usb2 ports from connecting to 
>> 
>> 
>> Another problem is... some of my test devices don't work. They detonate a 
>> kernel BUG line in the vhci controller: 
>> Nov 14 14:35:29 kernel-tester kernel: [ 229.636543] kernel BUG at 
>> drivers/usb/usbip/vhci_hcd.c:683! 
>> 
>> This is happening even using the vhci module as it is downloaded from Ubuntu 
>> (no intervention on my behalf in this experiment), so it seems they just 
>> stopped working some kernels ago. 
>
>It will be harder to debug that way unless you know which kernel release the 
>Ubuntu module is coming from. What's the kernel release on Ubuntu? 

I'm testing with latest 17.10 release of Ubuntu. It comes with kernel 4.13, but 
I have upgraded it to 4.14 from mainline for these tests.

>
>> I'm still bisecting the kernel for this, but for now I have filed a bug at 
>> bugzilla with the specific device and kernel versions: 
>> https://bugzilla.kernel.org/show_bug.cgi?id=197867 
>
>Will you be able to send me complete dmesg for this. I see excerpts but not 
>the 
>full dmesg. 
>

I'm attaching dmesg for the issue: dmesg-device-bug.txt


>Also, will you be able to revert the usb3 commit 
>1c9de5bf428612458427943b724bea51abde520a 
>
>and see if any of the problems go away. 
>
>thanks, 
>-- Shuah 
>

I'm on it and will send results later.

Thanks,
Juan


Juan Antonio Zea Herranz 
Proyectos y consultoría | Qindel Group 
E: juan@qindel.com 
T: +34 91 766 24 21 
M: +34 637 74 63 09 
W: qindel.comNov 14 14:35:00 kernel-tester kernel: [  200.746942] vhci_hcd vhci_hcd.0: 
pdev(0) rhport(0) sockfd(3)
Nov 14 14:35:00 kernel-tester kernel: [  200.746947] vhci_hcd vhci_hcd.0: 
devid(131081) speed(2) speed_str(full-speed)
Nov 14 14:35:00 kernel-tester kernel: [  200.919770] vhci_hcd: vhci_device 
speed not set
Nov 14 14:35:00 kernel-tester kernel: [  200.973759] usb 1-1: new full-speed 
USB device number 5 using vhci_hcd
Nov 14 14:35:00 kernel-tester kernel: [  201.036783] vhci_hcd: vhci_device 
speed not set
Nov 14 14:35:00 kernel-tester kernel: [  201.090804] usb 1-1: SetAddress 
Request (5) to port 0
Nov 14 14:35:00 kernel-tester kernel: [  201.113992] usb 1-1: New USB device 
found, idVendor=05ba, idProduct=000a
Nov 14 14:35:00 kernel-tester kernel: [  201.113997] usb 1-1: New USB device 
strings: Mfr=1, Product=2, 

Re: [PATCH] usb: dwc3: Enable the USB snooping

[Felipe Balbi]

Hi,

Ran Wang  writes:
> Add support for USB3 snooping by asserting bits
> in register DWC3_GSBUSCFG0 for data and descriptor.

we know *how* to enable a feature :-) It's always the same, you fiddle
with some registers and it works. What you failed to tell us is:

a) WHY do you need this?
b) WHY do we need another DT property for this?
c) WHAT does this mean for PCI devices?

> Signed-off-by: Changming Huang 
> Signed-off-by: Rajesh Bhagat 
> Signed-off-by: Ran Wang 
> ---
>  drivers/usb/dwc3/core.c | 24 
>  drivers/usb/dwc3/core.h | 10 ++
>  2 files changed, 34 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 07832509584f..ffc078ab4a1c 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -236,6 +236,26 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
>   return -ETIMEDOUT;
>  }
>  
> +/*
> + * dwc3_enable_snooping - Enable snooping feature
> + * @dwc3: Pointer to our controller context structure
> + */
> +static void dwc3_enable_snooping(struct dwc3 *dwc)
> +{
> + u32 cfg;
> +
> + cfg = dwc3_readl(dwc->regs, DWC3_GSBUSCFG0);
> + if (dwc->dma_coherent) {
> + cfg &= ~DWC3_GSBUSCFG0_SNP_MASK;
> + cfg |= (AXI3_CACHE_TYPE_SNP << DWC3_GSBUSCFG0_DATARD_SHIFT) |
> + (AXI3_CACHE_TYPE_SNP << DWC3_GSBUSCFG0_DESCRD_SHIFT) |
> + (AXI3_CACHE_TYPE_SNP << DWC3_GSBUSCFG0_DATAWR_SHIFT) |
> + (AXI3_CACHE_TYPE_SNP << DWC3_GSBUSCFG0_DESCWR_SHIFT);

This "value << shift" looks super clumsy. I would rather have something
akin to:

cfg |= DWC3_GSBUSCFG0_DATARD_CACHEABLE |
DWC3_GSBUSCFG0_DESCRD_CACHEABLE ...

and so on.

> + }
> +
> + dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, cfg);

this will *always* read and write GSBUSCFG0 even for those platforms
which don't need to change anything on this register. You should just
bail out early if !dwc->dma_coherent

Also, I think dma_coherent is likely not the best name for this property.

Another question is: Why wasn't this setup properly during
coreConsultant instantiation of the RTL? Do you have devices on the
market already that need this or is this some early FPGA model or
test-only ASIC?

> @@ -776,6 +796,8 @@ static int dwc3_core_init(struct dwc3 *dwc)
>   /* Adjust Frame Length */
>   dwc3_frame_length_adjustment(dwc);
>  
> + dwc3_enable_snooping(dwc);
> +
>   usb_phy_set_suspend(dwc->usb2_phy, 0);
>   usb_phy_set_suspend(dwc->usb3_phy, 0);
>   ret = phy_power_on(dwc->usb2_generic_phy);
> @@ -1021,6 +1043,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>   _threshold);
>   dwc->usb3_lpm_capable = device_property_read_bool(dev,
>   "snps,usb3_lpm_capable");
> + dwc->dma_coherent = device_property_read_bool(dev,
> + "dma-coherent");
>  
>   dwc->disable_scramble_quirk = device_property_read_bool(dev,
>   "snps,disable_scramble_quirk");
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 4a4a4c98508c..6e6a66650e53 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -153,6 +153,14 @@
>  
>  /* Bit fields */
>  
> +/* Global SoC Bus Configuration Register 0 */
> +#define AXI3_CACHE_TYPE_SNP  0x2 /* cacheable */
> +#define DWC3_GSBUSCFG0_DATARD_SHIFT  28
> +#define DWC3_GSBUSCFG0_DESCRD_SHIFT  24
> +#define DWC3_GSBUSCFG0_DATAWR_SHIFT  20
> +#define DWC3_GSBUSCFG0_DESCWR_SHIFT  16
> +#define DWC3_GSBUSCFG0_SNP_MASK  0x



> +
>  /* Global Debug Queue/FIFO Space Available Register */
>  #define DWC3_GDBGFIFOSPACE_NUM(n)((n) & 0x1f)
>  #define DWC3_GDBGFIFOSPACE_TYPE(n)   (((n) << 5) & 0x1e0)
> @@ -859,6 +867,7 @@ struct dwc3_scratchpad_array {
>   *   3   - Reserved
>   * @imod_interval: set the interrupt moderation interval in 250ns
>   * increments or 0 to disable.
> + * @dma_coherent: set if enable dma-coherent.

you're not enabling dma coherency, you're enabling cache snooping. And
this property should describe that. Also, keep in mind that different
devices may want different cache types for each of those fields, so your
property would have to be a lot more complex. Something like:

snps,cache-type = , , ...

Then driver would have to parse this properly to setup GSBUSCFG0. In any
case, I still want to know why do you really need this? What's the
reason? What happens if you don't fix GSBUSCFG0? What's the value you
have there by default? Why isn't that default good enough?

ps: since you're fiddling with DT, you should also include
devicetree@vger

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

[PATCH net,stable] net: cdc_ncm: GetNtbFormat endian fix

[Bjørn Mork]

The GetNtbFormat and SetNtbFormat requests operate on 16 bit little
endian values. We get away with ignoring this most of the time, because
we only care about USB_CDC_NCM_NTB16_FORMAT which is 0x.  This
fails for USB_CDC_NCM_NTB32_FORMAT.

Fix comparison between LE value from device and constant by converting
the constant to LE.

Reported-by: Ben Hutchings 
Fixes: 2b02c20ce0c2 ("cdc_ncm: Set NTB format again after altsetting switch for 
Huawei devices")
Cc: Enrico Mioso 
Cc: Christian Panton 
Signed-off-by: Bjørn Mork 
---
 drivers/net/usb/cdc_ncm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 47cab1bde065..9e1b74590682 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -771,7 +771,7 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct 
usb_interface *intf, u8 data_
int err;
u8 iface_no;
struct usb_cdc_parsed_header hdr;
-   u16 curr_ntb_format;
+   __le16 curr_ntb_format;
 
ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
if (!ctx)
@@ -889,7 +889,7 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct 
usb_interface *intf, u8 data_
goto error2;
}
 
-   if (curr_ntb_format == USB_CDC_NCM_NTB32_FORMAT) {
+   if (curr_ntb_format == cpu_to_le16(USB_CDC_NCM_NTB32_FORMAT)) {
dev_info(>dev, "resetting NTB format to 16-bit");
err = usbnet_write_cmd(dev, USB_CDC_SET_NTB_FORMAT,
   USB_TYPE_CLASS | USB_DIR_OUT
-- 
2.11.0

--
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 4.4 27/56] cdc_ncm: Set NTB format again after altsetting switch for Huawei devices

[Bjørn Mork]

Ben Hutchings  writes:
> On Tue, 2017-07-11 at 17:21 +0200, Enrico Mioso wrote:
>> From: Enrico Mioso 
>> 
>> commit 2b02c20ce0c28974b44e69a2e2f5ddc6a470ad6f upstream.
> [...]
>> --- a/drivers/net/usb/cdc_ncm.c
>> +++ b/drivers/net/usb/cdc_ncm.c
>> @@ -724,8 +724,10 @@ int cdc_ncm_bind_common(struct usbnet *d
>>  u8 *buf;
>>  int len;
>>  int temp;
>> +int err;
>>  u8 iface_no;
>>  struct usb_cdc_parsed_header hdr;
>> +u16 curr_ntb_format;
> [...]
>> +err = usbnet_read_cmd(dev, USB_CDC_GET_NTB_FORMAT,
>> +  USB_TYPE_CLASS | USB_DIR_IN | 
>> USB_RECIP_INTERFACE,
>> +  0, iface_no, _ntb_format, 2);
>> +if (err < 0) {
>> +goto error2;
>> +}
>> +
>> +if (curr_ntb_format == USB_CDC_NCM_NTB32_FORMAT) {
> [...]
>
> usbnet_read_cmd() doesn't do any byte-swapping, so it looks like
> curr_ntb_format will have little-endian byte order (__le16 not u16). 
> The comparison will then need to be done using
> le16_to_cpu(curr_ntb_format).

Right.  Thanks.  Fix coming up ASAP.

I cannot explain why since we are obviously reading and writing 2 bytes
here, but for some reason I've always thought of this value as 8 bit.


Bjørn
--
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